Dynamic header support #140

erasmospunk · 2017-02-24T03:33:14Z

Block headers can have a dynamic size that is being indexed on a
new meta file "headers_offsets".

The offsets are 64bit in order to accommodate coins with big
headers that will accumulate GBs of header data after some years.
We don't store the offset of the genesis header as it's always 0,
this means that the offset for height 1 is stored at position 0,
for height 2 at 8 and so on.

This commit adds support for AuxPow coins like Namecoin, Dogecoin
and Equihash coins like Zcash

coveralls · 2017-02-24T03:36:07Z

Coverage increased (+13.3%) to 16.921% when pulling 5cb58074914c5816464c683787dad6a865b50e71 on erasmospunk:dynamic-headers into 0122336 on kyuupichan:master.

coveralls · 2017-02-24T10:10:26Z

Coverage increased (+13.3%) to 16.916% when pulling 60459ced19630e9170098ebf6178a16579c88584 on erasmospunk:dynamic-headers into 0122336 on kyuupichan:master.

erasmospunk · 2017-02-24T10:49:14Z

Small fix

coveralls · 2017-02-24T10:51:13Z

Coverage increased (+13.3%) to 16.916% when pulling 5027abb3921e3591dbb73158c74d5c609e6baced on erasmospunk:dynamic-headers into 0122336 on kyuupichan:master.

erasmospunk · 2017-02-24T15:03:26Z

Small refactor

coveralls · 2017-02-24T15:06:13Z

Coverage increased (+13.3%) to 16.921% when pulling 3ee04d09a6c62c01f315542d1e0f98941f173853 on erasmospunk:dynamic-headers into 0122336 on kyuupichan:master.

erasmospunk · 2017-02-24T15:29:42Z

Some clean ups

coveralls · 2017-02-24T15:32:01Z

Coverage increased (+13.3%) to 16.929% when pulling 972e16da1d402bd63df2f26381f60d7ea5f53484 on erasmospunk:dynamic-headers into 0122336 on kyuupichan:master.

erasmospunk · 2017-02-24T19:25:03Z

Fixed edge case

coveralls · 2017-02-24T19:27:02Z

Coverage increased (+13.3%) to 16.908% when pulling 615bf165e0b30b4dfcdcfbe24e537e0e68084c13 on erasmospunk:dynamic-headers into 0122336 on kyuupichan:master.

kyuupichan · 2017-02-26T07:51:33Z

John - I'll be leaving these changes for post 1.0. They are best reviewed if you can break them up into perhaps 3 or 4 parts. Let's just do 1 part at a time so you don't have to keep updating later ones if earlier ones change.

coveralls · 2017-02-26T22:10:54Z

Coverage increased (+13.2%) to 16.84% when pulling 0ca9c099295c13c216ca54fe6d64b9eb69aa7c8d on erasmospunk:dynamic-headers into b3005fb on kyuupichan:master.

erasmospunk · 2017-02-26T22:11:13Z

@kyuupichan just pushed the changes in smaller commits.

Please notify me when you are ready to merge so I can rebase to the latest master if needed.

erasmospunk · 2017-02-26T22:14:19Z

Rebased to the latest master

coveralls · 2017-02-26T22:16:28Z

Coverage increased (+13.2%) to 16.84% when pulling 2b670702127957db842d2048050284cc47295970 on erasmospunk:dynamic-headers into b3005fb on kyuupichan:master.

erasmospunk · 2017-02-26T22:21:47Z

Removed commit #141 from this pull request

coveralls · 2017-02-26T22:23:14Z

Coverage increased (+13.2%) to 16.84% when pulling 4530e216cef26da607c35c52dacacf0f7d39dcc1 on erasmospunk:dynamic-headers into b3005fb on kyuupichan:master.

coveralls · 2017-02-26T22:31:39Z

Coverage increased (+13.2%) to 16.802% when pulling bdda44548e6a0c8968c4bdc58aa7256cac1a0292 on erasmospunk:dynamic-headers into 472c738 on kyuupichan:master.

kyuupichan

This is great. In future please omit the 0 in [0:x].

kyuupichan · 2017-03-09T15:28:33Z

I'll try and take a look at this stuff this weekend. Regarding your Tx question, not looked in detail but would probably prefer the tx be a separate class like you have it, perhaps derived from the existing one.

coveralls · 2017-03-09T17:10:26Z

