net/net processing: Move tx inventory into net

jnewbery commented at 1:02 pm on February 12, 2021: member

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.

fanquake added the label P2P on Feb 12, 2021

jnewbery force-pushed on Feb 12, 2021

DrahtBot commented at 10:39 pm on February 12, 2021: contributor

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.

khwrylykhwr approved

jnewbery force-pushed on Feb 17, 2021

jnewbery force-pushed on Feb 18, 2021

jnewbery force-pushed on Feb 22, 2021

jnewbery commented at 8:48 am on February 22, 2021: member

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

jnewbery force-pushed on Feb 22, 2021

jnewbery commented at 7:10 pm on February 22, 2021: member

Rebase resulted in silent merge conflict with 9476886353dffb730dcb75799f2bd5e143425795. I’ll fix tomorrow.

jnewbery force-pushed on Feb 26, 2021

fanquake referenced this in commit e057e01b7b on Mar 18, 2021

jnewbery force-pushed on Mar 18, 2021

jnewbery marked this as ready for review on Mar 18, 2021

jnewbery force-pushed on Mar 18, 2021

jnewbery commented at 11:06 am on March 18, 2021: member

I’ve rebased this and moved it out of draft. It’s now ready for review.

in src/net.h:552 in 0fb9ffba48 outdated

572-        /** Minimum fee rate with which to filter inv's to this node */
573-        std::atomic<CAmount> minFeeFilter{0};
574-        CAmount lastSentFeeFilter{0};
575-        std::chrono::microseconds m_next_send_feefilter{0};
576-    };
577+    /** Whether this peer asked us to relay transactions. This only changes from

vasild commented at 12:23 pm on March 19, 2021:

0    /** Whether we should relay transactions to this peer (he asked us to in his version message and this is not a `ConnectionType::BLOCK_RELAY` connection). This only changes from

jnewbery commented at 9:55 am on March 22, 2021:

That’s better. I’ve updated the comment.

in src/net.cpp:627 in 0fb9ffba48 outdated

564@@ -565,12 +565,6 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
565     X(addrBind);

vasild commented at 12:52 pm on March 19, 2021:

(comment on irrelevant line)

In the commit message of 465e9da51c56beaa97f5ffcdf2132471b591a8d5 [net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded:

0     This is currently redundant information with m_tx_relay->fRelayTxes,
1-    but m_tx_relay is moved into net_processing, then we'll need these
2+    but when m_tx_relay is moved into net_processing, then we'll need these
3     separate fields in CNode.

jnewbery commented at 9:55 am on March 22, 2021:

Thanks! Fixed.

in src/net.cpp:1110 in 0fb9ffba48 outdated

975                                                HasAllDesirableServiceFlags(node->nServices),
976-                                               peer_relay_txes, peer_filter_not_null, node->nKeyedNetGroup,
977-                                               node->m_prefer_evict, node->addr.IsLocal()};
978+                                               node->m_relays_txs.load(), node->m_bloom_filter_loaded.load(),
979+                                               node->nKeyedNetGroup, node->m_prefer_evict, node->addr.IsLocal()};
980             vEvictionCandidates.push_back(candidate);

vasild commented at 1:18 pm on March 19, 2021:

This can be reduced to just:

0vEvictionCandidates.push_back(*node);

By using a conversion function from CNode to NodeEvictionCandidate.

Maybe out of the scope of this PR.

jnewbery commented at 9:54 am on March 22, 2021:

Seems reasonable, but I agree it’s out of scope for this PR.

in src/net_processing.cpp:1790 in 0fb9ffba48 outdated

1517-            pnode->PushTxInventory(wtxid);
1518-        } else {
1519-            pnode->PushTxInventory(txid);
1520+    LOCK(m_peer_mutex);
1521+    for(auto& it : m_peer_map) {
1522+        Peer& peer = *it.second;

vasild commented at 2:00 pm on March 19, 2021:

0    for (auto& [node_id, peer] : m_peer_map) {

jnewbery commented at 9:29 am on March 20, 2021:

That means node_id is unused, which causes warnings depending on your compiler flags.

vasild commented at 8:49 am on March 23, 2021:

Which compiler flags produce a warning? I don’t get any with either clang 12 or gcc 10:

0    std::map<int, int> m;
1    for (const auto& [x, y] : m) { // no warning
2        std::cout << x;
3    } 
4    for (const auto& [x, y] : m) { // warning: unused structured binding declaration [-Wunused-variable]
5    }

jnewbery commented at 10:30 am on March 23, 2021:

Hmmm, if you search for “c++ structured binding unused variable”, there do seem to be a lot of people claiming that they’ve encountered this warning. Like you, I’ve been unable to reproduce it, either locally or on godbolt.org with various compiler versions.

In any case, I’m going to leave this as it is, since it’s only one more line and no less clear to explicitly declare the Peer object. Thank you for pointing this out though!

in src/net_processing.cpp:1277 in 0fb9ffba48 outdated

1022@@ -983,6 +1023,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node, bool& fUpdateConnectionTim
1023         PeerRef peer = RemovePeer(nodeid);
1024         assert(peer != nullptr);
1025         misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->m_misbehavior_score);
1026+        m_wtxid_relay_peers -= peer->m_wtxid_relay;

vasild commented at 2:24 pm on March 19, 2021:

This was moved from below without the accompanying assert, better move the assert with it.

0        m_wtxid_relay_peers -= peer->m_wtxid_relay;
1        assert(m_wtxid_relay_peers >= 0);

jnewbery commented at 9:54 am on March 22, 2021:

Done.

vasild commented at 9:09 am on March 23, 2021:

Now the move of the decrement happens in commit [net processing] Move m_wtxid_relay to Peer while the move of the assert happens in [net processing] Move tx relay data to Peer.

I think both moves belong to the first commit.

jnewbery commented at 10:33 am on March 23, 2021:

Oops. Now fixed. Thank you!

in src/net_processing.cpp:1050 in 0fb9ffba48 outdated

1046@@ -1006,7 +1047,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node, bool& fUpdateConnectionTim
1047     assert(nPeersWithValidatedDownloads >= 0);
1048     m_outbound_peers_with_protect_from_disconnect -= state->m_chain_sync.m_protect;
1049     assert(m_outbound_peers_with_protect_from_disconnect >= 0);
1050-    m_wtxid_relay_peers -= state->m_wtxid_relay;
1051     assert(m_wtxid_relay_peers >= 0);

vasild commented at 2:25 pm on March 19, 2021:

This assert can/should be moved up, where the decrement was moved to.

jnewbery commented at 9:54 am on March 22, 2021:

Done.

in src/net_processing.cpp:2907 in 0fb9ffba48 outdated

2644@@ -2597,9 +2645,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2645             return;
2646         }
2647         if (pfrom.GetCommonVersion() >= WTXID_RELAY_VERSION) {
2648-            LOCK(cs_main);
2649-            if (!State(pfrom.GetId())->m_wtxid_relay) {
2650-                State(pfrom.GetId())->m_wtxid_relay = true;
2651+            if (!peer->m_wtxid_relay) {
2652+                peer->m_wtxid_relay = true;
2653                 m_wtxid_relay_peers++;

vasild commented at 3:49 pm on March 19, 2021:

Just a note:

Before this change both CNodeState::m_wtxid_relay and PeerManagerImpl::m_wtxid_relay_peers would have been modified under cs_main. After this change Peer::m_wtxid_relay and PeerManagerImpl::m_wtxid_relay_peers are not anymore modified together under cs_main.

This means that after peer->m_wtxid_relay = true and before m_wtxid_relay_peers++ the system is in an observable state where the sum of all Peer::m_wtxid_relay does not equal PeerManagerImpl::m_wtxid_relay_peers.

I think this is ok - couldn’t find any code that would be upset by that.

jnewbery commented at 9:54 am on March 22, 2021:

Yes, that’s a good observation. I agree that this is totally fine, and the fields don’t need to be atomically updated under cs_main.

in src/net_processing.cpp:255 in 0fb9ffba48 outdated

249@@ -217,7 +250,10 @@ struct Peer {
250     /** Work queue of items requested by this peer **/
251     std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
252 
253-    explicit Peer(NodeId id) : m_id(id) {}
254+    explicit Peer(NodeId id, bool blocks_only)

