p2p, refactor: add CInv transaction message helpers; use in net processing
#19590
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
Merged
|
Code Review ACK 3faeaab - would it be an wrong to make the new calls inline? |
to simplify the code and reach less from it into the CInv class internals
3faeaab to
c251d71
Compare
|
Thanks @jonasschnelli, good idea, done. Made the new helper functions implicitly inline by defining them in the class definition. Change from last push: |
|
Thanks for reviewing, @vasild. Done in https://github.com/jonatack/bitcoin/commits/CInv-type-refactoring. Will propose separately to keep the changes small and not invalidate review. |
This was referenced Jul 28, 2020
|
Code review ACK c251d71 |
This was referenced Jul 29, 2020
|
Code review ACK c251d71 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 31, 2020
…lpers; use in net processing c251d71 p2p, refactoring: use CInv helpers in net_processing.cpp (Jon Atack) 4254cd9 p2p: add CInv transaction message helper methods (Jon Atack) Pull request description: Following the merge of wtxid relay in bitcoin#18044, this is the first of three refactoring PRs (this one, bitcoin#19610, and bitcoin#19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation: - add `CInv` transaction message helper methods, defined in the class - use the new helpers in `net_processing.cpp` to simplify the code and improve encapsulation Test coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage) `p2p_segwit`, `p2p_tx_download`, `p2p_feefilter`, and `p2p_permissions`. ACKs for top commit: fjahr: Code review ACK c251d71 laanwj: Code review ACK c251d71 vasild: ACK c251d71 theStack: Code-Review ACK c251d71 hebasto: ACK c251d71, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: ead034b3c9e438909b4c5010c570d7930e69063c114290b051b7cebfd9bd5b19f573218bebe8a521256d32e830797f997adad3d85b4539c64ac5762b698e656d
|
Thanks to everyone for reviewing. The follow-up #19610 which makes the same change for INV block messages, is rebased and ready with tests green, if you're inclined to have a look. |
laanwj
added a commit
that referenced
this pull request
Sep 2, 2020
fb56d37 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery) aa36213 test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack) 24ee4f0 p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack) b1c8554 p2p: use CInv block message helpers in net_processing.cpp (Jon Atack) acd6642 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery) 5fdfb80 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery) 430e183 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery) 42ca561 [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery) 39f1dc9 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack) 471714e p2p: add CInv block message helper methods (Jon Atack) Pull request description: Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code. Some of the diffs are best reviewed with `-w` to ignore spacing. Co-authored by John Newbery. ACKs for top commit: laanwj: Code review ACK fb56d37 jnewbery: utACK fb56d37 vasild: ACK fb56d37 Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
laanwj
added a commit
that referenced
this pull request
Sep 3, 2020
…UBSan warning 7984c39 test framework: serialize/deserialize inv type as unsigned int (Jon Atack) 407175e p2p: change CInv::type from int to uint32_t (Jon Atack) Pull request description: Fixes UBSan implicit-integer-sign-change issue per #19610 (comment). Credit to Crypt-iQ for finding and reporting the issue and to vasild for the original review suggestion in #19590 (review). Closes #19678. ACKs for top commit: laanwj: ACK 7984c39 MarcoFalke: ACK 7984c39 🌻 vasild: ACK 7984c39 Tree-SHA512: 59f3a75f40ce066ca6f0bb1927197254238302b4073af1574bdbfe6ed580876437be804be4e47d51467d604f0d9e3a5875159f7f2edbb2351fdb2bb9465100b5
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 3, 2020
…processing fb56d37 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery) aa36213 test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack) 24ee4f0 p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack) b1c8554 p2p: use CInv block message helpers in net_processing.cpp (Jon Atack) acd6642 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery) 5fdfb80 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery) 430e183 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery) 42ca561 [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery) 39f1dc9 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack) 471714e p2p: add CInv block message helper methods (Jon Atack) Pull request description: Building on bitcoin#19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code. Some of the diffs are best reviewed with `-w` to ignore spacing. Co-authored by John Newbery. ACKs for top commit: laanwj: Code review ACK fb56d37 jnewbery: utACK fb56d37 vasild: ACK fb56d37 Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 3, 2020
…`, fix UBSan warning 7984c39 test framework: serialize/deserialize inv type as unsigned int (Jon Atack) 407175e p2p: change CInv::type from int to uint32_t (Jon Atack) Pull request description: Fixes UBSan implicit-integer-sign-change issue per bitcoin#19610 (comment). Credit to Crypt-iQ for finding and reporting the issue and to vasild for the original review suggestion in bitcoin#19590 (review). Closes bitcoin#19678. ACKs for top commit: laanwj: ACK 7984c39 MarcoFalke: ACK 7984c39 🌻 vasild: ACK 7984c39 Tree-SHA512: 59f3a75f40ce066ca6f0bb1927197254238302b4073af1574bdbfe6ed580876437be804be4e47d51467d604f0d9e3a5875159f7f2edbb2351fdb2bb9465100b5
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
May 12, 2021
…ssing Summary: Our code differs from core on this part. In order to kept the changes minimal and limit conflicts I kept our implementation but renamed the function to match core. Backport of [[bitcoin/bitcoin#19590 | core#19590]]. Test Plan: ninja all check-all ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9500
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following the merge of wtxid relay in #18044, this is the first of three refactoring PRs (this one, #19610, and #19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation:
add
CInvtransaction message helper methods, defined in the classuse the new helpers in
net_processing.cppto simplify the code and improve encapsulationTest coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage)
p2p_segwit,p2p_tx_download,p2p_feefilter, andp2p_permissions.