Coverage increased (+2.9%) to 18.649% when pulling f0cbdb69bb257781b108764bf625892dcab71708 on erasmospunk:dynamic-headers into 30bb832 on kyuupichan:master.

coveralls · 2017-03-09T17:24:58Z

Coverage increased (+2.9%) to 18.654% when pulling cf2de9b0e2edaa1bfa2bd3faa7db975984f996ca on erasmospunk:dynamic-headers into 30bb832 on kyuupichan:master.

kyuupichan · 2017-03-11T06:21:34Z

John, sorry this is just a bit too much of a large change all at once. I really appreciate the effort you've put into this and the tests, but I need it broken up a bit. A few other things:

Can we do the API changes first? Then maybe one or more changes, and finally the new coin definitions and tests.
I know the definition of block will change with the named tuple, but I'd like to keep calling it a block. So let's just stick with block and blocks rather than b, parsed_blocks, full_block etc.
I'd like to avoid the Deserializer construction you've done. I had to stare at it a while (and I learnt something new!). I don't want tx.py objects having a COIN member for now. The old classes won't need one; the methods of the new deserializer classes can be passed "cls" to a coin parameter if necessary when invoked from coins.py. But looking at your changes it seems all they really want is a basic_header_size to be passed.
perhaps coin.DESERIALIZER should be lower case and just be the function in tx.py for bitcoin and similar, and be a function in the coin class for aux_pow coins
Do we actually really need a read_header API in tx.py? Can we keep it as-is with read_tx and read_block (perhaps to be renamed read_tx_block)? I'd like to keep tx.py to be just about transactions; I think the knowledge of the block format and header size better belongs in the coin class.
in db.py I'd like to call fs_update_header_offsets unconditionally, and return doing nothing if STATIC_BLOCK_HEADERS, rather than testing call sites and asserting inside the function.

I realize this might be a bit of work. If you prefer I can have a go at doing the initial API changes, but I'm not sure I'll get everything. Then you can follow up with the bulk of the change. Or you can do the lot. But if you could break it up into at least 3 pieces it would help. One at a time is fine.

I've created a dynamic-headers branch on github. Can you work against that? Let me know how you'd like to proceed.

erasmospunk · 2017-03-13T22:09:59Z

@kyuupichan thanks for the quality code review.

Can we do the API changes first? Then maybe one or more changes, and finally the new coin definitions and tests.

Good point. Regarding the coin definitions, what do you think about a folder that contains a .py file per coin? This should keep clean the coins.py

Regarding the other points I will take a look in the next days.

kyuupichan · 2017-03-13T23:04:59Z

Let's keep it all in coins.py for now. Later something might be more appropriate,indeed. One advantage to having it all in coins.py is selecting the coin class is easy programatically (the lookup_ function); it might be more awkward if in separate files.

erasmospunk · 2017-03-13T23:05:23Z

@kyuupichan one thing that I noticed, when parsing the block it is useful to have the Deserializer::cursor property, especially for parsing a block footer if it exists.

erasmospunk · 2017-03-13T23:08:20Z

Although technically we don't need the footer...

coveralls · 2017-03-14T01:29:33Z

Coverage increased (+2.9%) to 18.65% when pulling 77669871e60f8d91ba09c599a385f0730393eee4 on erasmospunk:dynamic-headers into 9c6d2f5 on kyuupichan:dynamic-headers.

erasmospunk · 2017-03-14T01:31:12Z

Refactored code but haven't tried it yet, please review but don't merge.

One thing that I would like to refactor is to expose the Deserializer private methods like _read_varint and _read_nbytes that are essential to parse the dynamic headers.

Another difference is that we don't read the block footers anymore.

kyuupichan · 2017-03-14T11:24:09Z

lib/coins.py

-        return deserializer(block[cls.header_len(height):]).read_block()
+        if height > 0:
+            return cls.block_full(block, height)
+        else:


This looks like an infinite loop. I'll try and fix it here.

kyuupichan · 2017-03-14T11:41:06Z

server/block_processor.py

@@ -566,14 +567,14 @@ def backup_blocks(self, blocks):
        coin = self.coin
        for block in blocks:
            # Check and update self.tip
-            header = coin.block_header(block, self.height)
+            header, txs = coin.block_full(block, self.height)
            header_hash = coin.header_hash(header)


Here I prefer to keep the namedtuple as a single object and refer to its members, rather than assuming it has length 2. I'll update that too, here.

I'm merging your first commit of the 4 and following up with a couple of tweaks as mentioned above. If you could rebase that would be great. But before doing so let me make a couple of comments on the 2nd commit too.

