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

vasild commented at 1:39 pm on April 21, 2023: contributor

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:

 0tx-sender >--- connect -------> tx-recipient
 1tx-sender >--- VERSION -------> tx-recipient
 2tx-sender <--- VERSION -------< tx-recipient
 3tx-sender <--- WTXIDRELAY ----< tx-recipient (maybe) 
 4tx-sender <--- SENDADDRV2 ----< tx-recipient (maybe) 
 5tx-sender <--- SENDTXRCNCL ---< tx-recipient (maybe) 
 6tx-sender <--- VERACK --------< tx-recipient
 7tx-sender >--- VERACK --------> tx-recipient
 8tx-sender >--- TX ------------> tx-recipient
 9tx-sender >--- PING ----------> tx-recipient
10tx-sender <--- PONG ----------< tx-recipient
11tx-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

DrahtBot commented at 1:39 pm on April 21, 2023: contributor

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	dergoegge, kristapsk, MaxHillebrand, mzumsande, ajtowns, RandyMcMillan, brunoerg, amitiuttarwar, 0xB10C, naumenkogs
Approach NACK	maflcko
Stale ACK	pinheadmz

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:

#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.

fanquake added the label Brainstorming on Apr 21, 2023

vasild marked this as a draft on Apr 21, 2023

dergoegge commented at 1:47 pm on April 21, 2023: member

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

kristapsk commented at 1:58 pm on April 21, 2023: contributor

Concept ACK

MaxHillebrand commented at 2:03 pm on April 21, 2023: none

Concept ACK

maflcko commented at 2:06 pm on April 21, 2023: member

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?

sipa commented at 2:10 pm on April 21, 2023: member

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

kristapsk commented at 2:16 pm on April 21, 2023: contributor

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.

dergoegge commented at 2:21 pm on April 21, 2023: member

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.

mzumsande commented at 3:31 pm on April 21, 2023: contributor

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.

DrahtBot added the label CI failed on Apr 21, 2023

vasild commented at 6:51 am on April 22, 2023: contributor

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:

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).
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.

mzumsande commented at 1:59 pm on April 22, 2023: contributor

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.

vasild commented at 9:34 am on April 23, 2023: contributor

@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

ajtowns commented at 9:26 am on April 26, 2023: contributor

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.

amitiuttarwar commented at 1:11 pm on April 27, 2023: contributor

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.

RandyMcMillan commented at 1:19 pm on April 27, 2023: contributor

Concept ACK

RandyMcMillan commented at 1:31 pm on April 27, 2023: contributor

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.

RandyMcMillan commented at 1:39 pm on April 27, 2023: contributor

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?

brunoerg commented at 4:40 pm on April 27, 2023: contributor

Concept ACK

brunoerg commented at 4:06 pm on April 28, 2023: contributor

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?

0xB10C commented at 11:15 pm on April 29, 2023: contributor

Concept ACK!

instagibbs commented at 1:18 pm on May 4, 2023: member

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.

in src/net.h:381 in 2f3f5203f2 outdated

376@@ -377,6 +377,8 @@ class CNode
377     /** Offset inside the first vSendMsg already sent */
378     size_t nSendOffset GUARDED_BY(cs_vSend){0};
379     uint64_t nSendBytes GUARDED_BY(cs_vSend){0};
380+    /** Disconnect after this or more bytes have been written to the socket. */
381+    std::optional<uint64_t> m_disconnect_after_send_bytes GUARDED_BY(cs_vSend);

ajtowns commented at 1:35 am on May 6, 2023:

Rather than doing this, it might make more sense to have simplified relay logic be to send the messages:

VERSION
[after receiving VERSION] VERACK
[after receiving VERACK] INV
[after receiving GETDATA] TX
PING
[after receiving PONG] disconnect

?

vasild commented at 12:29 pm on May 12, 2023:

Yes, and maybe also send wtxidrelay. Any reason not to send TX directly and avoid the INV, like below?

VERSION
[after receiving VERSION] WTXIDRELAY VERACK
[after receiving VERACK] TX
PING
[after receiving PONG] disconnect

mzumsande commented at 3:13 pm on May 12, 2023:

Any reason not to send TX directly and avoid the INV, like below?

It has been suggested to stop processing unrequested TXs messages, see #21224. While that didn’t make it, the idea is still floating around. If bitcoin core itself started relying on unrequested TX msgs being processed, that would probably kill it once for all. Maybe that’s ok, but we should make sure that there aren’t alternative clients that would ignore unrequested TXs.

ajtowns commented at 10:58 pm on May 12, 2023:

Also, if you send the INV first and wait for a request to send the actual tx, you might be a little more confident that the peer you connected to is really relaying txs, not just gathering statistics.

I was thinking that you could send a fixed VERSION string and no feature messages (WTXIDRELAY, SENDADDRV2, etc), as a way of minimising how identifiable your node is versus other clients doing sensitive-relay. (I don’t think you can have a larger anonymity set than that, since “send a single tx and close the connection” seems pretty distinct)

ajtowns commented at 11:34 am on May 16, 2023:

We don’t accept transactions while in IBD, so sending an INV first might be an easy way to catch that condition too. Checking that we’re on the same tip, and respecting feefilter might also catch those cases, and be worth doing anyway though.

vasild commented at 9:05 am on June 1, 2023:

we should make sure that there aren’t alternative clients that would ignore unrequested TXs

There is a precedent: from https://developer.bitcoin.org/reference/p2p_networking.html#tx

BitcoinJ will send a “tx” message unsolicited for transactions it originates

vasild commented at 9:06 am on June 1, 2023:

For now I kept it to send TX right away as this is simpler and somebody blackhole-ing the transaction is ok.

vasild commented at 5:12 pm on March 21, 2024:

To summarize:

Sending INV, then waiting for GETDATA, then sending TX could have the following benefits compared to directly sending unsolicited TX:

There was a proposal to reject unsolicited transactions: #21224
Existent non-Bitcoin Core nodes that would ignore unsolicited transactions
If you send the INV first and wait for a request to send the actual tx, you might be a little more confident that the peer you connected to is really relaying txs, not just gathering statistics
We don’t accept transactions while in IBD, so sending an INV first might be an easy way to catch that condition too

The downsides are:

It will make the code more complex.
We have to wait for GETDATA for some time and if we don’t get it “on time”, then disconnect without sending the transaction, risking an unnecessary disconnect from otherwise proper peer due to a (temporary) Tor or I2P connection slowdown.

I see the advantages as near-null and the “more complex code” trumps those for me. The proposal to ignore unsolicited transactions was rejected. There is no known software that rejects such transactions. There is already a software that sends unsolicited transactions (BitcoinJ).

Most importantly - it does not hurt to send the transaction even if the peer is going to ignore it. We have connected already. If there is even 1% chance that the peer will relay it, then our best action is to send the transaction without bothering whether that is 1% or 99%. That is, both of the following achieve the same, but the first is more complex to implement, review and maintain:

Connect, send INV, wait… aha! that peer does not care about new transactions, disconnect (could even make a mistake if the wait is too short)
Connect, send TX, disconnect

If it is a proper peer they will relay it in both cases.
If it is a peer that does not relay transactions (but has signaled txrelay=1 in their VERSION message), in IBD or whatever, they will not relay the transaction in both cases.

vasild commented at 12:41 pm on May 12, 2023: contributor

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.

vasild force-pushed on Jun 1, 2023

vasild commented at 9:32 am on June 1, 2023: contributor

>> 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:

(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
(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.

DrahtBot removed the label CI failed on Jun 1, 2023

in src/net_processing.cpp:5941 in b6fd6b525d outdated

5925+                                "Omitting INV for unbroadcast transaction (txid=%s) from MEMPOOL reply peer=%d%s\n",
5926+                                txinfo.tx->GetHash().ToString(),
5927+                                pto->GetId(),
5928+                                fLogIPs ? strprintf(", peeraddr=%s", pto->addr.ToStringAddrPort()) : "");
5929+                            continue;
5930+                        }

vasild commented at 9:47 am on June 1, 2023:

This, together with the other change from commit 0ec7b979e7a9d4c30a97c39c2a64768d7c5662b1 net_processing: omit unbroadcast txs from replies to GETDATA and MEMPOOL alter the behavior even if sensitive relay is not used (e.g. disabled or Tor and I2P not reachable).

