net/net processing: Move tx inventory into net_processing #21160

jnewbery · 2021-02-12T13:02:58Z

This continues the work of moving application layer data into net_processing, by moving all tx data into the new Peer object added in #19607.

For motivation, see #19398.

DrahtBot · 2021-02-12T22:39:57Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
#24543 (net processing: Move remaining globals into PeerManagerImpl by dergoegge)
#24531 (Use designated initializers by MarcoFalke)
#24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
#24220 (validation: don't re-acquire cs_main during IBD in CChainState::IsInitialBlockDownload() by jonatack)
#24125 (p2p: Replace RecursiveMutex cs_tx_inventory with Mutex and rename it by w0xlt)
#21527 (net_processing: lock clean up by ajtowns)
#20726 (p2p: Add DISABLETX message for negotiating block-relay-only connections by sdaftuar)

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.

jnewbery · 2021-02-22T08:48:27Z

Rebased on master to pick up fix for interface_zmq.py in #21008.

jnewbery · 2021-02-22T19:10:33Z

Rebase resulted in silent merge conflict with 9476886. I'll fix tomorrow.

glozow

utACK 1066d10

jnewbery · 2022-03-25T17:55:37Z

There are still a few outstanding review comments that could be responded to or addressed in a follow-up. Leaving this here mostly as a reminder to myself:

net/net processing: Move tx inventory into net_processing #21160 (comment) "Same comment about lock scope here (and the hunk below as well)"
net/net processing: Move tx inventory into net_processing #21160 (comment) "This is the only place where these values are read outside of a lock which keeps their writes in sync. I suppose these rarely being out of sync is of no consequence?"
net/net processing: Move tx inventory into net_processing #21160 (comment) "Why not const Peer& peer here (and for MaybeSendFeefilter), since it's used read-only?"
net/net processing: Move tx inventory into net_processing #21160 (comment) "Why not make this a member function of Peer to avoid having to peek at the internal lock?"
net/net processing: Move tx inventory into net_processing #21160 (comment) "Afaics, all this needs is a bool for peer.m_tx_relay != nullptr, otherwise Peer is unused here. Maybe just pass the bool instead of requiring all of Peer?"
net/net processing: Move tx inventory into net_processing #21160 (comment) "Likewise, this doesn't need all of Peer, only a Peer::TxRelay."
net/net processing: Move tx inventory into net_processing #21160 (comment) "See comment above about changing the PushNodeVersion params, which would eliminate the need for the copies here."
net/net processing: Move tx inventory into net_processing #21160 (comment) "Instead of removing, I think you can do assert(peerman.getpeer(id).frelay==filter)"

CNode::m_relays_tx and CNode::m_bloom_filter_loaded access don't require the Peer::TxRelay::m_bloom_filter_mutex lock, so move them out of the lock scope. See bitcoin#21160 (comment) and bitcoin#21160 (comment).

See bitcoin#21160 (comment)

The Peer reference was only needed to check whether the peer was a blocks relay only peer, so instead just pass in a boolean. See bitcoin#21160 (comment).

MaybeSendFeeFilter() doesn't need acess to the entire peer object, so only pass a pointer to the TxFilter struct. See bitcoin#21160 (comment).

jnewbery · 2022-03-28T08:23:32Z

There are still a few outstanding review comments that could be responded to or addressed in a follow-up. Leaving this here mostly as a reminder to myself:

I believe I've now addressed all outstanding review comments on this PR, either here or in #24692.

maflcko · 2022-03-28T08:32:09Z

See also #24691

The Peer reference was only needed to check whether the peer was a blocks relay only peer, so instead just pass in a boolean. See bitcoin#21160 (comment).

MaybeSendFeeFilter() doesn't need acess to the entire peer object, so only pass a pointer to the TxFilter struct. See bitcoin#21160 (comment).

a40978d [fuzz] Assert that Peer.m_tx_relay.m_relay_txs has been set correctly (John Newbery) 0bca5f2 [net processing] PushNodeVersion() takes a const Peer& (John Newbery) 21154ff net_processing: move CNode data access out of lock (John Newbery) Pull request description: #21160 ([net/net processing]: Move tx inventory into net_processing) had some unaddressed review comments when it was merged. This branch addresses those comments. ACKs for top commit: MarcoFalke: review ACK a40978d dergoegge: ACK a40978d ajtowns: ACK a40978d Tree-SHA512: 46624e275f918c5f32d0adab0766e9b3ef8ebdbc74a3c8886d8a2e2ff1079029dcc371b40ef0d787609e9c05219b7456f3e2dfe4fb0cb7bf23ef966769aef1a1