Oh and that code was good enough to survive a forced testnet 3 block reorg via RPC

kyuupichan · 2017-03-14T11:51:17Z

server/db.py

+        # We don't store the offset of the genesis header as it's always 0
+        # so this means that the offset for height 1 is stored at
+        # position 0, for height 2 at 8 and so on.
+        if height > 0:


I'd rather waste 8 bytes on disk and not have the extra comment and if statement. Any reason not to store height 0 as 0 too? How is it not getting written? I might be missing something.

And can you add yourself to AUTHORS? This 2nd commit looks good otherwise.

Yes, block 0 offset is a special case to "bootstrap" the first offset when the headers_offsets_file is initially empty, otherwise dynamic_header_offset will not work.

Check how the fs_update_header_offsets calculates the offset of block N based on the header size of block N-1.

One pythonic "fix" could be:

try: pos = (height - 1) * 8 offset, = unpack('<Q', self.headers_offsets_file.read(pos, 8)) return offset except struct.error as e: if height == 0: return 0 else: raise Exception('Header offset not found: {}'.format(height))

Another alternative is to check and write the block 0 offset when creating the headers_offsets_file.

Yes, do you mind writing the initial 0? There is a similar issue with tx_count, and I don't like my current solution, it caused too many if statements.

kyuupichan · 2017-03-14T12:19:25Z

lib/coins.py

+            header_end = cls.BASIC_HEADER_SIZE
+        block.cursor = start
+        return block._read_nbytes(header_end)
+


Do you think the bulk of the block_header method here is best put in an aux-pow specific deserializer? I think that's what you did before and I think I prefer it now it's clearer what's happening. Sorry about that. Similarly for Zcash below.

So from here just call AuxPowDeserializer.block_header(cls.BASIC_HEADER_SIZE, cls.VERSION_AUX_POW) or something like that. Those 2 variables belong in the coin class as they are now. With that I think I'm OK with everything once you rebase. I'll merge it. Thanks.

One more thing, please put in a commit message the github text "Closes #xxx" for the 2 outstanding issues related to this aux pow support. I assume it does resolve them.

coveralls · 2017-03-14T19:11:53Z

Coverage increased (+2.9%) to 18.753% when pulling e33466876024f796e89a84acbea8391b43f582f5 on erasmospunk:dynamic-headers into 0fdd23a on kyuupichan:dynamic-headers.

coveralls · 2017-03-14T19:11:54Z

Coverage increased (+2.9%) to 18.753% when pulling e33466876024f796e89a84acbea8391b43f582f5 on erasmospunk:dynamic-headers into 0fdd23a on kyuupichan:dynamic-headers.

Block headers can have a dynamic size that is being indexed on a new meta file "headers_offsets". The offsets are 64 bits in order to accommodate coins with big headers that will accumulate GBs of header data after some years. Closes kyuupichan#128

Closes kyuupichan#83

coveralls · 2017-03-15T00:11:00Z

Coverage increased (+2.9%) to 18.753% when pulling cb9ab7e on erasmospunk:dynamic-headers into 0fdd23a on kyuupichan:dynamic-headers.

erasmospunk · 2017-03-15T11:13:58Z

Updated with the changes you asked. Also I made some tests with Dogecoin, Zcash and it seems to work without an issue like before.

kyuupichan · 2017-03-15T13:13:31Z

Thanks John. I'll merge this as it seems you're happy with non-BTC stuff. I'll run this on my server for a while, and if it's good I'll merge it into master.

erasmospunk mentioned this pull request Feb 24, 2017

Add support for variable length headers #128

Closed

erasmospunk force-pushed the dynamic-headers branch from 5cb5807 to 60459ce Compare February 24, 2017 10:07

erasmospunk force-pushed the dynamic-headers branch from 60459ce to 5027abb Compare February 24, 2017 10:48

erasmospunk force-pushed the dynamic-headers branch from 5027abb to 3ee04d0 Compare February 24, 2017 15:03

erasmospunk force-pushed the dynamic-headers branch from 3ee04d0 to 972e16d Compare February 24, 2017 15:29

erasmospunk force-pushed the dynamic-headers branch from 972e16d to 615bf16 Compare February 24, 2017 19:24

erasmospunk force-pushed the dynamic-headers branch from 615bf16 to 0ca9c09 Compare February 26, 2017 22:08

erasmospunk force-pushed the dynamic-headers branch from 0ca9c09 to 2b67070 Compare February 26, 2017 22:13

