net processing: Reduce resource usage for inbound block-relay-only connections #22778
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
from
238ce0b to
cb2c083
Compare
|
I've pushed a new branch that simplifies the implementation a bit. I'd previously wanted to amalgamate the various mutexes and atomics in |
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
from
cb2c083 to
bc91085
Compare
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
2 times, most recently
from
8653f7d to
4ee20ce
Compare
|
rebased |
|
Concept ACK |
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
from
4ee20ce to
bb89747
Compare
|
Rebased on the latest #21160 |
|
Some rough estimates on how much this will save: 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. |
@vasild This seems completely wrong. Lines 94 to 98 in 419afa9 the storage requirement for just that rolling bloom filter is ~1.8 * 50,000 * 6 = 540kB |
|
Thanks for the review @MarcoFalke. All review comments have been addressed. |
|
You did the rebase on the wrong machine. The typo from #22778 (comment) is back again. review ACK edf2944 💎 Show signatureSignature: |
…er_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).
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.
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)
🤦♂️ Thanks. Fixed. |
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
from
edf2944 to
9db82f1
Compare
|
review ACK 9db82f1 🖖 Show signatureSignature: |
|
Code review ACK 9db82f1 Only documentation changed since I last reviewed. |
|
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. |
|
ACK 9db82f1 |
That's great! I'm very happy to review notes & questions, or have a call to discuss. Feel free to message me. |
… by propagating some negative capabilities 2b3373c refactor: Propagate negative `!m_tx_relay_mutex` capability (Hennadii Stepanov) 5a6e3c1 refactor: Propagate negative `!m_most_recent_block_mutex` capability (Hennadii Stepanov) Pull request description: This PR is a follow-up for bitcoin/bitcoin#22778 and bitcoin/bitcoin#24062, and it seems [required](bitcoin/bitcoin#24931 (comment)) for bitcoin/bitcoin#24931. See details in the commit messages. ACKs for top commit: ajtowns: ACK 2b3373c w0xlt: ACK bitcoin/bitcoin@2b3373c Tree-SHA512: 8a4bb9641af8d79824ec12e2d6bfce0e09524094b298a2edcdb2ab115fbaa21215b9c97a6b059f8aa023551071fd5798eca66ab8d262a3f97246a91d960850d0
|
Post-merge ACK |
…lters This line was accidentally removed in bitcoin#22778.
…eceiving BIP37 filters e7a9133 [net processing] Set CNode::m_relays_txs=true when receiving BIP37 filters (dergoegge) Pull request description: This line was accidentally removed in bitcoin/bitcoin#22778. Receiving a `filterload` message implies that we should relay txs to the sender (`CNode::m_relays_txs = true`). `CNode::m_relays_txs` is only used for the inbound eviction logic, so removing the line might have slightly changed the eviction behaviour but nothing else. ACKs for top commit: laanwj: Code review ACK e7a9133 vasild: ACK e7a9133 Tree-SHA512: 19c5df0f579f707c6c7900d12a6b71ac69e802be64f7d2fdcc40ac714c918dc4c17def164592f8836cc105a03daefefca3ca5e10423145eca8db4348c27c9cfc
… BIP37 filters e7a9133 [net processing] Set CNode::m_relays_txs=true when receiving BIP37 filters (dergoegge) Pull request description: This line was accidentally removed in bitcoin#22778. Receiving a `filterload` message implies that we should relay txs to the sender (`CNode::m_relays_txs = true`). `CNode::m_relays_txs` is only used for the inbound eviction logic, so removing the line might have slightly changed the eviction behaviour but nothing else. ACKs for top commit: laanwj: Code review ACK e7a9133 vasild: ACK e7a9133 Tree-SHA512: 19c5df0f579f707c6c7900d12a6b71ac69e802be64f7d2fdcc40ac714c918dc4c17def164592f8836cc105a03daefefca3ca5e10423145eca8db4348c27c9cfc
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
TxRelaydata 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 aTxRelaydata 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
fRelayfield in theversionmessage may be set to0to 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 afilterloadorfilterclearmessage, but only if we have offeredNODE_BLOOMservices to that peer.NODE_BLOOMservices are disabled by default, and it has been recommended for some time that users not enableNODE_BLOOMservices on public connections, for privacy and anti-DoS reasons.Therefore, if we have not offered
NODE_BLOOMto the peer and it has setfRelayto0, then we know that it will never request transaction announcements, and that we can save resources by not initializing theTxRelaydata structure.