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

jonatack · 2020-07-28T11:01:45Z

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.

DrahtBot · 2020-07-28T17:03:10Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: don't add AlreadyHave transactions to recentRejects #19753 (p2p: don't add AlreadyHave transactions to recentRejects by troygiorshev)
validation: re-delegate absurd fee checking from mempool to clients #19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
Erlay: bandwidth-efficient transaction relay protocol #18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

…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 #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`. 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

…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

fjahr

Code review ACK 82c9297

src/protocol.h

jnewbery · 2020-08-03T16:09:24Z

I think a better approach than the changes within AlreadyHave() is to split that function up into AlreadyHaveTx() and AlreadyHaveBlock() since those are two totally separate concepts. @jonatack would you be interested in taking the commits from here: https://github.com/jnewbery/bitcoin/commits/2020-07-split-already-have? (see also #19569 (comment))

jnewbery

Concept ACK. Needs rebase.

src/protocol.h

jonatack · 2020-08-08T14:12:17Z

Rebased with four commits pulled in from @jnewbery's master...jnewbery:2020-07-split-already-have branch as well as the changes in #19611.

laanwj · 2020-08-09T18:49:12Z

I think a better approach than the changes within AlreadyHave() is to split that function up into AlreadyHaveTx() and AlreadyHaveBlock() since those are two totally separate concepts.

Great idea. The block and transaction paths have drifted apart so much that it no longer makes sense to have a common function there.

src/protocol.h

and otherwise log that an unknown INV type was received. In INV processing, when handling transaction type inv messages, ToGenTxid() expects that we constructed the CInv ourselves or that we verified that it is for a transaction type CInv. Therefore, change this `else` branch into an `else if (inv.GenMsgTx())` to make this safer and log any INVs that fall through.

jonatack · 2020-08-26T10:07:34Z

Removed the unused helper methods from the first commit per git diff 533a9a0 125468a

vasild

ACK 125468a

jnewbery · 2020-08-26T15:32:31Z

I think the CInv::type change from int to uint32_t change should be added back (not necessarily the private qualifier). While fuzzing it brings up the following complaint #19678

Why? This seems unrelated to this PR (the UBSan warning happens before this PR). Changing the type of CInv.type (which is serialized/deserialized for network messages) is a very different change from the internal refactors in this PR. Can you save it for a different PR?

Crypt-iQ · 2020-08-26T16:18:42Z

@jnewbery I thought since it was already added previously, it would fit into the scope of the PR. No problem to be added into another PR, either I or somebody else could do that.

jonatack · 2020-08-27T09:14:40Z

Dropped the CInv::type type change commit in favor of #19818. Thanks everyone.

laanwj · 2020-08-27T13:10:46Z

Code review ACK fb56d37
Agree it's better to split out the P2P change.

jnewbery · 2020-09-01T08:50:26Z

utACK fb56d37

vasild

ACK fb56d37

…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

