p2p: Ensure transaction announcements are only queued for fully connected peers #26569

dergoegge · 2022-11-24T19:42:47Z

TxRelay::m_next_inv_send_time is initialized to 0, which means that any txids in TxRelay::m_tx_inventory_to_send will be announced on the first call to PeerManagerImpl::SendMessages for a fully connected peer (i.e. it completed the version handshake).

Prior to #21160, TxRelay::m_tx_inventory_to_send was guaranteed to be empty on the first SendMessages call, as transaction announcements were only queued for fully connected peers. #21160 replaced a CConnman::ForEachNode call with a loop over PeerManagerImpl::m_peer_map, in which the txid for a transaction to be relayed is added to TxRelay::m_tx_inventory_to_send for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as ForEachNode has a "fully connected check" before calling a function for each node.

DrahtBot · 2022-11-24T19:42:49Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, jnewbery
Concept ACK	naumenkogs, 1440000bytes
Stale ACK	mzumsande, luke-jr, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24125 (p2p: Replace RecursiveMutex m_tx_inventory_mutex with Mutex and rename it by w0xlt)

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.

maflcko

good catch

src/net_processing.cpp

test/functional/p2p_tx_privacy.py

jnewbery · 2022-11-24T20:58:41Z

Concept ACK.

Did you consider setting m_next_inv_send_time to a time point in the future inside SetTxRelay()? That seems like a simpler change, and avoids adding a new instance of ForEachNode() which we'd like to get rid off eventually.

dergoegge · 2022-11-25T12:55:15Z

@jnewbery Really good question! I should have put my reasoning for this in the PR description.

Did you consider setting m_next_inv_send_time to a time point in the future inside SetTxRelay()? That seems like a simpler change, and avoids adding a new instance of ForEachNode() which we'd like to get rid off eventually.

This actually was my initial approach, however I ended up going with ForEachNode for a couple reasons:

If we initialize m_next_inv_send_time to a time point in the future then we will still announce transactions that were received during the handshake which seems fine from a privacy perspective given the random delay. However,
- I could see how that leads to a followup bug in which the spy is able to tell which of the announced transactions were received during the handshake (e.g. see point below). We can eliminate the possibility entirely by going with the ForEachNode approach.
- A node can't know if a peer enables wtxid relay until the handshake completes and it might add txids to m_tx_inventory_to_send but announce them as MSG_WTX after the handshake. (Didn't test but i think) This again leaks which transactions were received between the spy sending version and wtxidrelay.
Storing the txids pre-handshake completion feels a bit weird from a resource consumption perspective. We rely on the 60 second p2p timeout (-peertimeout) to clear m_tx_inventory_to_send for peers that do not complete the handshake. Again not really an issue now but this doesn't feel robust w.r.t. to future changes to me.

glozow

ACK 4dc67b2

Agree with the approach to not add to m_tx_inventory_to_send until the handshake completes, and that not knowing wtxidrelay yet is also a good reason. Likewise not a fan of ForEachNode here but don't see a way around it given only CNode knows whether it's fully connected.

src/net_processing.cpp

luke-jr

ACK 959cc24

mzumsande

Concept ACK

test/functional/p2p_tx_privacy.py

maflcko

lgtm

src/net_processing.cpp

naumenkogs · 2022-11-29T08:25:11Z

Concept ACK.

Looking forward to resolving the comments :)

… fully connected peers

mzumsande

Code Review ACK 1220dfa

If you touch again, could run flake8 or similar over the test (it complains about an unused p2p_lock import and some blank lines)

jnewbery

Looks good. A couple of minor suggestions inline in the functional test.

test/functional/p2p_tx_privacy.py

luke-jr

re-ACK 152a42a

… pre-verack This commit documents our assumption about TxRelay::m_tx_inventory_to_send being empty prior to version handshake completion. The added Assume acts as testing oracle for our fuzzing tests to potentially detect if the assumption is violated.

glozow

ACK fbcd991

src/net_processing.cpp

test/functional/p2p_tx_privacy.py

ghost · 2022-11-30T22:00:35Z

Concept ACK

maflcko

ACK 8f2dac5 🔝

Show signature

Signature:

jnewbery

utACK 8f2dac5

jnewbery · 2022-12-02T14:11:09Z

src/net_processing.cpp

+            Assume(WITH_LOCK(
+                tx_relay->m_tx_inventory_mutex,
+                return tx_relay->m_tx_inventory_to_send.empty() &&
+                       tx_relay->m_next_inv_send_time == 0s));


(not in this PR) it might be nice to set explicitly set m_next_inv_send_time to a future time here. It's slightly odd that we go through the transaction sending logic on the first pass through SendMessages() even though m_tx_inventory_to_send will be empty.

… fully connected peers Github-Pull: bitcoin#26569 Rebased-From: 845e3a3

… pre-verack This commit documents our assumption about TxRelay::m_tx_inventory_to_send being empty prior to version handshake completion. The added Assume acts as testing oracle for our fuzzing tests to potentially detect if the assumption is violated. Github-Pull: bitcoin#26569 Rebased-From: ce63fca

Github-Pull: bitcoin#26569 Rebased-From: 8f2dac5

fanquake · 2022-12-02T16:08:13Z

Added this for backport in #26616.

…ueued for fully connected peers

ghost · 2022-12-03T00:10:44Z

Love that it was merged in less than 10 days, improves bitcoin and reviewed by more than 6 devs :)