vasild commented at 4:02 pm on March 19, 2021:

explicit can be removed because now we have 2 arguments.

jnewbery commented at 9:54 am on March 22, 2021:

Nice! Removed.

in src/net_processing.cpp:863 in 0fb9ffba48 outdated

647+{
648+    if (peer.m_tx_relay != nullptr) {
649+        LOCK(peer.m_tx_relay->m_tx_inventory_mutex);
650+        peer.m_tx_relay->m_tx_inventory_known_filter.insert(hash);
651+    }
652+}

vasild commented at 4:07 pm on March 19, 2021:

Why not a Peer method?

jnewbery commented at 9:54 am on March 22, 2021:

I’m trying to keep Peer as a data structure without any internal logic.

in src/net_processing.cpp:1712 in 0fb9ffba48 outdated

1518-        } else {
1519-            pnode->PushTxInventory(txid);
1520+    LOCK(m_peer_mutex);
1521+    for(auto& it : m_peer_map) {
1522+        Peer& peer = *it.second;
1523+        if (!peer.m_tx_relay) return;

vasild commented at 4:30 pm on March 19, 2021:

nit: I think the rest of the code around m_tx_relay uses == nullptr or != nullptr.

jnewbery commented at 9:31 am on March 20, 2021:

We don’t have a project style on whether to compare to nullptr or use a pointer’s bool conversion. Both are fine.

in src/net_processing.cpp:4830 in 0fb9ffba48 outdated

4505-                        pto->m_tx_relay->nNextInvSend = m_connman.PoissonNextSendInbound(current_time, INBOUND_INVENTORY_BROADCAST_INTERVAL);
4506+                        peer->m_tx_relay->m_next_inv_send_time = m_connman.PoissonNextSendInbound(current_time, INBOUND_INVENTORY_BROADCAST_INTERVAL);
4507                     } else {
4508-                        pto->m_tx_relay->nNextInvSend = PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL);
4509+                        // Use half the delay for outbound peers, as there is less privacy concern for them.
4510+                        peer->m_tx_relay->m_next_inv_send_time = PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL);

vasild commented at 4:42 pm on March 19, 2021:

This comment // Use half... was removed from master when {INBOUND,OUTBOUND}_INVENTORY_BROADCAST_INTERVAL were introduced. I think it better be removed because it implies OUTBOUND is INBOUND/2.

jnewbery commented at 9:54 am on March 22, 2021:

Oops! Thank you. Removed.

in src/qt/rpcconsole.cpp:1171 in 0fb9ffba48 outdated

1113@@ -1114,7 +1114,6 @@ void RPCConsole::updateDetailWidget()
1114         peerAddrDetails += "<br />" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal));
1115     ui->peerHeading->setText(peerAddrDetails);
1116     ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices));
1117-    ui->peerRelayTxes->setText(stats->nodeStats.fRelayTxes ? ts.yes : ts.no);

vasild commented at 4:50 pm on March 19, 2021:

This change would only set the text if stats->fNodeStateStatsAvailable is true and will leave it “uninitialized” otherwise. What would be displayed in that case? Empty? Should something like n/a be displayed?

This looks like a regression in the UI - previously that text would always have been set (it was always known) and now - not anymore.

jnewbery commented at 9:54 am on March 22, 2021:

In reality, I think fNodeStateStatsAvailable will almost always be true, so this isn’t a big issue (if it was, then the other stats in nodeStateStats not being available would also be an issue).

in src/rpc/net.cpp:200 in 0fb9ffba48 outdated

192@@ -193,7 +193,6 @@ static RPCHelpMan getpeerinfo()
193         }
194         obj.pushKV("services", strprintf("%016x", stats.nServices));
195         obj.pushKV("servicesnames", GetServicesNames(stats.nServices));
196-        obj.pushKV("relaytxes", stats.fRelayTxes);

vasild commented at 4:53 pm on March 19, 2021:

Similarly to the UI - the fields relaytxes and minfeefilter would have always been present before and now - only if fStateStats is true.

jnewbery commented at 9:54 am on March 22, 2021:

As above for the gui peer console.

vasild commented at 5:06 pm on March 19, 2021: contributor

It seems sub-optimal that new fields are added - CNode::m_relays_txs and CNode::m_bloom_filter_loaded that are redundant/duplicates of Peer::TxRelay::m_relay_txs and Peer::TxRelay::m_bloom_filter != nullptr. This could be confusing and leaves a chance that in the future they go out of sync. Can the duplicates be avoided somehow?

There are two user-visible changes that may be considered as regressions in the UI and in RPC.

DrahtBot added the label Needs rebase on Mar 19, 2021

jnewbery force-pushed on Mar 20, 2021

jnewbery commented at 9:31 am on March 20, 2021: member

Rebased

DrahtBot removed the label Needs rebase on Mar 20, 2021

MarcoFalke commented at 8:31 am on March 22, 2021: member

Doesn’t compile

jnewbery force-pushed on Mar 22, 2021

jnewbery commented at 9:34 am on March 22, 2021: member

Thanks Marco. Bad rebase should be fixed now.

jnewbery commented at 9:54 am on March 22, 2021: member

Thanks for the review @vasild! I think I’ve addressed all of your review comments now.

jnewbery force-pushed on Mar 22, 2021

jnewbery commented at 9:57 am on March 22, 2021: member

It seems sub-optimal that new fields are added - CNode::m_relays_txs and CNode::m_bloom_filter_loaded that are redundant/duplicates of Peer::TxRelay::m_relay_txs and Peer::TxRelay::m_bloom_filter != nullptr. This could be confusing and leaves a chance that in the future they go out of sync. Can the duplicates be avoided somehow?

