Relay own transactions only via short-lived Tor or I2P connections #27509

To improve privacy, relay locally submitted transactions (from the wallet or via RPC) to the P2P network only via Tor or I2P short-lived connections.

• Introduce a new connection type for relaying sensitive data (our own transactions) with the following properties:

  • started whenever there are local unbroadcast transactions to be sent
  • only opened to Tor or I2P peers
  • opened regardless of max connections limits
  • after handshake is completed one local transaction is pushed to the peer and the connection is closed
  • ignore all incoming messages after handshake is completed

• Relay locally submitted transactions (from the wallet or via RPC) using this new mechanism, to a few Tor or I2P peers.

• Hide those transactions from GETDATA and MEMPOOL requests, as if we don't have them.

• Once we get an INV from somebody, request the transaction with GETDATA, as if we didn't have it before.

• After we receive the full transaction as a TX message, in reply to our GETDATA request, only then consider the transaction has propagated through the network and remove it from the unbroadcast list.

• INV it to others as a result of the TX message we get, like what we do with transactions that are not ours.

The messages exchange should look like this:

tx-sender >--- connect -------> tx-recipient
tx-sender >--- VERSION -------> tx-recipient
tx-sender <--- VERSION -------< tx-recipient
tx-sender <--- WTXIDRELAY ----< tx-recipient (maybe) 
tx-sender <--- SENDADDRV2 ----< tx-recipient (maybe) 
tx-sender <--- SENDTXRCNCL ---< tx-recipient (maybe) 
tx-sender <--- VERACK --------< tx-recipient
tx-sender >--- VERACK --------> tx-recipient
tx-sender >--- TX ------------> tx-recipient
tx-sender >--- PING ----------> tx-recipient
tx-sender <--- PONG ----------< tx-recipient
tx-sender disconnects

Whenever a new local transaction is received (from the wallet or RPC), the node will send it to 5 (SENSITIVE_RELAY_NUM_BROADCAST_PER_TX) recipients right away. If after 10-15 mins we still have not heard anything about the transaction from the network, then it will be sent to 5 more peers (see PeerManagerImpl::ReattemptInitialBroadcast()).

A few considerations:

• The short-lived sensitive relay connections are very cheap and fast wrt network traffic. It is expected that some of those peers will blackhole the transaction. Just one honest/proper peer is enough for successful propagation.
• The peers that receive the transaction could deduce that this is initial transaction broadcast from the transaction originator. This is ok, they can't identify the sender.

TODO:

• Disable if -connect is used or Tor and I2P are not reachable.
• Use I2P transient connections even if listening on I2P.
• Do something with the user-agent string (could reveal identity if it has been personalized) and make sure no identifying information in the VERSION message. Done: took the suggestion below.
• Improve the condition when we consider the transaction seen by the network: right now a single INV from somebody suffices (this is still an improvement over master which is ok right after pushing the tx to a peer). We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not the ones we broadcasted to (via short-lived connection). Edit: One INV should be enough because after that we broadcast the transaction to everybody we are connected to (like if it is not ours).
• Idea: start with one connection per transaction (not 5 as it is now), optimistically assuming it will be sufficient. (numbers are examples) After 1 minute, if still not received from the network, try 2 connections, after a few minutes try more. If still unsuccessful after 10 minutes, then fall back to the old mechanism.

This is supposed to address: #3828 #14692 #19042 #24557 #25450

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

For detailed information about the code coverage, see the test coverage report.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28222 (Use shared_ptr for CNode inside CConnman by willcl-ark)
• #28170 (p2p: adaptive connections services flags by furszy)
• #28107 (util: Type-safe transaction identifiers by dergoegge)
• #28043 (fuzz: Test headers pre-sync through p2p interface by dergoegge)
• #27837 (net: introduce block tracker to retry to download blocks after failure by furszy)
• #27826 (validation: log which peer sent us a header by Sjors)
• #27675 (p2p: Drop m_recently_announced_invs bloom filter by ajtowns)
• #27539 (init: Fixes for file descriptor accounting by Empact)
• #27407 (net, refactor: Privatise CNode send queue by dergoegge)
• #27213 (p2p: Diversify automatic outbound connections with respect to networks by amitiuttarwar)
• #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
• #26697 (logging: use bitset for categories by LarryRuane)
• #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
• #25832 (tracing: network connection tracepoints by 0xB10C)
• #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
• #25284 (net: Use serialization parameters for CAddress serialization by MarcoFalke)
• #25268 (refactor: Introduce EvictionManager by dergoegge)

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.

