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

rpc: expose CBlockIndex::nTx in getblock(header) #13451

Merged
merged 1 commit into from Jun 14, 2018

Conversation

Projects
None yet
6 participants
@instagibbs
Copy link
Member

instagibbs commented Jun 12, 2018

Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/

Including the coinbase in the txoutproof seems the most effective fix, however results in a significant efficiency downgrade. Transactors will not even know a priori what the size of their proof will be within a couple orders of magnitude, unless they use the mid-state of SHA2 as detailed in the blog post.

Some applications, like Elements blockchain platform that take SPV-style proofs have optional access to a bitcoind to verify these proofs of inclusion and check depth in the chain. Returning CBlockIndex::nTx would allow an extremely easy and compact way of checking the depth of the tree, with no additional overhead to the codebase, and works with pruned nodes.

getblockheader is arguably not the place for it, but as mentioned before, is a natural workflow for us checking depth of a block in a possibly pruned node.

We should also ensure that verifytxoutproof ends up validating this depth fact as well, but left this for another PR.

@@ -694,6 +695,7 @@ static UniValue getblockheader(const JSONRPCRequest& request)
" \"bits\" : \"1d00ffff\", (string) The bits\n"
" \"difficulty\" : x.xxx, (numeric) The difficulty\n"
" \"chainwork\" : \"0000...1f3\" (string) Expected number of hashes required to produce the current chain (in hex)\n"
" \"nTx\" : n, (numeric) The number of transactions in the block. This result can also be returned for pruned blocks, as it's stored in the block index.\n"

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 12, 2018

Member

This result can also be returned for pruned blocks, as it's stored in the block index

Should be a comment in the source code instead?

I believe we have a pruning test, so it would make sense to add an assert_equal(nTx, whatever) there?

@MarcoFalke MarcoFalke changed the title expose CBlockIndex::nTx in getblockheader rpc: expose CBlockIndex::nTx in getblockheader Jun 12, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 12, 2018

Similar to mediantime, this should also be exposed in getblock

@instagibbs instagibbs force-pushed the instagibbs:expose_nTx branch from f9eda3e to 86edf4a Jun 13, 2018

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jun 13, 2018

more tests, exposure and rewording as per @MarcoFalke suggestions

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jun 13, 2018

If it's not too much trouble I'd also like this backported for 0.16.2

@instagibbs instagibbs changed the title rpc: expose CBlockIndex::nTx in getblockheader rpc: expose CBlockIndex::nTx in getblock(header) Jun 13, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 13, 2018

utACK 86edf4a

Also, tagged for backport, since it is a rather trivial rpc change with tests already included.

@MarcoFalke MarcoFalke added this to the 0.16.2 milestone Jun 13, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Jun 13, 2018

utACK 86edf4a.

@jtimon

This comment has been minimized.

Copy link
Member

jtimon commented Jun 14, 2018

utACK 86edf4a

1 similar comment
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 14, 2018

utACK 86edf4a

@laanwj laanwj merged commit 86edf4a into bitcoin:master Jun 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 14, 2018

Merge #13451: rpc: expose CBlockIndex::nTx in getblock(header)
86edf4a expose CBlockIndex::nTx in getblock(header) (Gregory Sanders)

Pull request description:

  Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/

  Including the coinbase in the txoutproof seems the most effective fix, however results in a significant efficiency downgrade. Transactors will not even know a priori what the size of their proof will be within a couple orders of magnitude, unless they use the mid-state of SHA2 as detailed in the blog post.

  Some applications, like Elements blockchain platform that take SPV-style proofs have optional access to a bitcoind to verify these proofs of inclusion and check depth in the chain. Returning `CBlockIndex::nTx` would allow an extremely easy and compact way of checking the depth of the tree, with no additional overhead to the codebase, and works with pruned nodes.

  `getblockheader` is arguably not the place for it, but as mentioned before, is a natural workflow for us checking depth of a block in a possibly pruned node.

  We should also ensure that `verifytxoutproof` ends up validating this depth fact as well, but left this for another PR.

Tree-SHA512: af4cf48e704c6088f8da06a477fda1aaa6f8770cee9b876c4465d1075966d6a95831a88817673fe5a0d6bbcdc1ffcbc1892e2be0d838c60fc6958d33eacdcc14

@instagibbs instagibbs referenced this pull request Jun 14, 2018

Merged

[0.16.2] Backports #13455

fanquake added a commit to fanquake/bitcoin that referenced this pull request Jun 15, 2018

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jun 26, 2018

Added to #13455 for backport.

laanwj added a commit that referenced this pull request Jul 9, 2018

Merge #13452: rpc: have verifytxoutproof check the number of txns in …
…proof structure

d280617 [qa] Add a test for merkle proof malleation (Suhas Daftuar)
ed82f17 have verifytxoutproof check the number of txns in proof structure (Gregory Sanders)

Pull request description:

  Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/

  This change would at least allow `verifytxoutproof` to properly validate that the proof matches a known block, with known number of transactions any time after the full block is processed. This should neuter the attack entirely.

  The negative is that a header-only processed block/future syncing mode would cause this to fail until the node has imported the data required.

  related: #13451

  `importprunedfunds` needs this check as well. Can expand it to cover this if people like the idea.

Tree-SHA512: 0682ec2b622a38b29f3f635323e0a8b6fc071e8a6fd134c954579926ee7b516e642966bafa667016744ce49c16e19b24dbc8801f982a36ad0a6a4aff6d93f82b

laanwj added a commit that referenced this pull request Jul 9, 2018

Merge #13455: [0.16.2] Backports
9fd3e00 depends: Update Qt download url (fanquake)
f7401c8 Fix parameter count check for importpubkey. (Kristaps Kaupe)
cbd2f70 expose CBlockIndex::nTx in getblock(header) (Gregory Sanders)
ce8aa54 Add Windows shutdown handler (Chun Kuan Lee)
18b0c69 Bugfix: Include <memory> for std::unique_ptr (Luke Dashjr)

Pull request description:

  Backports:
  * #12859 Bugfix: Include <memory> for std::unique_ptr
  * #13131 Add Windows shutdown handler
  * #13451 rpc: expose CBlockIndex::nTx in getblock(header)
  * #13507 RPC: Fix parameter count check for importpubkey
  * #13544 depends: Update Qt download url

  to the 0.16 branch.

Tree-SHA512: eeaec52d001d5c81e67dda3a2d3fee7a9445e569366e597b18e81d802c1b7f89e545afd53d094740c37c1714050304979398b9860144454d3a5cb5abc9e9eaca

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment