-
Notifications
You must be signed in to change notification settings - Fork 38.6k
Net processing: follow ups to #20799 (removing support for v1 compact blocks) #25147
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
Conversation
maflcko
left a comment
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
|
Concept ACK. |
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)
3ab9f69 to
97e4a5d
Compare
|
Thanks for the review @MarcoFalke and @naumenkogs. I've addressed all your comments. |
maflcko
left a comment
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.
still lgtm
97e4a5d to
8184dcb
Compare
|
ACK 8184dcbdd18d8b732dc285a05884ecda115062e1 |
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)
8184dcb to
bf6526f
Compare
|
Code review ACK bf6526f |
|
ACK bf6526f |
…oving support …
This implements two of the suggestions from code reviews of PR 20799: