p2p, refactor: add `CInv` transaction message helpers; use in net processing by jonatack · Pull Request #19590 · bitcoin/bitcoin

jonatack · 2020-07-26T16:16:07Z

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 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.

DrahtBot · 2020-07-26T18:31:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

[RFC] Package-relay: sender-initiated #19621 ([RFC] Package-relay: sender-initiated by ariard)
Enable fetching of orphan parents from wtxid peers #19569 (Enable fetching of orphan parents from wtxid peers by sipa)
Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766 (Disable fee estimation in blocksonly mode by darosior)

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.

jonasschnelli · 2020-07-27T07:36:38Z

Code Review ACK 3faeaab673ad3fd0f9c80407b0e625a1fa5ff4ae - 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

jonatack · 2020-07-27T09:25:02Z

Thanks @jonasschnelli, good idea, done. Made the new helper functions implicitly inline by defining them in the class definition. Change from last push: git diff 3faeaab c251d710

vasild

ACK c251d71

Would it be possible to also handle the rest of MSG_* values and make CInv::type private?
Would it make sense to change CInv::type from int to enum GetDataMsg? Or maybe to uint32_t (ac88e2e has changed enum GetDataMsg from int to uint32_t)?

jonatack · 2020-07-28T10:06:30Z

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.

theStack

Code-Review ACK c251d71

hebasto

ACK c251d71, I have reviewed the code and it looks OK, I agree it can be merged.

fjahr · 2020-07-29T12:40:17Z

Code review ACK c251d71

laanwj · 2020-07-30T14:07:45Z

Code review ACK c251d71

…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

jonatack · 2020-07-31T07:37:53Z

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.

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

…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

…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

…`, 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

…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

DrahtBot mentioned this pull request Jul 26, 2020

Remove mempool global #19556

Merged

DrahtBot added the P2P label Jul 26, 2020

jonatack added 2 commits July 27, 2020 11:06

p2p: add CInv transaction message helper methods

4254cd9

p2p, refactoring: use CInv helpers in net_processing.cpp

c251d71

to simplify the code and reach less from it into the CInv class internals

jonatack force-pushed the CInv-transaction-message-helpers branch from 3faeaab to c251d71 Compare July 27, 2020 09:25

vasild approved these changes Jul 27, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 27, 2020

Enable fetching of orphan parents from wtxid peers #19569

Merged

This was referenced Jul 28, 2020

p2p: refactor AlreadyHave(), CInv::type, INV/TX processing #19610

Merged

p2p: change CInv::type from int to uint32_t, fix UBSan warning #19611

Closed

theStack approved these changes Jul 28, 2020

View reviewed changes

hebasto approved these changes Jul 29, 2020

View reviewed changes

This was referenced Jul 29, 2020

[RFC] Package-relay: sender-initiated #19621

Closed

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Merged

laanwj merged commit 17de75b into bitcoin:master Jul 30, 2020

jonatack deleted the CInv-transaction-message-helpers branch July 30, 2020 14:21

jonatack mentioned this pull request Aug 27, 2020

p2p: change CInv::type from int to uint32_t, fix UBSan warning #19818

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p, refactor: add `CInv` transaction message helpers; use in net processing#19590

p2p, refactor: add `CInv` transaction message helpers; use in net processing#19590
laanwj merged 2 commits intobitcoin:masterfrom
jonatack:CInv-transaction-message-helpers

jonatack commented Jul 26, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Jul 26, 2020 •

edited

Loading

Uh oh!

jonasschnelli commented Jul 27, 2020

Uh oh!

jonatack commented Jul 27, 2020 •

edited

Loading

Uh oh!

vasild left a comment

Uh oh!

jonatack commented Jul 28, 2020

Uh oh!

theStack left a comment

Uh oh!

hebasto left a comment

Uh oh!

fjahr commented Jul 29, 2020

Uh oh!

laanwj commented Jul 30, 2020

Uh oh!

jonatack commented Jul 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

jonatack commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented Jul 27, 2020

Uh oh!

jonatack commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jul 28, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jul 29, 2020

Uh oh!

laanwj commented Jul 30, 2020

Uh oh!

jonatack commented Jul 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonatack commented Jul 26, 2020 •

edited

Loading

DrahtBot commented Jul 26, 2020 •

edited

Loading

jonatack commented Jul 27, 2020 •

edited

Loading