Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Feb 9, 2024

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

  • Introduce a new connection type for private broadcast of transactions with the following properties:

    • started whenever there are local 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, PING is sent and after receiving PONG the connection is closed
    • ignore all incoming messages after handshake is completed (except PONG)
  • Broadcast transactions submitted via sendrawtransaction using this new mechanism, to a few Tor or I2P peers. Keep doing this until we receive an INV about this transaction from one of our ordinary peers (this takes about 1 second on mainnet).

  • The transaction is stored in peerman and does not enter the mempool.

  • Once we get an INV from somebody, then the normal flow executes: we request the transaction with GETDATA, receive it with a TX message, put it in our mempool and broadcast it to all our existent connections (as if we see it for the first time).

  • 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 storage in peerman, ending the private broadcast attempts.

The messages exchange should look like this:

tx-sender >--- connect -------> tx-recipient
tx-sender >--- VERSION -------> tx-recipient (dummy VERSION with no revealing data)
tx-sender <--- VERSION -------< tx-recipient
tx-sender <--- WTXIDRELAY ----< tx-recipient (maybe)
tx-sender <--- SENDADDRV2 ----< tx-recipient (maybe)
tx-sender <--- SENDTXRCNCL ---< tx-recipient (maybe)
tx-sender <--- VERACK --------< tx-recipient
tx-sender >--- VERACK --------> tx-recipient
tx-sender >--- TX ------------> tx-recipient
tx-sender >--- PING ----------> tx-recipient
tx-sender <--- PONG ----------< tx-recipient
tx-sender disconnects