That does seem a bit of a shame, but note that even though the data seems redundant, it’s used for different purposes in net_processing and net. TxRelay in net_processing is data that is used in relaying transactions to that peer. m_relay_txs and m_bloom_filter in net are simply metrics used in the eviction logic. net_processing informs net of any events that might be used in net’s eviction logic (e.g. min ping time, last tx received, last block received), and these new metrics (m_relay_txs and m_bloom_filter) are no different from those.

vasild commented at 9:18 am on March 23, 2021: contributor

Reviewed 66baf28fe2b76d3f214714604e9e24e83f16c20e - the patch looks correct and I do not think it introduces bugs.

I will try to see when the UI and RPC fields could become empty/missing when they would have been filled/present before this PR.

jnewbery force-pushed on Mar 23, 2021

jnewbery commented at 10:35 am on March 23, 2021: member

Thanks for the re-review @vasild. I’ve updated the commits to address #21160 (review).

in src/net_processing.cpp:227 in 72182a950c outdated

208@@ -209,6 +209,39 @@ struct Peer {
209     /** Whether a ping has been requested by the user */
210     std::atomic<bool> m_ping_queued{false};
211 
212+    /** Whether this peer relays txs via wtxid */
213+    bool m_wtxid_relay{false};

glozow commented at 10:38 pm on March 25, 2021:

I thought this could be const because you’d get the info from the VERSION and it wouldn’t ever change, but then I realized the Peer struct is created before then, so it couldn’t be const… is that accurate?

jnewbery commented at 7:35 am on April 30, 2021:

Correct. The Peer object is constructed before we receive the wtxidrelay message, so we can’t make this const.

DrahtBot added the label Needs rebase on Mar 29, 2021

jnewbery commented at 11:30 am on March 29, 2021: member

Rebased

jnewbery force-pushed on Mar 29, 2021

DrahtBot removed the label Needs rebase on Mar 29, 2021

jnewbery renamed this:
~~Net/Net processing: Move tx inventory into net_processing~~
net/net processing: Move tx inventory into net_processing
on Mar 30, 2021

DrahtBot added the label Needs rebase on Mar 30, 2021

jnewbery commented at 3:48 pm on March 30, 2021: member

rebased

jnewbery force-pushed on Mar 30, 2021

DrahtBot removed the label Needs rebase on Mar 30, 2021

jnewbery force-pushed on Apr 1, 2021

jnewbery commented at 8:16 am on April 1, 2021: member

Rebased

in src/net_processing.cpp:317 in 6976a1c02a outdated

250@@ -218,7 +251,10 @@ struct Peer {
251     /** Work queue of items requested by this peer **/
252     std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
253 
254-    explicit Peer(NodeId id) : m_id(id) {}
255+    Peer(NodeId id, bool blocks_only)

glozow commented at 6:39 pm on April 29, 2021:

Why’d you delete the explicit?

jnewbery commented at 9:37 am on April 30, 2021:

No good reason. I’ve put it back.

vasild commented at 10:49 am on May 7, 2021:

explicit prevents implicit conversions for single-argument constructors. After the change above, the constructor has 2 arguments. See https://stackoverflow.com/a/121163

MarcoFalke commented at 11:22 am on May 7, 2021:

explicit will also prevent implicit conversion for multiple-argument constructors. Is there a reason why {int,bool} should implicitly be converted into Peer?

vasild commented at 11:57 am on May 7, 2021:

Alright: https://stackoverflow.com/questions/39122101/explicit-constructor-taking-multiple-arguments

MarcoFalke commented at 2:06 pm on September 2, 2021:

Was this addressed?

jnewbery commented at 9:28 am on September 24, 2021:

fixed now

in src/net_processing.cpp:1712 in 6976a1c02a outdated

1528-        } else {
1529-            pnode->PushTxInventory(txid);
1530+    LOCK(m_peer_mutex);
1531+    for(auto& it : m_peer_map) {
1532+        Peer& peer = *it.second;
1533+        if (!peer.m_tx_relay) return;

glozow commented at 6:46 pm on April 29, 2021:

Why is this return instead of break? Maybe I’m misunderstanding what this is doing, but I thought that we’d continue iterating through the others to relay to them, but returning seems like we’re just skipping over the rest?

jnewbery commented at 9:39 am on April 30, 2021:

Yikes. Good catch. In a previous iteration of this PR, I had a ForEachPeer() helper that took a lambda, and this return was effectively a continue (the lambda would return and then be called for the remaining peers). Now fixed!

jnewbery force-pushed on Apr 30, 2021

jnewbery commented at 9:50 am on April 30, 2021: member

Thanks for the review @glozow. I’ve rebased on master and addressed your review comments.

#21527 moves around the locks in net_processing, which interacts a bit with how this PR moves m_wtxid_relay around. I think it makes sense to focus on merging that PR first, since that could simplify the locking here a bit.

MarcoFalke added the label Needs rebase on May 3, 2021

DrahtBot commented at 9:34 am on May 3, 2021: contributor

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

jnewbery commented at 10:55 am on May 3, 2021: member

I’ll rebase this after #21527 is merged

jnewbery marked this as a draft on May 11, 2021

jnewbery force-pushed on May 28, 2021

jnewbery commented at 10:14 am on May 28, 2021: member

I’ve rebased this on top of the net_processing: add m_mutex_message_handling commit from #21527. I’m leaving as WIP for now, but it’s in a state where people can review it. I’ll move it out of draft as soon as #21527 or #22053 is merged (should be a trivial rebase).

jnewbery force-pushed on Jun 2, 2021

jnewbery force-pushed on Aug 13, 2021

jnewbery marked this as ready for review on Aug 13, 2021

jnewbery commented at 10:25 am on August 13, 2021: member

I’d rebased this on top of some of the work in #21527, since they conflicted a little. However, it seems that 21527 has now been abandoned, so I’ve removed that commit and marked this PR ready for review again. If 21527 does get picked up again and merged, then it should be easy enough to rebase this on top of it.

DrahtBot removed the label Needs rebase on Aug 13, 2021

in src/net_processing.cpp:4866 in cef5afc817 outdated

4766                 // Time to send but the peer has requested we not relay transactions.
4767                 if (fSendTrickle) {
4768-                    LOCK(pto->m_tx_relay->cs_filter);
4769-                    if (!pto->m_tx_relay->fRelayTxes) pto->m_tx_relay->setInventoryTxToSend.clear();
4770+                    LOCK(peer->m_tx_relay->m_bloom_filter_mutex);
4771+                    if (!peer->m_tx_relay->m_relay_txs) peer->m_tx_relay->m_tx_inventory_to_send.clear();

glozow commented at 12:48 pm on August 16, 2021:

I have a question about this code (not related to this PR though) - why don’t we lock m_tx_inventory_mutex here?

jnewbery commented at 10:00 am on August 17, 2021:

Hmmm, actually m_tx_inventory_mutex is held at this point (see line 4747 above):

0        if (peer->m_tx_relay != nullptr) {
1                LOCK(peer->m_tx_relay->m_tx_inventory_mutex);
2                ...

In fact, all access of m_tx_inventory_to_send (and setInventoryTxToSend in master) is guarded by m_tx_inventory_mutex (cs_tx_inventory in master). It looks like an oversight in #13123 that a lock annotation wasn’t added to setInventoryTxToSend. It could also be added to m_next_inv_send_time (nNextInvSend in master).

We could add those annotations in a follow-up, since this branch is simply moving the data members and not changing any behavior here. I also have a separate follow-up branch that rationalizes these mutexes to a single mutex.

jnewbery force-pushed on Aug 16, 2021

jnewbery commented at 1:32 pm on August 16, 2021: member

Rebased on master

in src/net_processing.cpp:3349 in 203bfe156b outdated

3204@@ -3212,6 +3205,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3205             pfrom.AddKnownTx(txid);
3206         }
3207 
3208+        LOCK2(cs_main, g_cs_orphans);
3209+

glozow commented at 3:32 pm on August 16, 2021:

In 203bfe156b0a25a36c19fba397c8b2577bab1e9b: Noted that the line LOCK2(cs_main, g_cs_orphans) was moved down a few lines, and my reasoning for why this is okay was:

Whether a peer does wtxid relay, i.e. the value of peer->m_wtxid_relay, wouldn’t change, and either way wouldn’t require cs_main or g_cs_orphans
The peer inventory data is guarded by cs_tx_inventory and that’s grabbed within AddKnownTx

jnewbery commented at 10:03 am on August 17, 2021:

Right. This LOCK(cs_main) was required to access the CNodeState in the line below. Now that m_wtxid_relay is in Peer, we don’t need to hold cs_main until a little bit later (when we access m_txrequest).

in src/net_processing.cpp:1277 in 203bfe156b outdated

1194@@ -1199,6 +1195,8 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
1195         PeerRef peer = RemovePeer(nodeid);
1196         assert(peer != nullptr);
1197         misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->m_misbehavior_score);
1198+        m_wtxid_relay_peers -= peer->m_wtxid_relay;

glozow commented at 3:38 pm on August 16, 2021:

In 203bfe156b0a25a36c19fba397c8b2577bab1e9b: not changed in this PR, but it’s odd to me that we’re subtracting a bool from an int? Why isn’t it if (peer->m_wtxid_relay) m_wtxid_relay_peers -= 1; ?

jnewbery commented at 10:05 am on August 17, 2021:

I agree that would be clearer.

in src/net_processing.cpp:244 in 2533ea5161 outdated

234         // a) it allows us to not relay tx invs before receiving the peer's version message
235         // b) the peer may tell us in its version message that we should not relay tx invs
236         //    unless it loads a bloom filter.
237-        bool fRelayTxes GUARDED_BY(cs_filter){false};
238-        std::unique_ptr<CBloomFilter> pfilter PT_GUARDED_BY(cs_filter) GUARDED_BY(cs_filter){nullptr};
239+        bool m_relay_txs GUARDED_BY(m_bloom_filter_mutex){false};

glozow commented at 4:44 pm on August 16, 2021:

In 2533ea5161d2cb15b3a43e0fea9b146f525aea97

Noting that we have 3 variables named m_relay_txs: in NodeEvictionCandidate, Peer.TxRelay, and CNodeStateStats. And we have a CNode.m_relays_txs (very similar but not same name) where CNode.m_relays_txs == Peer.m_tx_relay.m_relay_txs always, and they are not the same thing as Peer.m_tx_relay != nullptr.

jnewbery commented at 10:07 am on August 17, 2021:

Yes, this is slightly unfortunate. Please let me know if you have suggestions for improving the naming.

MarcoFalke commented at 5:15 pm on August 22, 2021:

Is there a reason for the bool to exist? What would happen if we delayed initialization of the whole txrelay struct to when the version message is received?

Something similar is done for address relay, IIRC.

jnewbery commented at 12:25 pm on August 23, 2021:

@MarcoFalke yes, that’s the plan. This PR is intended to be a simple refactor, and delaying initialization of the TxRelay struct requires more focused discussion so should be done in a separate PR.

I’ve opened PR #22778, which implements delaying constructing m_tx_relay until the version message is received. Note however that m_relay_txs is still required - a peer that we’ve offered NODE_BLOOM services to may send us a version with fRelay=0, but later request that we start announcing transactions by sending filterload or filterclear, so we need to initialize the TxRelay struct during version handling but use m_relay_txs to gate whether we announce transactions.

I’m going to mark this conversation as resolved, but please let me know in #22778 if you have any other thoughts/feedback.

glozow commented at 4:50 pm on August 16, 2021: member

code review ACK 2533ea5161d2cb15b3a43e0fea9b146f525aea97

in src/net_processing.cpp:1748 in 2533ea5161 outdated

1705@@ -1666,22 +1706,17 @@ void PeerManagerImpl::SendPings()
1706 
1707 void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid)
1708 {
1709-    WITH_LOCK(cs_main, _RelayTransaction(txid, wtxid););
1710-}
1711-
1712-void PeerManagerImpl::_RelayTransaction(const uint256& txid, const uint256& wtxid)

vasild commented at 9:03 am on August 19, 2021:

Commit 203bfe156b0a25a36c19fba397c8b2577bab1e9b [net processing] Move m_wtxid_relay to Peer introduces a usage of CConnMan::ForEachNode() with a lambda that returns true and false, however it should have a return type of void.

This is later removed in a subsequent commit.

jnewbery commented at 1:33 pm on August 19, 2021:

fixed.

vasild commented at 11:57 am on August 19, 2021: contributor

2533ea5161d2cb15b3a43e0fea9b146f525aea97 looks fine on the low level.

It is best if internal refactors like this one do not introduce user visible changes (even less if those changes look like “regressions” – missing fields whereas fields were present before) or code smells like the same data being stored in two different places (that can go out of sync). See an earlier comment.

While those may look minor, it is difficult for me to judge whether the benefits of this PR outweight the above.

Wrt the missing fields in getpeerinfo: I think that may happen (fStateStats be false) at least in the case where the node is removed after CConnMan::GetNodeStats() is called, maybe in other cases too.

jnewbery force-pushed on Aug 19, 2021

jnewbery commented at 1:40 pm on August 19, 2021: member

Thanks for the review @vasild. I understand your arguments about the extra fields in CNode and removing the stats from the GUI and RPC when they’re not available. I don’t have anything to add beyond my responses from the first time you raised those points.

jonatack commented at 4:22 pm on August 19, 2021: contributor

Thanks for the review @vasild. I understand your arguments about the extra fields in CNode and removing the stats from the GUI and RPC when they’re not available.

(If helpful, mentioning #21160 (review) here, as that thread is closed as resolved.)

MarcoFalke commented at 5:16 pm on August 22, 2021: member

Concept ACK. This will simplify -nomempool

glozow commented at 8:51 am on August 23, 2021: member

re ACK a192c23b2cb45e7c9a06bb00714347ef38df701f via git range-diff 2533ea5...a192c23, no overall changes, diff moved between 2 commits.

DrahtBot added the label Needs rebase on Aug 23, 2021

jnewbery force-pushed on Aug 23, 2021

jnewbery commented at 5:18 pm on August 23, 2021: member

rebased

DrahtBot removed the label Needs rebase on Aug 23, 2021

amitiuttarwar commented at 6:45 pm on August 25, 2021: contributor

concept ACK

jnewbery commented at 10:00 am on August 27, 2021: member

rebased

jnewbery force-pushed on Aug 27, 2021

glozow commented at 10:58 am on September 1, 2021: member

reACK 34bd2711e7ffa0d5a7b563d15d79f8ae1d8cef43 via git range-diff a192c23...34bd271

hebasto commented at 11:02 am on September 1, 2021: member

Concept ACK.

in src/net_processing.cpp:2716 in bc3bc6ceb7 outdated

2592@@ -2593,6 +2593,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2593         if (pfrom.m_tx_relay != nullptr) {
2594             LOCK(pfrom.m_tx_relay->cs_filter);
2595             pfrom.m_tx_relay->fRelayTxes = fRelay; // set to true after we get the first filter* message
2596+            if (fRelay) pfrom.m_relays_txs = true;

MarcoFalke commented at 11:44 am on September 2, 2021:

nit bc3bc6ceb7265b8ea9f530066e6b79c75881f2e4: Is there a reason to not use the shorter and more clear version pfrom.m_relays_txs = fRelay?

jnewbery commented at 10:18 am on September 24, 2021:

Because of the comment for m_relay_txs in net.h:

/** ... This only changes from false to true. It will never change
 *  back to false. Used only in inbound eviction logic. */

This doesn’t work:

0            pfrom.m_relays_txs &= fRelay;

since the &= operator isn’t defined for std::atomic<bool>. I could use:

0            pfrom.m_relays_txs = pfrom.m_relay_txs && fRelay;

but I don’t think that’s an improvement.

vasild commented at 1:50 pm on September 24, 2021:

pfrom.m_relays_txs = pfrom.m_relay_txs && fRelay; could change it from true to false.

naumenkogs commented at 9:58 am on September 29, 2021:

I think the comment is a good justification to keeping the code this way. The only problem I can see is the temptation of future devs to change this.

jnewbery commented at 4:22 pm on September 29, 2021:

pfrom.m_relays_txs = pfrom.m_relay_txs && fRelay; could change it from true to false.

:man_facepalming: You’re right of course. I meant pfrom.m_relay_txs || fRelay;

theuni commented at 5:50 pm on October 26, 2021:

Setting these values should be fine outside of the lock, right?

Can you move this out of the lock scope to make that clear?

jnewbery commented at 8:19 am on March 28, 2022:

Can you move this out of the lock scope to make that clear?

Done in #24692

in src/net_processing.cpp:227 in c46d1f5eef outdated

222@@ -223,6 +223,9 @@ struct Peer {
223     /** Whether a ping has been requested by the user */
224     std::atomic<bool> m_ping_queued{false};
225 
226+    /** Whether this peer relays txs via wtxid */
227+    bool m_wtxid_relay{false};

MarcoFalke commented at 1:53 pm on September 2, 2021:

c46d1f5eef34ec210a21d4700d6f2d4ca0b6941a: Is there some intuition available why this doesn’t need any Mutex or atomic? The other members seem to have one.

jnewbery commented at 10:28 am on September 24, 2021:

No reason. I’ve updated this to be an atomic.

in src/test/fuzz/util.cpp:218 in eeb0e84fc5 outdated

212         node.nVersion = version;
213         node.SetCommonVersion(std::min(version, PROTOCOL_VERSION));
214     }
215-    if (node.m_tx_relay != nullptr) {
216-        LOCK(node.m_tx_relay->cs_filter);
217-        node.m_tx_relay->fRelayTxes = filter_txs;

MarcoFalke commented at 2:01 pm on September 2, 2021:

eeb0e84fc5c259dfd00f7673ad4a51582b100282: Would be nice to not break the fuzz test. Maybe a test-only mock method on PeerMan can do this?

jnewbery commented at 10:36 am on September 24, 2021:

Does this break the fuzz test? fRelayTxes (m_relay_txs after this PR) will get set after the node receives and processes the version message. Presumably the fuzzer will be able to set the fRelay field on that message to true or false.

MarcoFalke commented at 1:55 pm on October 25, 2021:

There is only one fuzz test to send more than one message type in one session, so this will break all other fuzz tests that only send one message type.

jnewbery commented at 12:10 pm on October 29, 2021:

That seems like a bad approach to me. There’s all kinds of logic that happens inside the version and verack handling. Skipping over that by reaching into PeerManager and CConnman internals and updating individual fields means that the CNode, CNodeState and Peer objects are only partially initialized when you send in the fuzzed network messages.

MarcoFalke commented at 12:41 pm on October 29, 2021:

There shouldn’t be any fields that are left uninitialized or partially initialized.

If you don’t like reaching into internals, the only alternative I see is sending the fields that are set here in p2p messages.

jnewbery commented at 12:51 pm on October 29, 2021:

There shouldn’t be any fields that are left uninitialized or partially initialized.

Here’s one example. CNode.fClient is set here:

https://github.com/bitcoin/bitcoin/blob/baa9fc941cac76b35630da16d77fa2a8b0cc1755/src/net_processing.cpp#L2600

depending on the nServices field in the version message. FillNode() in fuzz/util.cpp will never set that field to true, even if (!(nServices & NODE_NETWORK) && !(nServices & NODE_NETWORK_LIMITED)). That means that the CNode object created by the fuzz test setup can be in a state that’s impossible for it to be in the running software.

MarcoFalke commented at 5:00 pm on November 22, 2021:

Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/23575

in src/net_processing.cpp:317 in eeb0e84fc5 outdated

313@@ -284,8 +314,9 @@ struct Peer {
314     /** Work queue of items requested by this peer **/
315     std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
316 
317-    explicit Peer(NodeId id)
318+    Peer(NodeId id, bool blocks_only)

MarcoFalke commented at 2:09 pm on September 2, 2021:

eeb0e84fc5c259dfd00f7673ad4a51582b100282: Would be nice to rename this from blocks_only to initialize_tx_relay (or similar). blocks_only is confusingly similar named to the setting -blocksonly, but it really means block-relay-only.

Moreover, the details why tx relay isn’t initialized here don’t matter here, so a bool with a more general name seems fine.

jnewbery commented at 9:30 am on September 24, 2021:

Done

MarcoFalke commented at 1:56 pm on October 25, 2021:

Obviously that means you’ll have to toggle the code after renaming the symbol ;)

jnewbery commented at 11:57 am on October 29, 2021:

Done! (https://github.com/bitcoin/bitcoin/pull/21160#discussion_r739150583)

in src/net_processing.cpp:1374 in eeb0e84fc5 outdated

1324@@ -1286,6 +1325,11 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c
1325         ping_wait = GetTime<std::chrono::microseconds>() - peer->m_ping_start.load();
1326     }
1327 
1328+    if (peer->m_tx_relay != nullptr) {
1329+        stats.fRelayTxes = WITH_LOCK(peer->m_tx_relay->cs_filter, return peer->m_tx_relay->fRelayTxes);
1330+        stats.minFeeFilter = peer->m_tx_relay->minFeeFilter.load();
1331+    }

MarcoFalke commented at 2:11 pm on September 2, 2021:

eeb0e84fc5c259dfd00f7673ad4a51582b100282: any reason to make this code more fragile by removing the else in the move? The else used to set those to false, and 0, respectively.

jnewbery commented at 10:43 am on September 24, 2021:

I don’t think I agree that this is more fragile. The default is that there connection isn’t being used to relay transactions, and the feefilter is set to zero. Those stats are only updated if the node is relaying transactions/has set a feefilter.

I’ve reverted this to use if/else and remove the default initialization, since that’s closer to the current code.

in src/net_processing.h:33 in eeb0e84fc5 outdated

28@@ -29,6 +29,8 @@ struct CNodeStateStats {
29     int m_starting_height = -1;
30     std::chrono::microseconds m_ping_wait;
31     std::vector<int> vHeightInFlight;
32+    bool fRelayTxes{false};
33+    CAmount minFeeFilter{0};

MarcoFalke commented at 2:13 pm on September 2, 2021:

eeb0e84fc5c259dfd00f7673ad4a51582b100282: Why not -1 (an impossible number), like the height values above? Or maybe leave it uninitialized to make memory sanitizers more useful?

jnewbery commented at 10:44 am on September 24, 2021:

As above (https://github.com/bitcoin/bitcoin/pull/21160#discussion_r715508638)

MarcoFalke commented at 2:29 pm on September 2, 2021: member

Concept ACK 34bd2711e7ffa0d5a7b563d15d79f8ae1d8cef43 🤺

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Concept ACK 34bd2711e7ffa0d5a7b563d15d79f8ae1d8cef43 🤺
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUi+Jwv/Ur+qsT+NGY4mzH2EdkIAAFERJYtimCUi2QIug57wZnFMdbuGmmQ1rLH8
 87VdAD3sG1SLDH76CkToopAYidMgxwpX2I07fV9YY+Rf3V3brS+aSSBJP0jLH0c2e
 9lHfrOA+kwK6L9tEBuODUBlAM3oKglqa8R50SUha9EHZ+dQGnkNufNtH7PA9XEBuS
10HSZU4w2+u6j5IHvN94lgIe+HdympyeMK+JRvzOYZrX5IThzhZ3k1Ho5aoJtjv60b
11FIPZ/oskBKG1dlnQU02OPZLwyjXOBAvxfiXpNHGEiBrUFQEmgLbldPXJsBGZtQv/
12XN5S7YvSSpKSv7dMeGKUxbjesA5WYJH5/f5vzb2LADPhpdNIqbHn6aL/60mEKqpq
132++CjN5J7o9mUoGgVOlH2rHe/x8gu8kaSBRhLnDI7kkIjbxCfLJQ2nR3hjxo5URh
14N602XW1Uxmos1hl0JsMRZ6GTzT5Zw9lPyA4q/JDqcirR0+RnUTBZStvAeAQhF4+B
15lgygIrVL
16=+2rA
17-----END PGP SIGNATURE-----

Timestamp of file with hash 98a6c2ba1a3b3b76dacfb7357c8a39198b4cbb57741f9a173f774636f7632c66 -

DrahtBot added the label Needs rebase on Sep 6, 2021

MarcoFalke commented at 1:01 pm on September 14, 2021: member

Are you still working on this?

jnewbery commented at 1:11 pm on September 14, 2021: member

Are you still working on this? @MarcoFalke

Yes. Apologies for the delay. I do intend to rebase and address your review comments soon.

jnewbery force-pushed on Sep 24, 2021

jnewbery commented at 10:45 am on September 24, 2021: member

Thanks for the review @MarcoFalke. I’ve addressed your review comments and rebased on master.

DrahtBot removed the label Needs rebase on Sep 24, 2021

jaysonmald35 approved

in src/net.h:551 in ae98de60d4 outdated

556@@ -557,6 +557,16 @@ class CNode
557     // m_tx_relay == nullptr if we're not relaying transactions with this peer
558     std::unique_ptr<TxRelay> m_tx_relay;
559 
560+    /** Whether we should relay transactions to this peer (their version
561+     *  message did not include fRelay=false and this is not a block-relay-only
562+     *  connection). This only changes from false to true. It will never change

naumenkogs commented at 8:07 am on September 29, 2021:

This only changes from false to true

Could you mention that this happens only at the beginning of the connection lifetime?

jnewbery commented at 4:33 pm on September 29, 2021:

m_relays_txs can be set to true after initial connection, if the peer sends a filterload or filterclear message.

naumenkogs commented at 7:12 am on September 30, 2021:

Right, thanks.

naumenkogs commented at 10:11 am on September 29, 2021: member

ACK ce772d53048a8d0d9934a8310dbedcbdafdc34ec A good change for better readability with no behavior changes.

DrahtBot added the label Needs rebase on Oct 22, 2021

jnewbery force-pushed on Oct 22, 2021

jnewbery commented at 1:29 pm on October 22, 2021: member

Rebased on master

DrahtBot removed the label Needs rebase on Oct 22, 2021

DrahtBot added the label Needs rebase on Oct 22, 2021

jnewbery force-pushed on Oct 22, 2021

jnewbery commented at 4:51 pm on October 22, 2021: member

rebased

DrahtBot removed the label Needs rebase on Oct 22, 2021

MarcoFalke commented at 1:58 pm on October 25, 2021: member

Feedback:

Concept ACK b0dfefec249302cd9d9f121fe081a9a51294d0ab 🌠

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Concept ACK b0dfefec249302cd9d9f121fe081a9a51294d0ab  🌠
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
 7pUhy2QwAgOZpQ3kI/F3nHCLHVgGEA/5M63KOPO69nBlzM7p/gYJFYjqiZjK75dOQ
 8KpaENaMtNwzsLyJwca+JUpBwLMjStXbP6/tfK/HnwkDMXEv5n1SMyrRs7lI12gKG
 9IzttDNqQSAc8VeqBa5qUjPclKlrz7BxLQoaZmmhCxpDmEpaqU9YrJMtez3PTYmag
10dpfX+Yk57FFwrgUxXI5fnw0zWo4EwH7Xa0oiU8TeiUpd2xR0u3B4kUIL2UiGmPv5
11PptowC4ziKcilYu7jBnt0meUEVMnAAG1WoiL2aGnARXWatfSQRHsLqgOjonHzXuE
125s2kC/1LJsWXzlY39LNO/cF0WvXShlpVuZ8V5MpJLG13HV/RhwROYhRJJVTmJeeE
13sXlJmWc46VA+9QXqLdNJ08qFCIbHgakHd2F5bHxV+yEtIfcR2xeHfuiVc6vFwyzW
14Zr/WIsfQGnVagth2H5bJNZVfEavUXPnkZW0xSsTBogwsdBYiue7oqCVsQ0ANzxaf
15W4vi+jNl
16=GzKO
17-----END PGP SIGNATURE-----

in src/net_processing.cpp:320 in b0dfefec24 outdated

314@@ -282,8 +315,9 @@ struct Peer {
315     /** Work queue of items requested by this peer **/
316     std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
317 
318-    explicit Peer(NodeId id)
319+    explicit Peer(NodeId id, bool tx_relay)
320         : m_id(id)
321+        , m_tx_relay(tx_relay ? nullptr : std::make_unique<TxRelay>())

glozow commented at 2:14 pm on October 25, 2021:

I see that this bool was renamed from blocks_only to tx_relay based on Marco’s comment #21160 (review). The rename makes sense to me, but it’s a little odd that if tx_relay=true you make m_tx_relay null… I would’ve expected it to be the other way around? :thinking:

theuni commented at 6:30 pm on October 26, 2021:

Agreed. Can’t parse this as-is.

jnewbery commented at 11:23 am on October 29, 2021:

Oops. Fixed properly now.

in src/net_processing.cpp:4031 in a65a919ad1 outdated

3910@@ -3910,7 +3911,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3911         {
3912             LOCK(pfrom.m_tx_relay->cs_filter);
3913             pfrom.m_tx_relay->pfilter.reset(new CBloomFilter(filter));
3914+            pfrom.m_bloom_filter_loaded = true;

theuni commented at 5:50 pm on October 26, 2021:

Same comment about lock scope here (and the hunk below as well)

jnewbery commented at 8:19 am on March 28, 2022:

Done in #24692

in src/net.cpp:1107 in a65a919ad1 outdated

1054             NodeEvictionCandidate candidate = {node->GetId(), node->nTimeConnected, node->m_min_ping_time,
1055                                                node->nLastBlockTime, node->nLastTXTime,
1056                                                HasAllDesirableServiceFlags(node->nServices),
1057-                                               peer_relay_txes, peer_filter_not_null, node->nKeyedNetGroup,
1058-                                               node->m_prefer_evict, node->addr.IsLocal(),
1059+                                               node->m_relays_txs.load(), node->m_bloom_filter_loaded.load(),

theuni commented at 5:54 pm on October 26, 2021:

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?

jnewbery commented at 7:42 am on March 28, 2022:

Yes, I believe this is fine. The only time both of these values are read is in CompareNodeTXTime() as a tie-break between nodes that have the same m_last_tx_time. In the (extremely rare) window condition where a filterclear or filterload message has caused these to be written, but there’s a read between m_bloom_filter_loaded and m_relays_txs being read, then the only difference is that two nodes that have the same m_last_tx_time might be ordered differently.

in src/net_processing.cpp:419 in 4bd66febf3 outdated

415@@ -385,7 +416,7 @@ class PeerManagerImpl final : public PeerManager
416         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
417 
418     /** Send a version message to a peer */
419-    void PushNodeVersion(CNode& pnode, int64_t nTime);
420+    void PushNodeVersion(CNode& pnode, Peer& peer, int64_t nTime);

theuni commented at 6:41 pm on October 26, 2021:

Why not const Peer& peer here (and for MaybeSendFeefilter), since it’s used read-only?

jnewbery commented at 8:19 am on March 28, 2022:

Done in #24692

in src/net_processing.cpp:857 in 4bd66febf3 outdated

833@@ -803,6 +834,14 @@ static void PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& ins
834     }
835 }
836 
837+static void AddKnownTx(Peer& peer, const uint256& hash)

theuni commented at 6:43 pm on October 26, 2021:

Why not make this a member function of Peer to avoid having to peek at the internal lock?

jnewbery commented at 7:49 am on March 28, 2022:

I’ve been trying to keep Peer as a pure data structure and have all the program logic contained in the PeerManagerImpl functions. We’ve previously had logic split between net_processing functions, CNodeState methods and CNode methods, which has made it difficult to follow the program logic.

in src/net_processing.cpp:1128 in 4bd66febf3 outdated

1124@@ -1086,7 +1125,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count
1125 
1126 } // namespace
1127 
1128-void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime)
1129+void PeerManagerImpl::PushNodeVersion(CNode& pnode, Peer& peer, int64_t nTime)

theuni commented at 6:48 pm on October 26, 2021:

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?

jnewbery commented at 8:20 am on March 28, 2022:

Done in #24692

in src/net_processing.cpp:4546 in 4bd66febf3 outdated

4459@@ -4420,12 +4460,12 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
4460     }
4461 }
4462 
4463-void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, std::chrono::microseconds current_time)
4464+void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::microseconds current_time)

theuni commented at 6:50 pm on October 26, 2021:

Likewise, this doesn’t need all of Peer, only a Peer::TxRelay.

jnewbery commented at 8:20 am on March 28, 2022:

Done in #24692

in src/net_processing.cpp:1205 in 4bd66febf3 outdated

1203-        m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer));
1204+        m_peer_map.emplace_hint(m_peer_map.end(), nodeid, peer);
1205     }
1206     if (!pnode->IsInboundConn()) {
1207-        PushNodeVersion(*pnode, GetTime());
1208+        PushNodeVersion(*pnode, *peer, GetTime());

theuni commented at 6:59 pm on October 26, 2021:

See comment above about changing the PushNodeVersion params, which would eliminate the need for the copies here.

jnewbery commented at 8:20 am on March 28, 2022:

Done in #24692

theuni commented at 7:04 pm on October 26, 2021: member

Concept ACK.

My only reservation here is that we lose a few sync guarantees now that some things are done outside of cs_main. For example m_wtxid_relay_peers and the actual count may momentarily disagree. Are you confident that nothing is relying on the assumption that they’re in sync?

jnewbery commented at 11:25 am on October 29, 2021: member

Thanks for the reviews @glozow and @theuni! I’ve resolved @glozow’s review comment and rebased on master. I plan to address @theuni’s review comments very soon.

jnewbery force-pushed on Oct 29, 2021

MarcoFalke commented at 11:36 am on October 29, 2021: member

There is also this feedback, which you haven’t responded to: #21160 (comment)

DrahtBot added the label Needs rebase on Dec 1, 2021

jnewbery force-pushed on Dec 1, 2021

jnewbery commented at 1:50 pm on December 1, 2021: member

rebased. I still intend to address @theuni’s comments.

DrahtBot removed the label Needs rebase on Dec 1, 2021

DrahtBot added the label Needs rebase on Dec 10, 2021

fanquake referenced this in commit 498fe4b780 on Dec 14, 2021

MarcoFalke commented at 7:14 pm on December 16, 2021: member

This has been needing rebase and stuff addressed for more than a week, so I’ve removed it from hi-prio for now. (Can be added back later)

jnewbery commented at 8:18 pm on December 16, 2021: member

This has been needing rebase and stuff addressed for more than a week, so I’ve removed it from hi-prio for now. (Can be added back later)

Thanks @MarcoFalke. That’s fair. I do intend to rebase this. Just have a lot going on right now.

MarcoFalke commented at 8:27 pm on December 16, 2021: member

Absolutely. Take your time and maybe see you rebase this next year :heart:

jnewbery force-pushed on Dec 20, 2021

jnewbery commented at 11:00 pm on December 20, 2021: member

This is needed for some other work, so I’ve rebased on latest master. I still haven’t addressed @theuni’s review comments.

DrahtBot removed the label Needs rebase on Dec 21, 2021

MarcoFalke commented at 12:32 pm on December 22, 2021: member

re-ACK 0d025d56c1882ef1d4f3e0d78d337a158791f4d9 🚦

only changes are:

Rebase
Fix Peer constructor (tx_relay bool)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK  0d025d56c1882ef1d4f3e0d78d337a158791f4d9 🚦
 4
 5only changes are:
 6
 7* Rebase
 8* Fix Peer constructor (tx_relay bool)
 9
10-----BEGIN PGP SIGNATURE-----
11
12iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
13pUjPdwv/ZLe2SAvo1xnZxY38S+0T2XwINcmvufQZrieVafJfRGQaBarFiOKtW/yN
14AfMfoYSKuXHHJFiMx7IZdMFiN1224OnHgxtt7RCtW+NVsb2ZKZjqeIbYceN4Ex4Z
15Q2JZT06+X2wO7oChrhMOaV9QujS6A0aEfCeWBOed0NNZnRVh4Xeb4rD5/yNFLdKO
16kVchsNjNKDdX166g6k6fH82o8MRaTV84pd2t7Yw80ZZL0hqBt5X9AS7HKVrXQfs6
17KI9WqBmTPWAQkJ9pIu5LbcHM26ayQ7MJTx/kh1pVRA97I2uid/5Q7yZLCPxblP6h
18wP0hJgJEIGXaU9th0PcSErTJd27JOPkhSkRtU/EONWhJxt0E2foldrNdEYZ+USK2
19a6OoBValo+ieXDFVgH/8eBm5eQoEhrmfhbOMakEbkULzD8fRIx6SIOlqeT6yWojj
208L5S6HudaTQsEq+h6w1iv6VyWrsn6VrZSNASNRGPdcsxSEsPc8CoofisO10rXULK
21YmWgVqV6
22=QnMV
23-----END PGP SIGNATURE-----

in src/test/fuzz/util.cpp:260 in b6eae07255 outdated

237@@ -238,10 +238,6 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
238     assert(node.nVersion == version);
239     assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
240     assert(node.nServices == remote_services);
241-    if (node.m_tx_relay != nullptr) {
242-        LOCK(node.m_tx_relay->cs_filter);
243-        assert(node.m_tx_relay->fRelayTxes == filter_txs);

MarcoFalke commented at 12:35 pm on December 22, 2021:

nit in b6eae072553bd2811feed0149644a9a906489f35:

Instead of removing, I think you can do assert(peerman.getpeer(id).frelay==filter)

jnewbery commented at 8:16 am on March 28, 2022:

PeerManager doesn’t have a public GetPeerRef() method. The Peer objects are intentionally private to PeerManagerImpl.

MarcoFalke commented at 9:06 am on March 29, 2022:

The following diff compiles for me:

 0diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
 1index d57c0081db..6766fbf2d9 100644
 2--- a/src/test/fuzz/util.cpp
 3+++ b/src/test/fuzz/util.cpp
 4@@ -225,7 +225,7 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
 5     const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
 6     const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
 7     const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max());
 8-    const bool filter_txs = fuzzed_data_provider.ConsumeBool();
 9+    const bool relay_txs{fuzzed_data_provider.ConsumeBool()};
10 
11     const CNetMsgMaker mm{0};
12 
13@@ -241,7 +241,7 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
14                 uint64_t{1},                                    // dummy nonce
15                 std::string{},                                  // dummy subver
16                 int32_t{},                                      // dummy starting_height
17-                filter_txs),
18+                relay_txs),
19     };
20 
21     (void)connman.ReceiveMsgFrom(node, msg_version);
22@@ -255,6 +255,9 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
23     assert(node.nVersion == version);
24     assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
25     assert(node.nServices == remote_services);
26+    CNodeStateStats statestats;
27+    assert(peerman.GetNodeStateStats(node.GetId(), statestats));
28+    assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
29     node.m_permissionFlags = permission_flags;
30     if (successfully_connected) {
31         CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)};