8b726bf test: Coin Selection, duplicated preset inputs selection (furszy) 9d73176 test: wallet, coverage for CoinsResult::Erase function (furszy) 195f0df wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) e5d097b [test] Add p2p_tx_privacy.py (dergoegge) c842670 [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge) e15b306 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge) 95fded1 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow) d464b2a tests: Test for migrating encrypted wallets (Andrew Chow) 7a97a56 wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow) Pull request description: Backports remaining changes on the 24.0.1 milestone. Currently backports: * #26594 * #26569 * #26560 ACKs for top commit: josibake: ACK 8b726bf Tree-SHA512: db77ec1a63a7b6a4412750a0f4c0645681fc346a5df0a7cd38d5d27384e1d0fa95f3953af90042afe131ddbd4b6a6e009527095f13e9f58c0190cd378738a9e5

DrahtBot added the P2P label Nov 24, 2022

fanquake added the Needs backport (24.x) label Nov 24, 2022

dergoegge force-pushed the 2022-11-foreachnode-txrelay- branch 4 times, most recently from 8ea1f7d to c51d8b9 Compare November 24, 2022 20:34

maflcko reviewed Nov 24, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

test/functional/p2p_tx_privacy.py Outdated Show resolved Hide resolved

dergoegge force-pushed the 2022-11-foreachnode-txrelay- branch from c51d8b9 to 4dc67b2 Compare November 24, 2022 20:52

DrahtBot mentioned this pull request Nov 25, 2022

p2p: Replace RecursiveMutex m_tx_inventory_mutex with Mutex and rename it #24125

Closed

glozow reviewed Nov 28, 2022

View reviewed changes

maflcko reviewed Nov 28, 2022

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

dergoegge force-pushed the 2022-11-foreachnode-txrelay- branch from 4dc67b2 to 959cc24 Compare November 28, 2022 16:43

luke-jr approved these changes Nov 29, 2022

View reviewed changes

mzumsande reviewed Nov 29, 2022

View reviewed changes

test/functional/p2p_tx_privacy.py Outdated Show resolved Hide resolved

maflcko approved these changes Nov 29, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Nov 29, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

[net processing] Ensure transaction announcements are only queued for…

845e3a3

… fully connected peers

dergoegge force-pushed the 2022-11-foreachnode-txrelay- branch 2 times, most recently from 8c7d192 to 1220dfa Compare November 29, 2022 14:06

fanquake added this to the 24.0.1 milestone Nov 29, 2022

mzumsande reviewed Nov 29, 2022

View reviewed changes

dergoegge force-pushed the 2022-11-foreachnode-txrelay- branch from 1220dfa to 152a42a Compare November 29, 2022 16:22

jnewbery reviewed Nov 29, 2022

View reviewed changes

test/functional/p2p_tx_privacy.py Show resolved Hide resolved

test/functional/p2p_tx_privacy.py Outdated Show resolved Hide resolved

luke-jr approved these changes Nov 29, 2022

View reviewed changes

dergoegge force-pushed the 2022-11-foreachnode-txrelay- branch from 152a42a to fbcd991 Compare November 30, 2022 12:27

glozow reviewed Nov 30, 2022

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

maflcko reviewed Nov 30, 2022

View reviewed changes

test/functional/p2p_tx_privacy.py Outdated Show resolved Hide resolved

[test] Add p2p_tx_privacy.py

8f2dac5

dergoegge force-pushed the 2022-11-foreachnode-txrelay- branch from fbcd991 to 8f2dac5 Compare November 30, 2022 16:42

maflcko approved these changes Dec 1, 2022

View reviewed changes

maflcko requested review from luke-jr and mzumsande December 1, 2022 18:33

jnewbery reviewed Dec 2, 2022

View reviewed changes

fanquake merged commit 78aee0f into bitcoin:master Dec 2, 2022

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 2, 2022

[net processing] Ensure transaction announcements are only queued for…

e15b306

… fully connected peers Github-Pull: bitcoin#26569 Rebased-From: 845e3a3

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 2, 2022

[test] Add p2p_tx_privacy.py

e5d097b

Github-Pull: bitcoin#26569 Rebased-From: 8f2dac5

fanquake mentioned this pull request Dec 2, 2022

[24.x] Backports for 24.0.1 #26616

Merged

fanquake removed the Needs backport (24.x) label Dec 2, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 2, 2022

Merge bitcoin#26569: p2p: Ensure transaction announcements are only q…

ac5bdf6

…ueued for fully connected peers

ajtowns mentioned this pull request Jan 23, 2023

refactor: Continue moving application data from CNode to Peer #26621

Closed

bitcoin locked and limited conversation to collaborators Dec 3, 2023

p2p: Ensure transaction announcements are only queued for fully connected peers #26569

p2p: Ensure transaction announcements are only queued for fully connected peers #26569

Uh oh!

Conversation

dergoegge commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Nov 24, 2022

Uh oh!

dergoegge commented Nov 25, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luke-jr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Nov 29, 2022

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Nov 30, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

fanquake commented Dec 2, 2022

Uh oh!

ghost commented Dec 3, 2022

Uh oh!

Uh oh!

dergoegge commented Nov 24, 2022 •

edited

Loading

DrahtBot commented Nov 24, 2022 •

edited

Loading

luke-jr left a comment •

edited

Loading