net processing: Reduce resource usage for inbound block-relay-only connections

jnewbery commented at 12:20 pm on August 23, 2021: member

block-relay-only connections are additional outbound connections that bitcoind makes since v0.19. They participate in block relay, but do not propagate transactions or addresses. They were introduced in #15759.

When creating an outbound block-relay-only connection, since we know that we’re never going to announce transactions over that connection, we can save on memory usage by not a TxRelay data structure for that connection. When receiving an inbound connection, we don’t know whether the connection was opened by the peer as block-relay-only or not, and therefore we always construct a TxRelay data structure for inbound connections.

However, it is possible to tell whether an inbound connection will ever request that we start announcing transactions to it. The fRelay field in the version message may be set to 0 to indicate that the peer does not wish to receive transaction announcements. The peer may later request that we start announcing transactions to it by sending a filterload or filterclear message, but only if we have offered NODE_BLOOM services to that peer. NODE_BLOOM services are disabled by default, and it has been recommended for some time that users not enable NODE_BLOOM services on public connections, for privacy and anti-DoS reasons.

Therefore, if we have not offered NODE_BLOOM to the peer and it has set fRelay to 0, then we know that it will never request transaction announcements, and that we can save resources by not initializing the TxRelay data structure.

fanquake added the label P2P on Aug 23, 2021

in src/net_processing.cpp:2764 in 238ce0b02b outdated

2641@@ -2643,10 +2642,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2642         // set nodes not capable of serving the complete blockchain history as "limited nodes"
2643         pfrom.m_limited_node = (!(nServices & NODE_NETWORK) && (nServices & NODE_NETWORK_LIMITED));
2644 
2645-        LOCK(peer->m_tx_relay_mutex);
2646-        if (peer->m_tx_relay != nullptr) {
2647+        // We don't initialize the m_tx_relay data structure if:
2648+        // - this is an outbound block-relay-only connection; or
2649+        // - fRelay=false and we're not offering NODE_BLOOM to this peer
2650+        //   (NODE_BLOOM means that the peer may turn on tx relay later)

MarcoFalke commented at 1:15 pm on August 23, 2021:

in 238ce0b02b1f7ac78c8c0466cef33628c6d49359:

If it is turned on later, we could keep it a nullptr until then and only initialize if and when we receive a filter* message?

jnewbery commented at 2:02 pm on August 23, 2021:

Perhaps, but I think it’s better to allocate resources for the worst-case scenario up front, rather than try to reduce resource usage and later find out that you need to allocate a new structure.

naumenkogs commented at 10:09 am on September 30, 2021:

rather than try to reduce resource usage and later find out that you need to allocate a new structure

What exactly is difficult about this?

MarcoFalke commented at 10:27 am on October 1, 2021:

What exactly is difficult about this?

I think I agree with John that there is little benefit (or even a risk) when giving remote peers the option to force you to allocate MBs of memory by sending a single msg, instead of allocating the memory on startup/creation of the connection.

vasild commented at 11:29 am on October 1, 2021:

Hmm, allocating upfront is like giving a remote peer the option and he always exercises it immediately after connecting, no?

If there is a chance that the peer would not exercise the option (not send filter* message) and we never allocate the MBs, that is to be preferred?

MarcoFalke commented at 11:35 am on October 1, 2021:

Yes, and I think this is preferable because there are less (late) surprises. Neither on the live network, nor when developing. For example, massif will display the real usage from the start. Or, imagine someone under-estimating the resource usage and thus increasing the maxconnections count, then running into OOM later on.

vasild commented at 11:58 am on October 1, 2021:

The counter scenario is somebody running into OOM because MBs of memory were allocated needlessly (allocated but never used because the peer(s) never sent filter*).

MarcoFalke commented at 12:24 pm on October 1, 2021:

I think if you want to reduce memory usage, you should disable bip37, not enable it with the hope that no one (or only a small fraction of your incoming peers) will use it.

jnewbery commented at 4:24 pm on October 7, 2021:

I agree with @MarcoFalke that if memory consumption is a concern, then you shouldn’t enable BIP37 bloom filters for public peers.

MarcoFalke commented at 1:15 pm on August 23, 2021: member

Concept ACK

DrahtBot commented at 1:25 pm on August 23, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25156 (refactor: Introduce PeerManagerImpl::RejectIncomingTxs by MarcoFalke)
#25144 (refactor: Pass Peer& to Misbehaving() by MarcoFalke)
#24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
#21527 (net_processing: lock clean up by ajtowns)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)
#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.

DrahtBot added the label Needs rebase on Aug 23, 2021

jnewbery force-pushed on Aug 27, 2021

jnewbery commented at 8:37 am on August 27, 2021: member

I’ve pushed a new branch that simplifies the implementation a bit. I’d previously wanted to amalgamate the various mutexes and atomics in Peer::TxRelay as part of this PR, but it’s easier to wait until #21527 to do that, otherwise there are all kinds of lock ordering/inversion issues to work around.

jnewbery force-pushed on Aug 27, 2021

DrahtBot removed the label Needs rebase on Aug 27, 2021

jnewbery force-pushed on Aug 27, 2021

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

rebased

naumenkogs commented at 4:04 pm on September 1, 2021: member

Concept ACK

DrahtBot added the label Needs rebase on Sep 6, 2021

jnewbery force-pushed on Sep 24, 2021

jnewbery commented at 11:13 am on September 24, 2021: member

Rebased on the latest #21160

DrahtBot removed the label Needs rebase on Sep 24, 2021

vasild commented at 7:49 am on September 30, 2021: member

Some rough estimates on how much this will save:

0(gdb) p sizeof(CNode::TxRelay)
1$1 = 152

Assuming 50 incoming connections and 20% of them being block-only, that makes 10 peers will benefit from this. 152 bytes each, so 1520 bytes will be saved.

jnewbery commented at 8:11 am on September 30, 2021: member

Some rough estimates on how much this will save… @vasild This seems completely wrong. TxRelay contains a CRollingBloomFilter parametrized by {50000, 0.000001}. According to this code comment:

https://github.com/bitcoin/bitcoin/blob/419afa93419e6840f78cb94b4a39d826eb10e139/src/bloom.h#L94-L98

the storage requirement for just that rolling bloom filter is ~1.8 * 50,000 * 6 = 540kB

in src/net_processing.cpp:268 in 48c896ee7f outdated

252-        // Set of transaction ids we still have to announce.
253-        // They are sorted by the mempool before relay, so the order is not important.
254+        /** Set of transaction ids we still have to announce (txid for
255+         *  non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the
256+         *  mempool to sort transactions in dependency order before relay, so
257+         *  this does not have to be sorted. */

naumenkogs commented at 9:53 am on September 30, 2021:

At first I was thinking we could leave a TODO here for switching to unordered_set, but when I’m looking how it’s used, it seems we can just leave it like this, right?

jnewbery commented at 1:38 pm on October 7, 2021:

I think that the size of this set is typically small enough that the complexity of insertion (lg(n) -vs- constant for an unordered_set) is not important.

vasild commented at 9:56 am on September 30, 2021: member

Well, I said “rough”, but did not mean that rough. Somehow I wrongly assumed that none of the constructors allocate extra memory from the heap :disappointed:. Now I checked that CRollingBloomFilter{50000, 0.000001} creates and fills a vector of 67'396 uint64_t elements, or 539'168 bytes. For 10 peers that would save ~5MB.

I wonder if it would make sense to delay the creation of CRollingBloomFilter::data until it is actually needed (e.g. create it in the first insert, rather than in the constructor, would be changes isolated solely inside CRollingBloomFilter). CRollingBloomFilter is also used here:

BanMan::m_discouraged, would benefit from lazy creation because we may never ban anybody, but is just one filter per program instance.
Peer::m_addr_known, looks like if created then it will be certainly used, one per peer.
PeerManagerImpl::m_recent_rejects and PeerManagerImpl::m_recent_confirmed_transactions, likely to be used, one per program instance.
CNodeState::m_recently_announced_invs, would benefit like CNode::TxRelay::filterInventoryKnown, one per peer.

Maybe only CNodeState::m_recently_announced_invs is worth considering?

in src/net_processing.cpp:262 in 48c896ee7f outdated

266+        /** The next time after which we will send an `inv` message containing
267+         *  transaction announcements to this peer. */
268         std::chrono::microseconds m_next_inv_send_time{0};
269 
270-        /** Minimum fee rate with which to filter inv's to this node */
271+        /** Minimum fee rate with which to filter transaction announcements to this node */

naumenkogs commented at 9:57 am on September 30, 2021:

This also could mention BIP133, or perhaps be flipped with the next variable referencing it :)

