-
Notifications
You must be signed in to change notification settings - Fork 274
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
add: support for getblocktemplate in template mode #163
add: support for getblocktemplate in template mode #163
Conversation
The one CI job failing seems to be unrelated. Happens on |
0803d7f
to
edcb7e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, looks quite good!
Could you also rebase to make CI happy? |
c0e1ddb
to
9c2ac81
Compare
Thanks! Addressed your review and rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The PR looks great! Thanks for including doc comments for all the fields, the resource links in the PR's description and the example code. My only nit is what @stevenroose already brought up regarding slice arguments. |
Pushed f36e4e5 which has slice arguments for the edit: forgot to adapt the tests in previously pushed commit |
e8bfd72
to
f36e4e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more nit :)
c63c258
to
a2d6bc1
Compare
Proposal mode and longpolling on the template mode are not yet included.
a2d6bc1
to
3106381
Compare
Done! |
ACK, let's await CI |
The failing job seems to be unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
PR rust-bitcoin#163 implemented support for getblocktemplate. However, during the implementation the version_bits_available HashMap the map fields were switched. This became apparent during testing against a v0.21.1 Bitcoin Core node, which includes support for the Taproot softfork via the 2nd version bit. Previously, no softfork was defined. ``` $ bitcoin-cli getblocktemplate "{\"rules\": [\"segwit\"]}" ... "vbavailable": { "taproot": 2 }, ... ```
1f5c57a fix: version_bits_available maps String to u32 (0xb10c) Pull request description: PR #163 implemented support for `getblocktemplate`. However, during the implementation the `version_bits_available` HashMap the map fields were switched. This became apparent during testing against a v0.21.1 Bitcoin Core node, which includes support for the Taproot softfork via the 2nd version bit. Previously, no softfork was defined. ``` $ bitcoin-cli getblocktemplate "{\"rules\": [\"segwit\"]}" ... "vbavailable": { "taproot": 2 }, ... ``` ACKs for top commit: sgeisler: utACK 1f5c57a tcharding: utACK 1f5c57a Tree-SHA512: ebae449b93ca8d428e8c0e6dcbeca20765966500e3582368b1a6dc3a60e3205c507121b06a22b3e0e4091efc4828cc17dd65049afebf09695d9a5abc50b1dc5c
Partly addresses #133
This adds basic support for the
getblocktemplate
RCP to the client. The default 'template' mode, in which Bitcoin Core builds and returns a block template to the RPC client, is implemented. The 'proposal' mode, where the client submits a block to Bitcoin Core for validation (before it's mined; proof-of-work is not checked) as well as longpolling for changes to a given block template are not implemented yet. However, the basic implementation proposed here can be extended to support both of these features without changing the API.I've focused on the subset of features (also called "capabilities") that Bitcoin Core actually implements from BIP 22 and BIP 23. I've tested this against mainnet, testnet, regtest and signet with the Rust code provided below.
Reviewers might find the following resources useful:
Code for playing around with this