I think it is beneficial in that case too, but is not the purpose of this PR to improve that. Should this be guarded by if (UseSensitiveRelay())?

pinheadmz commented at 6:27 pm on June 2, 2023:

I was just about to ask! Wouldn’t this only hurt users who don’t have sensitive relay available? Maybe MEMPOOL requests don’t matter so much but wouldn’t omitting from GETDATA prevent our own TX from being broadcast at all?

vasild commented at 12:35 pm on June 5, 2023:

For GETDATA, this only applies to transactions which we have not INVed to the peer. Otherwise INVed transactions will be sent via:

https://github.com/bitcoin/bitcoin/blob/f4a8269dfc144cc918570bdb870aa5143a11c1fe/src/net_processing.cpp#L2307

vasild marked this as ready for review on Jun 1, 2023

in src/init.cpp:1876 in b6fd6b525d outdated

1865@@ -1865,6 +1866,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1866 
1867     connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", DEFAULT_I2P_ACCEPT_INCOMING);
1868 
1869+    if (args.GetBoolArg("-sensitiverelayowntx", DEFAULT_SENSITIVE_RELAY_OWN_TX)) {

brunoerg commented at 2:24 pm on June 1, 2023:

In e310a0d285db0b82b0c201a56f92a7bfe4663226, you added these verifications, I think it would be useful if you add coverage for them in b6fd6b525d3ee4d89331dc04d86f854cb1decccd

Suggestion (did it to test e310a0d285db0b82b0c201a56f92a7bfe4663226):

 0diff --git a/test/functional/p2p_local_tx_relay.py b/test/functional/p2p_local_tx_relay.py
 1index 0b115352d..46215110e 100755
 2--- a/test/functional/p2p_local_tx_relay.py
 3+++ b/test/functional/p2p_local_tx_relay.py
 4@@ -140,12 +140,12 @@ class P2PLocalTxRelay(BitcoinTestFramework):
 5
 6     def setup_nodes(self):
 7         # Start a SOCKS5 proxy server.
 8-        socks5_server_config = Socks5Configuration()
 9-        socks5_server_config.addr = ("127.0.0.1", p2p_port(new_p2p_index()))
10-        socks5_server_config.unauth = True
11-        socks5_server_config.auth = True
12+        self.socks5_server_config = Socks5Configuration()
13+        self.socks5_server_config.addr = ("127.0.0.1", p2p_port(new_p2p_index()))
14+        self.socks5_server_config.unauth = True
15+        self.socks5_server_config.auth = True
16
17-        self.socks5_server = Socks5Server(socks5_server_config)
18+        self.socks5_server = Socks5Server(self.socks5_server_config)
19         self.socks5_server.start()
20
21         ports_base = p2p_port(MAX_NODES) + 15000
22@@ -171,7 +171,7 @@ class P2PLocalTxRelay(BitcoinTestFramework):
23             [
24                 "-peerbloomfilters", # needed to test replies to MEMPOOL
25                 "-sensitiverelayowntx",
26-                f"-proxy={socks5_server_config.addr[0]}:{socks5_server_config.addr[1]}"
27+                f"-proxy={self.socks5_server_config.addr[0]}:{self.socks5_server_config.addr[1]}"
28             ]
29         ])
30         self.start_nodes()
31@@ -243,6 +243,15 @@ class P2PLocalTxRelay(BitcoinTestFramework):
32         self.log.info("Waiting for normal broadcast to another observer")
33         observer_outbound.wait_for_inv([inv])
34
35+        self.log.info("Test an invalid usage of -sensitiverelayowntx throw an init error")
36+        self.stop_node(0)
37+        args_errors = {
38+            "Sensitive relay of own transactions requested (-sensitiverelayowntx), but none of Tor or I2P networks
39is reachable": ["-sensitiverelayowntx"],
40+            "Sensitive relay of own transactions requested (-sensitiverelayowntx), but -connect is also configured.
41 They are incompatible because the sensitive relay needs to open a new connection to a randomly chosen Tor or I2P pe
42er" : ["-sensitiverelayowntx", "-connect='127.0.0.1'", f"-proxy={self.socks5_server_config.addr[0]}:{self.socks5_ser
43ver_config.addr[1]}"]
44+        }
45+        for msg, args in args_errors.items():
46+            node0.assert_start_raises_init_error(extra_args=args, expected_msg=f'Error: {msg}')
47+
48
49 if __name__ == '__main__':
50     P2PLocalTxRelay().main()

vasild commented at 9:11 am on June 2, 2023:

Added this test, but in feature_config_args.py because it checks interaction between some config args. That fits better than p2p_local_tx_relay.py:

Test how locally submitted transactions are sent to the network when sensitive relay is used.

in src/net.cpp:1658 in 31dd230738 outdated

