e71c4df168 refactor: replace manual promise with SyncWithValidationInterfaceQueue (ANtutov)
Pull request description:
`BroadcastTransaction()` now waits for validation callbacks using the built-in `validation_signals>SyncWithValidationInterfaceQueue()` instead of creating a local `std::promise` and scheduling a lambda. This removes an unnecessary allocation and uses the canonical API.
ACKs for top commit:
maflcko:
review ACK e71c4df168🌃
rkrux:
lgtm ACK e71c4df168
sedited:
ACK e71c4df168
Tree-SHA512: 602994ba3c2ac91996068aee6eac7e788c3832d7ab949519a9420d2b59e2a67d2d4e67c3c9191ba60e9caa75f1524a95b0851fcd40b6732f6a9956a011b4a120
8937221304 doc: add release notes for 29415 (Vasil Dimov)
582016fa5f test: add unit test for the private broadcast storage (Vasil Dimov)
e74d54e048 test: add functional test for private broadcast (Vasil Dimov)
818b780a05 rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON (Vasil Dimov)
eab595f9cf net_processing: retry private broadcast (Vasil Dimov)
37b79f9c39 net_processing: stop private broadcast of a transaction after round-trip (Vasil Dimov)
2de53eee74 net_processing: handle ConnectionType::PRIVATE_BROADCAST connections (Vasil Dimov)
30a9853ad3 net_processing: move a debug check in VERACK processing earlier (Vasil Dimov)
d1092e5d48 net_processing: modernize PushNodeVersion() (Vasil Dimov)
9937a12a2f net_processing: move the debug log about receiving VERSION earlier (Vasil Dimov)
a098f37b9e net_processing: reorder the code that handles the VERSION message (Vasil Dimov)
679ce3a0b8 net_processing: store transactions for private broadcast in PeerManager (Vasil Dimov)
a3faa6f944 node: extend node::TxBroadcast with a 3rd option (Vasil Dimov)
95c051e210 net_processing: rename RelayTransaction() to better describe what it does (Vasil Dimov)
bb49d26032 net: implement opening PRIVATE_BROADCAST connections (Vasil Dimov)
01dad4efe2 net: introduce a new connection type for private broadcast (Vasil Dimov)
94aaa5d31b init: introduce a new option to enable/disable private broadcast (Vasil Dimov)
d6ee490e0a log: introduce a new category for private broadcast (Vasil Dimov)
Pull request description:
_Parts of this PR are isolated in independent smaller PRs to ease review:_
* [x] _https://github.com/bitcoin/bitcoin/pull/29420_
* [x] _https://github.com/bitcoin/bitcoin/pull/33454_
* [x] _https://github.com/bitcoin/bitcoin/pull/33567_
* [x] _https://github.com/bitcoin/bitcoin/pull/33793_
---
To improve privacy, broadcast locally submitted transactions (from the `sendrawtransaction` RPC) 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:
* started whenever there are local transactions to be sent
* opened to Tor or I2P peers or IPv4/IPv6 via the Tor proxy
* 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 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 `INV` from one of our ordinary peers, 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 >--- INV/TX --------> tx-recipient
tx-sender <--- GETDATA/TX ----< 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 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 (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.
---
<details>
<summary>How to test this?</summary>
Thank you, @stratospher and @andrewtoth!
Start `bitcoind` with `-privatebroadcast=1 -debug=privatebroadcast`.
Create a wallet and get a new address, go to the Signet faucet and request some coins to that address:
```bash
build/bin/bitcoin-cli -chain="signet" createwallet test
build/bin/bitcoin-cli -chain="signet" getnewaddress
```
Get a new address for the test transaction recipient:
```bash
build/bin/bitcoin-cli -chain="signet" loadwallet test
new_address=$(build/bin/bitcoin-cli -chain="signet" getnewaddress)
```
Create the transaction:
```bash
# Option 1: `createrawtransaction` and `signrawtransactionwithwallet`:
txid=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .txid')
vout=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .vout')
echo "txid: $txid"
echo "vout: $vout"
tx=$(build/bin/bitcoin-cli -chain="signet" createrawtransaction "[{\"txid\": \"$txid\", \"vout\": $vout}]" "[{\"$new_address\": 0.00001000}]" 0 false)
echo "tx: $tx"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" signrawtransactionwithwallet "$tx" | jq -r '.hex')
echo "signed_tx: $signed_tx"
# OR Option 2: `walletcreatefundedpsbt` and `walletprocesspsbt`:
# This makes it not have to worry about inputs and also automatically sends back change to the wallet.
# Start `bitcoind` with `-fallbackfee=0.00003000` for instance for 3 sat/vbyte fee.
psbt=$(build/bin/bitcoin-cli -chain="signet" walletcreatefundedpsbt "[]" "[{\"$new_address\": 0.00001000}]" | jq -r '.psbt')
echo "psbt: $psbt"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" walletprocesspsbt "$psbt" | jq -r '.hex')
echo "signed_tx: $signed_tx"
```
Finally, send the transaction:
```bash
raw_tx=$(build/bin/bitcoin-cli -chain="signet" sendrawtransaction "$signed_tx")
echo "raw_tx: $raw_tx"
```
</details>
---
<details>
<summary>High-level explanation of the commits</summary>
* 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: implement opening PRIVATE_BROADCAST connections`
* Prepare `BroadcastTransaction()` for private broadcast requests:
* `net_processing: rename RelayTransaction to better describe what it does`
* `node: extend node::TxBroadcast with a 3rd option`
* `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: move the debug log about receiving VERSION earlier`
* `net_processing: modernize PushNodeVersion()`
* `net_processing: move a debug check in VERACK processing earlier`
* `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`
* New tests:
* `test: add functional test for private broadcast`
* `test: add unit test for the private broadcast storage`
</details>
---
**This PR would resolve the following issues:**
https://github.com/bitcoin/bitcoin/issues/3828 Clients leak IPs if they are recipients of a transaction
https://github.com/bitcoin/bitcoin/issues/14692 Can't configure bitocoind to only send tx via Tor but receive clearnet transactions
https://github.com/bitcoin/bitcoin/issues/19042 Tor-only transaction broadcast onlynet=onion alternative
https://github.com/bitcoin/bitcoin/issues/24557 Option for receive events with all networks, but send transactions and/or blocks only with anonymous network[s]?
https://github.com/bitcoin/bitcoin/issues/25450 Ability to broadcast wallet transactions only via dedicated oneshot Tor connections
https://github.com/bitcoin/bitcoin/issues/32235 Tor: TX circuit isolation
**Issues that are related, but (maybe?) not to be resolved by this PR:**
https://github.com/bitcoin/bitcoin/issues/21876 Broadcast a transaction to specific nodes
https://github.com/bitcoin/bitcoin/issues/28636 new RPC: sendrawtransactiontopeer
---
Further extensions:
* Have the wallet do the private broadcast as well, https://github.com/bitcoin/bitcoin/issues/11887 would have to be resolved.
* Have the `submitpackage` RPC do the private broadcast as well, [draft diff in the comment below](https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2972293733), thanks ismaelsadeeq!
* Add some stats via RPC, so that the user can better monitor what is going on during and after the broadcast. Currently this can be done via the debug log, but that is not convenient.
* Make the private broadcast storage, currently in peerman, persistent over node restarts.
* Add (optional) random delay before starting to broadcast the transaction in order to avoid correlating unrelated transactions based on the time when they were broadcast. Suggested independently of this PR [here](https://github.com/bitcoin/bitcoin/issues/30471).
* Consider periodically sending transactions that did not originate from the node as decoy, discussed [here](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2035414972).
* Consider waiting for peer's FEEFILTER message and if the transaction that was sent to the peer is below that threshold, then assume the peer is going to drop it. Then use this knowledge to retry more aggressively with another peer, instead of the current 10 min. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648).
* It may make sense to be able to override the default policy -- eg so submitrawtransaction can go straight to the mempool and relay, even if txs are normally privately relayed. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3427086681).
* As a side effect we have a new metric available - the time it takes for a transaction to reach a random node in the network (from the point of view of the private broadcast recipient the tx originator is a random node somewhere in the network). This can be useful for monitoring, unrelated to privacy characteristics of this feature.
---
_A previous incarnation of this can be found at https://github.com/bitcoin/bitcoin/pull/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._
ACKs for top commit:
l0rinc:
code review diff ACK 8937221304
andrewtoth:
ACK 8937221304
pinheadmz:
ACK 8937221304
w0xlt:
ACK 8937221304 with nit https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875
mzumsande:
re-ACK 8937221304
Tree-SHA512: d51dadc865c2eb080c903cbe2f669e69a967e5f9fc64e9a20a68f39a67bf0db6ac2ad682af7fa24ef9f0942a41c89959341a16ba7b616475e1c5ab8e563b9b96
Extend `PeerManager` with a transaction storage and a new method
`InitiateTxBroadcastPrivate()` which:
* adds a transaction to that storage and
* calls `CConnman::PrivateBroadcast::NumToOpenAdd()` to open dedicated
privacy connections that will pick an entry from the transaction
storage and broadcast it.
Extend `node::TxBroadcast` with a 3rd option to not add the
transaction to the mempool and broadcast privately.
This is a non-functional change - `BroadcastTransaction()` will not
do anything if the 3rd options is passed and is not used by any of
its callers.
Rename `PeerManager::RelayTransaction()` to
`PeerManager::InitiateTxBroadcastToAll()`. The transaction is not
relayed when the method returns. It is only enqueued for a possible
broadcasting at a later time. Also, there will be another method which
only does so to Tor or I2P peers.
07a926474b node: change a tx-relay on/off flag to enum (Vasil Dimov)
Pull request description:
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
```
Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.
ACKs for top commit:
optout21:
ACK 07a926474b
kevkevinpal:
ACK [07a9264](07a926474b)
laanwj:
Concept and code review ACK 07a926474b. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.
glozow:
utACK 07a926474b
Tree-SHA512: ec8f6fa56a6d2422a0fbd5941ff2792685e8d8e7b9dd50bba9f3e21ed9b4a4a26c89b0d7e4895d48f30b7a635f2eddd894af26b5266410952cbdaf5c40b42966
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
```
Change this to an `enum`, so it is more readable and easier to extend
with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Now that we are storing `CTxMemPool::CompareDepthAndScore` parameters using
`std::variant` we have no portable zero-overhead way of accessing them,
so use `std::visit` and drop `bool wtxid` in-parameter.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
When using `sendrawtransaction` the ALREADY_IN_CHAIN error help string
may be confusing.
Rename TransactionError::ALREADY_IN_CHAIN to
TransactionError::ALREADY_IN_UTXO_SET and update the rpc help string.
Remove backwards compatibility alias as no longer required.
Add TransactionError to node namespace and include it directly instead of
relying on indirect include through common/messages.h
This is a followup to a previous commit which moved the TransactionError enum.
These changes were done in a separate followup just to keep the previous commit
more minimal and easy to review.
This is a commit in preparation for the next few commits. The functions
are moved to methods to avoid their re-declaration for the purpose of
passing in BlockManager options.
The functions that were now moved into the BlockManager should no longer
use the params as an argument, but instead use the member variable.
In the moved ReadBlockFromDisk and UndoReadFromDisk, change
the function signature to accept a reference to a CBlockIndex instead of
a raw pointer. The pointer is expected to be non-null, so reflect that
in the type.
To allow for the move of functions to BlockManager methods all call
sites require an instantiated BlockManager, or a callback to one.
f685a13bef doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f08 refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef
rajarshimaitra:
tACK f685a13bef
mjdietzx:
crACK f685a13bef
LarryRuane:
Code review, test ACK f685a13bef
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
This commit fixes some slightly unexpected behaviour when:
- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
as the mempool transaction but a different witness (the "new tx")
Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).
Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.
Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.
Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).
Fix by only adding the transaction to the broadcast set when it's added
to the mempool.
680eb56d82 [net processing] Don't pass CConnman to RelayTransactions (John Newbery)
a38a4e8f03 [net processing] Move RelayTransaction into PeerManager (John Newbery)
Pull request description:
This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.
ACKs for top commit:
ajtowns:
ACK 680eb56d82
Tree-SHA512: 8c93491a4392b6369bb7f090de326a63cd62a088de59026e202f226f64ded50a0cf1a95ed703328860f02a9d2f64d3a87ca1bca9a6075b978bd111d384766235
We don't mark RelayTransaction as const. Even though it doesn't mutate
PeerManagerImpl state, it _is_ mutating the internal state of a CNode
object, by updating setInventoryTxToSend. In a subsequent commit, that
field will be moved to the Peer object, which is owned by
PeerMangerImpl.
This requires PeerManagerImpl::ReattemptInitialBroadcast() to no longer
be const.
This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.
Check absurd fee in BroadcastTransaction and RPC,
return TransactionError::MAX_FEE_EXCEEDED instead
of TxValidationResult::TX_NOT_STANDARD because this
is client preference, not a node-wide policy.
Previously, `tx` was being read after having `std::move` called on it. The
std::move operator indicates to the compiler that this object may be "moved
from", so we shouldn't subsequently read from it. The current code is not
problematic since tx is passed in as a const ref. But this `std::move` is at
best misleading & at worst problematic, so remove it.
When I originally implemented the unbroadcast set in 18038, it just tracked
txids. After 18038 was merged, I offered a patch to 18044 to make the
unbroadcast changes compatible with wtxid relay. In this patch, I updated
`unbroadcast_txids` to a map of txid -> wtxid. Post merge review comments shed
light on the fact that this update was unnecessary, and distracting. So, this
commit updates the unbroadcast ids back to a set.
This adds a field to CNodeState that tracks whether to relay transactions with
that peer via wtxid, instead of txid. As of this commit the field will always
be false, but in a later commit we will add a way to negotiate turning this on
via p2p messages exchanged with the peer.
- Mempool tracks locally submitted transactions (wallet or rpc)
- Transactions are removed from set when the node receives a GETDATA request
from a peer, or if the transaction is removed from the mempool.