Summary: Partial backport of [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@471714e Depends on D9500. Test Plan: ninja all check ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9501

…ions Summary: Partial backport of [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@42ca561?diff=split&w=1 This will help narrowing the cs_main scope when introducing the proofs. There are more simplifications and de-duplication to come in follow-up backports. Depends on D9501. Note that the commit bitcoin/bitcoin@39f1dc9 is skipped because it is a segwit only commit. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9502

Summary: Partial backport of [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@430e183 Depends on D9502. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9503

Summary: Partial backport of [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@5fdfb80 Depends on D9503. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9504

Summary: Partial backport if [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@acd6642 Depends on D9504. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9505

Summary: Partial backport of [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@b1c8554 Depends on D9505. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9506

Summary: Partial backport of [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@24ee4f0 There is very few remaining when removed the segwit content. Depends on D9506. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9507

Summary: ``` and otherwise log that an unknown INV type was received. In INV processing, when handling transaction type inv messages, ToGenTxid() expects that we constructed the CInv ourselves or that we verified that it is for a transaction type CInv. Therefore, change this `else` branch into an `else if (inv.GenMsgTx())` to make this safer and log any INVs that fall through. ``` Completes backport of [[bitcoin/bitcoin#19610 | core#19610]]: bitcoin/bitcoin@fb56d37 Depends on D9507. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9508

This was referenced Jul 28, 2020

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

Closed

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

Merged

DrahtBot added the P2P label Jul 28, 2020

This was referenced Jul 28, 2020

Enable fetching of orphan parents from wtxid peers #19569

Merged

Remove mempool global #19556

Merged

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

Closed

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

Merged

jonatack force-pushed the CInv-block-message-helpers branch from 66398c1 to 82c9297 Compare July 30, 2020 15:50

DrahtBot added the Needs rebase label Jul 31, 2020

fjahr reviewed Aug 1, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

jnewbery reviewed Aug 4, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

jonatack changed the title ~~p2p, refactor: add CInv block message helpers; use in net processing~~ p2p: refactor AlreadyHave(), CInv::type and inv/tx processing Aug 8, 2020

jonatack changed the title ~~p2p: refactor AlreadyHave(), CInv::type and inv/tx processing~~ p2p: refactor AlreadyHave(), CInv::type, INV/TX processing Aug 8, 2020

jonatack force-pushed the CInv-block-message-helpers branch 2 times, most recently from bbd5638 to 4789cff Compare August 8, 2020 14:06

DrahtBot removed the Needs rebase label Aug 8, 2020

laanwj reviewed Aug 9, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

bitcoin deleted a comment Aug 10, 2020

DrahtBot added the Needs rebase label Aug 10, 2020

jonatack force-pushed the CInv-block-message-helpers branch from 4789cff to 792d5a4 Compare August 10, 2020 20:23

DrahtBot removed the Needs rebase label Aug 10, 2020

jonatack and others added 3 commits August 26, 2020 11:57

p2p: make gtxid(.hash) and fAlreadyHave localvars const

24ee4f0

test: use CInv::MSG_WITNESS_TX flag in p2p_segwit

aa36213

jonatack force-pushed the CInv-block-message-helpers branch from 533a9a0 to 125468a Compare August 26, 2020 10:06

vasild approved these changes Aug 26, 2020

View reviewed changes

jonatack force-pushed the CInv-block-message-helpers branch from 125468a to fb56d37 Compare August 27, 2020 09:14

jonatack mentioned this pull request Aug 27, 2020

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

Merged

DrahtBot mentioned this pull request Aug 27, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

vasild approved these changes Sep 2, 2020

View reviewed changes

laanwj merged commit 505b39e into bitcoin:master Sep 2, 2020

jonatack deleted the CInv-block-message-helpers branch September 2, 2020 12:51

jnewbery mentioned this pull request Nov 5, 2020

Backport wtxid orphan fetch to v0.20 #20317

Closed

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 AlreadyHave(), CInv::type, INV/TX processing #19610

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

jonatack commented Jul 28, 2020 •

edited

Loading

DrahtBot commented Jul 28, 2020 •

edited

Loading

fjahr left a comment

jnewbery commented Aug 3, 2020

jnewbery left a comment

jonatack commented Aug 8, 2020

laanwj commented Aug 9, 2020

jonatack commented Aug 26, 2020 •

edited

Loading

vasild left a comment

jnewbery commented Aug 26, 2020

Crypt-iQ commented Aug 26, 2020

jonatack commented Aug 27, 2020

laanwj commented Aug 27, 2020 •

edited

Loading

jnewbery commented Sep 1, 2020

vasild left a comment

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

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

Conversation

jonatack commented Jul 28, 2020 • edited Loading

DrahtBot commented Jul 28, 2020 • edited Loading

Conflicts

fjahr left a comment

Choose a reason for hiding this comment

jnewbery commented Aug 3, 2020

jnewbery left a comment

Choose a reason for hiding this comment

jonatack commented Aug 8, 2020

laanwj commented Aug 9, 2020

jonatack commented Aug 26, 2020 • edited Loading

vasild left a comment

Choose a reason for hiding this comment

jnewbery commented Aug 26, 2020

Crypt-iQ commented Aug 26, 2020

jonatack commented Aug 27, 2020

laanwj commented Aug 27, 2020 • edited Loading

jnewbery commented Sep 1, 2020

vasild left a comment

Choose a reason for hiding this comment

jonatack commented Jul 28, 2020 •

edited

Loading

DrahtBot commented Jul 28, 2020 •

edited

Loading

jonatack commented Aug 26, 2020 •

edited

Loading

laanwj commented Aug 27, 2020 •

edited

Loading