1649@@ -1650,7 +1650,25 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1650         if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
1651             return;
1652 
1653-        CSemaphoreGrant grant(*semOutbound);
1654+        const size_t sensitive_relay_connections_to_open{m_sensitive_relay_connections_to_open.load()};
1655+        const bool tor_reachable{IsReachable(NET_ONION)};
1656+        const bool i2p_reachable{IsReachable(NET_I2P)};
1657+        const bool open_sensitive_relay{sensitive_relay_connections_to_open > 0 &&

brunoerg commented at 3:01 pm on June 1, 2023:

In 31dd230738c43a529c63331eb23135e42d083e8f: At this point (creation of open_sensitive_relay), is there any possibility of both tor and i2p to be unreachable?

vasild commented at 3:58 pm on June 1, 2023:

Good observation. If both Tor and I2P are unreachable then m_sensitive_relay_connections_to_open should always be zero. Inside PeerManagerImpl::ScheduleLocalTxForRelay() UseSensitiveRelay() would return false and m_connman.ScheduleSensitiveRelayConnections() which increments the counter will never be called.

Now, that is the logic of some code far away, in another file. I did not want to rely on it because if we enter here and both are unreachable, then it will end up in an infinite loop. An assert that at least one is reachable is the other option, but I am trying to code this “defensively”, following @sdaftuar’s comment from another PR that excessive asserts in the networking code are better avoided.

vasild commented at 9:12 am on June 2, 2023:

Added a comment in the code in case somebody else wonders the same.

in test/functional/p2p_local_tx_relay.py:41 in b6fd6b525d outdated

36+MAX_BLOCK_RELAY_ONLY_CONNECTIONS = 2
37+SENSITIVE_RELAY_NUM_BROADCAST_PER_TX = 5
38+
39+g_p2p_index = None
40+def new_p2p_index():
41+    global g_p2p_index

brunoerg commented at 6:35 pm on June 1, 2023:

I think there is no need of creating a global variable like g_p2p_index. We could do something like:

 0diff --git a/test/functional/p2p_local_tx_relay.py b/test/functional/p2p_local_tx_relay.py
 1index 0b115352d..9f4c6aaa5 100755
 2--- a/test/functional/p2p_local_tx_relay.py
 3+++ b/test/functional/p2p_local_tx_relay.py
 4@@ -36,12 +36,6 @@ MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8
 5 MAX_BLOCK_RELAY_ONLY_CONNECTIONS = 2
 6 SENSITIVE_RELAY_NUM_BROADCAST_PER_TX = 5
 7 
 8-g_p2p_index = None
 9-def new_p2p_index():
10-    global g_p2p_index
11-    g_p2p_index += 1
12-    return g_p2p_index
13-
14 # Fill addrman with these addresses. Must have enough Tor addresses, so that even
15 # if all 10 default connections are opened to a Tor address (!?) there must be more
16 # for sensitive relays.
17@@ -135,13 +129,16 @@ class P2PLocalTxRelay(BitcoinTestFramework):
18     def set_test_params(self):
19         self.disable_autoconnect = False
20         self.num_nodes = 1
21-        global g_p2p_index
22-        g_p2p_index = self.num_nodes - 1
23+        self.g_p2p_index = self.num_nodes - 1
24+
25+    def new_p2p_index(self):
26+        self.g_p2p_index += 1
27+        return self.g_p2p_index
28 
29     def setup_nodes(self):
30         # Start a SOCKS5 proxy server.
31         socks5_server_config = Socks5Configuration()
32-        socks5_server_config.addr = ("127.0.0.1", p2p_port(new_p2p_index()))
33+        socks5_server_config.addr = ("127.0.0.1", p2p_port(self.new_p2p_index()))
34         socks5_server_config.unauth = True
35         socks5_server_config.auth = True

vasild commented at 9:13 am on June 2, 2023:

I removed new_p2p_index() because it was used in just one place. Earlier it was used also for all listeners behind the SOCKS5 proxy, but I had to stop using p2p_port() for that because it exceeded MAX_NODES (12).

in src/net.cpp:1789 in b6fd6b525d outdated

1787+        // * OUTBOUND_FULL_RELAY if GetTryNewOutboundPeer() is true (a stale tip is detected)
1788+        // * BLOCK_RELAY if it's time to try an extra block-relay-only peer (to confirm our tip is current)
1789+        // * FEELER if it's time to try a feeler
1790+        // * else retry the loop (sleep a bit and start from the top of this list)
1791+
1792+        if (open_sensitive_relay) {

brunoerg commented at 7:53 pm on June 1, 2023:

I suppose it bypass the maxconnection, does it? If so, it worths to update the -maxconnections mentioning it.

vasild commented at 9:16 am on June 2, 2023:

Yes. That is a bit hidden/implicit in the creation of the grant, in master:

0CSemaphoreGrant grant(*semOutbound);

this would hang if there are too many connections, waiting for a free slot to be available (somebody to disconnect).

Added a mention to it in the description of -maxconnections.

vasild force-pushed on Jun 2, 2023

vasild commented at 9:09 am on June 2, 2023: contributor

b6fd6b525d...92fb45b5ef: rebase and address suggestions

DrahtBot added the label CI failed on Jun 2, 2023

in src/net_processing.cpp:1438 in 504453a394 outdated

1455+    if (pnode.IsSensitiveRelayConn()) {
1456+        my_services = NODE_NONE;
1457+        my_time = 0;
1458+        your_services = NODE_NONE;
1459+        your_addr = CService{};
1460+        my_user_agent = "/pynode:0.0.1/";

pinheadmz commented at 6:11 pm on June 2, 2023:

Why use a funny value here instead of our actual subver? I’m guessing it’s for extra anonymity? Could there be any downsides though, like nodes that refuse connections to certain agents?

brunoerg commented at 8:55 pm on June 2, 2023:

It seems for extra anonymity, but I have a similar doubt, couldn’t I make easier for someone to censor us?

instagibbs commented at 8:57 pm on June 2, 2023:

unless we want to sybil the network and fake long-term connections, it’s obvious what this connection will be about? Just retry with new peers until someone honest propagates it?

vasild commented at 12:43 pm on June 5, 2023:

TLDR: @instagibbs, right! :)

First, the problem with the user-agent string is that it can be customized which will totally reveal the identity of the sender. Next idea is to use the non-customized user-agent even if -uacomment= is configured. But then, why even reveal the version - whether it is 26.0 or 27.0? Right after a new release there will be few nodes that run the just-released version and thus the anonymity set will be small. So this string better be constant - either empty string or whatever other constant. /pynode:0.0.1/ was suggested here - #27509 (comment)

pinheadmz commented at 2:44 pm on June 5, 2023:

Ok understood. In that case, would it make sense to be very explicit about what this connection is about? I mean user agent could essentially be “I am going to send you one TX and then disconnect”. Additional behavior could even be encoded on the recipient side, maybe to ensure we don’t disconnect prematurely ?

luke-jr commented at 10:02 pm on July 27, 2023:

I don’t like outright lying in the UA. Why not just send it blank? Or at least some kind of standard for “I am not telling you”

vasild commented at 12:49 pm on July 31, 2023:

(beware, https://bikeshed.com/)

I think any constant is ok and bitcoin-submittx’s string (/pynode:0.0.1/) was already suggested by @sipa here #27509 (comment). That suggestion has 6 positive reactions. It is about increasing the anonymity set, in case different versions or different softwares use this technique.

some kind of standard for “I am not telling you”

I don’t think there is, but we can standardize /pynode:0.0.1/ as “I am not telling you”? ;-)

Should I change this to an empty string? If this comment collects more :+1: + :heart: than #27509 (comment) then I will change it.

in src/net_processing.cpp:3739 in 504453a394 outdated

3621+            // them a transaction below their threshold. This is ok because this
3622+            // relay logic is designed to work even in cases when the peer drops
3623+            // the transaction (due to it being too cheap, or for other reasons).
3624+            PushUnbroadcastTx(pfrom, *peer, interruptMsgProc);
3625+
3626+            MaybeSendPing(pfrom, *peer, GetTime<std::chrono::microseconds>());

pinheadmz commented at 6:16 pm on June 2, 2023:

Why is this extra ping needed for sensitive connections ?

vasild commented at 1:26 pm on June 5, 2023:

Just one ping will be send. It is needed because without it the receiving bitcoind may ignore the TX message since the sender disconnects immediately after that, see #4432. Sending a ping and waiting for pong ensures that the preceding TX message has been processed (unless somebody implements out-of-order messages processing).

ajtowns commented at 0:50 am on July 6, 2023:

This implementation of that logic doesn’t seem ideal to me:

sending the TX without doing INV/GETDATA first risks having the TX be ignored as unrequested
PushUnbroadcastTx doesn’t actually send the TX, but MaybeSendPing does actually send the PING, so you might get the PONG and closes the connection before the other end has actually received/processed the TX

vasild commented at 1:29 pm on July 6, 2023:

sending the TX without doing INV/GETDATA first risks having the TX be ignored as unrequested

I thought about doing INV first and waiting for get data but decided to not do that for the following reasons:

Bitcoin Core accepts unsolicited transactions. According to https://developer.bitcoin.org/reference/p2p_networking.html#tx there is already some software that sends unsolicited transactions.
It would add complexity to the implementation.
After INV we would have to wait for some time for GETDATA, then in some cases when the wait is too little we risk cutting the connection just before we get GETDATA.
Last, but this is the most important IMO: it does not matter. There is redundancy: the transaction will be sent to more than one peer and we will retry if we don’t hear back from the network about it. It is expected and is ok if some of the peers we send the transaction drops it.

PushUnbroadcastTx doesn’t actually send the TX, but MaybeSendPing does actually send the PING, so you might get the PONG and closes the connection before the other end has actually received/processed the TX

I don’t think so. Both PushUnbroadcastTx() and MaybeSendPing() end up calling PushMessage() which enqueues the message in CNode::vSendMsg. This is done synchronously so the transaction will go before the ping. Then messages from CNode::vSendMsg are processed and sent in the order they were added (FIFO).

ajtowns commented at 2:14 pm on July 6, 2023:

I don’t think so.

You’re right – I missed seeing the call to ProcessGetData and assumed it wasn’t picked up until the next ProcessMessages.

in src/net_processing.cpp:4956 in 504453a394 outdated

4910@@ -4788,6 +4911,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4911                     if (ping_time.count() >= 0) {
4912                         // Let connman know about this successful ping-pong
4913                         pfrom.PongReceived(ping_time);
4914+                        if (pfrom.IsSensitiveRelayConn()) {
4915+                            pfrom.fDisconnect = true;

pinheadmz commented at 6:19 pm on June 2, 2023:

Why disconnect the sensitive connection on PONG? Could that ever happen before we send our tx messages out?

pinheadmz commented at 7:20 pm on June 2, 2023:

(I just realized I answered my own question from #27509 (review))

But still, don’t we also send automatic PINGs ? Could that result in premature disconnection?

vasild commented at 1:32 pm on June 5, 2023:

Why disconnect the sensitive connection on PONG?

Because then we assume the TX message that preceded the PING message has been processed.

Could that ever happen before we send our tx messages out?

Yes, if the peer sends us unsolicited PONG without us sending PING before that.

But still, don’t we also send automatic PINGs?

Yes, and for the sensitive relay connections this should be the first PING. The point is to send TX before any PING.

maflcko commented at 3:46 pm on June 5, 2023:

Yes, if the peer sends us unsolicited PONG without us sending PING before that.

I don’t think the peer can guess the right ping nonce reliably , can it?

vasild commented at 11:33 am on July 6, 2023:

Oh, yes, this is in the “nonce match” branch of the logic that handles PONG. So, the answer to “Could that ever happen before we send our tx messages out?” is just “No”.

in test/functional/p2p_filter.py:154 in d2cb1585e3 outdated

150+        tx.vout[0].nValue -= (amount + fee)
151+        tx.vout.append(CTxOut(amount, filter_peer.watch_script_pubkey))
152+        tx.rehash()
153+
154+        tx_sender = self.nodes[0].add_p2p_connection(P2PInterface())
155+        tx_sender.send_message(msg_tx(tx))

pinheadmz commented at 6:35 pm on June 2, 2023:

Just so I understand: this change is required because with the PR we wouldn’t send our own TX in response to a MEMPOOL message, so you quickly spin up a dummy peer to send the TX to us first. Then we can assert that it was relayed to fitler_peer ?

vasild commented at 1:36 pm on June 5, 2023:

Yes, exactly. Before this change the test sent the transaction to the node using RPC and so it becomes locally-submitted, sensitive, unbroadcast, our own, etc transaction. With this change to the test it sends the transaction to the node via the P2P interface, so it is treated as normal, foreign, non-sensitive transaction and thus included in the MEMPOOL reply.

Btw, if changes from c70da509e4224f738fa0229e1f434a854629acf2 net_processing: omit unbroadcast txs from replies to GETDATA and MEMPOOL are restricted only to if (UseSensitiveRelay()) { then this tweak to the test will not be necessary.

in src/net_processing.cpp:2082 in da000ed0a6 outdated

2077@@ -2078,6 +2078,13 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta
2078 
2079 bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid)
2080 {
2081+    if (UseSensitiveRelay()) {
2082+        LOCK(m_mempool.cs);
2083+        if (m_mempool.IsUnbroadcastTx(gtxid)) {
2084+            return false;
2085+        }

pinheadmz commented at 6:43 pm on June 2, 2023:

nit: don’t we usually do this kind of logic in one line?

vasild commented at 7:51 am on June 6, 2023:

The guidelines allow both and I prefer this because: https://github.com/vasild/bitcoin/wiki/if-on-the-same-line

in src/net_processing.cpp:4327 in da000ed0a6 outdated

4218@@ -4210,6 +4219,23 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4219         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
4220         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
4221 
4222+        if (UseSensitiveRelay() && m_mempool.RemoveUnbroadcastTx(txid)) {
4223+            LogPrintLevel( /* Continued */
4224+                BCLog::SENSITIVE_RELAY,
4225+                BCLog::Level::Info,
4226+                "Received own transaction (txid=%s) from the network from peer=%d%s; "
4227+                "stopping sensitive broadcast and broadcasting it to everybody as if it is not our transaction\n",

pinheadmz commented at 6:45 pm on June 2, 2023:

Super cool idea! Is there common chance for failure here though? This relies on a chain of peers from the sensitive relay node back to us that haven’t seen the tx yet.

vasild commented at 7:59 am on June 6, 2023:

This chain of peers is desired / demanded to consider the transaction successfully propagated. Lets say we are connected to 30 peers (some inbound, some outbound). If somebody of them gets the transaction (from somebody else, not from us) he will tell us about it due to the normal flooding mechanism because he will not be aware we have it. If all our 30 peers stay silent about the transaction, it means that they did not get it which is a sign it propagated only to some part of the network or did not propagate at all.

My understanding is that normally it takes a few seconds for a transaction to reach every node in the network due to the flooding mechanism.

pinheadmz commented at 7:05 pm on June 2, 2023: member

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

brunoerg commented at 8:53 pm on June 2, 2023: contributor

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

0- Once we get an INV from somebody, request the transaction with GETDATA, as if we didn't have it before.
1
2- 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

0We 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?

vasild force-pushed on Jun 5, 2023

vasild commented at 4:43 am on June 5, 2023: contributor

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)

vasild force-pushed on Jun 5, 2023

DrahtBot removed the label CI failed on Jun 5, 2023

instagibbs commented at 1:38 pm on June 5, 2023: member

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

vasild commented at 2:06 pm on June 5, 2023: contributor

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.

pinheadmz commented at 2:46 pm on June 5, 2023: member

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 ?

vasild commented at 4:17 pm on June 5, 2023: contributor

@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!

instagibbs commented at 4:24 pm on June 5, 2023: member

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.

pinheadmz commented at 5:15 pm on June 5, 2023: member

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

in src/init.cpp:1870 in f73bb4cbc4 outdated

1865@@ -1865,6 +1866,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1866 
1867     connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", DEFAULT_I2P_ACCEPT_INCOMING);
1868 
1869+    if (args.GetBoolArg("-sensitiverelayowntx", DEFAULT_SENSITIVE_RELAY_OWN_TX)) {
1870+        if (!IsReachable(NET_ONION) && !IsReachable(NET_I2P)) {

mzumsande commented at 8:57 pm on June 5, 2023:

There could be a race with the torcontrol thread, which may set NET_ONION to reachable before this line is encountered or only later.

vasild commented at 2:53 pm on June 8, 2023:

Right, I relaxed the condition a bit to error only if we are sure Tor is unreachable and will remain unreachable. There is still a chance that the tor control thread will not set Tor to reachable, but there is also a chance that Tor is set to reachable here, but the Tor server is down. Later at runtime we do check again the reachable networks and in general the code must work when some or all of those networks go down.

in src/bitcoin-cli.cpp:454 in b33fd07632 outdated

450@@ -451,6 +451,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
451         if (conn_type == "block-relay-only") return "block";
452         if (conn_type == "manual" || conn_type == "feeler") return conn_type;
453         if (conn_type == "addr-fetch") return "addr";
454+        if (conn_type == "senstitive-relay") return "srelay";

mzumsande commented at 8:59 pm on June 5, 2023:

typo: sensitive-relay (extra “t”)

vasild commented at 2:47 pm on June 8, 2023:

Fixed.

in src/init.cpp:489 in a35e4056e2 outdated

485@@ -486,7 +486,7 @@ void SetupServerArgs(ArgsManager& argsman)
486     argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
487     argsman.AddArg("-listen", strprintf("Accept connections from outside (default: %u if no -proxy, -connect or -maxconnections=0)", DEFAULT_LISTEN), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
488     argsman.AddArg("-listenonion", strprintf("Automatically create Tor onion service (default: %d)", DEFAULT_LISTEN_ONION), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
489-    argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> connections to peers (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
490+    argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> connections to peers (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u. It does not apply to short-lived sensitive relay connections either.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

mzumsande commented at 9:12 pm on June 5, 2023:

If this bypasses -maxconnections, could we run into trouble if we don’t have sufficient file descriptors (see RaiseFileDescriptorLimit() call in init)?

vasild commented at 3:02 pm on June 8, 2023:

Yes, at least I should raise the fd limit, but by how much? Should we limit the number of sensitive relay connections? I think it makes sense to try as hard as possible even if that means sometimes a temporary error of running out of file descriptors. Or is that not a good idea? I guess in some cases a node operator may submit many transactions at once to their node. What do you think would be the best approach?

vasild commented at 10:13 am on June 9, 2023:

I put a cap on the maximum number of sensitive relay connections that can be opened. Breaking through the max open file descriptors looks like a bad idea - it would also hamper access to the filesystem, even if temporarily. I assume somebody may submit thousands of local transactions in no time.

in src/net.cpp:1825 in a35e4056e2 outdated

1780-        // block-relay-only peer (to confirm our tip is current, see below) or the next_feeler
1781-        // timer to decide if we should open a FEELER.
1782-
1783-        if (!m_anchors.empty() && (nOutboundBlockRelay < m_max_outbound_block_relay)) {
1784+        // Determine what type of connection to open, in order of priority:
1785+        // * SENSITIVE_RELAY if needed

mzumsande commented at 9:51 pm on June 5, 2023:

I’m a bit worried about this priority: What would happen if we, for some reason, aren’t able to make successful sensitive relay connections (e.g. we think that we can connect to Tor but misconfigured something, or maybe we can connect but don’t have any addresses for these networks in our addrman). Wouldn’t we try again and again (since this is the first priority and m_sensitive_relay_connections_to_open is never reduced) and never attempt to make an outbound connection of another type?

vasild commented at 2:58 pm on June 8, 2023:

Yes! I changed this to not try 2 consecutive sensitive relay connections. That is - after attempting one, give way to others (if any, otherwise sleep 0.5s) and only then try another sensitive relay.

mzumsande commented at 9:58 pm on June 5, 2023: contributor

reviewed just the first few commits so far

vasild commented at 8:12 am on June 6, 2023: contributor

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.

vasild force-pushed on Jun 8, 2023

vasild commented at 2:47 pm on June 8, 2023: contributor

2eae1f8f13...ff88e08363: address suggestions

DrahtBot added the label CI failed on Jun 8, 2023

vasild force-pushed on Jun 9, 2023

vasild commented at 10:10 am on June 9, 2023: contributor

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

DrahtBot added the label Needs rebase on Jun 9, 2023

vasild force-pushed on Jun 27, 2023

vasild commented at 8:42 am on June 27, 2023: contributor

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())

DrahtBot removed the label Needs rebase on Jun 27, 2023

DrahtBot removed the label CI failed on Jun 28, 2023

in src/net_processing.cpp:3372 in bde055d28f outdated

3375+                      BCLog::Level::Debug,
3376+                      "Disconnecting: no transactions relay with this peer (connected in vain), peer=%d%s\n",
3377+                      node.GetId(),
3378+                      fLogIPs ? strprintf(", peeraddr=%s", node.addr.ToStringAddrPort()) : "");
3379+        node.fDisconnect = true;
3380+        m_connman.ScheduleSensitiveRelayConnections(1);

pinheadmz commented at 7:50 pm on July 18, 2023:

IIUC this line schedules a replacement sensitive connection because the one we were passed in has the wrong services and so it gets dumped. I think this is the only line in this function that makes it apply strictly to only sensitive relays. So I wonder if that should be asserted at the top? Or could be renamed to PushUnbroadcastTxToSensistiveRelay()? At the very least maybe sensitive-only should be explained in the function description on L933 ?

vasild commented at 12:35 pm on July 27, 2023:

IIUC this line schedules a replacement sensitive connection because the one we were passed in has the wrong services and so it gets dumped.

Yes.

I think this is the only line in this function that makes it apply strictly to only sensitive relays. So I wonder if that should be asserted at the top? Or could be renamed to PushUnbroadcastTxToSensistiveRelay()? At the very least maybe sensitive-only should be explained in the function description on L933 ?

Right. While this function will work if called for non-sensitive-relay peer (it will send the tx to the peer), this is not the intention because it will be bad for privacy and it is only called under if (pfrom.IsSensitiveRelayConn()) {.

Renamed as suggested and added Assume();

in src/net_processing.cpp:3403 in bde055d28f outdated

3397+    // Randomly pick some transaction from unbroadcast_txids.
3398+    const uint64_t n{FastRandomContext{}.randrange(unbroadcast_txids.size())};
3399+    auto tx_iter = unbroadcast_txids.begin();
3400+    for (uint64_t i = 0; i < n; ++i) {
3401+        ++tx_iter;
3402+    }

pinheadmz commented at 7:55 pm on July 18, 2023:

wow, is this really the fastest way to get a random element from std::set ? funny.

maflcko commented at 1:01 pm on July 27, 2023:

nit: Any reason to re-implement std::advance()?

vasild commented at 1:42 pm on July 27, 2023:

Yeah, it does not provide a direct access to an arbitrary element. This is O(unbroadcast_txids.size()) if faster is required we have to change the container (or extend it with supplementary vector). I shortened this a bit to:

0const auto tx_iter = std::next(unbroadcast_txids.begin(), FastRandomContext{}.randrange(unbroadcast_txids.size()));

This is still linear, but at least a bit short and (I hope) as easy to read.

in src/net_processing.cpp:3739 in bde055d28f outdated

3726+            // them a transaction below their threshold. This is ok because this
3727+            // relay logic is designed to work even in cases when the peer drops
3728+            // the transaction (due to it being too cheap, or for other reasons).
3729+            PushUnbroadcastTx(pfrom, *peer, interruptMsgProc);
3730+
3731+            MaybeSendPing(pfrom, *peer, GetTime<std::chrono::microseconds>());

pinheadmz commented at 8:01 pm on July 18, 2023:

Would it make any sense here to manually set peer.m_ping_queued = true to guarantee (not “maybe”) sending ping?

vasild commented at 3:08 am on July 30, 2023:

Right! Better to ensure explicitly than rely on the internals of MaybeSendPing(), the clock and the fact this is the first call to MaybeSendPing() for this peer.

in test/functional/test_framework/socks5.py:50 in bde055d28f outdated

40@@ -40,6 +41,16 @@ def __init__(self):
41         self.af = socket.AF_INET # Bind address family
42         self.unauth = False  # Support unauthenticated
43         self.auth = False  # Support authentication
44+        # An array of objects like:
45+        # {
46+        #     "listen_addr": "127.0.0.1"
47+        #     "listen_port": 28276
48+        #     "node": python_p2p_node,
49+        #     "requested_to_addr": "the_client_asked_to_connect_to_this_addr.onion",

pinheadmz commented at 8:29 pm on July 18, 2023:

I don’t see .node used below and I don’t think you really use .requested_to_addr anywhere after setting it below? Could this just be an array of CAddress or something like that?

vasild commented at 3:19 am on July 30, 2023:

Both .node and .requested_to_addr members are used by the test - .node is used to check/query what messages bitcoind has sent to this peer and .requested_to_addr is used to check that the connection was made to an .onion address (and not e.g. an IPv4 address).

in test/functional/test_framework/socks5.py:131 in bde055d28f outdated

126@@ -116,6 +127,30 @@ def handle(self):
127             cmdin = Socks5Command(cmd, atyp, addr, port, username, password)
128             self.serv.queue.put(cmdin)
129             logger.info('Proxy: %s', cmdin)
130+
131+            if self.serv.conf.destinations_used < len(self.serv.conf.destinations):

pinheadmz commented at 8:30 pm on July 18, 2023:

Does this guy need an else? Maybe raise error that a connection request was made after all destinations have been used?

vasild commented at 3:37 am on July 30, 2023:

I didn’t do that because my idea was to fall back to the old behavior of just closing the connection after the first N connections have been redirected (all destinations have been used). But you are right, I added a warning message now to ease diagnosing test behavior if that happens. Didn’t raise because there is no error from the point of view of socks5.py.

in src/txmempool.cpp:1017 in bde055d28f outdated

1013+        return true;
1014+    }
1015+    return false;
1016+}
1017+
1018+bool CTxMemPool::IsUnbroadcastTx(const GenTxid& gtxid) const

pinheadmz commented at 8:33 pm on July 18, 2023:

Why did you need this to support wtxid? Is that how we are verifying our TX made the round trip out and back (without being malleated)?

vasild commented at 4:08 am on July 30, 2023:

Just for convenience to be able to check if this is an unbroadcast transaction from PeerManagerImpl::AlreadyHaveTx() which has the id as GenTxid.

CTxMemPool::m_unbroadcast_txids tracks transactions by txid (not wtxid). This is relevant: #18044 (review).

pinheadmz commented at 8:36 pm on July 18, 2023: member

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 ;-)

in test/functional/p2p_local_tx_relay.py:153 in bde055d28f outdated

145+    "eciohu5nq7vsvwjjc52epskuk75d24iccgzmhbzrwonw6lx4gdva.b32.i2p",
146+    "ejlnngarmhqvune74ko7kk55xtgbz5i5ncs4vmnvjpy3l7y63xaa.b32.i2p",
147+    "fhzlp3xroabohnmjonu5iqazwhlbbwh5cpujvw2azcu3srqdceja.b32.i2p",
148+    "[fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa]",
149+    "[fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]",
150+    "[fcdc:73ae:b1a9:1bf8:d4c2:811:a4c7:c34e]",

pinheadmz commented at 5:33 pm on July 26, 2023:

I added some extra logs to bitcoind and ran the test, these addrs fail IsRoutable(). I assume these are CJDNS addresses?

jonatack commented at 7:21 pm on July 26, 2023:

Yes, these three are CJDNS.

pinheadmz commented at 7:22 pm on July 26, 2023:

does the test node need a config flag to route?

vasild commented at 4:16 am on July 30, 2023:

Right! I added -cjdnsreachable. This changed the test from:

0TestFramework (DEBUG): Could not add [fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa] to node0's addrman (collision?)

to

0TestFramework (DEBUG): Added [fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa] to node0's addrman

Thanks!

in src/net.cpp:2967 in bde055d28f outdated

2964+                      BCLog::Level::Debug,
2965+                      "Omitting send of message '%s', peer=%d%s\n",
2966+                      msg.m_type,
2967+                      pnode->GetId(),
2968+                      fLogIPs ? strprintf(", peeraddr=%s", pnode->addr.ToStringAddrPort()) : "");
2969+        return;

maflcko commented at 12:53 pm on July 27, 2023:

Not sure if the low level net layer is the right place to inspect high level p2p messages and then decide to drop them. It may be good to enumerate why this is needed (if at all).

vasild commented at 4:35 am on July 30, 2023:

I guess an alternative would be to have a new method PeerManagerImpl::PushMessage() which does this check and then calls CConnman::PushMessage(). And replace all calls to m_connman.PushMessage() (there are 53 of them) with PushMessage(). Would that be desirable?

maflcko commented at 7:05 am on July 30, 2023:

It may be good to enumerate why this is needed (if at all).

vasild commented at 1:14 pm on July 31, 2023:

It is good to have all unneeded messages dropped from a central place (where all messages go through) even if it seems that the code will not send such messages now - this way it is proof wrt future changes. The code that decides which message to send is scattered all over the place and is difficult to follow.

I added this comment

0// Ensure sensitive relay connections only send VERSION, VERACK, TX or PING. Others are not needed and may degrade privacy.

in src/net_processing.cpp:2148 in bde055d28f outdated

2140@@ -2092,6 +2141,26 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid
2141     };
2142 }
2143 
2144+bool PeerManagerImpl::UseSensitiveRelay() const
2145+{
2146+    return gArgs.GetBoolArg("-sensitiverelayowntx", DEFAULT_SENSITIVE_RELAY_OWN_TX) &&

maflcko commented at 12:57 pm on July 27, 2023:

shouldn’t this use the peerman options struct?

vasild commented at 4:30 am on July 30, 2023:

I can’t find an options struct in PeerManager or in PeerManagerImpl. Did you mean struct CConnman::Options? gArgs is used in some other places in PeerManagerImpl too, so I assumed it is ok.

Edit: I see the options struct was added recently and caused this not to compile. Rebased and changed this to use PeerManager::m_opts.

maflcko commented at 1:10 pm on July 27, 2023: member

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.

vasild commented at 4:37 am on July 30, 2023: contributor

bde055d28f...96ea776776: address suggestions

vasild force-pushed on Jul 30, 2023

DrahtBot added the label CI failed on Jul 30, 2023

vasild force-pushed on Jul 30, 2023

vasild commented at 5:05 am on July 30, 2023: contributor

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

in src/net.h:53 in 7a3206dc8e outdated

48@@ -49,6 +49,8 @@ class CNode;
49 class CScheduler;
50 struct bilingual_str;
51 
52+/** Default for -sensitiverelayowntx. */
53+static const bool DEFAULT_SENSITIVE_RELAY_OWN_TX{false};

dergoegge commented at 11:31 am on July 31, 2023:

this should go into net_processing.h where it is used

vasild commented at 12:40 pm on August 2, 2023:

Moved, thanks!

in src/net_processing.cpp:4312 in 7a3206dc8e outdated

4308@@ -4144,6 +4309,23 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4309         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
4310         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
4311 
4312+        if (UseSensitiveRelay() && m_mempool.RemoveUnbroadcastTx(txid)) {

dergoegge commented at 12:01 pm on July 31, 2023:

Using the txid here can leak the tx origin.

e.g. an attacker changes the witness of your tx (such that the tx is invalid) and sends it to you, if you then announce the original tx to your peers it is obvious that the tx originated from you.

vasild commented at 12:43 pm on August 2, 2023:

Good! This should be moved a bit below, after the transaction has been added to the mempool: const MempoolAcceptResult result = m_chainman.ProcessTransaction(ptx); and conditional on result.

in src/net_processing.cpp:4955 in 7a3206dc8e outdated

4941@@ -4760,6 +4942,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4942                     if (ping_time.count() >= 0) {
4943                         // Let connman know about this successful ping-pong
4944                         pfrom.PongReceived(ping_time);
4945+                        if (pfrom.IsSensitiveRelayConn()) {

dergoegge commented at 12:14 pm on July 31, 2023:

Unless I’m missing something, the pings on sensitive connections never timeout. So if the peer doesn’t respond to the ping the connection only closes after 20min (TIMEOUT_INTERVAL).

vasild commented at 12:46 pm on August 2, 2023:

Yes. This is the case for all connections - if the peer goes silent, but does not close the connection, then we will close it after 20 minutes. Do you think that is not suitable for sensitive relay connections? If yes, what to do better?

dergoegge commented at 12:54 pm on August 2, 2023:

Oh for some reason I thought our ping timeout was smaller than TIMEOUT_INTERVAL (turns out it’s the same). Feel free to consider this a nit.

But I think it would be reasonable for these connections to timeout much quicker anyway, since we don’t care about keeping them around.

in src/net_processing.cpp:3864 in 7a3206dc8e outdated

3850@@ -3698,6 +3851,18 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3851         return;
3852     }
3853 
3854+    if (pfrom.IsSensitiveRelayConn()) {

dergoegge commented at 12:31 pm on July 31, 2023:

Not processing messages that are irrelevant to this type of tx relay is the right move but it still leaves room for tx origin leakage through fingerprinting in the version handshake.

In other words, if it is possible to fingerprint a node solely based on version handshake messages then it is possible for the receiver of a “sensitive-relay” tx to figure out which public addresses belong to the node that broadcast the tx.

vasild commented at 12:54 pm on August 2, 2023:

For sensitive relay connections it is not possible to fingerprint based on the version handshake or any other messages exchanged (see the OP under “The messages exchange should look like this”).

The only non-constant in the VERSION message we send is the peer nonce, then VERACK has no payload and the only payload in the PING is the ping nonce.

dergoegge commented at 1:00 pm on August 2, 2023:

The only non-constant in the VERSION message we send is the peer nonce

Yes and this can be used for fingerprinting, because we disconnect incoming connections that match an already existing nonce.

https://github.com/bitcoin/bitcoin/blob/2fa60f0b683cefd7956273986dafe3bde00c98fd/src/net_processing.cpp#L3361-L3366

And I wouldn’t be surprised if there are more ways to fingerprint in the version handshake. This will also end up being a maintenance concern going forward, mostly as a burden for reviewers as it’s hard to test for these things.

vasild commented at 7:31 am on August 3, 2023:

“we disconnect incoming connections that match an already existing nonce” but only for connections with incomplete handshake. So the spy has to try to connect back to the node copying the nonce before they send ther VERACK, ie before they know this is a sensitive relay connection.

dergoegge commented at 2:42 pm on August 3, 2023:

So the spy has to try to connect back to the node copying the nonce before they send ther VERACK, ie before they know this is a sensitive relay connection.

They know it’s likely a sensitive relay connection based on the version message they receive. So they withhold their verack on that connection and then keep connecting and sending version messages to all public addresses until they found the node that disconnects them for sending the right nonce. After that they can send the verack and receive the tx.

vasild force-pushed on Jul 31, 2023

vasild commented at 1:02 pm on July 31, 2023: contributor

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

in src/net.cpp:2960 in 2541f09439 outdated

2955+    if (pnode->IsSensitiveRelayConn() &&
2956+        msg.m_type != NetMsgType::VERSION &&
2957+        msg.m_type != NetMsgType::VERACK &&
2958+        msg.m_type != NetMsgType::TX &&
2959+        msg.m_type != NetMsgType::PING) {
2960+        // Ensure sensitive relay connections only send VERSION, VERACK, TX or PING. Others are not needed and may degrade privacy.

maflcko commented at 1:26 pm on July 31, 2023:

0        // Ensure sensitive relay connections only send the above message types. Others are not needed and may degrade privacy.

(nit), to avoid having to touch this line again in the future, if something changes.

vasild commented at 12:39 pm on August 2, 2023:

Done

in src/net_processing.cpp:4338 in 2541f09439 outdated

4323+            m_txrequest.ForgetTxHash(wtxid);
4324+            ScheduleTxForRelayToAll(txid, wtxid);
4325+            pfrom.m_last_tx_time = GetTime<std::chrono::seconds>();
4326+            return;
4327+        }
4328+

dergoegge commented at 2:01 pm on July 31, 2023:

What about children of local txs? They currently leak tx origin as well.

They should be treated as orphans until the parent tx is scheduled for relay to all.

DrahtBot removed the label CI failed on Jul 31, 2023

in src/net_processing.cpp:3728 in 2541f09439 outdated

3723+            // them a transaction below their threshold. This is ok because this
3724+            // relay logic is designed to work even in cases when the peer drops
3725+            // the transaction (due to it being too cheap, or for other reasons).
3726+            PushUnbroadcastTxToSensistiveRelay(pfrom, *peer, interruptMsgProc);
3727+
3728+            peer->m_ping_queued = true; // Ensure a ping will be send: mimick a request via RPC.

pinheadmz commented at 5:24 pm on July 31, 2023:

nit: s/send/sent

vasild commented at 12:39 pm on August 2, 2023:

Done

pinheadmz commented at 6:54 pm on July 31, 2023: member

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)

pinheadmz commented at 7:30 pm on July 31, 2023: member

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

-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)
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)
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)
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!

maflcko commented at 5:59 am on August 1, 2023: member

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.

vasild commented at 11:31 am on August 1, 2023: contributor

@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?

maflcko commented at 12:13 pm on August 1, 2023: member

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.

dergoegge commented at 12:41 pm on August 1, 2023: member

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.

sipa commented at 1:03 pm on August 1, 2023: member

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.

maflcko commented at 1:09 pm on August 1, 2023: member

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.

vasild commented at 4:11 pm on August 1, 2023: contributor

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 ;-)

maflcko commented at 4:27 pm on August 1, 2023: member

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.

dergoegge commented at 10:18 am on August 2, 2023: member

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).

maflcko commented at 10:37 am on August 2, 2023: member

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.

in src/init.cpp:1880 in 2541f09439 outdated

1872@@ -1853,6 +1873,22 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1873 
1874     connOptions.m_i2p_accept_incoming = args.GetBoolArg("-i2pacceptincoming", DEFAULT_I2P_ACCEPT_INCOMING);
1875 
1876+    if (args.GetBoolArg("-sensitiverelayowntx", DEFAULT_SENSITIVE_RELAY_OWN_TX)) {
1877+        // If -listenonion is set, then NET_ONION may not be reachable now
1878+        // but may become reachable later, thus only error here if it is not
1879+        // reachable and will not become reachable for sure.
1880+        if (!IsReachable(NET_I2P) && !IsReachable(NET_ONION) && !listenonion) {

vasild commented at 10:39 am on August 2, 2023:

moving discussion from #27509 (comment)

-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.

This check was supposed to prevent that but it was too strict. Apparently you have listenonion=1 which means that later, after startup, we will connect to the Tor control and may set the Tor proxy and make the Tor network reachable. It is a bit weird that listenonion remains with the default 1 when onlynet=ipv4 is used. It means the node will only make outgoing connections to IPv4, but will listen for and accept incoming connections from the Tor network :eyes:.

I relaxed the check to also trigger the error if onlynet has been used (and if !IsReachable(NET_ONION) at the time of the check, that means onlynet was used but tor was omitted from it).

Is “only use tor for sensitive relay” a possible configuration?

Yes, if the node does not have I2P connectivity. Then only Tor will be used. But if the node has both Tor and I2P connectivity then both will be used. Do you mean that if both networks are reachable, to use only Tor for sensitive relay? I didn’t do that because I see no usecase for it.

pinheadmz commented at 7:35 pm on August 2, 2023:

I meant, if IPv4 and Tor are the only reachable networks, that Tor connections would only be made for sensitive relay connections. That’s why I tried -onlynet=ipv4 -sensitiverelayowntx=1

vasild commented at 7:35 am on August 3, 2023:

No, I did not consider that.

dergoegge commented at 10:41 am on August 2, 2023: member

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.

in src/net.cpp:1641 in 2541f09439 outdated

1636+                      "closed before opening a new one\n",
1637+                      num_needed,
1638+                      num_opened);
1639+        net = std::nullopt;
1640+        return;
1641+    }

vasild commented at 11:22 am on August 2, 2023:

moving discussion from #27509 (comment)

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

MAX_SENSITIVE_RELAY_CONNECTIONS (64) is supposed to cap it. Did it exceed 64?

pinheadmz commented at 7:33 pm on August 2, 2023:

02023-08-02T19:32:23Z [sensitiverelay:debug]
1  Request for 5 new connections, incremented the number of connections to open from 90 to 95

and,

02023-08-02T19:58:39Z [sensitiverelay:debug] Requested to open 170 connection(s), trying to open one

pinheadmz commented at 8:57 pm on August 2, 2023:

It’s up to 332 now, after 45 min the wallet tried another rebroadcast. Also worth noting that so far none of these signet TXs are coming back to me, appearing on signet explorers, or reducing the mempool unbroadcastcount. I’ll try to figure out why thats not working.

in src/net_processing.h:90 in 2541f09439 outdated

85+
86+    /**
87+     * Schedule a local transaction to be relayed. This is done asynchronously
88+     * either via short-lived privacy connections or via `ScheduleTxForRelayToAll()`.
89+     */
90+    virtual void ScheduleLocalTxForRelay(const uint256& txid, const uint256& wtxid) = 0;

vasild commented at 12:24 pm on August 2, 2023:

moving discussion from #27509 (comment)

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

This looks to be the same in master (I only read the code). Maybe it can be improved. When a new transaction arrives, BroadcastTransaction() is called which adds it to the mempool (via node.mempool->AddUnbroadcastTx()) and also schedules it for relay (via node.peerman->ScheduleLocalTxForRelay()).

However after a restart, LoadMempool() only adds it to the mempool (via pool.AddUnbroadcastTx()). BroadcastTransaction() is not called and neither ScheduleLocalTxForRelay() is called.

This should improve it:

 0diff --git i/src/net_processing.cpp w/src/net_processing.cpp
 1index b541cba0b9..1300d0daf0 100644
 2--- i/src/net_processing.cpp
 3+++ w/src/net_processing.cpp
 4@@ -510,13 +510,14 @@ public:
 5     bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
 6         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
 7     bool SendMessages(CNode* pto) override
 8         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, g_msgproc_mutex);
 9 
10     /** Implement PeerManager */
11-    void StartScheduledTasks(CScheduler& scheduler) override;
12+    void StartScheduledTasks(CScheduler& scheduler) override
13+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
14     void CheckForStaleTipAndEvictPeers() override;
15     std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
16         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
17     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
18     bool IgnoresIncomingTxs() override { return m_opts.ignore_incoming_txs; }
19     void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
20@@ -1869,22 +1870,33 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
21         m_txreconciliation = std::make_unique<TxReconciliationTracker>(TXRECONCILIATION_VERSION);
22     }
23 }
24 
25 void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler)
26 {
27+    AssertLockNotHeld(m_peer_mutex);
28+
29     // Stale tip checking and peer eviction are on two different timers, but we
30     // don't want them to get out of sync due to drift in the scheduler, so we
31     // combine them in one function and schedule at the quicker (peer-eviction)
32     // timer.
33     static_assert(EXTRA_PEER_CHECK_INTERVAL < STALE_CHECK_INTERVAL, "peer eviction timer should be less than stale tip check timer");
34     scheduler.scheduleEvery([this] { this->CheckForStaleTipAndEvictPeers(); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
35 
36     // schedule next run for 10-15 minutes in the future
37     const std::chrono::milliseconds delta = 10min + GetRandMillis(5min);
38     scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta);
39+
40+    if (m_opts.sensitiverelayowntx) {
41+        for (const auto& txid : m_mempool.GetUnbroadcastTxs()) {
42+            CTransactionRef tx = m_mempool.get(txid);
43+            if (tx != nullptr) {
44+                ScheduleLocalTxForRelay(txid, tx->GetWitnessHash());
45+            }
46+        }
47+    }
48 }
49 
50 /**
51  * Evict orphan txn pool entries based on a newly connected
52  * block, remember the recently confirmed transactions, and delete tracked
53  * announcements for them. Also save the time of the last tip update and

I did not include it in this PR yet. What do you think?

It looks like an improvement in master too, but I only did it if sensitive relay is used because the purpose of this PR is not to change the behavior when -sensitiverelayowntx=0 compared to master.

pinheadmz commented at 3:08 pm on August 3, 2023:

OK I see. In that case, it can be a follow-up. Probably there is some reason we keep the scheduler and don’t resend right on startup (we wouldn’t want spy nodes detecting our restart! …?)

vasild force-pushed on Aug 2, 2023

vasild commented at 12:38 pm on August 2, 2023: contributor

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

logging: introduce a new category for sensitive relay 5468894d3f

init: introduce a new option to enable/disable sensitive tx relay

Co-authored-by: brunoerg <brunoely.gc@gmail.com>

3c7ad75236

net: introduce a new connection type for relaying our transactions

We will open a short-lived connection to a random Tor or I2P peer,
send our transaction to that peer and close the connection.

cc28d939de

net: move peers counting before grant acquisition in ThreadOpenConnections()

In a subsequent commit we will need to conditionally acquire the
semaphore grant based on the collected stats (number and types of opened
connections). Thus move the snippet that does the counting earlier. It
jumps accross the fixed seeds addition, but both are unrelated and can
be done in any order.

This is a non-functional change, a mechanical move that is easy to
verify. The code should behave identically before and after it.

7ddce4f98d

net: implement opening SENSITIVE_RELAY connections

Implement opening `ConnectionType::SENSITIVE_RELAY` connections with the
following properties:
* They have higher priority over other connection types
* Don't wait for outbound connection slot to be available
* Only to Tor or I2P
* Open such connections only when requested and don't maintain N opened
  connections of this type. Rather, open N connections once, when asked,
  and fall back to other types (e.g. `OUTBOUND_FULL_RELAY`).

e0f3cfb34e

net_processing: rename RelayTransaction to better describe what it does

Rename `PeerManager::RelayTransaction()` to
`PeerManager::ScheduleTxForRelayToAll()`. The transaction is not relayed
when the method returns. It is only scheduled for relaying at a later
time. Also, there will be another method which only schedules for relay
to Tor or I2P peers.

86d34b2877

net_processing: reorder the code that handles the VERSION message

Change the order in which code snippets are executed as a result of
receiving the `VERSION` message. Move the snippets that do
`PushMessage()` near the end. This will help with handling of
sensitive relay connections - they do not require any of that.

This is a non-functional change.

ff1b8094b7

net_processing: handle ConnectionType::SENSITIVE_RELAY connections

For connections of type `ConnectionType::SENSITIVE_RELAY`:
* After receiving VERACK, relay a transaction from the list of locally
  submitted transactions and disconnect
* Don't process any messages after VERACK
* Don't send any messages other than the minimum required for the
  transaction relay

f03a753c5a

mempool: extend IsUnbroadcastTx() to support wtxid 2a5cf8b115

net_processing: omit unbroadcast txs from replies to GETDATA and MEMPOOL 8ac8abc3f7

net_processing: relay local transactions via the SENSITIVE_RELAY mechanism d37aa61d96

mempool: return whether the tx was removed from RemoveUnbroadcastTx() 76c4e85a62

net_processing: improve own-tx-received-by-the-network assumption

This applies only if sensitive relay is used.

Before this change, we would consider a local transaction relayed
successfully to the network right after we push it to a peer (when
`PushMessage()` returns, we still have not even sent it to the peer
yet).

Instead wait for somebody to send us `INV` about that transaction,
then retrieve it with `GETDATA` (as if we don't have it already) and
only after receiving it, consider it relayed. This is a better
indication that the peer we sent it to has successfully received it,
not blackholed it and the transaction has propagated to the network.

d080ebf512

test: improve debug log message from P2PConnection::connection_made()

This is used in both cases - TCP server (accept) and TCP client (connect).
The message "Connected & Listening address:port" is confusing.

Print both ends of the TCP connection.

a894587c56

test: extend the SOCKS5 Python proxy to actually connect to a destination

If requested, make the SOCKS5 Python proxy redirect connections to a set
of given destinations. Actually act as a real proxy, connecting the
client to a destination, except that the destination is not what the
client asked for.

This would enable us to "connect" to Tor addresses from the functional
tests.

594f7fe6f5

test: support WTX INVs from P2PDataStore and fix a comment f8661495e0

test: add functional test for local tx relay 36074c093b

vasild force-pushed on Aug 4, 2023

in src/net_processing.cpp:4307 in 6e6f83b0f7 outdated

4299@@ -4144,6 +4300,23 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4300         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
4301         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
4302 
4303+        if (m_opts.sensitiverelayowntx && m_mempool.RemoveUnbroadcastTx(txid)) {
4304+            LogPrintLevel( /* Continued */
4305+                BCLog::SENSITIVE_RELAY,
4306+                BCLog::Level::Info,
4307+                "Received own transaction (txid=%s) from the network from peer=%d%s; "

vasild commented at 4:13 pm on August 4, 2023:

moving discussion from #27509 (comment)

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.

The witness was not serialized, so the recipients rejected the transaction :face_with_spiral_eyes: I fixed this and updated the test to catch cases where bitcoind would not accept or further relay the transaction to other peers. Now the test has one more bitcoind, which is used as a recipient.

pinheadmz commented at 5:49 pm on August 4, 2023:

🕺 ah nice! I thought there might be something wrong with the transactions themselves, but when I restarted without sensitive relay everything broadcast fine. I didn’t think of serialization being the issue! So, great catch.

vasild commented at 4:15 pm on August 4, 2023: contributor

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

DrahtBot added the label Needs rebase on Aug 6, 2023

DrahtBot commented at 5:47 pm on August 6, 2023: contributor

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

maflcko commented at 2:33 pm on August 31, 2023: member

@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.

pinheadmz commented at 2:14 pm on September 12, 2023: member

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?)

vasild commented at 8:01 am on September 13, 2023: contributor

@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.

in src/init.cpp:603 in 36074c093b

599@@ -600,6 +600,7 @@ void SetupServerArgs(ArgsManager& argsman)
600                    OptionsCategory::NODE_RELAY);
601     argsman.AddArg("-minrelaytxfee=<amt>", strprintf("Fees (in %s/kvB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)",
602         CURRENCY_UNIT, FormatMoney(DEFAULT_MIN_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
603+    argsman.AddArg("-sensitiverelayowntx", strprintf("Relay locally submitted transactions via short lived connections to Tor or I2P for improved privacy. There are two aspects of this - linking transaction-origin/Bitcoin-owner with IP-address/geolocation and linking seemingly unrelated transactions to the same owner/origin (this applies even if the node is running in tor-only mode) (default: %u)", DEFAULT_SENSITIVE_RELAY_OWN_TX), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);

luke-jr commented at 7:08 pm on September 15, 2023:

Maybe -walletsensitiverelay?

vasild commented at 7:13 am on September 16, 2023:

I used “own” instead of “wallet” because local transactions could originate also from the RPC and they may have nothing to do with the wallet, there may not be a wallet at all. Come to think about this, maybe “private” is better word than “sensitive” and “broadcast” is better than “relay”. So, maybe I should change this to something like -privatebroadcast…

amitiuttarwar commented at 9:01 pm on September 29, 2023: contributor

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 :)

fanquake marked this as a draft on Oct 2, 2023

naumenkogs commented at 9:37 am on October 2, 2023: member

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

DrahtBot commented at 1:54 am on December 31, 2023: contributor

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.

vasild commented at 2:18 pm on January 3, 2024: contributor

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

vasild commented at 1:53 pm on February 9, 2024: contributor

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.

vasild closed this on Feb 9, 2024

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

Code Coverage

Reviews

Conflicts