net processing: Reduce resource usage for inbound block-relay-only connections #22778
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 23, 2021
Contributor
|
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. |
This was referenced Aug 23, 2021
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
from
238ce0b to
cb2c083
Compare
Contributor
Author
|
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
Contributor
Author
|
rebased |
This was referenced Aug 27, 2021
Contributor
|
Concept ACK |
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
from
4ee20ce to
bb89747
Compare
Contributor
Author
|
Rebased on the latest #21160 |
This was referenced Sep 24, 2021
Contributor
|
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. |
Contributor
Author
@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 |
Contributor
Author
|
Thanks for the review @MarcoFalke. All review comments have been addressed. |
Member
|
You did the rebase on the wrong machine. The typo from #22778 (comment) is back again. review ACK edf29446658ee9105b6451573687dc74a92604b9 💎 Show signatureSignature: |
maflcko
reviewed
May 18, 2022
…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)
Contributor
Author
🤦♂️ Thanks. Fixed. |
jnewbery
force-pushed
the
2021-02-tx-relay-init
branch
from
edf2944 to
9db82f1
Compare
Member
|
review ACK 9db82f1 🖖 Show signatureSignature: |
Member
|
Code review ACK 9db82f1 Only documentation changed since I last reviewed. |
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. |
Contributor
|
ACK 9db82f1 |
Contributor
Author
That's great! I'm very happy to review notes & questions, or have a call to discuss. Feel free to message me. |
This was referenced May 20, 2022
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
May 20, 2022
… 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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 28, 2022
Contributor
|
Post-merge ACK |
dergoegge
added a commit
to dergoegge/bitcoin
that referenced
this pull request
Jun 22, 2022
…lters This line was accidentally removed in bitcoin#22778.
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
Jun 23, 2022
…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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 27, 2022
… 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.