Concept ACK - cool!

Not sure about "only" relaying to Tor and I2P but seems useful as an option.

after handshake is completed one local transaction is pushed to the peer and the connection is closed

#4432 comes to mind

Concept ACK

Concept ACK

Looking at the current implementation, I am not sure how useful this is going to be, if the transaction is sent along with the potentially uniquely identifying UA comment, or other fingerprints in the version message. Maybe it makes sense to define a static version message, similar to how the tor browser replaces headers with constants?

@MarcoFalke Good idea! Perhaps it's worth mimicking bitcoin-submittx (https://github.com/laanwj/bitcoin-submittx/blob/master/bitcoin-submittx#L28) ?

Looking at the current implementation, I am not sure how useful this is going to be, if the transaction is sent along with the potentially uniquely identifying UA comment, or other fingerprints in the version message. Maybe it makes sense to define a static version message, similar to how the tor browser replaces headers with constants?

Or it could randomize it, from the list of different version strings.

One shot connections + immediate tx broadcast without inv,getdata exchange leaks that the tx is likely being broadcast for the first time (easy to censor the tx as the receiver).

Perhaps it is worth combining this with #27213 and instead of one-shot connections we broadcast only to existing selected connections on these privacy networks. Our own transactions would then still benefit from being broadcast along side other transactions.

Concept ACK. Not sure if this should be the default or an optional mode (in some cases, a faster and more reliable transmission may be more important than privacy).