…rocessing

…oin#21160 a40978d [fuzz] Assert that Peer.m_tx_relay.m_relay_txs has been set correctly (John Newbery) 0bca5f2 [net processing] PushNodeVersion() takes a const Peer& (John Newbery) 21154ff net_processing: move CNode data access out of lock (John Newbery) Pull request description: bitcoin#21160 ([net/net processing]: Move tx inventory into net_processing) had some unaddressed review comments when it was merged. This branch addresses those comments. ACKs for top commit: MarcoFalke: review ACK a40978d dergoegge: ACK a40978d ajtowns: ACK a40978d Tree-SHA512: 46624e275f918c5f32d0adab0766e9b3ef8ebdbc74a3c8886d8a2e2ff1079029dcc371b40ef0d787609e9c05219b7456f3e2dfe4fb0cb7bf23ef966769aef1a1

jonatack · 2022-05-20T12:38:41Z

src/net_processing.cpp

+    } else {
+        stats.m_relay_txs = false;
+        stats.m_fee_filter_received = 0;
+    }


Moving fRelayTxes/m_relay_txs from CNodeStats to CNodeStateStats broke -netinfo (and perhaps other clients that expect this field to be non-null). Proposing a fix.

Moving fRelayTxes/m_relay_txs from CNodeStats to CNodeStateStats broke -netinfo (and perhaps other clients that expected this field to be non-null). Proposing a fix.

Fixed in #25176.

…etpeerinfo#relaytxes a17c5e9 Rename NetinfoRequestHandler::is_block_relay data member to is_tx_relay (Jon Atack) f0bb7db Fix frequent -netinfo JSON errors from null getpeerinfo#relaytxes (Jon Atack) Pull request description: CLI -netinfo frequently returns "error: JSON value is not a boolean as expected" since the merge of bitcoin#21160, which moved fRelayTxes (renamed to m_relay_txs in that pull) from CNodeStats to CNodeStateStats. This change made getpeerinfo "relaytxes" an optional field that can return UniValue IsNull(). It is the only optional field consumed by -netinfo where the latter didn't already handle that case. See also bitcoin#24691. Also rename the NetinfoRequestHandler::is_block_relay data member to is_tx_relay and inverse its boolean logic. The naming is out of date and incorrect, as lack of request of tx relay does not imply block relay, and a preference for tx relay doesn't imply that block relay isn't happening. Thanks to Marco Falke and Martin Zumsande for their feedback on this. (I may look at reducing the number of optional node stats fields via refactoring at the net processing level, but ongoing refactoring there may make that slow or complicated and this is a one-line fix that works now.) ACKs for top commit: mzumsande: Code review ACK a17c5e9 Tree-SHA512: dc54ce80b78122874a6794555f99e5b328a1574b52bb3e7f974c699c2b759a60ea0807a6483c5bc0414a950d853c0eeeb13dcc1b790d3917c6ee4c9c99fe159f

CNode::m_relays_tx and CNode::m_bloom_filter_loaded access don't require the Peer::TxRelay::m_bloom_filter_mutex lock, so move them out of the lock scope. See bitcoin/bitcoin#21160 (comment) and bitcoin/bitcoin#21160 (comment).

…or fully connected peers 8f2dac5 [test] Add p2p_tx_privacy.py (dergoegge) ce63fca [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge) 845e3a3 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge) Pull request description: `TxRelay::m_next_inv_send_time` is initialized to 0, which means that any txids in `TxRelay::m_tx_inventory_to_send` will be announced on the first call to `PeerManagerImpl::SendMessages` for a fully connected peer (i.e. it completed the version handshake). Prior to #21160, `TxRelay::m_tx_inventory_to_send` was guaranteed to be empty on the first `SendMessages` call, as transaction announcements were only queued for fully connected peers. #21160 replaced a `CConnman::ForEachNode` call with a loop over `PeerManagerImpl::m_peer_map`, in which the txid for a transaction to be relayed is added to `TxRelay::m_tx_inventory_to_send` for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as `ForEachNode` has a "fully connected check" before calling a function for each node. ACKs for top commit: MarcoFalke: ACK 8f2dac5 🔝 jnewbery: utACK 8f2dac5 Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9

CNode::m_relays_tx and CNode::m_bloom_filter_loaded access don't require the Peer::TxRelay::m_bloom_filter_mutex lock, so move them out of the lock scope. See bitcoin/bitcoin#21160 (comment) and bitcoin/bitcoin#21160 (comment).

