net processing: Only support version 2 compact blocks #20799
Conversation
I think there may be some edge-case scenarios where a peer skips the download of witnesses because they are never read (due to an assumevalid setting), but that has nothing to do with compact blocks. So: Concept ACK on removing code that doesn't serve any use case in practice. |
|
Concept ACK on removing non-witness cmpctblock support. IIRC its only purpose is serving Bitcoin Core v0.13.0 nodes (0.12.x didn't have compact blocks; 0.13.1 added segwit activation parameters). The first commit here seems to mix removal of functionality with some refactoring. Is it possible to separate those more cleanly into separate commits? |
Yes, I'll see what I can do to split it up a bit more. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
|
Concept ACK, looks like a good simplification. Worth reviewing the diffs with |
jnewbery
force-pushed
the
2020-12-remove-cmpctblock-v1
branch
from
b46f099
to
f8ff628
Compare
|
Rebased and broken up the first commit. I've also dropped the last two commits (Clean up PeerManager::BlockChecked() and Clean up MaybeSetPeerAsAnnouncingHeaderAndIDs()). There's already enough happening in this PR. |
jnewbery
force-pushed
the
2020-12-remove-cmpctblock-v1
branch
from
f8ff628
to
befb068
Compare
jnewbery
changed the title
|
Concept ACK |
There was a problem hiding this comment.
Reviewed first three commits up to 55c9231 then started getting bogged down in the granular ones going over some of the same parts repeatedly and with the renamings; skim-reviewed them for now and will need to return. All of the commits build cleanly rebased to master with p2p_compactblocks.py passing on each.
jnewbery
force-pushed
the
2020-12-remove-cmpctblock-v1
branch
2 times, most recently
from
f3c8cf2
to
77cd1a8
Compare
|
Thanks for the review @jonatack @ariard @sipa @MarcoFalke . I think I've addresses all review comments now. This PR is probably easier to review if rebased after #21009 is merged. That PR always sets |
- use better local variable names - drop unnecessary if statements
fPreferHeaderAndIDs -> m_requested_hb_cmpctblocks fProvidesHeaderAndIDs -> m_provides_cmpctblocks
…elay if segwit is enabled This introduces an early exit in PeerManagerImpl::NewPoWValidBlock() if segwit has not been activated for the block. This means that we won't cache the block/compact block for fast relay and won't relay the cmpctblock immediately to peers that have requested hb compact blocks. This is fine because any block where segwit is not yet activated is buried deep in the chain, and so compact block relay will not be effective. It's ok not to cache the block/compact block for fast relay for the same reason - the block must be very deeply buried in the block chain. ProcessBlockAvailability() also won't get called for all nodes. This is also fine, since that function only updates hashLastUnknownBlock and pindexBestKnownBlock, and is called early in every SendMessages() call.
…beSetPeerAsAnnouncingHeaderAndIDs()
jnewbery
force-pushed
the
2020-12-remove-cmpctblock-v1
branch
from
4c6d983
to
a50e34c
Compare
|
Thanks for the reviews @dergoegge and @MarcoFalke. I've addressed all review comments and rebased on latest master. |
There was a problem hiding this comment.
re-ACK a50e34c after rebase 👓
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK a50e34c367608fcec9697893981bfa294a7c08d after rebase 👓
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgSIwwAx5jdanYqhTgoX2H5SG6pQ9aoBujOCn7R5sg8loLagrlzE9YOMkNxqTHJ
HY3V4lI3HFaa6TGqgowJNY2MbxNGQ3zqfepGsYe18NINnbnNDt6ZHG54uFQ65foV
hv4GRKRAZ4NGZ2mq28LVwLrVJTyQPIepurdlGLRmzSpRPcLnO81maL+48vFgle7T
H6o1sabgMWGffyfk6sUF+DxgYlk7JiKV8qVYcW3THAZhPuU1UBULB/fCZHnNjNEI
in5vTyIjEaraWyCpawc2OED0MjXghU1bwi8VU630hNE9DdnpBYMhq2TXKC52KM/n
SHIokmIIPk4mpif8S6RBZZGwUS5B28PgAoRfteEk5ce0P+CeewWnMppZFl7lMkSk
aHxl6e7UU7jqjtvotOE5TAubGMG0vn4oWnPx0k3PTtMMFRuSRj5gVvyH5D0y0BbL
90+0z3sS709purkQ+5q+zx11l1sMewFHTxcuVOyjMZ6aM2jd3SFi2MDfB/kPeyqd
mNXfG6pu
=E4BX
-----END PGP SIGNATURE-----
|
ACK a50e34c - Only changes since my last review were a rebase and some nits. Reviewed using |
All uses of CBlockHeaderAndShortTxIDs in the product code are constructed with fUseWTXID=true, so remove the parameter. There is one use of the CBlockHeaderAndShortTxIDs constructor with fUseWTXID=false in the unit tests. This is used to construct a CBlockHeaderAndShortTxIDs for a block with only the coinbase transaction, so setting fUseWTXID to true or false makes no difference. Suggested in bitcoin#20799 (review)
The only place that segwit=True is for a block that contains only the coinbase transaction. Since the witness commitment is optional if none of the transactions have a witness, we can leave it out. This doesn't change the test coverage, which is testing p2p compact block logic. Suggested in bitcoin#20799 (comment)
|
Review suggestions implemented in #25147 |
All uses of CBlockHeaderAndShortTxIDs in the product code are constructed with fUseWTXID=true, so remove the parameter. There is one use of the CBlockHeaderAndShortTxIDs constructor with fUseWTXID=false in the unit tests. This is used to construct a CBlockHeaderAndShortTxIDs for a block with only the coinbase transaction, so setting fUseWTXID to true or false makes no difference. Suggested in bitcoin#20799 (review)
The only place that segwit=True is for a block that contains only the coinbase transaction. Since the witness commitment is optional if none of the transactions have a witness, we can leave it out. This doesn't change the test coverage, which is testing p2p compact block logic. Suggested in bitcoin#20799 (comment)
The only place that segwit=True is for a block that contains only the coinbase transaction. Since the witness commitment is optional if none of the transactions have a witness, we can leave it out. This doesn't change the test coverage, which is testing p2p compact block logic. Suggested in bitcoin#20799 (comment)
The only place that segwit=True is for a block that contains only the coinbase transaction. Since the witness commitment is optional if none of the transactions have a witness, we can leave it out. This doesn't change the test coverage, which is testing p2p compact block logic. Suggested in bitcoin#20799 (comment)
…for v1 compact blocks) bf6526f [test] Remove segwit argument from build_block_on_tip() (John Newbery) c65bf50 Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor (John Newbery) Pull request description: This implements two of the suggestions from code reviews of PR 20799: - Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor - Remove segwit argument from build_block_on_tip() ACKs for top commit: dergoegge: Code review ACK bf6526f naumenkogs: ACK bf6526f Tree-SHA512: d553791d1364b9e655183755e829b195c9b47f59c62371dbae49d9c0f8d84fec58cf18f4dde89591672ef5658e18c9cf0206c2efd70606980f87e506bc3bd4e5
All uses of CBlockHeaderAndShortTxIDs in the product code are constructed with fUseWTXID=true, so remove the parameter. There is one use of the CBlockHeaderAndShortTxIDs constructor with fUseWTXID=false in the unit tests. This is used to construct a CBlockHeaderAndShortTxIDs for a block with only the coinbase transaction, so setting fUseWTXID to true or false makes no difference. Suggested in bitcoin#20799 (review)
The only place that segwit=True is for a block that contains only the coinbase transaction. Since the witness commitment is optional if none of the transactions have a witness, we can leave it out. This doesn't change the test coverage, which is testing p2p compact block logic. Suggested in bitcoin#20799 (comment)
…oving support …
All uses of CBlockHeaderAndShortTxIDs in the product code are constructed with fUseWTXID=true, so remove the parameter. There is one use of the CBlockHeaderAndShortTxIDs constructor with fUseWTXID=false in the unit tests. This is used to construct a CBlockHeaderAndShortTxIDs for a block with only the coinbase transaction, so setting fUseWTXID to true or false makes no difference. Suggested in bitcoin/bitcoin#20799 (review)
The only place that segwit=True is for a block that contains only the coinbase transaction. Since the witness commitment is optional if none of the transactions have a witness, we can leave it out. This doesn't change the test coverage, which is testing p2p compact block logic. Suggested in bitcoin/bitcoin#20799 (comment)
Compact blocks are used for efficient relay of blocks, either through High Bandwidth or Low Bandwidth mode. See BIP 152 for full details.
For compact block relay to work, the receiver must have a mempool containing transactions which are likely to be included in the block. The receiver uses these transactions to reconstruct the block from the short transaction ids included in the
cmpctblockmessage. Compact blocks are therefore only useful for relaying blocks at or near the tip of the block chain. For older blocks, the recipient won't have the transactions in their mempool and so would need to request them using agetblocktxnmessage. In such cases, just requesting the full block is more efficient.BIP 152 supports two versions: version 1 (without witnesses) and version 2 (with witnesses). Version 2 is required to reconstruct segwit blocks. Segwit was activated in August 2017, and providing non-witness blocks to peers is no longer useful. Since the witnesses are not included, the peer would not be able to fully validate all the consensus rules on the provided block.
Therefore, stop supporting version 1 compact blocks. Ignore
sendcmpctmessages with version=1, and don't advertise support by sendingsendcmpctwith version=1. Only sendsendcmpctto peers withNODE_WITNESS. Respond to all requests for compact blocks or blocktxns with witness-serialized blocks and transactions.