It might also be helpful to combine this with some heuristics that the tx percolated through the network: For example, require that 2 of your regular full outbound connections (that you didn't send it to ) have inv'ed the tx back to you - otherwise try again after some time.

potentially uniquely identifying UA comment, or other fingerprints in the version message

added to the TODO in the OP

One shot connections + immediate tx broadcast without inv,getdata exchange leaks that the tx is likely being broadcast for the first time (easy to censor the tx as the receiver).

Yes. I was thinking its probably ok to broadcast to a few peers, not just one. And if they blackhole it, then the code will retry. We can retry more aggressively if it keeps being blackholed. I think it is ok to assume that there are some honest nodes in our addrman.

Perhaps it is worth combining this with #27213 and instead of one-shot connections we broadcast only to existing selected connections on these privacy networks.

There are two aspects of this:

1. Linking a bitcoin transaction to IP address/geolocation. Broadcasting to long-lived Tor/I2P connections solves this (modulo other issues like linking Tor nodes with IP addresses).
2. Linking one bitcoin transaction with another bitcoin transaction that otherwise are unrelated. To solve/mitigate this we need to send each transaction over its individual connection and that connection to not be linkable to other connections.

Not sure if this should be the default or an optional mode

Right, also assuming there will be bugs/unexpected things - maybe have an option to enable/disable this. Hardest would be to introduce it without an option to disable it. Softest approach would be: 1. introduce under an option with default off, 2. in next version flip the default to on (if it seems to be working well)

It might also be helpful to combine this with some heuristics that the tx percolated through the network: For example, require that 2 of your regular full outbound connections (that you didn't send it to ) have inv'ed the tx back to you - otherwise try again after some time.

I actually changed the behavior to do something like that in 512077bb8805eacb671eabcf9d62463a2a6e6a39 net_processing: relay local transactions via the SENSITIVE_RELAY mechanism in this PR: see where RemoveUnbroadcastTx() was called before / is called now. Maybe that deserves a separate commit. Before we would be ok as long as we pushed the tx to the peer. Now we wait until we receive an INV about that tx from somebody. There is some room for improvement - added in the TODO in the OP.

I think that in the suggested approach we might reveal information by not inv'ing our own tx to others. If a spy is connected to each node, we might send the tx to them first currently (master), but with the PR, if we are now the only node that never inv's the tx to them at all (provided that all other nodes do that eventually with the flooding mechanism) that would equally identify us as the source.

@mzumsande, right, ideally, after the initial broadcast via the short-lived connection, the node should behave as if it does not know the tx:

• before it gets INV for that tx, exclude it from GETDATA and MEMPOOL replies
• if it gets INV for that tx, request the entire tx via GETDATA
• broadcast to others

Concept ACK.

Might be good to have some way of being able to configure where to connect to for sensitive relay, defaulting to tor/i2p, but so that you could perhaps enable specific ipv4/ipv6 addresses as well, particularly for testing purposes.

concept ACK

a high level question: the privacy benefit comes from the guarantees of using the altnets - the lifecycle of the connection is expected to reveal that the tx is an initial broadcast. if a spy is able to reliably fingerprint a node between networks, this mechanism could potentially degrade privacy. could it be worthwhile to mitigate this by occasionally broadcast non-wallet txs via the same short-lived connections, or broadcast a couple additional txs over the same connection? obv some big tradeoffs, just want to consider the possibility. even if we are able to ensure no leaks now, it is difficult to guarantee this over time of developing the project.

Concept ACK

Disagree with:

opened regardless of max connections limits

The connection should be established after another tor/i2p connection is dropped - this way the connection appears to be just another connection - nothing special about it.

It may also be useful to randomize the duration of the connection?

Additionally - padding the connection with dummy txs during its lifecycle may help obfuscate the real payload?

Concept ACK

One shot connections + immediate tx broadcast without inv,getdata exchange leaks that the tx is likely being broadcast for the first time (easy to censor the tx as the receiver).

I agree. My main concern is about how to ensure the transaction has not been censored. It seems easy for an attacker to identify someone opened a connection just to relay its own transaction.

Now we wait until we receive an INV about that tx from somebody

Just to clarify: We would open the short-lived connection to relay our own transaction and close it right after and then, we wait until we receive an INV about that tx from somebody, if not, we would try to do it (open a new short-lived conn...) all over again?

Concept ACK!

We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not the ones we broadcasted to (via short-lived connection).

Ideally, if outbound has clearnet connections, we would resume normal operation IFF one of those connections INV's it. This reduces risk of sybil influencing the node.

Thanks all for the feedback!

Just to clarify: We would open the short-lived connection to relay our own transaction and close it right after and then, we wait until we receive an INV about that tx from somebody, if not, we would try to do it (open a new short-lived conn...) all over again?

Yes. But we may as well send to a few peers at the same time, then wait. And, I am not sure, maybe if it does not work for a long time, then fall back to the existent mechanism?

The worst that can happen is if our addrman is filled with malicious Tor peers that blackhole such initial announcements. But even if 90% of them are blackholes, then after a dozen attempts we should eventually send to a honest node which will propagate it to the network. Attempts are cheap and don't reveal any of the identity of the sender.

>> This is ready for review and testing <<

... if the transaction is sent along with the potentially uniquely identifying UA comment, or other fingerprints in the version message. Maybe it makes sense to define a static version message ... @MarcoFalke, I went with a completely static VERSION message

... Perhaps it's worth mimicking bitcoin-submittx @sipa, done

Or it could randomize it, from the list of different version strings. @kristapsk, could be, but I did not do that (yet). I think a constant suffices and is the simplest thing to do.

Not sure if this should be the default or an optional mode @mzumsande, I made it optional, guarded by a new option -sensitiverelayowntx which is "off" by default. If all goes well, we can consider flipping the default to "on" after one release.

It might also be helpful to combine this with some heuristics that the tx percolated through the network: For example, require that 2 of your regular full outbound connections (that you didn't send it to ) have inv'ed the tx back to you - otherwise try again after some time.

That makes sense and is a room for improvement, let me summarize:

• In master we consider the transaction percolated through the network after we push it to a peer (no wait for INV from somebody else).
• In this PR, currently, we want for INV from somebody, ask for the full transaction with GETDATA and after we receive it, then consider it done.

I am trying to strike a balance between simplicity (too simple and maybe it will break) and robustness (requires more complexity). Now, at the cost of more complexity we can require more:

• an INV from more than one peer.
• an INV from IPv4, IPv6 or CJDNS peer.
• an INV from an outbound peer.

We have to start behaving normally after we get the first INV, which means to relay to all peers and include the transaction in GETDATA and MEMPOOL replies. Thus to achieve any of the above, it would be necessary to split the list of "unbroadcast" transactions in two:

1. (now) transactions that we have to send via short-lived sensitive relay connections and we hide from GETDATA and MEMPOOL and ask for the full transaction once we get INV
2. (new list) transactions that we have to send via short-lived sensitive relay connections, but we have already received them via at least one INV, so include them in GETDATA and MEMPOOL

If a spy is connected to each node, we might send the tx to them first currently (master), but with the PR, if we are now the only node that never inv's the tx to them at all (provided that all other nodes do that eventually with the flooding mechanism) that would equally identify us as the source. @mzumsande, I believe that is fixed now - once we get an INV, we will flood it to everybody and start behaving normally.

Might be good to have some way of being able to configure where to connect to for sensitive relay, defaulting to tor/i2p, but so that you could perhaps enable specific ipv4/ipv6 addresses as well, particularly for testing purposes. @ajtowns, sensitive relay via IPv4 seems like a way to shoot yourself in the foot. I extended the testing framework a bit to have a proper functional test for this.

... if a spy is able to reliably fingerprint a node between networks, this mechanism could potentially degrade privacy ... @amitiuttarwar, no because the spy will not be able to relate the sensitive relay connection to another connection with the transaction originator on the same network.

The connection should be established after another tor/i2p connection is dropped - this way the connection appears to be just another connection - nothing special about it. It may also be useful to randomize the duration of the connection? Additionally - padding the connection with dummy txs during its lifecycle may help obfuscate the real payload? @RandyMcMillan, I am not sure why another connection should be dropped. I think adding more transactions in the broadcast is going to bring little value because they will be transactions that have already propageted through the network. A spy would be able to identify which one is the unbroadcast transaction. The point here is that even that the spy knows this is initial broadcast, they can't identify the sender and the worst they can do is blackhole the transaction.

One shot connections + immediate tx broadcast without inv,getdata exchange leaks that the tx is likely being broadcast for the first time (easy to censor the tx as the receiver).

I agree. My main concern is about how to ensure the transaction has not been censored. It seems easy for an attacker to identify someone opened a connection just to relay its own transaction. @brunoerg, Yes, that should be ok.

Ideally, if outbound has clearnet connections, we would resume normal operation IFF one of those connections INV's it. This reduces risk of sybil influencing the node. @instagibbs, Yes, see a few lines above, we would need two lists of unbroadcast transactions.

b6fd6b525d...92fb45b5ef: rebase and address suggestions

concept ACK

Reviewed all code - really great job with this! Very cool idea. I will continue to track this PR and look forward to testing it out more and trying to break the functional tests ;-)

I have a few questions below about implementation

Correct me please if I'm missing anything. Considering:

- Once we get an INV from somebody, request the transaction with GETDATA, as if we didn't have it before.

- After we receive the full transaction as a TX message, in reply to our GETDATA request, only then consider the transaction has propagated through the network and remove it from the unbroadcast list.

and

We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not the ones we broadcasted to (via short-lived connection)

In case my node doesn't accept inbound connections and supports clearnet + tor, however, my addrman has majority ipv4/6 addresses, seems like there is a chance of not having any connection with a Tor peer, so it would make harder to know that my transaction has been propagated? Seems like #27213 could boost this feature?

92fb45b5ef...c9acf32606: make the python linter happy and avoid a race in the test - wait_for_connect() could time out as if a connection never arrived even if it did. This happens if the peer is connects and disconnects before wait_for_connect() is called.

c9acf32606...2eae1f8f13: make the python linter happy (even more)

would like to see an updated OP that details what changes have been made, including precisely how often these connections are being made

In case my node doesn't accept inbound connections and supports clearnet + tor, however, my addrman has majority ipv4/6 addresses, seems like there is a chance of not having any connection with a Tor peer, so it would make harder to know that my transaction has been propagated? @brunoerg, Why? I couldn't follow. But anyway - as of now this PR does not wait for more than one peer to give back the transaction to us.

General question about this: what will be the impact on tor/i2p nodes? (Like, resources-wise for those operators) are there even enough hidden services nodes with inbound slots to support this? In a world where all TXs are broadcast to the network through "exit nodes" are there any other possible drawbacks ?

@instagibbs added some more info to the OP, but feel free to reach on #bitcoin-core-dev to clarify things. Once a local transaction is is received, 5 such connections will be made right away. If we still consider the transaction unbroadcast after 10-15 mins, then it will be retried (5 new connections). These 10-15 mins are how master does things now but I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minutes broadcast to 10 peers.

General question about this: what will be the impact on tor/i2p nodes? (Like, resources-wise for those operators) are there even enough hidden services nodes with inbound slots to support this? @pinheadmz, good question. Traffic-wise - the selected recipients will receive the transaction via a short-lived connection, but even now they do receive the transaction in one way or another. The overhead is the extra handshake of the new connection. Some quick tests show, from the point of view of the tx sender: sent 180 bytes (excluding the transaction itself), received 239 bytes.

Inbound slots-wise: Some back of the envelope calculation:

• The historical maximum for the Bitcoin network of incoming transactions has been about 10 new transactions per second, typically that is 3-4 tx/sec. Lets say 3..10 tx/sec.
• Each new tx causes 5 new connections to be opened, so for one second 15..50 new connections will be opened in total in that second (3*5..10*5).
• Lets say one short-lived connection takes 2..10 seconds. That is a moving window of 2..10 seconds.
• In the next 2..10 seconds they will not be closed and so for 2..10 seconds 30..500 new connections will be opened (2*15..10*50).
• The constant load of those 30..500 extra connections will be spread evenly amongst ~10k listening Tor or I2P nodes. So each listening Tor or I2P node will have to handle 0.003..0.05 more incoming connections.

In a world where all TXs are broadcast to the network through "exit nodes" are there any other possible drawbacks ?

By "exit nodes" you mean the new sensitive relay mechanism and not Tor-to-IPv4 exit nodes, right? If the Tor or I2P network stops working then the Bitcoin nodes that are connected to it and have the sensitive relay enabled will not be able to broadcast their transactions. Maybe we should eventually fall back to the old relay mechanism after some time has passed in unsuccessful sensitive relay attempts.

But notice - as the author of this, I have a blind spot for its drawbacks!

I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minutes broadcast to 10 peers.

I think for first cut matching current behavior is probably best. If Tor is being sybiled, I don't think hammering things even harder is going to fix much. Time-sensitive users should probably just avoid using this feature altogether, at least until there is some sort of fallback mechanism introduced.

Tor-to-IPv4 exit nodes,

I was more asking about this, not as much from a vulnerability standpoint but more like, the bottleneck created by all TXs going through a specific subset of network nodes first. I think your envelope math is reassuring though - I didn't realize there were so many Tor nodes already

reviewed just the first few commits so far

Tor-to-IPv4 exit nodes,

I was more asking about this, not as much from a vulnerability standpoint but more like, the bottleneck created by all TXs going through a specific subset of network nodes first. I think your envelope math is reassuring though - I didn't realize there were so many Tor nodes already

Well, ok, we don't know how many of those Tor or I2P nodes are also connected to clearnet. For this secondary propagation (after sensitive relay has fired the transaction) however it does not matter if sensitive relay was used initially or not. The transaction will go to/from all the networks anyway due to the flooding mechanism like it happens currently.

2eae1f8f13...ff88e08363: address suggestions

ff88e08363...5fd41b9fbf: limit the maximum number of sensitive relay connections that can be opened (discussion: #27509 (review))

5fd41b9fbf...bde055d28f:

• rebase due to conflicts
• adjust the added test to expect that some non-sensitive relay connection may be opened before sensitive tx relay is complete; this is because the code was changed to be less aggressive and may open other connections even if sensitive relay ones are needed
• extract a move-only change in ThreadOpenConnections() to a separate commit to make it easier to review (new commit: ec6f4ac210 net: move peers counting before grant acquisition in ThreadOpenConnections())

code review ACK bde055d28f5d41d9e923f770d130010e72441e91

Read through each commit and checked build at each commit. Left a few questions / comments below. I am going to spend more time examining the test coverage and playing with the feature on signet in the next few days.

Really excellent work! Commit layout makes this branch easy and legible to review. It's a great idea and I am also very excited about an actual working socks5 proxy in the test suite ;-)

Not sure how effective it is to put the transaction into the mempool. I know that your implementation treats the positive case (reply to an inv as if the tx wasn't there, or withhold the tx in reply to a getdata). However, a failure to reply to an otherwise valid inv can still be used to query the mempool. Or similarly, a failure to (also) withhold related transactions in an otherwise valid getdata can be used to query the mempool. (For example, if the transaction was a CPFP and the parent can't be in your mempool due to the minimum fee-rate, but your node happily replies).

I don't think there is a general solution to this, other than to never add the transaction to the mempool.

bde055d28f...96ea776776: address suggestions

96ea776776...7a3206dc8e: rebase due to conflicts

7a3206dc8e...2541f09439: add a comment wrt #27509 (review)

code review re-ACK 2541f09439099ec3e73f7c5a12f809f190e6af1d

confirmed changes since last ack were rebase and small review comments addressed. Found a typo, and played with the feature on signet (longer report about that to follow)

Ran with this feature on signet and found a few unexpected behaviors:

1. -sensitiverelay=1 and -onlynet=ipv4 should probably throw an init error. In this configuration, sensitive stuff seems to be ignored and new wallet TXs are relayed via ipv4 peers like normal. Is "only use tor for sensitive relay" a possible configuration? 👉 #27509 (review)

2. Rapidly sending multiple transactions "...incremented the number of connections to open from 20 to 25..." this number grows rapidly and decreases slowly (especially on signet) I wonder if it should max out at some point or reduce the tx:relay-peer ratio from 5:1 to just 1:1 👉 #27509 (review)

3. Restarting the node and reloading the wallet logged messages about pushing the unbroadcast TXs to the mempool, but I do not think any sensitive-relay connections were triggered to deliver those. Eventually I tried sending another new TX and then, even though only 5 sensitive connections were being opened for that one new TX, over time those connections were used for some of the backlog ("pretending peer requested tx..."). Until the 5 sensitive connections dropped down to 0. **update: eventually ReattemptInitialBroadcast triggered and the backlog increased the count to 100 sensitive connections 👉 #27509 (review)

4. Finally, on signet I never got an INV for one of my own txs back. so getmempoolinfo : unbroadcastcount only ever increased. I notice that the tests only use python P2PInterface()'s for peers and the observer_outbound peer and is manually forced to inv our txid... makes me wonder if real bitcoind peers have different enough relay behavior that it doesnt work as well in production. For example, we pretend our sensitive relay peers requested a tx when they didn't. Does bitcoind still relay TXs it didn't getdata for? Also I suppose given that some of my signet TXs weren't going out, there could be an issue with orphans and peers that are missing my parents. I may try writing tests with more network bitcoind nodes, or set it up locally but routed through Tor. 👉 #27509 (review)

I'm going to continue playing with all this and see if I can figure anything out. It's definitely cool to see some srelay connections pop up in the -netinfo dashboard!

Approach NACK, (but Concept ACK). Leaving a NACK only to have DrahtBot create an anchor to this comment, to avoid GitHub from hiding it as well.

I don't think you've seen #27509 (comment) and GitHub is already hiding it by default.

@MarcoFalke, I am not sure I fully understand in your comment. Can you elaborate on the CPFP example?

In general, wrt the privacy of the transaction originator, do you think that the "sensitive relay" mechanism from this PR could be worse than the current "send to all" from master in some cases? Or do you think that it is better than master but it can be done even better?

do you think that the "sensitive relay" mechanism from this PR could be worse

Yes, my understanding is that this pull strictly decreases privacy when the underlying wallet makes use of transactions chains (whether or not for intentional or accidental CPFP shouldn't matter), or transaction replacements, or mutually exclusive transactions ("conflicts", "double-spends" or insufficient rbf-fee), or if the transaction recipient is the attacker and can create transactions spending the "sensitive" output. The CPFP example is just an example. The general idea is that it is impossible to hide a transaction being in the mempool once it is added to the mempool.

The reason why this pull makes it strictly worse, is because previously an attacker could only guess which was the originating node. Now, the attacker can uniquely identify the node.

For example (using the CPFP example):

• Wallet creates a low_fee_tx and relays it to the whole network.
• A day later, Wallet sees the low_fee_tx was dropped from the local (small) mempool (with a lower minfee than the rest of the network) and creates a high_fee_tx child, adding back the parent to the mempool.
• The attacker receives high_fee_tx as "sensitive" transaction. (With less precision the attack should work as well, if the attacker received the transactions as a fluffed transaction from the network).
• The attacker, being connected to all clearnet network nodes and knowing the minfee of all network nodes (via the feefilter message), ask all nodes for high_fee_tx and low_fee_tx.
• The only node that withholds high_fee_tx and replies with low_fee_tx, despite it violating the minfeefilter it requested its peers to honour, is the victim node. Thus, it is uniquely identified.

I don't think this unique identification is currently possible.

Let me know if I am missing something, or if I should come up with more examples.

I think this approach is better suited for a separate tool. Otherwise fingerprinting becomes a direct vector for tx origin analysis (see my inline comments), which seems like a regression from what we have now.

I think the only correct approach to implement something like this, is to keep the unbroadcast transaction within the wallet (in a special unbroadcast state until it's echoed back). In this unbroadcast state, rebroadcasts can happen (directly to sensitive relay peers only, bypassing the mempool), but outputs are not available for creating descendant transactions.

but outputs are not available for creating descendant transactions.

In theory they could be used to create transactions, under the assumption that created transactions generally and eventually relay, but that can be fixed independent of the relay issue here. Pretty sure there is a tracking issue about this already, which likely affects -nowalletbroadcast and -nospendzeroconfchange users.

Thank you for looking into this! I will address trivial stuff/comments first and then will try to address the other concerns in chronological order.

I am open to exploring in more detail the option of not putting the transaction in the mempool. I considered this in the beginning but I got the impression that this is going to be more complicated. Anyway I want to first try to address all concerns or, if not possible, get convinced that putting the transaction in the mempool is non-workable. Edit: some parts of this PR will be reused even if changed to skip the mempool.

I don't like the idea of a separate tool because that's like saying "we can't get the current software to do it properly, so we will create another software". @MarcoFalke, yes, more concrete examples to try to shoot this down are welcome ;-)

I considered this in the beginning but I got the impression that this is going to be more complicated.

My guess would have been that it will be a lot simpler, at least for reviewers.

Edit: some parts of this PR will be reused even if changed to skip the mempool.

Yes, everything except for the mempool / high level p2p stuff can probably stay as-is.

@MarcoFalke, yes, more concrete examples to try to shoot this down are welcome ;-)

Would be good to explain what is wrong/missing from the example and explanations that are given above.

I don't like the idea of a separate tool because that's like saying "we can't get the current software to do it properly, so we will create another software".

"we can't get the current software to do it properly" that is kinda true w.r.t. fingerprinting, or it would at least require significant effort to avoid it completely. Fingerprinting negates the usefulness of this feature, because "The peers that receive the transaction could deduce that this is initial transaction broadcast from the transaction originator. This is ok, they can't identify the sender" is no longer true (for listening nodes).

I'd prefer a separate tool as we otherwise turn fingerprinting into a big maintenance issue. A separate tool guarantees that there is no internal state overlap which makes fingerprinting very hard (if not impossible).

I'd prefer a separate tool as we otherwise turn fingerprinting into a big maintenance issue. A separate tool guarantees that there is no internal state overlap which makes fingerprinting very hard (if not impossible).

Not sure why this requires a separate tool? I think a separate internal module (or internal library that is linked, or whatever) should be able to achieve the same, with the added benefit that users can use it in Bitcoin Core without having to fiddle with another binary.

I think a separate internal module (or internal library that is linked, or whatever) should be able to achieve the same, with the added benefit that users can use it in Bitcoin Core without having to fiddle with another binary.

Sure but that is the "significant effort" i was referring to. You would need to completely decouple this new logic (and its state) from the other full node networking and message processing logic.

2541f09439...6e6f83b0f7: rebase due to conflicts and address some suggestions and issues. Also ditch UseSensitiveRelay() and use m_opts.sensitiverelayowntx directly.

6e6f83b0f7...36074c093b: rebase, fix a bug where we sent the transaction without the witness, and extend the test to cover that

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

@vasild Re your question on IRC about the wallet when not adding the tx to the mempool: Not counting non-mempool change is a pre-existing issue with the wallet, and I think should be fixed unrelated to this pull request. As I said previously, this can be observed with current wallet settings, see #27509 (comment). So I think all you'd need to do here is add yet another setting to the wallet (disabled by default) to enable this feature.

Conceptually, it seems easier to fix, review and test the local wallet issue about counting change, than it is to fix the remote p2p behavior of withholding or supplying transactions that are in or out of the mempool.

I'm noticing this in the log when sending a tx:

2023-09-12 10:12:40 2023-09-12T14:12:40Z [miner] Submitting wtx 86a83ee7c540e833b1132969f13ce770adae8f80e50bba6038eaec6ff1185657 to mempool for relay

but we are not actually submitting anything to the mempool, right ...?

Or yes we are but not to the part of the mempool where we relay to peers over clear net? (i.e. is that what "m_unbroadcast_txids" is?)

@pinheadmz that log is from before this PR and I did not modify it. It is true that right now on master and on this PR the transaction is added to the mempool and also, in addition to m_unbroadcast_txids. Thus I think the message is ok.

However, given the feedback above and on IRC, I will change this to not add the transaction to the mempool. I am just figuring out how to exactly handle it. Then this message "Submitting ... to mempool for relay" would be inaccurate. It is a good opportunity to discuss this f2f.

we discussed this proposal in person, and I wanted to share some of my current thoughts here:

• I agree with the alternate approach to keep the transaction out of the mempool. it's very difficult to ensure that there aren't any information leaks, and even if we are able to achieve that with the current code, it is unrealistic to be able to maintain that over time
• the idea of starting via RPC submission only seems like a good way to incrementally develop this feature
• in the current implementation, we schedule opening 5 connections per transaction, but then there's randomness around which one of the transactions each connection decides to broadcast. this could lead to a lot of variability if there are multiple transactions queued around the same time, which doesn't seem desirable. It makes more sense to me for there to be a direct mapping @vasild

I will change this to not add the transaction to the mempool. I am just figuring out how to exactly handle it.

my recommendation is to make this PR a draft until you have a new proposal and are ready for further review. happy to take a look when the new approach is figured out :)

Concept ACK. Looking for the next actions w.r.t not adding to the mempool.

<!--13523179cfe9479db18ec6c5d236f789-->

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

This is relevant and I am rewriting it as per the discussions above.

I opened a new PR at #29415 to continue with this.

in the current implementation, we schedule opening 5 connections per transaction, but then there's randomness around which one of the transactions each connection decides to broadcast. this could lead to a lot of variability if there are multiple transactions queued around the same time, which doesn't seem desirable. It makes more sense to me for there to be a direct mapping @amitiuttarwar, I was thinking about this, but realized that once connman has opened N private broadcast connections they are identical from peerman's point of view. They don't have to be tied to transactions like "connection1 for txA", "connection2 for txB". Since connection1 and connection2 are identical, it may as well be "connection2 for txA" and "connection1 for txB".

To avoid the variability, e.g. one transaction to be broadcast more times than another, I introduced a priority for the transactions based on how many times one has been broadcast and how recently. Then peerman picks the tx with a higher priority when it starts processing a private broadcast connection.

I don't feel strongly about this, but it seems to be working well and avoids communication between peerman and connman like:

• peerman to connman: open 1 connection for txA
• connman to peerman: this connection is for txA

Also, if a transaction is quickly received back from the network and removed from the broadcast list, then the remaining connections created due to it will be used for broadcasting others instead.