erasmospunk force-pushed the dynamic-headers branch from 2b67070 to 4530e21 Compare February 26, 2017 22:20

erasmospunk force-pushed the dynamic-headers branch from 4530e21 to bdda445 Compare February 26, 2017 22:29

kyuupichan reviewed Feb 26, 2017

View reviewed changes

erasmospunk force-pushed the dynamic-headers branch from bdda445 to 094b860 Compare February 26, 2017 22:35

erasmospunk force-pushed the dynamic-headers branch from a98699e to f0cbdb6 Compare March 9, 2017 17:07

erasmospunk force-pushed the dynamic-headers branch from f0cbdb6 to cf2de9b Compare March 9, 2017 17:22

erasmospunk changed the base branch from master to dynamic-headers March 14, 2017 01:26

erasmospunk force-pushed the dynamic-headers branch from cf2de9b to 7766987 Compare March 14, 2017 01:26

kyuupichan reviewed Mar 14, 2017

View reviewed changes

erasmospunk force-pushed the dynamic-headers branch from 7766987 to e334668 Compare March 14, 2017 19:08

erasmospunk added 3 commits March 15, 2017 02:05

Dynamic header support

1eff2ec

Block headers can have a dynamic size that is being indexed on a new meta file "headers_offsets". The offsets are 64 bits in order to accommodate coins with big headers that will accumulate GBs of header data after some years. Closes kyuupichan#128

Fix support for Namecoin and Dogecoin, add Zcash support

bf3725c

Closes kyuupichan#83

added raw block tests

cb9ab7e

erasmospunk force-pushed the dynamic-headers branch from e334668 to cb9ab7e Compare March 15, 2017 00:08

kyuupichan merged commit cd990c4 into kyuupichan:dynamic-headers Mar 15, 2017

erasmospunk deleted the dynamic-headers branch March 21, 2018 17:45

Dynamic header support #140

Dynamic header support #140

Conversation

erasmospunk commented Feb 24, 2017

coveralls commented Feb 24, 2017 • edited Loading

coveralls commented Feb 24, 2017 • edited Loading

erasmospunk commented Feb 24, 2017

coveralls commented Feb 24, 2017 • edited Loading

erasmospunk commented Feb 24, 2017

coveralls commented Feb 24, 2017 • edited Loading

erasmospunk commented Feb 24, 2017

coveralls commented Feb 24, 2017 • edited Loading

erasmospunk commented Feb 24, 2017

coveralls commented Feb 24, 2017 • edited Loading

kyuupichan commented Feb 26, 2017

coveralls commented Feb 26, 2017 • edited Loading

erasmospunk commented Feb 26, 2017

erasmospunk commented Feb 26, 2017

coveralls commented Feb 26, 2017 • edited Loading

erasmospunk commented Feb 26, 2017

coveralls commented Feb 26, 2017 • edited Loading

coveralls commented Feb 26, 2017 • edited Loading

kyuupichan left a comment

Choose a reason for hiding this comment

kyuupichan commented Mar 9, 2017

coveralls commented Mar 9, 2017 • edited Loading

coveralls commented Mar 9, 2017 • edited Loading

kyuupichan commented Mar 11, 2017 • edited Loading

erasmospunk commented Mar 13, 2017

kyuupichan commented Mar 13, 2017

erasmospunk commented Mar 13, 2017

erasmospunk commented Mar 13, 2017

coveralls commented Mar 14, 2017 • edited Loading

erasmospunk commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 14, 2017

coveralls commented Mar 14, 2017 • edited Loading

coveralls commented Mar 15, 2017 • edited Loading

erasmospunk commented Mar 15, 2017

kyuupichan commented Mar 15, 2017

coveralls commented Feb 24, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading

coveralls commented Feb 24, 2017 •

edited

Loading

coveralls commented Feb 26, 2017 •

edited

Loading

coveralls commented Feb 26, 2017 •

edited

Loading

coveralls commented Feb 26, 2017 •

edited

Loading

coveralls commented Feb 26, 2017 •

edited

Loading

coveralls commented Mar 9, 2017 •

edited

Loading

coveralls commented Mar 9, 2017 •

edited

Loading

kyuupichan commented Mar 11, 2017 •

edited

Loading

coveralls commented Mar 14, 2017 •

edited

Loading

coveralls commented Mar 14, 2017 •

edited

Loading

coveralls commented Mar 15, 2017 •

edited

Loading