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

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

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

(comment on irrelevant line)

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

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

Thanks! Fixed.

This can be reduced to just:

vEvictionCandidates.push_back(*node);

By using a conversion function from CNode to NodeEvictionCandidate.

Maybe out of the scope of this PR.

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

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

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

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

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

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!

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

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

Done.

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.

Oops. Now fixed. Thank you!

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

Done.

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.

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.

explicit can be removed because now we have 2 arguments.

Nice! Removed.

Why not a Peer method?

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

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

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

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.

Oops! Thank you. Removed.

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.

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

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

As above for the gui peer console.

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?

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

Why'd you delete the explicit?

No good reason. I've put it back.

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

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

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

Was this addressed?

fixed now

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?

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!

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

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

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

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.

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

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

I agree that would be clearer.

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.

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

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.

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

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.

fixed.

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

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:

            pfrom.m_relays_txs &= fRelay;

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

            pfrom.m_relays_txs = pfrom.m_relay_txs && fRelay;

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

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

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.

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;

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

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

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

Done in #24692

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

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

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

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.

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.

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.

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.

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.

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

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.

Done

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

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

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.

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.

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

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

This only changes from false to true

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

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

Right, thanks.

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:

Agreed. Can't parse this as-is.

Oops. Fixed properly now.

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

Done in #24692

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?

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.

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

Done in #24692

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

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.

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?

Done in #24692

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

Done in #24692

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

Done in #24692

nit in b6eae072553bd2811feed0149644a9a906489f35:

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

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

The following diff compiles for me:

diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
index d57c0081db..6766fbf2d9 100644
--- a/src/test/fuzz/util.cpp
+++ b/src/test/fuzz/util.cpp
@@ -225,7 +225,7 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
     const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
     const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
     const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max());
-    const bool filter_txs = fuzzed_data_provider.ConsumeBool();
+    const bool relay_txs{fuzzed_data_provider.ConsumeBool()};
 
     const CNetMsgMaker mm{0};
 
@@ -241,7 +241,7 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
                 uint64_t{1},                                    // dummy nonce
                 std::string{},                                  // dummy subver
                 int32_t{},                                      // dummy starting_height
-                filter_txs),
+                relay_txs),
     };
 
     (void)connman.ReceiveMsgFrom(node, msg_version);
@@ -255,6 +255,9 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
     assert(node.nVersion == version);
     assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
     assert(node.nServices == remote_services);
+    CNodeStateStats statestats;
+    assert(peerman.GetNodeStateStats(node.GetId(), statestats));
+    assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
     node.m_permissionFlags = permission_flags;
     if (successfully_connected) {
         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".

Thanks. Done in #24692.

nit:

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

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

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.