It does two things:

Rename the filter_txs symbol to a better name
Restore the check that m_relay_txs is correctly set after the “version msg roundtrip”.

jnewbery commented at 2:55 pm on March 29, 2022:

Thanks. Done in #24692.

DrahtBot added the label Needs rebase on Jan 6, 2022

jnewbery force-pushed on Feb 3, 2022

DrahtBot removed the label Needs rebase on Feb 3, 2022

DrahtBot added the label Needs rebase on Feb 4, 2022

jnewbery force-pushed on Feb 4, 2022

DrahtBot removed the label Needs rebase on Feb 4, 2022

DrahtBot added the label Needs rebase on Mar 7, 2022

[net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded

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.

36346703f8

[net processing] Move m_wtxid_relay to Peer

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.

785f55f7ee

[net processing] Move tx relay data to Peer 575bbd0dea

scripted-diff: rename TxRelay members

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

1066d10f71

jnewbery commented at 11:46 am on March 18, 2022: member

Rebased

jnewbery force-pushed on Mar 18, 2022

DrahtBot removed the label Needs rebase on Mar 18, 2022

in src/net_processing.cpp:428 in 1066d10f71

424@@ -394,7 +425,7 @@ class PeerManagerImpl final : public PeerManager
425         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
426 
427     /** Send a version message to a peer */
428-    void PushNodeVersion(CNode& pnode);
429+    void PushNodeVersion(CNode& pnode, Peer& peer);

dergoegge commented at 8:26 am on March 24, 2022:

nit:

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

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

dergoegge commented at 8:40 am on March 24, 2022: member

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

Are you still planning to address Cory’s comments?

glozow commented at 2:26 pm on March 25, 2022: member

utACK 1066d10f71e6800c78012d789ff6ae19df0243fe

fanquake merged this on Mar 25, 2022

fanquake closed this on Mar 25, 2022

jnewbery commented at 5:55 pm on March 25, 2022: member

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:

#21160 (review) “Same comment about lock scope here (and the hunk below as well)”
#21160 (review) “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?”
#21160 (review) “Why not const Peer& peer here (and for MaybeSendFeefilter), since it’s used read-only?”
#21160 (review) “Why not make this a member function of Peer to avoid having to peek at the internal lock?”
#21160 (review) “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?”
#21160 (review) “Likewise, this doesn’t need all of Peer, only a Peer::TxRelay.”
#21160 (review) “See comment above about changing the PushNodeVersion params, which would eliminate the need for the copies here.”
#21160 (review) “Instead of removing, I think you can do assert(peerman.getpeer(id).frelay==filter)”

jnewbery deleted the branch on Mar 25, 2022

jnewbery referenced this in commit 21154ff927 on Mar 28, 2022

jnewbery referenced this in commit 5011edc39f on Mar 28, 2022

jnewbery referenced this in commit 02fd1931fc on Mar 28, 2022

jnewbery referenced this in commit 2432ec4af8 on Mar 28, 2022

jnewbery commented at 8:23 am on March 28, 2022: member

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.

MarcoFalke commented at 8:32 am on March 28, 2022: member

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

Conflicts