jnewbery commented at 1:39 pm on October 7, 2021:

Done!

in src/net_processing.cpp:278 in 48c896ee7f outdated

272         std::atomic<CAmount> m_fee_filter_received{0};
273+        /** Fee rate below which we requested our peer not send us transaction
274+         *  announcements. It is *not* a p2p protocol violation for the peer to
275+         *  send us transactions with a lower fee rate than this. See BIP133. */
276         CAmount m_fee_filter_sent{0};
277+        /** The next time after which we will send a `feefilter` message to this peer. */

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

I think this comment is not very useful currently w.r.t. why we would update feefilter at all, and why it has to be scheduled.

jnewbery commented at 1:40 pm on October 7, 2021:

I think that kind of comment would be more at home on or in the MaybeSendFeefilter() function.

in src/net_processing.cpp:2664 in bb89747af0 outdated

2656@@ -2658,8 +2657,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2657         // set nodes not capable of serving the complete blockchain history as "limited nodes"
2658         pfrom.m_limited_node = (!(nServices & NODE_NETWORK) && (nServices & NODE_NETWORK_LIMITED));
2659 
2660-        {
2661+        // We don't initialize the m_tx_relay data structure if:
2662+        // - this is an outbound block-relay-only connection; or
2663+        // - fRelay=false and we're not offering NODE_BLOOM to this peer
2664+        //   (NODE_BLOOM means that the peer may turn on tx relay later)
2665+        const bool disable_tx_relay = pfrom.IsBlockOnlyConn() ||

naumenkogs commented at 10:13 am on September 30, 2021:

Inverted this could be: const bool enable_tx_relay = !pfrom.IsBlockOnlyConn() && (fRelay || (pfrom.GetLocalServices() & NODE_BLOOM)).

Don’t you think it’s better?

jnewbery commented at 1:44 pm on October 7, 2021:

I actually think it’s clearer this way, since it matches the english language comment above, and enabling tx relay is the default. I tried inverting the boolean, but it didn’t seem any better to me.

That said, if other contributors also think it’d be clearer to invert the boolean logic here, I’m happy to switch it.

vasild commented at 1:58 pm on October 7, 2021:

I think if (enabled) is clearer than if (!disabled). @naumenkogs’s variant has less !. Can the English comment can be reworded: we only initialize this if not block-only and …?

jnewbery commented at 4:22 pm on October 7, 2021:

Done!

MarcoFalke commented at 10:24 am on October 1, 2021: member

@vasild e.g. create it in the first insert, rather than in the constructor

See discussion at #22778 (review)

in src/net_processing.cpp:2668 in bb89747af0 outdated

2666+        //   (NODE_BLOOM means that the peer may turn on tx relay later)
2667+        const bool disable_tx_relay = pfrom.IsBlockOnlyConn() ||
2668+                                      (!fRelay && !(pfrom.GetLocalServices() & NODE_BLOOM));
2669+        if (!disable_tx_relay) {
2670+            LOCK(peer->m_tx_relay_mutex);
2671+            peer->m_tx_relay = std::make_unique<Peer::TxRelay>();

vasild commented at 10:13 am on October 7, 2021:

If this can be executed concurrently by two threads, then the allocation may happen twice - thread1:lock,alloc,unlock, thread2:lock,alloc,unlock.

If this cannot be executed concurrently then the mutex is not needed. I think this is the case because message processing is single threaded and we would have quit earlier if we received a second VERSION message.

If, for some reason, the mutex is deemed necessary then the pattern should be:

0LOCK(peer->m_tx_relay_mutex);
1if (peer->m_tx_relay != nullptr) {
2    peer->m_tx_relay = std::make_unique<Peer::TxRelay>();
3}
4// unlock

jnewbery commented at 2:03 pm on October 7, 2021:

It’s impossible for the if (msg_type == NetMsgType::VERSION) { block of code to be executed more than once, so it would be logically impossible for the m_tx_relay member to be allocated twice. However, I’ve updated the code to check for nullness first so it’s immediately obvious from looking at this code that it can’t be allocated twice.

jnewbery force-pushed on Oct 7, 2021

jnewbery commented at 4:25 pm on October 7, 2021: member

Thanks for the reviews @naumenkogs and @vasild. I believe I’ve addressed all of your review comments.

naumenkogs commented at 8:30 am on October 8, 2021: member

ACK 7ef58ab5ca3e0652442eaf56c5c91ce192df324a

DrahtBot added the label Needs rebase on Oct 22, 2021

vasild commented at 1:27 pm on October 22, 2021: member

This PR includes #21160. The extra commits that this PR includes, on top of #21160, look good to me. I have some [reservations for #21160](/bitcoin-bitcoin/21160/#pullrequestreview-733743042), so I skip ACKing this one too.

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

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

DrahtBot removed the label Needs rebase on Dec 1, 2021

DrahtBot added the label Needs rebase on Dec 10, 2021

jnewbery commented at 10:24 am on December 22, 2021: member

rebased

jnewbery force-pushed on Dec 22, 2021

DrahtBot removed the label Needs rebase on Dec 22, 2021

jnewbery marked this as a draft on Dec 23, 2021

DrahtBot added the label Needs rebase on Jan 6, 2022

in src/net_processing.cpp:2751 in 61fea13352 outdated

2713+        // We only initialize the m_tx_relay data structure if:
2714+        // - this isn't an outbound block-relay-only connection; and
2715+        // - fRelay=true or we're offering NODE_BLOOM to this peer
2716+        //   (NODE_BLOOM means that the peer may turn on tx relay later)
2717+        const bool enable_tx_relay = !pfrom.IsBlockOnlyConn() &&
2718+                                      (fRelay || (pfrom.GetLocalServices() & NODE_BLOOM));

mzumsande commented at 2:18 am on January 9, 2022:

I think that means peers to which we’d connect in -blocksonly mode, also wouldn’t initialize m_tx_relay anymore. But -blocksonly mode until now allows to submit and relay transactions via RPC (sendrawtransaction), even if it that is obviously bad for privacy. With this change, attempting to send a tx via RPC in -blocksonly mode might lead to all of our peers disconnecting us because they wouldn’t have m_tx_relay enabled anymore.

jnewbery commented at 9:14 am on March 30, 2022:

Good catch. I’ve updated the inv and tx logic to explicitly test whether the peer is a block-relay-only connect instead of testing the existence of the Peer::TxRelay struct. That means that if we send a transaction to a peer while we’re in blocksonly mode, then they will still process it.

MarcoFalke commented at 9:18 am on March 30, 2022:

With this change, attempting to send a tx via RPC in -blocksonly mode might lead to all of our peers disconnecting us because they wouldn’t have m_tx_relay enabled anymore.

Isn’t there a test already that should check for this?

jnewbery commented at 9:23 am on March 30, 2022:

With this change, attempting to send a tx via RPC in -blocksonly mode might lead to all of our peers disconnecting us because they wouldn’t have m_tx_relay enabled anymore.

Isn’t there a test already that should check for this?

I would guess that:

https://github.com/bitcoin/bitcoin/blob/bdbabc50ba6c87ded97ea2bbacd3605c59cd12d0/test/functional/p2p_blocksonly.py#L43

would catch it. This branch was previously a draft since it was shared as a WIP, and I’m not sure if that test was failing.

mzumsande commented at 10:25 am on March 30, 2022:

I did check that when I wrote my earlier comment: That test didn’t fail because the peer we sent the tx to was just a python P2PInterface(), which wouldn’t disconnect us for sending a tx like a full node would have.

jnewbery commented at 10:27 am on March 30, 2022:

I guess the test could be changed to subclass P2PInterface and fail if the node under test sends a tx message.

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

jnewbery force-pushed on Mar 25, 2022

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

Rebased now that #21160 has been merged. I haven’t addressed @mzumsande’s comment #22778 (review) yet, so leaving in draft.

DrahtBot removed the label Needs rebase on Mar 25, 2022

DrahtBot added the label Needs rebase on Mar 30, 2022

jnewbery force-pushed on Mar 30, 2022

jnewbery marked this as ready for review on Mar 30, 2022

jnewbery force-pushed on Mar 30, 2022

jnewbery commented at 9:20 am on March 30, 2022: member

Thanks for the review @mzumsande. I’ve responded to your review comment. Let m know what you think.

I’ve also marked this as ready for review.

Note the behaviour change for feefilter messages. With the current branch, for a peer that indicates that they don’t want us to relay messages with fRelay=false in the version message, we will no longer send feefilter messages. I think that’s fine, but it can easily be changed by pulling the m_fee_filter_sent and m_next_send_feefilter members out of Peer::TxRelay and into Peer. That makes sense since all other members of TxRelay are concerned with relaying transactions to the peer, and those two members are concerned with receiving transactions from the peer, so they don’t really belong int TxRelay.

in src/net_processing.cpp:2755 in a23d716a7b outdated

2750+        //   (NODE_BLOOM means that the peer may turn on tx relay later)
2751+        if (!pfrom.IsBlockOnlyConn() &&
2752+            (fRelay || (pfrom.GetLocalServices() & NODE_BLOOM))) {
2753+            auto* tx_relay = [&peer]() {
2754+                LOCK(peer->m_tx_relay_mutex);
2755+                Assume(!peer->m_tx_relay);

MarcoFalke commented at 9:40 am on March 30, 2022:

#21596 :smiling_face_with_tear:

0net_processing.cpp:2754:31: error: reading variable 'm_tx_relay' requires holding mutex 'peer->m_tx_relay_mutex' [-Werror,-Wthread-safety-analysis]
1                Assume(!peer->m_tx_relay);
2                              ^
31 error generated.

jnewbery commented at 9:48 am on March 30, 2022:

Yeah, I’ll remove the lambda.

MarcoFalke commented at 9:52 am on March 30, 2022:

I think the issue isn’t with the lambda, but with Assume/Assert.

jnewbery commented at 10:16 am on March 30, 2022:

Thanks. Removed.

MarcoFalke commented at 10:20 am on March 30, 2022:

Just for reference, an alternative to removing would be to temporarily asssign the value to a bool and then Assume(name_of_bool).

MarcoFalke commented at 12:32 pm on March 30, 2022:

Another reference: If #24714 is merged, you may be able to re-add the Assume as it was.

jnewbery commented at 12:44 pm on March 30, 2022:

I’ll leave this for now, and add the Assume() back if 24714 is merged. I’m 100% sure that this code path can only get hit once for each peer, so the Assume() was only there to catch weird regressions.

jnewbery commented at 7:47 am on March 31, 2022:

I’ll leave this for now, and add the Assume() back if 24714 is merged.

Now done.

jnewbery force-pushed on Mar 30, 2022

DrahtBot removed the label Needs rebase on Mar 30, 2022

jnewbery force-pushed on Mar 30, 2022

jnewbery force-pushed on Mar 31, 2022

jnewbery commented at 7:47 am on March 31, 2022: member

Rebased now that #24714 has been merged. I’ve also added a setter method for Peer.m_tx_relay and made the member variable private.

in src/net_processing.cpp:2765 in d5c191806f outdated

2752@@ -2744,7 +2753,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2753         // set nodes not capable of serving the complete blockchain history as "limited nodes"
2754         pfrom.m_limited_node = (!(nServices & NODE_NETWORK) && (nServices & NODE_NETWORK_LIMITED));
2755 
2756-        if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
2757+        // We only initialize the m_tx_relay data structure if:
2758+        // - this isn't an outbound block-relay-only connection; and
2759+        // - fRelay=true or we're offering NODE_BLOOM to this peer
2760+        //   (NODE_BLOOM means that the peer may turn on tx relay later)
2761+        if (!pfrom.IsBlockOnlyConn() &&

dergoegge commented at 8:29 am on April 21, 2022:

Maybe out of scope for this PR but we should also be able to skip the txrelay setup for feeler connections.

jnewbery commented at 11:01 am on April 26, 2022:

I agree that we could skip it, and also agree that it’s out of scope for this PR (which shouldn’t change behaviour).

Also note that we only have at most one feeler connection at a time, so this wouldn’t result in much resource saving (whereas it’s potentially very significant for inbound block-relay-only peers, since there could be many of them).

dergoegge commented at 9:31 am on April 21, 2022: member

Concept ACK

I think moving m_fee_filter_sent and m_next_send_feefilter out of TxRelay as you mentioned in #22778 (comment) would be good and probably the right change. To me, a PR that only introduced the behavior change for feefilter messages would seem unnecessary, so i am leaning more twoards not introducing the change here. A peer that sends fRelay=false could still send us transactions, why should we not let them know what our min fee is?

jnewbery force-pushed on Apr 26, 2022

jnewbery commented at 11:01 am on April 26, 2022: member

I think moving m_fee_filter_sent and m_next_send_feefilter out of TxRelay as you mentioned in #22778 (comment) would be good and probably the right change. To me, a PR that only introduced the behavior change for feefilter messages would seem unnecessary, so i am leaning more twoards not introducing the change here. A peer that sends fRelay=false could still send us transactions, why should we not let them know what our min fee is?

ok, updated this PR to move those fields out of the TxRelay struct. This PR should not change the feefilter behaviour for inbound or outbound block-relay-only peers.

in src/net_processing.cpp:249 in ee135a2a67 outdated

248         mutable RecursiveMutex m_bloom_filter_mutex;
249-        // We use m_relay_txs for two purposes -
250-        // a) it allows us to not relay tx invs before receiving the peer's version message
251-        // b) the peer may tell us in its version message that we should not relay tx invs
252-        //    unless it loads a bloom filter.
253+        /** Whether the peer wishes to receive transaction announements.

dergoegge commented at 12:10 pm on April 26, 2022:

0        /** Whether the peer wishes to receive transaction announcements.

jnewbery commented at 7:49 am on April 29, 2022:

Thanks. Fixed.

dergoegge commented at 2:13 pm on April 26, 2022: member

Code review ACK ee135a2a67b96024b66b094d28f9ab3ec6d634be

jnewbery force-pushed on Apr 29, 2022

dergoegge commented at 10:48 am on April 29, 2022: member

Code review ACK 4bebd4c72c9328ec6dc7ef5d8d1dd7b53d73fa22

Only change since i last reviewed was fixing a typo.

in src/net_processing.cpp:275 in b3d7612314 outdated

280+         *  permitted if the peer has NetPermissionFlags::Mempool. See BIP35. */
281         bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
282-        // Last time a "MEMPOOL" request was serviced.
283+        /** The last time a BIP35 `mempool` request was serviced. */
284         std::atomic<std::chrono::seconds> m_last_mempool_req{0s};
285+        /** The next time after which we will send an `inv` message containing

naumenkogs commented at 8:25 am on May 12, 2022:

Nit: it also determines (through fSendTrickle) sending mempool response

0
1// Respond to BIP35 mempool requests
2if (fSendTrickle && peer->m_tx_relay->m_send_mempool) {

naumenkogs commented at 8:29 am on May 12, 2022:

Arguably, this field could be moved out of TxRelay then. Or perhaps we should use another timer for mempool.

jnewbery commented at 8:39 pm on May 15, 2022:

Nit: it also determines (through fSendTrickle) sending mempool response

The response to a mempool message is one or more inv announcement messages for transactions in the mempool. I think the comment is correct.

Arguably, this field could be moved out of TxRelay then. Or perhaps we should use another timer for mempool.

I think it’d be better to simply remove support for the mempool message. I think it’s unlikely that many people are using it, and it’d be better to provide this functionality (exposing the contents of the mempool) through rpc rather than a ‘private’ p2p extension.

in src/net_processing.cpp:3367 in 4bebd4c72c outdated

3367@@ -3353,7 +3368,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3368         // Stop processing the transaction early if
3369         // 1) We are in blocks only mode and peer has no relay permission

naumenkogs commented at 8:54 am on May 12, 2022:

nit: add OR to the comment

jnewbery commented at 8:45 pm on May 15, 2022:

Done

naumenkogs commented at 8:55 am on May 12, 2022: member

Code review ACK 4bebd4c72c9328ec6dc7ef5d8d1dd7b53d73fa22

There seem to be a mistake in the commit message of the latter commit. It should say “fRelay=true OR we’re offering”, as opposed to “fRelay=true OR we’re not offering”. The same comment in the codebase is correct.

0We only initialize the m_tx_relay
1data structure if:
2
3- this isn't an outbound block-relay-only connection; AND
4- fRelay=true OR we're not offering NODE_BLOOM to this peer
5  (NODE_BLOOM means that the peer may turn on tx relay later)

jnewbery commented at 8:45 pm on May 15, 2022: member

There seem to be a mistake in the commit message of the latter commit. It should say “fRelay=true OR we’re offering”, as opposed to “fRelay=true OR we’re not offering”.

Good catch. Thanks! I’ve fixed the commit.

Also rebased on latest master.

jnewbery force-pushed on May 15, 2022

naumenkogs commented at 4:54 am on May 16, 2022: member

reACK c4455b7d86b782b7c010857e04be5a73dd1dbb43

in src/net_processing.cpp:4573 in 811783671d outdated

4566@@ -4565,10 +4567,11 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
4567 void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::microseconds current_time)
4568 {
4569     if (m_ignore_incoming_txs) return;
4570-    if (!peer.m_tx_relay) return;
4571     if (pto.GetCommonVersion() < FEEFILTER_VERSION) return;
4572     // peers with the forcerelay permission should not filter txs to us
4573     if (pto.HasPermission(NetPermissionFlags::ForceRelay)) return;
4574+    // Don't send feefilter messages to outbound block-relay-only peers

MarcoFalke commented at 12:09 pm on May 17, 2022:

nit in 811783671d2a9cb2e811f539c889612662ce158c:

Instead of repeating what the code does in the comment, maybe just leave it out (delete it) or explain why this is done. For example, it could say that the feefilter message to block-relay-only peers (or the absence of the message) shouldn’t change their behaviour, so it is safe to skip:

0// Outbound block-relay-only peers should not send any txs to us, regardless of a feefilter message

Also, the commit message could explain why this is a refactor. For example, it could mention that IsBlockOnlyConn() is exactly equal to !peer.m_tx_relay (as of this commit).

jnewbery commented at 2:31 pm on May 18, 2022:

Done

in src/net_processing.cpp:242 in b771e14436 outdated

236@@ -237,32 +237,48 @@ struct Peer {
237 
238     /** Whether this peer relays txs via wtxid */
239     std::atomic<bool> m_wtxid_relay{false};
240-    /** The feerate in the most recent BIP133 `feefilter` message sent to the peer. */
241+    /** The feerate in the most recent BIP133 `feefilter` message sent to the peer.
242+     *  It is *not* a p2p protocol violation for the peer to send us
243+     *  transactions with a lower fee rate than this. See BIP133. */

MarcoFalke commented at 3:22 pm on May 17, 2022:

b771e14436d5008fbd4a2235ccc88730362a9179: Seems odd to add a comment in the previous commit and then rewrite it in the next commit. Why not add the final comment in a single attempt?

jnewbery commented at 2:31 pm on May 18, 2022:

Agree. This was a rebase oopsie from moving commits around. Fixed.

in src/net_processing.cpp:245 in b771e14436 outdated

242+     *  It is *not* a p2p protocol violation for the peer to send us
243+     *  transactions with a lower fee rate than this. See BIP133. */
244     CAmount m_fee_filter_sent{0};
245-    /** Timestamp to send the next BIP133 `feefilter` message sent to the peer. */
246+    /** Timestamp after which we will send the next BIP133 `feefilter` message
247+      * to the peer. */

MarcoFalke commented at 3:22 pm on May 17, 2022:

same

jnewbery commented at 2:32 pm on May 18, 2022:

as above

in src/net_processing.cpp:249 in b771e14436 outdated

246+    /** Timestamp after which we will send the next BIP133 `feefilter` message
247+      * to the peer. */
248     std::chrono::microseconds m_next_send_feefilter{0};
249 
250     struct TxRelay {
251+        /** Protects m_bloom_filter and the m_relay_txs flag. */

MarcoFalke commented at 3:25 pm on May 17, 2022:

b771e14436d5008fbd4a2235ccc88730362a9179: Not sure how useful it is to repeat stuff that can be seen in the code in less than a second. Seems like a burden to maintain and keep updated for no good reason.

jnewbery commented at 2:32 pm on May 18, 2022:

removed

in src/net_processing.cpp:261 in b771e14436 outdated

262+         * us a `filterload` or `filterclear` message. See BIP37. */
263         bool m_relay_txs GUARDED_BY(m_bloom_filter_mutex){false};
264+        /** A bloom filter for which transactions to announce to the peer. See BIP37. */
265         std::unique_ptr<CBloomFilter> m_bloom_filter PT_GUARDED_BY(m_bloom_filter_mutex) GUARDED_BY(m_bloom_filter_mutex){nullptr};
266 
267+        /** Protects m_tx_inventory_known and the m_send_mempool flag */

MarcoFalke commented at 3:26 pm on May 17, 2022:

same

jnewbery commented at 2:32 pm on May 18, 2022:

removed

in src/net_processing.cpp:285 in 66807193bf outdated

281@@ -282,9 +282,18 @@ struct Peer {
282         std::atomic<CAmount> m_fee_filter_received{0};
283     };
284 
285+    /** protects m_tx_relay */

MarcoFalke commented at 3:38 pm on May 17, 2022:

66807193bf92dcad6d27a5bf8e2e47b97d0bb6b7: Same

If this is something useful to maintain, it should be done by an automated linter to not tax review.

jnewbery commented at 2:32 pm on May 18, 2022:

removed

MarcoFalke approved

MarcoFalke commented at 3:46 pm on May 17, 2022: member

looking good. review ACK c4455b7d86b782b7c010857e04be5a73dd1dbb43, some doc feedback 🚓

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3looking good. review ACK c4455b7d86b782b7c010857e04be5a73dd1dbb43, some doc feedback 🚓
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhgTQv9EW6RubZe9M3V45udjFTMx1NNg/Chg5MNKvi5Old75iobKOtqJ+4mrgAx
 8lBNIwRaJaW8JLuf173YQyQqWYcwcGJnbmN7xStR7OSJ7a0Z8LBSBab30yOGHhspy
 9eAV9vJSxzdgq7flinRA1rJe1CR8zupDqGgMbAWdqsWo+HyQcdiM2HAy5QINRCw3Y
10ykENjyjQFytzEBub9GFBrwe95hrJGvQhNO4ewazNvecZtTf0gllq/PM5MUANb+Av
11J9nzFORGRBmAb2d/7ARIGGGwC1qA6I4vsk0DaWH8MfbGwIcA+GnbUuZW+aSh2al9
12Prk7r1aUY+BczUcb5WqzoS4NRVF7x6MGdrY9FJBoXYrow/SNIPF3iTjuFJ8NSgD2
137v9liK6adx2g8Jz7MO5+UjgrDXXDEAKLvz68mSaVktZghpjWQbKdqo9v/oi6INni
14noBAcGDMNmGg+hg9lGmjmRmHvc1z6Kj15qmqDtDP4+Vl82vpbOHFwotynkc6qMAM
15nMIbay5Q
16=JQJR
17-----END PGP SIGNATURE-----

in src/net_processing.cpp:2100 in c4455b7d86 outdated

2095@@ -2056,7 +2096,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
2096 
2097         const CInv &inv = *it++;
2098 
2099-        if (peer.m_tx_relay == nullptr) {
2100+        if (tx_relay == nullptr) {
2101             // Ignore GETDATA requests for transactions from blocks-only peers.

MarcoFalke commented at 4:29 pm on May 17, 2022:

0            // Ignore GETDATA requests for transactions from block-relay-only peers and peers that asked us not to relay txs to them.

Does this comment need an update in the last commit?

jnewbery commented at 2:34 pm on May 18, 2022:

Thanks. Updated.

jnewbery force-pushed on May 18, 2022

jnewbery commented at 2:34 pm on May 18, 2022: member

Thanks for the review @MarcoFalke. All review comments have been addressed.

MarcoFalke commented at 2:50 pm on May 18, 2022: member

You did the rebase on the wrong machine. The typo from #22778 (comment) is back again.

review ACK edf29446658ee9105b6451573687dc74a92604b9 💎

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK edf29446658ee9105b6451573687dc74a92604b9 💎
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiqcQv+NNN0jFiVXfsQikTdVkZEyp8Re02fU5bsk1AYZlB45koiyFQehpyC87wS
 8e9vTQpGGy3lu3JoYNjuIeJSC07MPTy8anqunZ2tHfe0xz9XCksqqml4DiFWHhPIr
 9tLnRxuamaBUy3VzDZN1hulLsG2JI/NB4TuxTEkZOjesiPqiAep79ugKCvm7828At
10EYyS/NVO7uQq2x0li/1KFtzRbC9wG/32AkNYMa7je+dlMkrE8InHcLqhCpQp2c89
118dhmUfKMYcxock8yTaQQH6iqEH9vxSBYccLHd9rOpDCrxYcQW39u/EPom4Esqvna
12O48dF3aV+lS/ztEB0qH5TtE9p9AdTPZaUw2lHwTzcZ4VgKgD2IyJzemA6a4Wnao/
13ymBpWk9RJoToBpKUelqMrkrLhXneIDAtAO28aiAAi0SuNsBxIbLLgweYUemom11L
14P/78PSLAEe3QvVpubDPULGXR2JMueWkcHYy09/TT4s44J4DnD/bUF4mpy6vz3fI9
15ZuJzQdNA
16=3z05
17-----END PGP SIGNATURE-----

in src/net_processing.cpp:2092 in edf2944665 outdated

2087@@ -2051,8 +2088,9 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
2088 
2089         const CInv &inv = *it++;
2090 
2091-        if (peer.m_tx_relay == nullptr) {
2092-            // Ignore GETDATA requests for transactions from blocks-only peers.
2093+        if (tx_relay == nullptr) {
2094+            // Ignore GETDATA requests for transactions from blocks-only peers and

MarcoFalke commented at 2:52 pm on May 18, 2022:

0            // Ignore GETDATA requests for transactions from block-relay-only peers and

nit in the last commit: Maybe clarify while touching this line

jnewbery commented at 4:32 pm on May 18, 2022:

Done

[net processing] [refactor] Move m_next_send_feefilter and m_fee_filter_sent

Move m_next_send_feefilter and m_fee_filter_sent out of the `TxRelay`
data structure. All of the other members of `TxRelay` are related to
sending transactions _to_ the peer, whereas m_fee_filter_sent and
m_next_send_feefilter are both related to receiving transactions _from_
the peer. A node's tx relay behaviour is not always symmetrical (eg a
blocksonly node will ignore incoming transactions, but may still send
out its own transactions), so it doesn't make sense to group the
feefilter sending data with the TxRelay data in a single structure.

This does not change behaviour, since IsBlockOnlyConn() is always equal
to !peer.m_tx_relay. We still don't send feefilter messages to outbound
block-relay-only peers (tested in p2p_feefilter.py).

42e3250497

[net processing] Comment all TxRelay members

This fully comments all the TxRelay members. The only significant change
is to the comment for m_relay_txs. Previously the comment stated that
one of the purposes of the field was that "We don't relay tx invs before
receiving the peer's version message". However, even without the
m_relay_txs flag, we would not send transactions to the peer before
receiving the `version` message, since SendMessages() returns
immediately if fSuccessfullyConnected is not set to true, which only
happens once a `version` and `verack` message have been received.

290a8dab02

[net processing] Add m_tx_relay_mutex to protect m_tx_relay ptr b0a4ac9c26

[net processing] Don't initialize TxRelay for non-tx-relay peers.

Delay initializing the TxRelay data structure for a peer until we receive
a version message from that peer. At that point we'll know whether it
will ever relay transactions. We only initialize the m_tx_relay
data structure if:

- this isn't an outbound block-relay-only connection; AND
- fRelay=true OR we're offering NODE_BLOOM to this peer
  (NODE_BLOOM means that the peer may turn on tx relay later)

9db82f1bca

jnewbery commented at 4:32 pm on May 18, 2022: member

You did the rebase on the wrong machine. The typo from #22778 (comment) is back again.

:man_facepalming: Thanks. Fixed.

jnewbery force-pushed on May 18, 2022

MarcoFalke commented at 5:12 pm on May 18, 2022: member

review ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 🖖

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4 🖖
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUia8gv/Y52Be4OmX6OF+MVJkvO5yvf1pgfk+5UrMwxwRHD+JPU/j09OSlRh9Iqh
 8VQfCIISSQgLbmxXuOODYgIPxK3WDkTBSH1zTNZWxfcODcXr539kTsJMq0Uh40TbI
 9hPIR+BrUJ+NTW1zQOJtMJeUHPsWTuomqbqYJ6iX360WnUE5D/mi/m2rBaOaq97De
10XDb059Ww4NgYmQVJJk3xbqiNskVG/sZkW78W0676+Gum+awt0+XK5aJTH/rFHAio
11uDmpAPPGMfnxcNnOi3rj0d/nI/lEBYX1+/0hlnhOZI6y2ordKokRe7ePDSok1MgK
12piRGIPbmHzptjnVWvBFybTN7k+zlMBmouG498mmwS2HAw/EaNE+WV0gK1JHycnj5
13538RWYKG/qaKc8dPHFjvCnV2QXhmfcoADYnPGmvqVqdS40aAT77upwDd3KB3PYm5
140Jm22Y+CbqT1y6NdUIV0YPVKrjylNrls6FMN/RnBcVNna/iWvR+UBI/sF3pMQiz+
15i/LQZkTg
16=/JFT
17-----END PGP SIGNATURE-----

dergoegge commented at 5:48 pm on May 18, 2022: member

Code review ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4

Only documentation changed since I last reviewed.

LarryRuane commented at 6:33 pm on May 18, 2022: contributor

I was just speaking with @glozow, and I’d like to host review club for this PR on June 8, if it’s not already merged by then, or maybe even if it is.

naumenkogs commented at 6:13 am on May 19, 2022: member

ACK 9db82f1bca0bb51c2180ca4a4ad63ba490e79da4

fanquake merged this on May 19, 2022

fanquake closed this on May 19, 2022

jnewbery deleted the branch on May 19, 2022

jnewbery commented at 9:48 am on May 19, 2022: member

I was just speaking with @glozow, and I’d like to host review club for this PR on June 8, if it’s not already merged by then, or maybe even if it is.

That’s great! I’m very happy to review notes & questions, or have a call to discuss. Feel free to message me.

MarcoFalke referenced this in commit aac99faa66 on May 20, 2022

sidhujag referenced this in commit 3045432985 on May 28, 2022

LarryRuane commented at 6:08 pm on June 8, 2022: contributor

Post-merge ACK I’d appreciate it if a maintainer could add the review club label.

MarcoFalke added the label Review club on Jun 8, 2022

dergoegge referenced this in commit e7a9133766 on Jun 22, 2022

fanquake referenced this in commit e05564d706 on Jun 23, 2022

sidhujag referenced this in commit 4c520d1677 on Jun 27, 2022

DrahtBot locked this on Jun 9, 2023

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Conflicts