fanquake added the P2P label Feb 12, 2021

jnewbery mentioned this pull request Feb 12, 2021

Move remaining application layer data to net processing #19398

Closed

16 tasks

jnewbery force-pushed the 2021-02-tx-in-peer branch from c9e92b3 to a706cbe Compare February 12, 2021 13:10

jnewbery mentioned this pull request Feb 12, 2021

Net Processing: Move RelayTransaction() into PeerManager #21162

Merged

khwrylykhwr approved these changes Feb 14, 2021

View reviewed changes

This was referenced Feb 15, 2021

Make all of net_processing (and some of net) use std::chrono types #21015

Merged

Split orphan handling from net_processing into txorphanage #21148

Merged

jnewbery force-pushed the 2021-02-tx-in-peer branch from a706cbe to b3b2714 Compare February 17, 2021 20:07

DrahtBot mentioned this pull request Feb 18, 2021

net/net processing: Move addr data into net_processing #21186

Merged

jnewbery force-pushed the 2021-02-tx-in-peer branch from b3b2714 to 4290bac Compare February 18, 2021 09:23

jnewbery force-pushed the 2021-02-tx-in-peer branch from 4290bac to 0164ae2 Compare February 22, 2021 08:48

jnewbery force-pushed the 2021-02-tx-in-peer branch from 0164ae2 to bbbdb04 Compare February 22, 2021 09:17

DrahtBot mentioned this pull request Feb 23, 2021

p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool #21235

Merged

glozow reviewed Mar 25, 2022

View reviewed changes

fanquake merged commit 9344697 into bitcoin:master Mar 25, 2022

jnewbery deleted the 2021-02-tx-in-peer branch March 25, 2022 17:55

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Mar 28, 2022

net_processing: PushNodeVersion takes a const Peer&

5011edc

See bitcoin#21160 (comment)

jnewbery mentioned this pull request Mar 28, 2022

refactoring: [Net Processing] Follow-ups to #21160 #24692

Merged

jsarenik mentioned this pull request Mar 29, 2022

tx inventory: Accomodate RPC change for Bitcoin Core 24 janoside/btc-rpc-explorer#431

Closed

jnewbery mentioned this pull request Mar 30, 2022

p2p: Add 2 outbound block-relay-only connections #15759

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 2, 2022

Merge bitcoin#21160: net/net processing: Move tx inventory into net_p…

81df78d

…rocessing

jonatack reviewed May 20, 2022

View reviewed changes

jonatack mentioned this pull request May 20, 2022

Fix frequent -netinfo JSON errors from missing getpeerinfo#relaytxes #25176

Merged

jonatack mentioned this pull request Aug 24, 2022

p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates #25923

Closed

fanquake mentioned this pull request Sep 22, 2022

rpc, doc: getpeerinfo updates #26109

Merged

dergoegge mentioned this pull request Nov 24, 2022

p2p: Ensure transaction announcements are only queued for fully connected peers #26569

Merged

ajtowns mentioned this pull request Jan 23, 2023

refactor: Continue moving application data from CNode to Peer #26621

Closed

bitcoin locked and limited conversation to collaborators Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

net/net processing: Move tx inventory into net_processing #21160

net/net processing: Move tx inventory into net_processing #21160

Uh oh!

jnewbery commented Feb 12, 2021 •

edited

Loading

Uh oh!

DrahtBot commented Feb 12, 2021 •

edited

Loading

Uh oh!

jnewbery commented Feb 22, 2021

Uh oh!

jnewbery commented Feb 22, 2021

Uh oh!

glozow left a comment

Uh oh!

jnewbery commented Mar 25, 2022 •

edited

Loading

Uh oh!

jnewbery commented Mar 28, 2022

Uh oh!

maflcko commented Mar 28, 2022

Uh oh!

jonatack May 20, 2022 •

edited

Loading

Uh oh!

jonatack May 20, 2022

Uh oh!

Uh oh!

net/net processing: Move tx inventory into net_processing #21160

net/net processing: Move tx inventory into net_processing #21160

Uh oh!

Conversation

jnewbery commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Feb 22, 2021

Uh oh!

jnewbery commented Feb 22, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Mar 28, 2022

Uh oh!

maflcko commented Mar 28, 2022

Uh oh!

jonatack May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack May 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Feb 12, 2021 •

edited

Loading

DrahtBot commented Feb 12, 2021 •

edited

Loading

jnewbery commented Mar 25, 2022 •

edited

Loading

jonatack May 20, 2022 •

edited

Loading