Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement dynafed block header support for elements >= 0.18.1 #3440

Merged
merged 7 commits into from
Jan 28, 2020

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jan 25, 2020

This implements support for the upcoming elementsd dynafed block header
format, which is slightly different from either the bitcoin block header
format as well as the elementsd <= 0.17.0 block header format. A couple of
fields were added, 3 different serialization formats for the information
(null, compact and full) and some of those fields are not hashed into
the block hash, since they'd be self-referential (like the pre-0.18.0 solution
field).

In order to implement this I chose to also simplify the block hash generation,
to happen in parallel with the deserialization. This has a couple of
side-effects:

  • The block hash is computed only once, instead of multiple times when we
    call bitcoin_block_blkid. For compatibility I left that function, but it
    now just copies over the cached hash instead of re-serializing it.
  • We do not need any serialization code for the block header, since we are
    just consumers of blocks, and we don't have to generate new ones from its
    constituent parts.
  • We don't need to carry block header fields around that are otherwise of no
    use to us. It always felt weird having to remember constituent parts only
    to be able to re-serialize the header. Now we can just feed it into the
    hashing algorithm and skip the actual fields. This also ends up saving us a
    couple of allocations.

There are a couple of drive-by fixes, such as weening off of elementsd's
deprecated generate RPC in favor of generatetoaddress, but they should be
minor.

Fixes #3362

@cdecker
Copy link
Member Author

cdecker commented Jan 25, 2020

As a minor caveat, there are intermediate versions (v0.18.0, v0.18.1.1,
v0.18.1.2) that predate the introduction of the elided_root in the block
header params when serialized in the compact format. However, there is no
programmatic way to identify whether the block header will contain the field
or not, since the version field was not updated. In this PR I chose to
implement the parsing that includes the elided_root, therefore running any
of the versions above results in an incompatibility and will crash
lightningd. You've been warned ⚠️

More details on the rationale for the change can be found here: ElementsProject/elements#773

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack fa8248a

This avoids having to re-serialize the block header just to compute the
hash. It also frees us from having to carry around all the details in the
header and we can hand around a minimal version.
We're about to push a couple of varints as part of the dynafed blockparsing,
so we might as well make it easier for us.
Since we now compute the hash while deserializing the block header we can now
just use it, no reason to serialize the header just to hash it again. This
also allows us to throw away all the added dynafed fields in the next commit
instead of having to carry them around.
Changelog-Added: elements: Added support for the dynafed block header format and elementsd >=0.18.1
The `generate` has been deprecated since 0.16 and has been removed in 0.18.0
so we better use `generatetoaddress` instead, which is already what we do with
`bitcoind`. So we remove the override here.
@cdecker cdecker added this to the 0.8.1 milestone Jan 28, 2020
@cdecker cdecker merged commit 28080b2 into ElementsProject:master Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement dynafed support for Elements 0.18+
2 participants