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 react with 👎 to this comment and the bot will ignore it on the next update. 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))
|
To be clear, in what ways does the proposed feature resemble/differ from the
and from the guide:
Would monero's implementation be useful at all? |
|
@User087 There are a few differences from the feature you linked. From the same guide, the very last section:
The proposed feature here does not use any existing connections for transmitting transactions. It creates a new connection to a random peer for every transaction that is submitted. From my understanding, the |
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.
Normally `ConnectNode()` could choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`. Document both functions.
Implement opening `ConnectionType::PRIVATE_BROADCAST` connections with the following properties: * 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. * Since this is substantially different than what `OpenNetworkConnection()` does, open the private broadcast connections from a different thread instead of modifying `OpenNetworkConnection()` to also open those types of connections.
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. Only remove the transaction if it is the same as the one we sent: both txid and wtxid match. Don't remove transactions that have the same txid and different wxtid. Such transactions show that some of the private broadcast recipients malleated the witness and the transaction made it back to us. The witness could be either: * invalid, in which case the transaction will not be accepted in anybody's pool; or * valid, in which case either the original or the malleated transaction will make it to nodes' mempools and eventually be mined. Our response is to keep broadcasting the original. If the malleated transaction wins then we will eventually stop broadcasting the original when it gets stale and gets removed from the "to broadcast" storage cause it is not acceptable in our mempool.
Periodically check for stale transactions in peerman and if found, reschedule new connections to be opened by connman for broadcasting them.
6d697bb to
dbd76e6
Compare
|
@ismaelsadeeq, I thought about this, but decided to have a single global option to control the behavior given that the plan is to do private broadcast from other places as well. Otherwise it would be a messy situation where "my wallet is doing private broadcast but my RPC is not" or similar.
Link them together to the same originator, even if the UTXOs are unrelated. |
|
re-ACK dbd76e6 Changes from my last ACK cedbc2c ( 4480ca4 - Added a comment I tested |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
…ing.cpp b0344c2 logging: remove unused BCLog::UTIL (Vasil Dimov) d3b3af9 log: deduplicate category names and improve logging.cpp (Vasil Dimov) Pull request description: The code in `logging.cpp` needs to: * Get the category name given the flag (e.g. `BCLog::PRUNE` -> `"prune"`) * Get the flag given the category name (e.g. `"prune"` -> `BCLog::PRUNE`) * Get the list of category names sorted in alphabetical order Achieve this by using the proper std containers. The result is * less code (the diff of the first commit is +62 / -129) * faster code (to linear search and no copy+sort) * more maintainable code (the categories are no longer duplicated in `LogCategories[]` and `LogCategoryToStr()`) This behavior is preserved: `BCLog::NONE` -> `""` (lookup by `LogCategoryToStr()`) `""` -> `BCLog::ALL` (lookup by `GetLogCategory("")`) --- Also remove unused `BCLog::UTIL`. --- These changes (modulo the `BCLog::UTIL` removal) are part of bitcoin#29415 but they make sense on their own and would be good to have them, regardless of the fate of bitcoin#29415. Also, if this is merged, that would reduce the size of bitcoin#29415, thus the current standalone PR. ACKs for top commit: davidgumberg: crACK bitcoin@b0344c2 pinheadmz: ACK b0344c2 ryanofsky: Code review ACK b0344c2. Nice cleanup! Having to maintain multiple copies of the same mapping seemed messy and a like a possible footgun. I checked old and new mappings in both directions and confirmed no behavior should be changing. Tree-SHA512: 57f87a090932f9b33dc8e075d1855dba9b71a3243a0758511745483dec2d9c46d3b532eadab297e78164c9b7caba370986ee380696a45f0778a841082f8e21a7
|
I successfully completed an end-to-end test: I configured the Bitcoin Keeper android wallet (v2.5.1) to connect to my electrs instance (v0.10.10) which in turn connected to bitcoind at dbd76e6 on testnet4. A basic transaction created in Keeper was sent through electrs and out to the network via private broadcast without any complaints. No modifications needed in Keeper or electrs. As far as electrs was concerned, nothing was out of the ordinary: bitcoind logs: |
|
Concept ACK |
|
Needs trivial rebase in commit diff --cc test/functional/p2p_orphan_handling.py
index f2eed48603c,94b911f4b5a..00000000000
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
from test_framework.messages import (
CInv,
++<<<<<<< HEAD
+ CTxInWitness,
+ DEFAULT_ANCESTOR_LIMIT,
++=======
++>>>>>>> b817c3e45bd (test: move create_malleated_version() to messages.py for reuse)
MSG_TX,
MSG_WITNESS_TX,(Not a blocker for reviewing further.) |
| @@ -655,6 +655,15 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) | |||
| OptionsCategory::NODE_RELAY); | |||
| argsman.AddArg("-minrelaytxfee=<amt>", strprintf("Fees (in %s/kvB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)", | |||
| CURRENCY_UNIT, FormatMoney(DEFAULT_MIN_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); | |||
| argsman.AddArg("-privatebroadcast", | |||
| strprintf( | |||
| "Broadcast transactions submitted via sendrawtransaction RPC using short lived " | |||
There was a problem hiding this comment.
ee75225 two minor grammar touch-ups
| "Broadcast transactions submitted via sendrawtransaction RPC using short lived " | |
| "Broadcast transactions submitted via sendrawtransaction RPC using short-lived " |
and on the next line, s/networks without/networks, without/
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 10-15 mins 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: support overriding the proxy selection in ConnectNode()net: implement opening PRIVATE_BROADCAST connectionsPrepare
BroadcastTransaction()for private broadcast requests:net_processing: rename RelayTransaction to better describe what it doesnode: change a tx-relay on/off flag to a tri-statenet_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: 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 ONIndependent test framework improvement:
test: move create_malleated_version() to messages.py for reuseNew functional test that exercies some of the new code:
test: add functional test for local tx relayAdd an intermediate step that sends
INVmessage and waits for a request for the transaction. If reviewers like this, then I will squash it into the relevant commit, or otherwise drop it:fixup! net_processing: handle ConnectionType::PRIVATE_BROADCAST connectionsThis PR would close 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 closed 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.