Whenever a new transaction is received from sendrawtransaction RPC, the node will send it to 5 (NUM_PRIVATE_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 1 more peer (see PeerManagerImpl::ReattemptPrivateBroadcast()).

A few considerations:

  • The short-lived private broadcast connections are very cheap and fast wrt network traffic. It is expected that some of those peers could 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.

Some explanation of the commits:

  • New logging category and config option to enable private broadcast

    • log: introduce a new category for private broadcast
    • init: introduce a new option to enable/disable private broadcast
  • Implement the private broadcast connection handling on the CConnman side:

    • net: introduce a new connection type for private broadcast
    • net: move peers counting before grant acquisition in ThreadOpenConnections()
    • net: implement opening PRIVATE_BROADCAST connections
  • Prepare BroadcastTransaction() for private broadcast requests:

    • net_processing: rename RelayTransaction to better describe what it does
    • node: change a tx-relay on/off flag to a tri-state
    • net_processing: store transactions for private broadcast in PeerManager
  • Implement the private broadcast connection handling on the PeerManager side:

    • net_processing: reorder the code that handles the VERSION message
    • net_processing: handle ConnectionType::PRIVATE_BROADCAST connections
    • net_processing: stop private broadcast of a transaction after round-trip
    • net_processing: retry private broadcast
  • Engage the new functionality from sendrawtransaction:

    • rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON
  • Independent test framework improvements (also opened as a standalone PR at test: extend the SOCKS5 Python proxy to actually connect to a destination #29420):

    • test: improve debug log message from P2PConnection::connection_made()
    • test: extend the SOCKS5 Python proxy to actually connect to a destination
    • test: support WTX INVs from P2PDataStore and fix a comment
    • test: set P2PConnection::p2p_connected_to_node in peer_connect_helper()
  • New functional test that exercies some of the new code:

    • test: add functional test for local tx relay

This addresses:
#3828 Clients leak IPs if they are recipients of a transaction
#14692 Can't configure bitocoind to only send tx via Tor but receive clearnet transactions
#19042 Tor-only transaction broadcast onlynet=onion alternative
#24557 Option for receive events with all networks, but send transactions and/or blocks only with anonymous network[s]?
#25450 Ability to broadcast wallet transactions only via dedicated oneshot Tor connections

Related, but different:
#21876 Broadcast a transaction to specific nodes
#28636 new RPC: sendrawtransactiontopeer


Next step after this PR is done will be to have the wallet do the private broadcast as well, #11887 would have to be resolved.


A previous incarnation of this can be found at #27509. It puts the transaction in the mempool and (tries to) hide it from the outside observers. This turned out to be too error prone or maybe even impossible.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 9, 2024

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 andrewtoth, zzzi2p, nothingmuch
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:

  • #30212 (rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN by willcl-ark)
  • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
  • #30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
  • #30065 (init: fixes file descriptor accounting by sr-gi)
  • #30058 (Encapsulate warnings in generalized node::Warnings and remove globals by stickies-v)
  • #29798 (Logging cleanup by vasild)
  • #29680 (wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict by Eunovo)
  • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
  • #29625 (Several randomness improvements by sipa)
  • #29605 (net: Favor peers from addrman over fetching seednodes by sr-gi)
  • #29432 (Stratum v2 Template Provider (take 3) by Sjors)
  • #29431 (test/BIP324: disconnection scenarios during v2 handshake by stratospher)
  • #29346 (Stratum v2 Noise Protocol by Sjors)
  • #29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
  • #29256 (Improve new LogDebug/Trace/Info/Warning/Error Macros by ryanofsky)
  • #29015 (kernel: Streamline util library by ryanofsky)
  • #28710 (Remove the legacy wallet and BDB dependency by achow101)
  • #28488 (p2p: Evict outbound peers with high minFeeRate by naumenkogs)
  • #28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
  • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
  • #27826 (validation: log which peer sent us a header by Sjors)
  • #26697 (logging: use bitset for categories by LarryRuane)
  • #25832 (tracing: network connection tracepoints by 0xB10C)

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.

@1440000bytes
Copy link

I am not sure about this approach to improve privacy. Is it necessary to open new short lived tor/i2p connections for broadcasting the transaction? What are the trade-offs in this implementation vs a simple implementation to relay tx to one or more peers that our node is already connected to?

Related issues:

#21876
#28636

@vasild
Copy link
Contributor Author

vasild commented Feb 10, 2024

@1440000bytes, thanks for asking! There is some discussion at #27509 (the previous attempt on this).

Is it necessary to open new short lived tor/i2p connections for broadcasting the transaction?

Yes, it is. See below.

What are the trade-offs in this implementation vs a simple implementation to relay tx to one or more peers that our node is already connected to?

Sending the transaction over clearnet reveals the IP address/geolocation of the sender. A spy with many connections to the network could try to guess who was the originator. So, why not send it to our Tor peers only? Because it is relatively easy for a spy to fingerprint and link clearnet and Tor connections to the same peer. That is, a long running connection over Tor could be linked to a long running clearnet connection. This is why the proposed changes open a short-lived connection that does not reveal any of the identity of the sender.

Would this benefit nodes that don't have clearnet connections, e.g. Tor/I2P-only nodes? Yes! In the case where the sender sends two otherwise unrelated transactions over the same long-running Tor connection, the recipient will know that they have the same origin, even though they are not related on-chain. Using single shot connections fixes that too.

Related issues:

Linked in the OP, thanks!

src/logging.cpp Outdated Show resolved Hide resolved
@epiccurious
Copy link
Contributor

v2 Transport will be enabled by default in the next release (#29347).

If there were eventually a change to force clearnet transactions over v2 transport (so the details of the communications were encrypted), would that solve the same problem that this PR is aiming to solve?

@vasild
Copy link
Contributor Author

vasild commented Feb 11, 2024

@epiccurious, p2p encryption "solves" the spying from intermediate routers on clearnet (aka man-in-the-middle). Tor, I2P and CJDNS solve that too. While this PR uses only Tor and I2P it would solve that problem. But there is more - it will as well solve issues with spying bitcoin nodes.

vasild added a commit to vasild/bitcoin that referenced this pull request Feb 11, 2024
vasild and others added 3 commits May 21, 2024 15:04
Co-authored-by: brunoerg <brunoely.gc@gmail.com>
We will open a short-lived connection to a random Tor or I2P peer,
send our transaction to that peer and close the connection.
…tions()

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.
@vasild
Copy link
Contributor Author

vasild commented May 21, 2024

65ba8d0203...e85cc59bad: rebase, address suggestions and remove the argument of ProxyForClearnetPrivateBroadcast() because it is always IPv4 or IPv6 and we treat both in the same way.

The main thing I'm wondering currently is why are we tacking m_private_broadcast_connections_to_open loosely? It feels harder to reason about, but I don't see what the benefit of it is.

Continued at #29415 (comment) to avoid overwhelming the main discussion thread of the PR and to make replies close to each other (rather than scattered in the main thread).

src/net.h Outdated
* Number of `ConnectionType::PRIVATE_BROADCAST` connections to open.
* Whenever such a connection is opened this is decremented with 1.
*/
std::atomic_size_t m_private_broadcast_connections_to_open{0};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the main thread at #29415 (review)

The main thing I'm wondering currently is why are we tacking m_private_broadcast_connections_to_open loosely? It feels harder to reason about, but I don't see what the benefit of it is.

The reason for this is that it is simpler to implement that way (I think, somebody has a simpler proposal?). This is because the connman thread is creating the connections, they are consumed/used by the peerman thread and the requests for new connections can come from yet another (3rd) thread. For example, while connman is creating a new connection which is needed to broadcast a given transaction, that transaction may be successfully broadcast by peerman and received back from the network, rendering the just created new connection by connman unnecessary. This is harmless and I accept it because avoiding it would make things too complicated or slow.

vasild and others added 14 commits May 22, 2024 13:37
Implement opening `ConnectionType::PRIVATE_BROADCAST` 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 (or IPv4/IPv6 through the Tor proxy, if provided)
* Open such connections only when requested and don't maintain N opened
  connections of this type.
Rename `PeerManager::RelayTransaction()` to
`PeerManager::ScheduleTxForBroadcastToAll()`. The transaction is not relayed
when the method returns. It is only scheduled for broadcasting at a later
time. Also, there will be another method which only schedules for broadcast
to Tor or I2P peers.
Previously the `bool relay` argument to `BroadcastTransaction()`
designated:

```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```

Extend this with a third option to not add the transaction to the
mempool and broadcast privately.

This is a non-functional change - the new third option is not handled
inside `BroadcastTransaction()` and is not used by any of the callers.

The idea for the new `node/types.h` and the comments in it by Ryan.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Extend `PeerManager` with a transaction storage and a new method
`ScheduleTxForPrivateBroadcast()` which:
* adds a transaction to that storage and
* calls `CConnman::PrivateBroadcastAdd()` to open dedicated privacy
  connections that will pick an entry from the transaction storage and
  broadcast it.
Change the order in which code snippets are executed as a result of
receiving the `VERSION` message. Move the snippets that do
`MakeAndPushMessage()` near the end. This will help with handling of
private broadcast connections - they do not require any of that.

This is a non-functional change.
For connections of type `ConnectionType::PRIVATE_BROADCAST`:
* After receiving VERACK, relay a transaction from the list of
  transactions for private broadcast and disconnect
* Don't process any messages after VERACK
* Don't send any messages other than the minimum required for the
  transaction relay
Remove the transaction from the list of transactions to broadcast after
we receive it from the network.
Periodically check for stale transactions in peerman and if found,
reschedule new connections to be opened by connman for broadcasting
them.
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.
…tion

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.
Set `P2PConnection.p2p_connected_to_node` in
`P2PConnection.peer_connect_helper()` instead of
`TestNode.add_p2p_connection()` and
`TestNode.add_outbound_p2p_connection()`.

This way tests can create an instance of `P2PConnection` and use
`P2PConnection.peer_connect_helper()` directly.
@vasild
Copy link
Contributor Author

vasild commented May 22, 2024

e85cc59bad...057c79365c: in order to decide whether to private broadcast to IPv4/IPv6 peers, instead of (before this force push) requiring that -onion= is explicitly set, do this (after this force push): keep track if we ever managed to connect to an .onion address via the NET_ONION proxy, if yes assume that this proxy is a Tor proxy and connecting to IPv4/IPv6 addresses through it will be done via the Tor network and via a Tor exit node.

This is better because it is stronger guarantee (before the user could still misconfigure an orginary non-Tor proxy as -onion=) and works automatically, without requiring config options to be set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet