net/net processing: Move tx inventory into net_processing by jnewbery · Pull Request #21160 · bitcoin/bitcoin

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.

We'll move the transaction relay data into Peer in subsequent commits, but the inbound eviction logic needs to know if the peer is relaying txs and if the peer has loaded a bloom filter. This is currently redundant information with m_tx_relay->fRelayTxes, but when m_tx_relay is moved into net_processing, then we'll need these separate fields in CNode.

Also, remove cs_main guard from m_wtxid_relay_peers and make it atomic. This should be fine since we don't need m_wtxid_relay_peers to be synchronized with m_wtxid_relay exactly at all times. After this change, RelayTransaction no longer requires cs_main.

-BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren cs_filter m_bloom_filter_mutex ren fRelayTxes m_relay_txs ren pfilter m_bloom_filter ren cs_tx_inventory m_tx_inventory_mutex ren filterInventoryKnown m_tx_inventory_known_filter ren setInventoryTxToSend m_tx_inventory_to_send ren fSendMempool m_send_mempool ren nNextInvSend m_next_inv_send_time ren minFeeFilter m_fee_filter_received ren lastSentFeeFilter m_fee_filter_sent -END VERIFY SCRIPT-

jnewbery · 2022-03-18T11:46:56Z

Rebased

dergoegge

ACK 1066d10 - This is a good layer separation improvement with no behavior changes.

Are you still planning to address Cory's comments?

dergoegge · 2022-03-24T08:26:44Z

src/net_processing.cpp


    /** Send a version message to a peer */
-    void PushNodeVersion(CNode& pnode);
+    void PushNodeVersion(CNode& pnode, Peer& peer);


nit:

Suggested change

void PushNodeVersion(CNode& pnode, Peer& peer);

void PushNodeVersion(CNode& pnode, const Peer& peer);

Or like cory suggested this could also just be a bool.

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)"

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

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.

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

This was referenced Feb 5, 2022

p2p: Split network logging into two categories #24247 #24273

Closed

validation: don't re-acquire cs_main during IBD in CChainState::IsInitialBlockDownload() #24220

Closed

DrahtBot mentioned this pull request Feb 24, 2022

refactor: Release cs_main before MaybeSendFeefilter #24427

Merged

DrahtBot mentioned this pull request Mar 4, 2022

Additional thread safety annotations for CNode/Peer members accessed via the message processing thread #24474

Closed

DrahtBot added the Needs rebase label Mar 7, 2022

jnewbery mentioned this pull request Mar 17, 2022

net processing: Move remaining globals into PeerManagerImpl #24543

Merged

jnewbery added 4 commits March 18, 2022 11:21

[net processing] Move tx relay data to Peer

575bbd0

jnewbery force-pushed the 2021-02-tx-in-peer branch from 161eba6 to 1066d10 Compare March 18, 2022 11:47

DrahtBot removed the Needs rebase label Mar 18, 2022

This was referenced Mar 19, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

Use designated initializers #24531

Merged

dergoegge reviewed Mar 24, 2022

View reviewed changes

glozow reviewed Mar 25, 2022

View reviewed changes

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

net/net processing: Move tx inventory into net_processing#21160
fanquake merged 4 commits intobitcoin:masterfrom
jnewbery:2021-02-tx-in-peer

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!

jnewbery commented Mar 18, 2022

Uh oh!

dergoegge left a comment

Uh oh!

dergoegge Mar 24, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

	void PushNodeVersion(CNode& pnode, Peer& peer);
	void PushNodeVersion(CNode& pnode, const Peer& peer);

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!

jnewbery commented Mar 18, 2022

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge Mar 24, 2022

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

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