Broadcast own transactions only via short-lived Tor or I2P connections #29415
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29415. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This comment was marked as abuse.
This comment was marked as abuse.
bd82629 to
74ba7c7
Compare
|
@1440000bytes, thanks for asking! There is some discussion at #27509 (the previous attempt on this).
Yes, it is. See below.
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.
Linked in the OP, thanks! |
|
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? |
|
@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. |
Suggested by: David Gumberg (bitcoin#29415 (comment))
|
code review diff ACK 8937221 Github diff seems broken - https://github.com/bitcoin/bitcoin/compare/662270f5b8d96434faec9e6f55f566f4f2ba1261..89372213048adf37a47427112a1ff836ee84c50e
patch since last ACKMy interpretation of the changes:
I'm happy to reack if further changes are applied, but I think it's also fine to do the nits in follow-ups. |
|
|
||
| self.destinations_lock = threading.Lock() | ||
|
|
||
| def destinations_factory(requested_to_addr, requested_to_port): |
There was a problem hiding this comment.
nit: i got this intermittent failure, not sure what the cause might be.
Checking that rebroadcast works
Task exception was never retrieved
future: <Task finished name='Task-59' coro=<NetworkThread.create_listen_server() done, defined at /home/ubuntu/Projects/bitcoin/test/functional/test_framework/p2p.py:775> exception=OSError(98, "error while attempting to bind on address ('127.0.0.1', 24713): address already in use")>
Traceback (most recent call last):
File "/home/ubuntu/Projects/bitcoin/test/functional/test_framework/p2p.py", line 796, in create_listen_server
listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/asyncio/base_events.py", line 1572, in create_server
raise OSError(err.errno, msg) from None
OSError: [Errno 98] error while attempting to bind on address ('127.0.0.1', 24713): address already in use
There was a problem hiding this comment.
Could it be that there was another program listening on 127.0.0.1:24713, maybe not even bitcoin related?
Did you run just one functional test at the time, or two or more concurrently, e.g. in different terminals?
Can you reproduce with this patch and post the full log (e.g /tmp/bitcoin_func_test_.../test_framework.log:
[patch] add debug log before bind
diff --git i/test/functional/p2p_private_broadcast.py w/test/functional/p2p_private_broadcast.py
index 4c3739d8a0..4ae33853b9 100755
--- i/test/functional/p2p_private_broadcast.py
+++ w/test/functional/p2p_private_broadcast.py
@@ -212,12 +212,13 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
def on_listen_done(addr, port):
nonlocal actual_to_addr
nonlocal actual_to_port
actual_to_addr = addr
actual_to_port = port
+ self.log.debug(f"Binding on 127.0.0.1:{ports_base + i} to serve connection to proxy i={i}")
self.network_thread.listen(
addr="127.0.0.1",
port=ports_base + i,
p2p=listener,
callback=on_listen_done)
# Wait until the callback has been called.There was a problem hiding this comment.
The below patch enables dynamic port allocation and prevents this kind of error.
diff --git a/test/functional/p2p_private_broadcast.py b/test/functional/p2p_private_broadcast.py
index 4c3739d8a0..803444ccba 100755
--- a/test/functional/p2p_private_broadcast.py
+++ b/test/functional/p2p_private_broadcast.py
@@ -36,7 +36,6 @@ from test_framework.test_framework import (
BitcoinTestFramework,
)
from test_framework.util import (
- MAX_NODES,
assert_equal,
assert_not_equal,
assert_raises_rpc_error,
@@ -181,9 +180,6 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
self.socks5_server = Socks5Server(socks5_server_config)
self.socks5_server.start()
- # Tor ports are the highest among p2p/rpc/tor, so this should be the first available port.
- ports_base = tor_port(MAX_NODES) + 1
-
self.destinations = []
self.destinations_lock = threading.Lock()
@@ -215,9 +211,12 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
actual_to_addr = addr
actual_to_port = port
+ # Use port=0 to let the OS assign an available port. This
+ # avoids "address already in use" errors when tests run
+ # concurrently or ports are still in TIME_WAIT state.
self.network_thread.listen(
addr="127.0.0.1",
- port=ports_base + i,
+ port=0,
p2p=listener,
callback=on_listen_done)
# Wait until the callback has been called.
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 346a7c6526..74f6262fc7 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -794,8 +794,12 @@ class NetworkThread(threading.Thread):
# `proto` functions
listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
- logger.debug("Listening server on %s:%d should be started" % (addr, port))
- cls.listeners[(addr, port)] = listener
+ # When port=0, the OS assigns an available ephemeral port. Retrieve
+ # the actual port so callers know where the server is listening.
+ actual_port = listener.sockets[0].getsockname()[1]
+ logger.debug("Listening server on %s:%d should be started" % (addr, actual_port))
+ cls.listeners[(addr, actual_port)] = listener
+ port = actual_port
cls.protos[(addr, port)] = proto
callback(addr, port)There was a problem hiding this comment.
I don't think the test code is correct, and I think the comment is wrong as well:
# Tor ports are the highest among p2p/rpc/tor, so this should be the first available port.
ports_base = tor_port(MAX_NODES) + 1This will not pick "the first available port". Port ranges are tightly packed, so this will just pick the first used port of the next test. The test failure here is expected.
Generally, it is only possible to use ports that are assigned to the test itself. One way to achieve that is by calling tor_port(self.num_nodes) (or self.num_nodes + i). This way, the port is known to be unused, because that node does not exist. See also #33260
There was a problem hiding this comment.
The below patch enables dynamic port allocation
The patch above works and looks reasonable.
One way to achieve that is by calling
tor_port(self.num_nodes)
This works as well:
- ports_base = tor_port(MAX_NODES) + 1
+ ports_base = tor_port(self.num_nodes)I will address in a followup.
|
ACK 8937221 @l0rinc It shows the changes:
|
l0rinc
left a comment
There was a problem hiding this comment.
I went over the code again (not just the diff) and noticed a few things I'd like to get clarity on. I don't mind if you decide to tackle these in follow-ups, since I'm not 100% sure about them.
| } | ||
| const CTransactionRef& tx{*opt_tx}; | ||
|
|
||
| LogInfo("[privatebroadcast] P2P handshake completed, sending INV for txid=%s%s, peer=%d%s", |
There was a problem hiding this comment.
Not exactly sure what the protocol is here - is it okay to print sensitive data to console?
Given that this is a private tx, the user shouldn't share these with others - is that obvious to the user? Anyone sharing debug.log would leak which transactions they originated...
We do have a dedicated -debug=privatebroadcast category, should this be LogDebug instead?
Or have a LogInfo without details followed by a logdebug with the txid?
There was a problem hiding this comment.
Interesting angle. I did not think about this before. Local txid is no more secret if we use private broadcast compared to if we use current broadcast-to-all method. Do we ever log local txids with broadcast-to-all? I only found a debug log in CTxMemPool::RemoveUnbroadcastTx().
should this be
LogDebuginstead?
What would be the rationale - that it is ok to log semi-sensitive information with LogDebug() and not with LogInfo()? But then if a user shares their debug.log, then they might do that with or without enabling debug messages.
There was a problem hiding this comment.
that it is ok to log semi-sensitive information with LogDebug() and not with LogInfo()?
Yes, that's a deliberate choice
I think this should be done in a follow-up - we may want to desensitise other logs as well while we're there...
There was a problem hiding this comment.
Congrats on the merge <3
Added this as a follow-up: #34267
w0xlt
left a comment
There was a problem hiding this comment.
ACK 8937221 with nit #29415 (comment)
Private broadcast is a privacy feature, and users may share `debug.log` for support. Unconditional log messages about private broadcast (and especially those including a `(w)txid`) can leak which transactions a user originated and/or that they used private broadcast. Log private broadcast events with `LogDebug(BCLog::PRIVBROADCAST, ...)` so they are only emitted when `-debug=privatebroadcast` was explictly enabled. Keep the Tor/I2P reachability warning unconditional, but phrase it without a dedicated "[privatebroadcast]" tag. Ref: bitcoin#29415 (comment)
Private broadcast is a privacy feature, and users may share `debug.log` for support. Unconditional log messages about private broadcast (and especially those including a `(w)txid`) can leak which transactions a user originated and/or that they used private broadcast. Log private broadcast events with `LogDebug(BCLog::PRIVBROADCAST, ...)` so they are only emitted when `-debug=privatebroadcast` was explicitly enabled. Keep the Tor/I2P reachability warning unconditional, but phrase it without a dedicated "[privatebroadcast]" tag. Ref: bitcoin#29415 (comment)
Make `PeerManagerImpl::m_tx_for_private_broadcast` optional because it is needed for in private broadcast mode (`-privatebroadcast=1`). Requested in: bitcoin#29415 (comment) https://github.com/bitcoin/bitcoin/pull/29415/files#r2620864832
Make `PeerManagerImpl::m_tx_for_private_broadcast` optional because it is needed for in private broadcast mode (`-privatebroadcast=1`). Requested in: bitcoin#29415 (comment) https://github.com/bitcoin/bitcoin/pull/29415/files#r2620864832
ce63d37 test: use dynamic port allocation to avoid test conflicts (woltx) Pull request description: Use `port=0` for dynamic port allocation in test framework components to avoid intermittent "address already in use" errors when running tests concurrently or when ports are stuck in TIME_WAIT state. Example: #29415 (comment) Changes: - Update `socks5.py` and `p2p.py` to support dynamic port allocation - Convert `feature_proxy.py` and `feature_anchors.py` to use `port=0` ACKs for top commit: achow101: ACK ce63d37 vasild: ACK ce63d37 mzumsande: re-ACK ce63d37 Tree-SHA512: 4efcedca3bde209fbd1bdc2a4ae04b7d53515587d86e421ce61064f78c675c71b45d9782b514c5e7cfc0e92842c947d49f7a3fddb03fe619fcdec9b565f0ecbd
3e34067 test: use ephemeral ports in p2p_private_broadcast.py (w0xlt) Pull request description: The test `p2p_private_broadcast.py` gets some Python P2P nodes to listen and instructs the SOCKS5 proxy to redirect connections to them instead of to the requested addresses. This way the `bitcoind` which uses the proxy is tricked to think it has connected to real routable internet IP addresses or `.onion` addresses. Picking the ports where to Python P2P nodes to listen however is tricky to be done in a non-conflicting way, given that other tests may run in parallel. #34186 made it possible to let the OS select a free port, so use that in `p2p_private_broadcast.py`. --- _Suggested in #29415 (comment) ACKs for top commit: l0rinc: code review ACK 3e34067 polespinasa: tACK 3e34067 mzumsande: utACK 3e34067 Tree-SHA512: e94efd33a1845e1767aaada55f91c60bc5fc1166c281ef578a391e95e2791a922d84aa6ed1ce06e7d6ca1a65f84da52fd79d9b2f40705c1944a53c67b7392e4d
Parts of this PR are isolated in independent smaller PRs to ease review:
To improve privacy, broadcast locally submitted transactions (from the
sendrawtransactionRPC) to the P2P network only via Tor or I2P short-lived connections, or to IPv4/IPv6 peers but through the Tor network.Introduce a new connection type for private broadcast of transactions with the following properties:
PINGis sent and after receivingPONGthe connection is closedPONG)Broadcast transactions submitted via
sendrawtransactionusing this new mechanism, to a few peers. Keep doing this until we receive back 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
INVfrom one of our ordinary peers, then the normal flow executes: we request the transaction withGETDATA, receive it with aTXmessage, 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
TXmessage, in reply to ourGETDATArequest, 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:
Whenever a new transaction is received from
sendrawtransactionRPC, the node will send it to a few (NUM_PRIVATE_BROADCAST_PER_TX) recipients right away. If after some time we still have not heard anything about the transaction from the network, then it will be sent to 1 more peer (seePeerManagerImpl::ReattemptPrivateBroadcast()).A few considerations:
How to test this?
Thank you, @stratospher and @andrewtoth!
Start
bitcoindwith-privatebroadcast=1 -debug=privatebroadcast.Create a wallet and get a new address, go to the Signet faucet and request some coins to that address:
Get a new address for the test transaction recipient:
Create the transaction:
Finally, send the transaction:
High-level explanation of the commits
New logging category and config option to enable private broadcast
log: introduce a new category for private broadcastinit: introduce a new option to enable/disable private broadcastImplement the private broadcast connection handling on the
CConnmanside:net: introduce a new connection type for private broadcastnet: implement opening PRIVATE_BROADCAST connectionsPrepare
BroadcastTransaction()for private broadcast requests:net_processing: rename RelayTransaction to better describe what it doesnode: extend node::TxBroadcast with a 3rd optionnet_processing: store transactions for private broadcast in PeerManagerImplement the private broadcast connection handling on the
PeerManagerside:net_processing: reorder the code that handles the VERSION messagenet_processing: move the debug log about receiving VERSION earliernet_processing: modernize PushNodeVersion()net_processing: move a debug check in VERACK processing earliernet_processing: handle ConnectionType::PRIVATE_BROADCAST connectionsnet_processing: stop private broadcast of a transaction after round-tripnet_processing: retry private broadcastEngage the new functionality from
sendrawtransaction:rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ONNew tests:
test: add functional test for private broadcasttest: add unit test for the private broadcast storageThis PR would resolve the following issues:
#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
#32235 Tor: TX circuit isolation
Issues that are related, but (maybe?) not to be resolved by this PR:
#21876 Broadcast a transaction to specific nodes
#28636 new RPC: sendrawtransactiontopeer
Further extensions:
submitpackageRPC do the private broadcast as well, draft diff in the comment below, thanks ismaelsadeeq!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.