Net Processing: Move RelayTransaction() into PeerManager #21162

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:2021-02-relay-transactions-peer-manager changing 3 files +17 −15

jnewbery commented at 1:12 PM on February 12, 2021: member

This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.
jnewbery cross-referenced this on Feb 12, 2021 from issue Move remaining application layer data to net processing by jnewbery
fanquake added the label P2P on Feb 12, 2021
DrahtBot commented at 10:37 PM on February 12, 2021: contributor



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



Conflicts

No conflicts as of last run.
DrahtBot cross-referenced this on Feb 12, 2021 from issue [refactor] Move some net_processing globals into PeerManagerImpl by ajtowns
DrahtBot cross-referenced this on Feb 12, 2021 from issue RFC: Move Peer and PeerManagerImpl declarations to separate header by jnewbery
DrahtBot cross-referenced this on Feb 12, 2021 from issue net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers by MarcoFalke
DrahtBot cross-referenced this on Feb 13, 2021 from issue test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD by ariard
DrahtBot cross-referenced this on Feb 13, 2021 from issue net: make CNode::m_inbound_onion public, initialize explicitly by jonatack
DrahtBot cross-referenced this on Feb 14, 2021 from issue Erlay: bandwidth-efficient transaction relay protocol by naumenkogs
bitcoin deleted a comment on Feb 14, 2021
DrahtBot cross-referenced this on Feb 15, 2021 from issue Make all of net_processing (and some of net) use std::chrono types by dhruv
DrahtBot cross-referenced this on Feb 15, 2021 from issue Split orphan handling from net_processing into txorphanage by ajtowns
jnewbery commented at 1:30 PM on February 16, 2021: member

Rebased. This is now ready for review.
jnewbery force-pushed on Feb 16, 2021
jnewbery marked this as ready for review on Feb 16, 2021
jnewbery marked this as a draft on Feb 17, 2021
jnewbery marked this as ready for review on Feb 17, 2021
jnewbery force-pushed on Feb 17, 2021
jnewbery marked this as a draft on Feb 17, 2021
DrahtBot cross-referenced this on Feb 18, 2021 from issue net/net processing: Move addr data into net_processing by jnewbery
DrahtBot cross-referenced this on Feb 18, 2021 from issue net/net processing: Move tx inventory into net_processing by jnewbery
jnewbery force-pushed on Feb 18, 2021
jnewbery commented at 9:25 AM on February 18, 2021: member

I've removed the last two commits in this PR, so this is now just moving RelayTransaction() into PeerManagerImp. The final two commits are difficult to separate from the rest of the "Move tx inventory into net_processing" change (#21160) without ugly lock taking/releasing.
jnewbery marked this as ready for review on Feb 18, 2021
jnewbery force-pushed on Feb 19, 2021
ajtowns commented at 1:08 PM on March 3, 2021: contributor

Concept ACK, but ignoring for now since drahtbot says it conflicts with txorphanage
DrahtBot added the label Needs rebase on Mar 4, 2021

[net processing] Move RelayTransaction into PeerManager

We don't mark RelayTransaction as const. Even though it doesn't mutate
PeerManagerImpl state, it _is_ mutating the internal state of a CNode
object, by updating setInventoryTxToSend. In a subsequent commit, that
field will be moved to the Peer object, which is owned by
PeerMangerImpl.

This requires PeerManagerImpl::ReattemptInitialBroadcast() to no longer
be const.

a38a4e8f03

[net processing] Don't pass CConnman to RelayTransactions
Use the local m_connman instead
680eb56d82
jnewbery force-pushed on Mar 4, 2021
jnewbery commented at 10:24 AM on March 4, 2021: member

Rebased

Concept ACK, but ignoring for now since drahtbot says it conflicts with txorphanage @ajtowns txorphange merged. This is now ready for review :)
in src/net_processing.h:51 in a38a4e8f03 outdated
```
  46 | @@ -47,6 +47,10 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
  47 |      /** Whether this node ignores txs received over p2p. */
  48 |      virtual bool IgnoresIncomingTxs() = 0;
  49 |  
  50 | +    /** Relay transaction to all peers. */
  51 | +    virtual void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman)
```
ajtowns commented at 11:59 AM on March 4, 2021:

Hmm, I think this should be marked as const ? Even after setInventoryTxToSend is moved into Peer, updating that doesn't change PeerManagerImpl, only the contents of an object which PeerManagerImpl has a shared_ptr to. For better or worse const on a class only protects its pointers from changes, it doesn't protect the things pointed to from changing.

jnewbery commented at 5:27 PM on March 4, 2021:

More discussion here: #21236 (review)
ajtowns commented at 12:03 PM on March 4, 2021: contributor

Approach ACK; not convinced about the non-const bit.
DrahtBot removed the label Needs rebase on Mar 4, 2021
jnewbery cross-referenced this on Mar 4, 2021 from issue net processing: Extract `addr` send functionality into MaybeSendAddr() by jnewbery
DrahtBot commented at 4:50 PM on March 15, 2021: contributor



🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.
jnewbery added this to the "Blockers" column in a project

ajtowns commented at 5:01 AM on March 18, 2021: contributor

ACK 680eb56d828ce358b4e000c140f5b247ff5e6179
fanquake merged this on Mar 18, 2021
fanquake closed this on Mar 18, 2021
jnewbery deleted the branch on Mar 18, 2021
jnewbery removed this from the "Blockers" column in a project

sidhujag referenced this in commit 115c5127f3 on Mar 18, 2021
laanwj cross-referenced this on Mar 24, 2021 from issue [p2p] Introduce node rebroadcast module by amitiuttarwar
JaredTate cross-referenced this on Oct 30, 2021 from issue WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code by JaredTate
jsarenik cross-referenced this on Mar 29, 2022 from issue tx inventory: Accomodate RPC change for Bitcoin Core 24 by jsarenik
bitcoin locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#19398 Move remaining application layer data to net processing #20721 Net: Move ping data to net_processing #21061 [p2p] Introduce node rebroadcast module #21160 net/net processing: Move tx inventory into net_processing #21236 net processing: Extract `addr` send functionality into MaybeSendAddr()

Net Processing: Move RelayTransaction() into PeerManager #21162

Conflicts