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

While touching this, maybe BCLog::UTIL which is not used since #27896 should be dropped?

Done in #29419, thanks for the suggestion!

e672e55ddb49378bd5c8e4f03fadae5774310c70 does this mean there is only one bit left in LogFlags : uint32_t ?!

Yes. LogFlags is not stored on disk (I hope!) or participate in some permanent on-disk format. It is memory only and can be flipped to uint64_t when needed.

72f0dbc99e6eed26bef21965dabc5f0bebf01b33 if user sets -onlynet=onion do we require (by the time we get to this line) that NET_ONION is reachable? Or couldn't it come up later, similar to the -listenonion configuration?

if user sets -onlynet=onion do we require (by the time we get to this line) that NET_ONION is reachable?

No. -listen=1 -listenonion=1 -torcontrol=... -torpassword=... -onlynet=onion would get us here with NET_ONION being unreachable.

Or couldn't it come up later, similar to the -listenonion configuration?

Yes, if -listenonion=1 -torcontrol=... -torpassword=... is given then we will later, after startup, connect to the Tor daemon and possibly make NET_ONION reachable.

ok so then isn't it possible for a user to start with -privatebroadcast but i2p and onion aren't reachable yet? They would get an init error but the Tor daemon could become reachable later, by the time we need it.

It would be possible. -privatebroadcast=1 -listenonion=1 -torcontrol=... -torpassword=... would do exactly that and allow the startup to proceed. However, args.IsArgSet("-onlynet") was misplaced. It was intended to give error on things like -privatebroadcast=1 -listenonion=1 -torcontrol=... -torpassword=... -onlynet=ipv4 but it was too strong and would mistakenly forbid -privatebroadcast=1 -listenonion=1 -torcontrol=... -torpassword=... -onlynet=onion as well.

Changed.

7a0f492104c1621b7089a64fa330c72b06d30574 "privbcast" needs to be <= 6 characters to fit nicely in the table layout, or you could extend the column width:

https://github.com/bitcoin/bitcoin/blob/1cd2e29870e4ad3b7c57b1d567df0e6df56572b0/src/bitcoin-cli.cpp#L549-L552

Changed to privat. Feel free to suggest another name.

7a0f492104c1621b7089a64fa330c72b06d30574 I noticed this wasn't added to the fuzzer, dunno if it needs to be:

https://github.com/bitcoin/bitcoin/blob/1cd2e29870e4ad3b7c57b1d567df0e6df56572b0/src/test/util/net.h#L112-L119

Good catch, added!

4e19f9cc9834c0b76a2d03fea0921879de46fea9 Just to clarify: if a i2p sam session already exists, normally we would re-use it. But in the case of private broadcast we force a new transient session? Comment might be nice here.

Yes. Added a comment.

4e19f9cc9834c0b76a2d03fea0921879de46fea9 Feels like this log message is not in the right place since this function does not try to open a connection.

I think it is ok. This function is a helper of the caller which opens the connection. The doc of the function reads: Decide whether to open a private broadcast connection and if yes, to which network.

4e19f9cc9834c0b76a2d03fea0921879de46fea9 does this need underflow protection? Just like an Assume() even?

Added Assume()

532e63279a3c04fef8da2c1bb95e5280c94abc4e consider rephrasing the comment on the previous line since relay=true isn't a thing any more?

Right, changed.

eb9d7386ca82319ab682fc20ab35c13340d108a4 Hashing a hash? Does this just make the std::unordered_map lookups faster because the keys are smaller (looks like 64 bits?)

https://www.erisian.com.au/bitcoin-core-dev/log-2024-03-12.html#l-136 switched to SaltedTxidHasher.

07dee91112189be02c6694385de261570725b56b this would be unexpected if we had the txid in the priority list but not in the actual txid list, right? Maybe we should at least remove the txid from the priority list if this ever happens, or log something, or like, call the police or something?

Added Assume() that the entry is found.

07dee91112189be02c6694385de261570725b56b could this just be blank?

See this: #27509 (review)

I think /pynode:0.0.1/ is as good as any other of the options commented on the linked thread, but having a comment there specifying why this was picked is worth. Any future traveller will be so confuse to see that user agent without context

Agree with @sr-gi that having a comment explaining why this ua was chosen would be helpful.

Added a comment.

07dee91112189be02c6694385de261570725b56b do you need this here? I thought you had filtered out all the unnecessary messages already in CConnman::PushMessage()

Somehow I find this more clear. Some redundancy doesn't hurt.

07dee91112189be02c6694385de261570725b56b BroadcastEnd() returns a bool, seems like you could include that in the log message where you have the word "probably"

By "probably successful private broadcast" I meant that probably the transaction will successfully reach the network, not that we successfully gave it to one peer. Reworded.

~Also the return value of BroadcastEnd() was not used by the callers. Changed it to void.~ ignore this nonsense

From the PR main thread #29415 (comment):

The private broadcast succeeds frequently but not always

In the originator's log you can check to whom is the transaction being sent. Then in the recipient's logs you can check if it was accepted and further broadcast to everybody. Enable -debug=net -debug=privatebroadcast.

bc05ddc7b9369cf263bf88d0a773ca853d3c9af5 maybe I'm missing something, but should we not also adjust m_private_broadcast_connections_to_open when we get our privately broadcast tx back over regular relay? In my first local test, all 5 onion connections were completed before I got the tx back. But if the very first private connection relays the tx around and we get it back, do we still need to bother with the next 4?

Yes, good idea. Implemented.

4e19f9cc9834c0b76a2d03fea0921879de46fea9 what if MaybePickPrivateBroadcastNetwork() chooses i2p but we don't have any addrs for that network? I think the while loop below may try forever until addrman finally has something for i2p? But we could call MaybePickPrivateBroadcastNetwork() again or just switch to Tor if we know we have addrs there.

No, in that case Select() will return an invalid address and we will break; from the inside loop. Then the outside loop will call MaybePickPrivateBroadcastNetwork() again.

c09d6409c957af7c449a60778689d6715d9a6628 num_stale is a count of transactions but the other two numbers here count p2p connections, are we still going to use NUM_PRIVATE_BROADCAST_PER_TX here?

NUM_PRIVATE_BROADCAST_PER_TX is just for the initial broadcast. Then if a transaction gets stale, we retry one shot at a time. There is no special reason for this. Could be either way.

I also renamed the variables to make it clear that we are comparing number of transactions with number of connections.

e36a1b4848f8f762b550a9469993211d41bd6cb9 heh, makes the placement of this function in mempool.cpp a bit arbitrary now!

Yeah, it was before as well. Somehow the code and the comments around this say that "the transaction is sent to the mempool for broadcast" which is not quite accurate (in master). I don't think that the mempool is broadcasting the transaction. Will leave it as it is for now.

2b2e747455699236200fa3cddddfff7005f6a44a Could be overkill, but you could actually adjust the "dummy response" a few lines up to actually respond accurately, in particular the REPLY field could be used to report a connection failure back to bitcoind.

From https://www.rfc-editor.org/rfc/rfc1928

diff --git a/test/functional/test_framework/socks5.py b/test/functional/test_framework/socks5.py
index 48ad8bbeea..568e9fa64c 100644
--- a/test/functional/test_framework/socks5.py
+++ b/test/functional/test_framework/socks5.py
@@ -123,7 +123,14 @@ class Socks5Connection():
             port = (port_hi << 8) | port_lo
 
             # Send dummy response
-            self.conn.sendall(bytearray([0x05, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]))
+            self.conn.sendall(bytearray([
+                0x05, # Protocol version
+                0x00, # Reply (0x00 = success)
+                0x00, # Reserved (must be 0x00)
+                0x01, # Server bound address type (0x01 = ipv4)
+                0x00, 0x00, 0x00, 0x00, # Server bound ipv4 address
+                0x00, 0x00 # Server bound port
+            ]))
 
             cmdin = Socks5Command(cmd, atyp, addr, port, username, password)
             self.serv.queue.put(cmdin)

I will leave it as it is, looks too complicated after looking at doing this for some time.

d10a0649b018638574fa7dc316b3e783bf6cee69 just a guess, would this work?

1. start the node with -addnode=<ipv4> x10
2. wait for 10 outbounds to connect
3. addpeeraddress with all the onion and i2p addrs
4. send raw, etc...

No, because -addnode is counted separately from the 10 automatic outbound connections.

d10a0649b018638574fa7dc316b3e783bf6cee69 nit: slightly confusing to prefix these with listen_ I would expect, like: to_addr to_port requested_to_addr

Renamed.

d10a0649b018638574fa7dc316b3e783bf6cee69 sanity check: you can assert this because none of the private-broadcast peers are connected back to the tx_originator?

Yes, exactly.

typo: sensitive (this and next line)

Fixed, will be in next push.

It seems excessive that MAX_PRIVATE_BROADCAST_CONNECTIONS=64 has the power to reduce nMaxConnections to a lower value even in the case where private broadcast isn't enabled. Might be better to make that conditional on privatebroadcast, similar to how it's done with nBind.

Done, will be in next push. Good observation!

This raised the unrelated-to-this-PR question https://www.erisian.com.au/bitcoin-core-dev/log-2024-03-28.html#l-59

Wouldn't this fit better into node/transaction.h , together with BroadcastTransaction ?

I think so, but this drags some further changes, are you ok with those:

<details> <summary>[patch] move TxBroadcastMethod from primitives/transaction.h to node/transaction.h</summary>

diff --git i/src/interfaces/chain.h w/src/interfaces/chain.h
index 315211ae49..eeb5fef42b 100644
--- i/src/interfaces/chain.h
+++ w/src/interfaces/chain.h
@@ -4,12 +4,13 @@
 
 #ifndef BITCOIN_INTERFACES_CHAIN_H
 #define BITCOIN_INTERFACES_CHAIN_H
 
 #include <blockfilter.h>
 #include <common/settings.h>
+#include <node/transaction.h>
 #include <primitives/transaction.h> // For CTransactionRef
 #include <util/result.h>
 
 #include <functional>
 #include <memory>
 #include <optional>
@@ -215,13 +216,13 @@ public:
     //! [@param](/bitcoin-bitcoin/contributor/param/)[in] broadcast_method Whether to add the transaction to the
     //! mempool and how/whether to broadcast it.
     //! [@param](/bitcoin-bitcoin/contributor/param/)[out] err_string Set if an error occurs.
     //! [@return](/bitcoin-bitcoin/contributor/return/) False if the transaction could not be added due to the fee or for another reason.
     virtual bool broadcastTransaction(const CTransactionRef& tx,
                                       const CAmount& max_tx_fee,
-                                      TxBroadcastMethod broadcast_method,
+                                      node::TxBroadcastMethod broadcast_method,
                                       std::string& err_string) = 0;
 
     //! Calculate mempool ancestor and descendant counts for the given transaction.
     virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) = 0;
 
     //! For each outpoint, calculate the fee-bumping cost to spend this outpoint at the specified
diff --git i/src/node/transaction.h w/src/node/transaction.h
index 75c50bab28..01cdfaf1ff 100644
--- i/src/node/transaction.h
+++ w/src/node/transaction.h
@@ -29,12 +29,26 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
 /** Maximum burn value for sendrawtransaction, submitpackage, and testmempoolaccept RPC calls.
  * By default, a transaction with a burn value higher than this will be rejected
  * by these RPCs and the GUI. This can be overridden with the maxburnamount argument.
  */
 static const CAmount DEFAULT_MAX_BURN_AMOUNT{0};
 
+/**
+ * Methods to broadcast a local transaction.
+ * Used to influence BroadcastTransaction() and its callers.
+ */
+enum TxBroadcastMethod : uint8_t {
+    /// Add the transaction to the mempool and broadcast to all currently connected peers.
+    ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL,
+    /// Add the transaction to the mempool, but don't broadcast to anybody.
+    ADD_TO_MEMPOOL_NO_BROADCAST,
+    /// Omit the mempool and directly send the transaction via a few dedicated connections to
+    /// peers on privacy networks.
+    NO_MEMPOOL_PRIVATE_BROADCAST,
+};
+
 /**
  * Submit a transaction to the mempool and (optionally) relay it to all P2P peers.
  *
  * Mempool submission can be synchronous (will await mempool entry notification
  * over the CValidationInterface) or asynchronous (will submit and not wait for
  * notification), depending on the value of wait_callback. wait_callback MUST
diff --git i/src/primitives/transaction.h w/src/primitives/transaction.h
index 2051ebae5a..ccbeb3ec49 100644
--- i/src/primitives/transaction.h
+++ w/src/primitives/transaction.h
@@ -436,21 +436,7 @@ public:
     bool IsWtxid() const { return m_is_wtxid; }
     const uint256& GetHash() const LIFETIMEBOUND { return m_hash; }
     friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; }
     friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); }
 };
 
-/**
- * Methods to broadcast a local transaction.
- * Used to influence BroadcastTransaction() and its callers.
- */
-enum TxBroadcastMethod : uint8_t {
-    /// Add the transaction to the mempool and broadcast to all currently connected peers.
-    ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL,
-    /// Add the transaction to the mempool, but don't broadcast to anybody.
-    ADD_TO_MEMPOOL_NO_BROADCAST,
-    /// Omit the mempool and directly send the transaction via a few dedicated connections to
-    /// peers on privacy networks.
-    NO_MEMPOOL_PRIVATE_BROADCAST,
-};
-
 #endif // BITCOIN_PRIMITIVES_TRANSACTION_H
diff --git i/src/rpc/mempool.cpp w/src/rpc/mempool.cpp
index 14d303a0b1..ba69446f36 100644
--- i/src/rpc/mempool.cpp
+++ w/src/rpc/mempool.cpp
@@ -8,12 +8,13 @@
 #include <kernel/mempool_persist.h>
 
 #include <chainparams.h>
 #include <core_io.h>
 #include <kernel/mempool_entry.h>
 #include <node/mempool_persist_args.h>
+#include <node/transaction.h>
 #include <policy/rbf.h>
 #include <policy/settings.h>
 #include <primitives/transaction.h>
 #include <rpc/server.h>
 #include <rpc/server_util.h>
 #include <rpc/util.h>
@@ -91,13 +92,13 @@ static RPCHelpMan sendrawtransaction()
             AssertLockNotHeld(cs_main);
             NodeContext& node = EnsureAnyNodeContext(request.context);
             const TransactionError err = BroadcastTransaction(node,
                                                               tx,
                                                               err_string,
                                                               max_raw_tx_fee,
-                                                              ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL,
+                                                              node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL,
                                                               /*wait_callback=*/true);
             if (TransactionError::OK != err) {
                 throw JSONRPCTransactionError(err, err_string);
             }
 
             return tx->GetHash().GetHex();
@@ -953,13 +954,13 @@ static RPCHelpMan submitpackage()
                 // We do not expect an error here; we are only broadcasting things already/still in mempool
                 std::string err_string;
                 const auto err = BroadcastTransaction(node,
                                                       tx,
                                                       err_string,
                                                       /*max_tx_fee=*/0,
-                                                      ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL,
+                                                      node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL,
                                                       /*wait_callback=*/true);
                 if (err != TransactionError::OK) {
                     throw JSONRPCTransactionError(err,
                         strprintf("transaction broadcast failed: %s (%d transactions were broadcast successfully)",
                             err_string, num_broadcast));
                 }
diff --git i/src/wallet/rpc/backup.cpp w/src/wallet/rpc/backup.cpp
index fdbd44ffe5..2e61a0e945 100644
--- i/src/wallet/rpc/backup.cpp
+++ w/src/wallet/rpc/backup.cpp
@@ -10,12 +10,13 @@
 #include <clientversion.h>
 #include <core_io.h>
 #include <hash.h>
 #include <interfaces/chain.h>
 #include <key_io.h>
 #include <merkleblock.h>
+#include <node/transaction.h>
 #include <rpc/util.h>
 #include <script/descriptor.h>
 #include <script/script.h>
 #include <script/solver.h>
 #include <sync.h>
 #include <uint256.h>
@@ -308,13 +309,13 @@ RPCHelpMan importaddress()
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address or script");
         }
     }
     if (fRescan)
     {
         RescanWallet(*pwallet, reserver);
-        pwallet->ResubmitWalletTransactions(ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
+        pwallet->ResubmitWalletTransactions(node::ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
     }
 
     return UniValue::VNULL;
 },
     };
 }
@@ -479,13 +480,13 @@ RPCHelpMan importpubkey()
 
         pwallet->ImportPubKeys({pubKey.GetID()}, {{pubKey.GetID(), pubKey}} , /*key_origins=*/{}, /*add_keypool=*/false, /*internal=*/false, /*timestamp=*/1);
     }
     if (fRescan)
     {
         RescanWallet(*pwallet, reserver);
-        pwallet->ResubmitWalletTransactions(ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
+        pwallet->ResubmitWalletTransactions(node::ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
     }
 
     return UniValue::VNULL;
 },
     };
 }
@@ -1400,13 +1401,13 @@ RPCHelpMan importmulti()
                 nLowestTimestamp = timestamp;
             }
         }
     }
     if (fRescan && fRunScan && requests.size()) {
         int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, /*update=*/true);
-        pwallet->ResubmitWalletTransactions(ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
+        pwallet->ResubmitWalletTransactions(node::ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
 
         if (pwallet->IsAbortingRescan()) {
             throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");
         }
         if (scannedTime > nLowestTimestamp) {
             std::vector<UniValue> results = response.getValues();
@@ -1694,13 +1695,13 @@ RPCHelpMan importdescriptors()
         pwallet->ConnectScriptPubKeyManNotifiers();
     }
 
     // Rescan the blockchain using the lowest timestamp
     if (rescan) {
         int64_t scanned_time = pwallet->RescanFromTime(lowest_timestamp, reserver, /*update=*/true);
-        pwallet->ResubmitWalletTransactions(ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
+        pwallet->ResubmitWalletTransactions(node::ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
 
         if (pwallet->IsAbortingRescan()) {
             throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user.");
         }
 
         if (scanned_time > lowest_timestamp) {
diff --git i/src/wallet/test/wallet_tests.cpp w/src/wallet/test/wallet_tests.cpp
index 003d1aab29..2377c71e21 100644
--- i/src/wallet/test/wallet_tests.cpp
+++ w/src/wallet/test/wallet_tests.cpp
@@ -10,12 +10,13 @@
 #include <vector>
 
 #include <addresstype.h>
 #include <interfaces/chain.h>
 #include <key_io.h>
 #include <node/blockstorage.h>
+#include <node/transaction.h>
 #include <policy/policy.h>
 #include <rpc/server.h>
 #include <script/solver.h>
 #include <test/util/logging.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
@@ -819,13 +820,13 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
     });
     std::string error;
     m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
     auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
     m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
     auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
-    BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, ADD_TO_MEMPOOL_NO_BROADCAST, error));
+    BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, node::ADD_TO_MEMPOOL_NO_BROADCAST, error));
 
 
     // Reload wallet and make sure new transactions are detected despite events
     // being blocked
     // Loading will also ask for current mempool transactions
     wallet = TestLoadWallet(context);
@@ -861,13 +862,13 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
     auto handler = HandleLoadWallet(context, [&](std::unique_ptr<interfaces::Wallet> wallet) {
             BOOST_CHECK(rescan_completed);
             m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
             block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
             m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
             mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
-            BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, ADD_TO_MEMPOOL_NO_BROADCAST, error));
+            BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, node::ADD_TO_MEMPOOL_NO_BROADCAST, error));
             m_node.validation_signals->SyncWithValidationInterfaceQueue();
         });
     wallet = TestLoadWallet(context);
     // Since mempool transactions are requested at the end of loading, there will
     // be 2 additional AddToWallet calls, one from the previous test, and a duplicate for mempool_tx
     BOOST_CHECK_EQUAL(addtx_count, 2 + 2);
diff --git i/src/wallet/wallet.cpp w/src/wallet/wallet.cpp
index 0904399926..2566b2d543 100644
--- i/src/wallet/wallet.cpp
+++ w/src/wallet/wallet.cpp
@@ -24,12 +24,13 @@
 #include <interfaces/wallet.h>
 #include <kernel/chain.h>
 #include <kernel/mempool_removal_reason.h>
 #include <key.h>
 #include <key_io.h>
 #include <logging.h>
+#include <node/transaction.h>
 #include <outputtype.h>
 #include <policy/feerate.h>
 #include <primitives/block.h>
 #include <primitives/transaction.h>
 #include <psbt.h>
 #include <pubkey.h>
@@ -1999,13 +2000,13 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
     }
     return result;
 }
 
 bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx,
                                          std::string& err_string,
-                                         TxBroadcastMethod broadcast_method) const
+                                         node::TxBroadcastMethod broadcast_method) const
 {
     AssertLockHeld(cs_wallet);
 
     // Can't relay if wallet is not broadcasting
     if (!GetBroadcastTransactions()) return false;
     // Don't relay abandoned transactions
@@ -2016,19 +2017,19 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx,
     // Don't try to submit conflicted or confirmed transactions.
     if (GetTxDepthInMainChain(wtx) != 0) return false;
 
     // Submit transaction to mempool for relay
     const char* what{""};
     switch (broadcast_method) {
-    case ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL:
+    case node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL:
         what = "to mempool and for broadcast to all peers";
         break;
-    case ADD_TO_MEMPOOL_NO_BROADCAST:
+    case node::ADD_TO_MEMPOOL_NO_BROADCAST:
         what = "to mempool without broadcast";
         break;
-    case NO_MEMPOOL_PRIVATE_BROADCAST:
+    case node::NO_MEMPOOL_PRIVATE_BROADCAST:
         what = "for private broadcast without adding to the mempool";
         break;
     }
     WalletLogPrintf("Submitting wtx %s %s\n", wtx.GetHash().ToString(), what);
     // We must set TxStateInMempool here. Even though it will also be set later by the
     // entered-mempool callback, if we did not there would be a race where a
@@ -2095,13 +2096,13 @@ NodeClock::time_point CWallet::GetDefaultNextResend() { return FastRandomContext
 // The `force` option results in all unconfirmed transactions being submitted to
 // the mempool. This does not necessarily result in those transactions being relayed,
 // that depends on the `broadcast_method` option. Periodic rebroadcast uses the pattern
 // broadcast_method=ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL force=false, while loading into
 // the mempool (on start, or after import) uses
 // broadcast_method=ADD_TO_MEMPOOL_NO_BROADCAST force=true.
-void CWallet::ResubmitWalletTransactions(TxBroadcastMethod broadcast_method, bool force)
+void CWallet::ResubmitWalletTransactions(node::TxBroadcastMethod broadcast_method, bool force)
 {
     // Don't attempt to resubmit if the wallet is configured to not broadcast,
     // even if forcing.
     if (!fBroadcastTransactions) return;
 
     int submitted_tx_count = 0;
@@ -2136,13 +2137,13 @@ void CWallet::ResubmitWalletTransactions(TxBroadcastMethod broadcast_method, boo
 /** @} */ // end of mapWallet
 
 void MaybeResendWalletTxs(WalletContext& context)
 {
     for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
         if (!pwallet->ShouldResend()) continue;
-        pwallet->ResubmitWalletTransactions(ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL, /*force=*/false);
+        pwallet->ResubmitWalletTransactions(node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL, /*force=*/false);
         pwallet->SetNextResend();
     }
 }
 
 
 /** [@defgroup](/bitcoin-bitcoin/contributor/defgroup/) Actions
@@ -2344,13 +2345,13 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
     if (!fBroadcastTransactions) {
         // Don't submit tx to the mempool
         return;
     }
 
     std::string err_string;
-    if (!SubmitTxMemoryPoolAndRelay(*wtx, err_string, ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL)) {
+    if (!SubmitTxMemoryPoolAndRelay(*wtx, err_string, node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL)) {
         WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string);
         // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure.
     }
 }
 
 DBErrors CWallet::LoadWallet()
@@ -3389,13 +3390,13 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
 }
 
 void CWallet::postInitProcess()
 {
     // Add wallet transactions that aren't already in a block to mempool
     // Do this here as mempool requires genesis block to be loaded
-    ResubmitWalletTransactions(ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
+    ResubmitWalletTransactions(node::ADD_TO_MEMPOOL_NO_BROADCAST, /*force=*/true);
 
     // Update wallet transactions with current mempool transactions.
     WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
 }
 
 bool CWallet::BackupWallet(const std::string& strDest) const
diff --git i/src/wallet/wallet.h w/src/wallet/wallet.h
index 8f9881c304..ceadabf165 100644
--- i/src/wallet/wallet.h
+++ w/src/wallet/wallet.h
@@ -9,12 +9,13 @@
 #include <addresstype.h>
 #include <consensus/amount.h>
 #include <interfaces/chain.h>
 #include <interfaces/handler.h>
 #include <kernel/cs_main.h>
 #include <logging.h>
+#include <node/transaction.h>
 #include <outputtype.h>
 #include <policy/feerate.h>
 #include <primitives/transaction.h>
 #include <script/interpreter.h>
 #include <script/script.h>
 #include <support/allocators/secure.h>
@@ -631,13 +632,13 @@ public:
     ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress);
     void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override;
     /** Set the next time this wallet should resend transactions to 12-36 hours from now, ~1 day on average. */
     void SetNextResend() { m_next_resend = GetDefaultNextResend(); }
     /** Return true if all conditions for periodically resending transactions are met. */
     bool ShouldResend() const;
-    void ResubmitWalletTransactions(TxBroadcastMethod broadcast_method, bool force);
+    void ResubmitWalletTransactions(node::TxBroadcastMethod broadcast_method, bool force);
 
     OutputType TransactionChangeType(const std::optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend) const;
 
     /** Fetch the inputs and sign with SIGHASH_ALL. */
     bool SignTransaction(CMutableTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     /** Sign the tx given the input coins and sighash. */
@@ -676,13 +677,13 @@ public:
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] mapValue key-values to be set on the transaction.
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] orderForm BIP 70 / BIP 21 order form details to be set on the transaction.
      */
     void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm);
 
     /** Pass this transaction to node for optional mempool insertion and relay to peers. */
-    bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, TxBroadcastMethod broadcast_method) const
+    bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, node::TxBroadcastMethod broadcast_method) const
         EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     bool ImportScripts(const std::set<CScript> scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool ImportPrivKeys(const std::map<CKeyID, CKey>& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const std::map<CKeyID, CPubKey>& pubkey_map, const std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool ImportScriptPubKeys(const std::string& label, const std::set<CScript>& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

</details>

Hmm, after looking at that diff I'm not so sure . It's probably a layer violation include node/ files from the wallet. On the other hand, having this enum in primitive/transaction.h still doesn't feel right, given that this isn't really a primitive property of a transaction like COutPoint. Maybe put it into another (new) file in kernel/, and handle it similar to enum class ChainstateRole and enum class MemPoolRemovalReason could be the best way?

Maybe it is ok to include node/ from the wallet:

$ git grep node/ origin/master -- "*wallet*"
origin/master:src/bench/wallet_balance.cpp:7:#include <node/chainstate.h>
origin/master:src/bench/wallet_balance.cpp:8:#include <node/context.h>
origin/master:src/bench/wallet_create.cpp:10:#include <node/context.h>
origin/master:src/bench/wallet_create_tx.cpp:10:#include <node/context.h>
origin/master:src/bench/wallet_ismine.cpp:12:#include <node/context.h>
origin/master:src/bench/wallet_loading.cpp:11:#include <node/context.h>
origin/master:src/qt/walletframe.cpp:7:#include <node/interface_ui.h>
origin/master:src/qt/walletmodel.cpp:21:#include <node/interface_ui.h>
origin/master:src/qt/walletview.cpp:22:#include <node/interface_ui.h>
origin/master:src/wallet/init.cpp:16:#include <node/context.h>
origin/master:src/wallet/init.cpp:17:#include <node/interface_ui.h>
origin/master:src/wallet/test/coinselector_tests.cpp:6:#include <node/context.h>
origin/master:src/wallet/test/init_test_fixture.h:10:#include <node/context.h>
origin/master:src/wallet/test/ismine_tests.cpp:7:#include <node/context.h>
origin/master:src/wallet/test/wallet_test_fixture.h:12:#include <node/context.h>
origin/master:src/wallet/test/wallet_tests.cpp:15:#include <node/blockstorage.h>
origin/master:test/functional/test_framework/wallet.py:141:            # Sort tx by ancestor count. See BlockAssembler::SortForBlock in src/node/miner.cpp

@TheCharlatan you probably dealt a lot with these kind of questions for kernel, do you have an opinion where this enum should be?

re: #29415 (review)

I'd suggest moving TxBroadcastMethod enum to node/types.h, which doesn't exist yet but is introduced in #29015, and could be added here as well.

It's probably a layer violation include node/ files from the wallet.

We don't want wallet code to call node functions or use internal node types, but node and wallet code do define a handful of "public" types (CNodeStats, CNodeStateStats, and SynchronizationState, AddressPurpose, isminetype, CRecipient, and CCoinControl) I mentioned recently in #29015 (review) that are useful even without depending on node or wallet libraries, and I think it makes sense to put the lightweight types in node/types.h, wallet/types.h, common/types.h files so they can be explained and used without dragging in other things. Other options I mentioned in that comment:

We do have a choice where about where to put public types which are not really part of any library. We can put them in the interfaces directory, or in common, or in the node and wallet directories. Personally, I think it makes sense to put them in the wallet directory when they are related to wallet functionality and used by wallet code, put them in the node directory when they are related to node functionality and used by node code, put them in the common directory when they are related to common functionality (like PSBT) and used by common code. The comments at the top of {node,wallet,common}/types.h files are meant to explain the idea that these are public types.

Moved to node/types.h (new file). Could put it elsewhere if there are better suggestions. Thanks for the suggestion!

Edit: btw, the doxygen comment should have \file on its own line. See https://www.doxygen.nl/manual/commands.html#cmdfile and run doxygen doc/Doxyfile and then inspect doc/doxygen/html/node_2types_8h.html.

The peer starts connecting to (clearnet) nodes and sends them a VERSION message which contains our nonce. If the peer manages to connect to us we would disconnect.

I'm confused by this attack. First, if the peer connects to us over clearnet it would be an inbound connection for us - we don't disconnect inbounds, see !pnode->IsInboundConn() a few lines below. Second, even if we did disconnect, I still don't see the how the attack could work - it would be a new connection with a new ID and a new nonce (the nonce is created using the peer id here, so it should be different for each peer).

I reread your statement and the code a few times and I think that the code is legit (or the attack the code is guarding against could happen). Let me elaborate:

1. We connect for a private broadcast to boopeer.onion and send them nonce=1234 in our VERSION message. For this an entry is added to CConnman::m_nodes that has IsInboundConn() == false.
2. The boo guy delays their VERACK on the private connection and connects to random nodes on clearnet. Assuming we also listen on clearnet address, they are lucky enough and manage to connect to us. They send us VERSION with nonce=1234. For this connection IsInboundConn() == true on our side and this code is executed:

https://github.com/bitcoin/bitcoin/blob/c8e3978114716bb8fb10695b9d187652f3ab4926/src/net_processing.cpp#L3532-L3537

passing nonce=1234 to CheckIncomingNonce() which finds the entry in m_nodes from 1. and returns false. Then the clearnet connection from 2. is closed by us.

Makes sense?

Yes, now it makes sense, thanks! I'm not sure where I got confused, I probably misread the existing code.

nit:

        assert_equal(len(tx_originator.getrawmempool()), 0)

Done.

In cf76f4f0fe1a3ef0730f70a035c10e01712b6d6b: Shouldn't we check here whether any private network is reachable? Because we can start a node with -privatebroadcast with NET_ONION becoming reachable later, no?

Done.

        "           \"private\"    - private broadcast; short-lived connection for broadcasting our transactions\n"

It was suggested that this be 6 chars or less: #29415 (review). Feel free to suggest another name.

I'd go with priv, but just my opinion

Changed to "priv".

Still documented as "privat"

Fixed, thanks!

Private connections are opened regardless of max connections limits, I think we could cover it in the functional test.

The current functional test already opens the usual 10 connections and then fires a private broadcast one. Is this what you mean?

Yes, I missed that. Nevermind.

In b1f65ba837a805dc8f855c02337c1593c77add6d: non-blocker: I think it would be more elegant if MaybePickPrivateBroadcastNetwork returns std::optional<Network> net.

Done.

In 413574e81ef4d608b9b0a2079f81a4e90ee2a341: You can use -test=addrman to make addrman deterministic. Running the test several times I realized that sometimes a collision happens, sometimes not.

Done.

     * - Get a transaction for broadcast, the one that has been broadcast fewer times and least recently

Done

         * Get the transaction that has been broadcast fewest times and least recently.

Done

nit: This could be const method?

Indeed the Find() method does not modify any of the members. I tried to make it const, but it has to return non-const iterators to the members... :(

Since we're touching these lines, maybe update to LogPrintLevel?

Done

With regards to this comment and:

We pick a random Tor (or I2P) peer.

To increase success rate and reduce latency, clearnet peers could be also be selected when using Tor, without harming privacy of broadcast...

Clearnet connections only need one Tor circuit, whereas hidden service connections which require two circuits for the connecting peer and one from the accepting peer.

Furthermore, if SOCKS proxy credentials are reused for the broadcast of a single transaction (but not all transactions in the private broadcast queue), multiple connections could share the same circuit/exit node. This specific optimization does introduce a censorship risk (even if BIP 324 is used [edit: due] to the MITM consideration) and there precedent for such malicious exit nodes.

From a privacy standpoint both optimizations would be more or less equivalent to broadcasting through isolated connections to peers with hidden service addresses, since the origin is still protected and being able to link these connections does not aid in deanonymization since these connections all pertain to the same data.

The main rationale for these optimizations is that Tor daemon builds circuits sequentially, and does not keep many built circuits in reserve (IIRC 2 unless their construction is explicitly requested through the control socket), so when SOCKS connections are opened with distinct credentials the connection establishment and time to first byte will typically scale linearly in the order that the SOCKS connections are accepted by tor daemon itself, and since building a new circuit can take on the order of seconds.

I added broadcast to IPv4 and IPv6 peers through the Tor proxy. Thanks!

In 9297437af2c2f45a7b9a12be78fa0e597c54ac79: Since the addrman is deterministic now, you can remove addresses that are conflicting.

--- a/test/functional/p2p_private_broadcast.py
+++ b/test/functional/p2p_private_broadcast.py
@@ -118,7 +118,6 @@ addresses = [
     "bsqbtctulf2g4jtjsdfgl2ed7qs6zz5wqx27qnyiik7laockryvszqqd.onion",
     "cwi3ekrwhig47dhhzfenr5hbvckj7fzaojygvazi2lucsenwbzwoyiqd.onion",
     "devinbtcmwkuitvxl3tfi5of4zau46ymeannkjv6fpnylkgf3q5fa3id.onion",
-    "devinbtctu7uctl7hly2juu3thbgeivfnvw3ckj3phy6nyvpnx66yeyd.onion",
     "devinbtcyk643iruzfpaxw3on2jket7rbjmwygm42dmdyub3ietrbmid.onion",
     "dtql5vci4iaml4anmueftqr7bfgzqlauzfy4rc2tfgulldd3ekyijjyd.onion",
     "emzybtc25oddoa2prol2znpz2axnrg6k77xwgirmhv7igoiucddsxiad.onion",

Also, maybe we do not need this verification anymore?

if res["success"]:
    self.log.info(f"Added {addr} to tx_originator's addrman")
else:
    self.log.info(f"Could not add {addr} to tx_originator's addrman (collision?)")

Right, removed the conflicting address and reduced the logging to log only if adding to the addrman fails.

have you considered using SoftSetBoolArg to set -walletbroadcast to 0 if -privatebroadcast was chosen? It's not the most user-friendly thing to introduce a new command arg that would only work if another arg was also changed from its default.

Yes, but I did not like the behavior of the wallet "unexpectedly" stopping to broadcast its own transactions because -privatebroadcast is enabled.

It's not the most user-friendly thing to introduce a new command arg that would only work if another arg was also changed from its default.

Yes, I agree. I do not like that either but somewhat prefer it over automatically switching off wallet broadcast. I can imagine GUI users who enable -privatebroadcast and still try to send transactions from their wallets and have no clue why they are not being mined.

Leaving this open to discussion and proposals.

I was pretty confused about this, maybe because I misunderstood the proposal. By just reading what the help for -privatebroadcast I wouldn't assume that transactions made using the wallet won't be broadcast. My understanding was that anything done via the wallet would work as usual, but that transactions sent via sendrawtransaction would use the private broadcast mechanism if set.

I think this should be mentioned in the help of -privatebroadcast.

I wonder if the option should be renamed -privaterpcbroadcast for now, and we can add -privatewalletbroadcast later.

We also won't be able to prevent users from using their wallet with sendrawtransaction anyway. So if the goal here is to prevent accidental privacy leaks from confused users, we might want to consider "hiding" the option or restricting it to test networks until wallet integration is complete.

I thought about this further and removed the requirement to set -walletbroadcast=0 when -privatebroadcast=1. The description of -privatebroadcast begins with

Broadcast transactions submitted via sendrawtransaction RPC using short lived connections...

so it should be clear that this applies to the sendrawtransaction RPC. In addition I extended the text with

Transactions submitted through the wallet are not affected by this option

Marking this as resolved because I removed the requirement to set -walletbroadcast=0 and extended the -privatebroadcast doc.

My understanding was that anything done via the wallet would work as usual, but that transactions sent via sendrawtransaction would use the private broadcast mechanism if set

Yes, this is exactly how it works now.

I think this requires an update to tor.md, which currently says: -onion=ip:port Set the proxy server to use for Tor onion services. You do not need to set this if it's the same as -proxy. After this PR, specifiying it explicitly is necessary if you want to do private broadcast to clearnet peers.

Updated.

Is there no better way of checking whether -proxy is an actual Tor proxy or just a regular SOCKS5 proxy? It feels like the amount of custom settings that this will require to run will make it hard to use

Hmm, maybe we can keep track of whether we have successfully managed to connect to at least one .onion address via the -proxy, then we can assume that is a Tor proxy? How does that sound?

Is there no better way of checking whether -proxy is an actual Tor proxy or just a regular SOCKS5 proxy?

maybe we can keep track of whether we have successfully managed to connect to at least one .onion address

Done in e85cc59bad...057c79365c, see #29415 (comment), also dropped the change in tor.md because that is not needed anymore.

Looks better now, thank you!

You probably meant onion_arg != "0"

Yes, fixed!

It looks like we make sure in net_processing to not send any of theses messages, and here we check again. I haven't thought about this long, but can this log be triggered, or could this be an Assume(false) instead?

Added Assume(false) and a comment.

This was triggered in the fuzz tests. I could complicate the fuzz tests to avoid sending unwanted messages if the random node happens to be a private broadcast, but CConnman::PushMessage() is called from a lot of places including from the RPC and that may change in the future as well. It is somewhat hard to judge that nobody will call PushMessage() with an unwanted message. Thus I think it is better to assume that this is the real check here in CConnman::PushMessage() and that the one in PeerManagerImpl::SendMessages() is an optimization. Removed the Assume(false).

This class is getting pretty large in the end, maybe put it into its own module (similar to TxReconciliationTracker or TxRequestTracker)

Yes, I was thinking the same. Moved away.

I don't think is needed, MEMPOOL is never tested.

Removed.

sendrawtransaction doc should be updated (it already mentions privacy issues).

Updated.

In 6b4693ee859d6bddf559a0b32749b1dafe015ef4 "net: implement opening PRIVATE_BROADCAST connections": Just a question: why using compare_exchange_weak instead of fetch_sub?

To avoid negative values. I added a comment in the source code to explain.

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

What is the rationale for 64? That seems like a lot of connections.

It could be lower. I picked 64 because I imagine somebody submitting 100s or even 1000s of transactions at the same time. Processing those serially would take a lot of time. Would you suggest another number?

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd:

nit: if yes -> if so

Done

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd:

nit: sorry for bikeshedding clearnet doesn't seem the best name for this. Maybe net or network?

Also, this is already referenced as net in the docs

I removed the argument to this function and renamed it to ProxyForIPv4or6PrivateBroadcast(). It is a bit more simpler now.

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

The way you are defining use_proxy could lead to proxy_opt having no value, yet use_proxy being true

            bool use_proxy;
            if (conn_type == ConnectionType::PRIVATE_BROADCAST) {
                const auto proxy_opt{ProxyForClearnetPrivateBroadcast(addrConnect.GetNetwork())};
                if (proxy_opt.has_value()) {
                    use_proxy = true;
                    proxy = proxy_opt.value();
                }
            } else {
                use_proxy = GetProxy(addrConnect.GetNetwork(), proxy);
            }

use_proxy can even be defined right after proxy (a few lined above) for simplicity

proxy_opt exists only inside the if. What matters after this if are the values of use_proxy and proxy. I think your suggestion is equivalent to the current code. I.e.

set use_proxy and proxy (normal)
if (special case)
    replace use_proxy and proxy with special values

vs

if (special case)
    set use_proxy and proxy to special values
else
    set use_proxy and proxy (normal)

Right?

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

I'm confused here. We choose not to use our permanent m_i2p_sam_session to avoid linking this to our permanent I2P ID, however, we use a pool of transient I2P sessions (m_unused_i2p_sessions) instead. I understand these transient sessions can be reused (or at least that's what I get from the docs of m_unused_i2p_sessions). So if the same (peer, session_id) pair is picked, the node will know that we created both transactions, wouldn't it?

The transient sessions are not reused. m_unused_i2p_sessions description reads:

If connecting to a host fails, then the created session is put to this pool for reuse.

The code is this:

https://github.com/bitcoin/bitcoin/blob/a786fd2041913d82ca90b561de309421bd24e41b/src/net.cpp#L472-L480

I.e. pop/extract a session from m_unused_i2p_sessions and if we cannot connect, then we did not use the session and thus put it back to m_unused_i2p_sessions.

Do you think that the description of m_unused_i2p_sessions can be improved? Would you suggest an exact wording?

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

I don't think we should be giving PRIVATE_BROADCAST priority over every other type of connection. At the very least, they should go after BLOCK_RELAY.

Having connections that keep us up to date should be more important than sending out our own stuff

A few considerations:

• Private broadcast connections are not blocked on the semOutbound grant. They come to this code right away.

• Even if there is a high demand for private broadcast connections, we would try to open such ones only half of the time in CConnman::ThreadOpenConnections() to avoid such starvation of only opening private broadcast and not others for a long time (see how MaybePickPrivateBroadcastNetwork() yields to other connection types if we previously opened a private broadcast one).

• Non-BLOCK_RELAY connections keep us up to date as well (with new blocks). I.e blocks could come via inbound, full-outbound, etc.

Given the above, I think the current code does not need to be changed.

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

This took me a while to grasp. num_needed is set to m_private_broadcast_connections_to_open.load(), which is only set to anything above 0 if there are transactions pending to be broadcast.

I think it'd be good to clarify this here, to make it clear that we won't try to open a connection of this type if we have no transactions pending

Added this sentence: "We try to open a connection of this type only if there are transactions pending, but up to a limit."

In 434a20371e441660147da6d0c6c1832cb0d0073b

nit: remove line break

Removed.

In 434a20371e441660147da6d0c6c1832cb0d0073b

nit: per one -> for each

Done

In 434a20371e441660147da6d0c6c1832cb0d0073b

This is missing docs

Added "A list of transactions to be broadcast privately."

In 434a20371e441660147da6d0c6c1832cb0d0073b

I don't follow this log. What is the "due to ..." trying to convey here?

"due to" = "because of" = "because there is a new transaction for broadcast - txid=...". I am open to suggestions for better wording.

In 434a20371e441660147da6d0c6c1832cb0d0073b

nit: I don't think these belong to this commit yet

Right! Moved to the corresponding commits.

In 434a20371e441660147da6d0c6c1832cb0d0073b

nit: 2024

Some of the code was written in 2023

In 434a20371e441660147da6d0c6c1832cb0d0073b

nit: 2024

Same, some of the code was written in 2023

In fecab592de8d0994e6d61e454e118d652ffd7b93

micro-nit: There is no your_ counterparty for this, so there's no need to call them my_. This is kind of assumed

I prefixed everything with my_ or your_ because otherwise it is unclear whether it is mine or yours. Deducting that without my/yours prefix requires an extra effort.

In fecab592de8d0994e6d61e454e118d652ffd7b93

nit:

                                "Got a PONG (the transaction will probably reach the network), "

Done

In fecab592de8d0994e6d61e454e118d652ffd7b93

                      "Never got a PONG (the transaction will probably not "

Done

In fecab592de8d0994e6d61e454e118d652ffd7b93

FinishBroadcast may sound better

Renamed to FinishBroadcast().

In fecab592de8d0994e6d61e454e118d652ffd7b93

Isn't iters->by_txid->first the same as txid? It would read way simpler by using the latter

Also, excuse my lack of cpp proficiency but, is it necessary to delete and re-add the item? Why is not updating priority enough (as done in the above two lines)?

Yes, iters->by_txid->first is the same as txid, changed it to use txid which is shorter.

It is necessary to remove and re-add because we have changed the key and C++ maps do not support modifying of the keys. That is, changing the key means that the entry will have to be in another place in the map, so it has to be removed from the old place and added to the new one.

In bb697ba5f51f5e31ec472f32682fbb6eb692fa37

AFAICT this is the only use of CConnman::PrivateBroadcastSub, meaning we should be able to simplify the method by unconditionally setting m_private_broadcast_connections_to_open to 0

Yes, if there is only one transaction submitted for broadcast. But if there is more than one, then m_private_broadcast_connections_to_open should not be set to 0.

For example:

• 3 transactions are submitted at approximately the same time: tx1, tx2, tx3.
• NUM_PRIVATE_BROADCAST_PER_TX=5 for each one, so m_private_broadcast_connections_to_open is set to 15.
• tx1 is broadcast 2 times, tx2 once and tx3 once, m_private_broadcast_connections_to_open is now 15-2-1-1=11.
• tx1 is received back from the network. We come to this code and see that tx1 has been broadcast two times and deduce that the other 5-2=3 we created are not needed and call PrivateBroadcastSub(3). m_private_broadcast_connections_to_open should be set to 11-3=8 (4 for tx2 and 4 for tx3), not 0.

In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd "net: implement opening PRIVATE_BROADCAST connections": m_max_private_broadcast is not used.

Right! Removed.

from the main thread at #29415#pullrequestreview-2041604763

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.

ba75010289f1d9bd3d3a6c635ad18b07cf914041

This is copy/paste the comment for NumToOpenSub() but doesn't apply to this get-only method.

Also, the comments for the two following functions are missing @param[in] n ... but I don't know how strict you need to be about doxygen comments for everything.

Fixed and added @param[in] comments.

What is enforced by clang (not by doxygen) via -Wdocumentation is a mismatch in the parameter names - e.g. if there is @param[in] throughput ... and the actual parameter is int thrughput, then it will give a warning (or error if -Werror is used).

ba75010289f1d9bd3d3a6c635ad18b07cf914041

Why pass this in here instead of just checking it before calling?

Could be either way. If there is more than one caller of this function, then having it here would avoid duplicating the check in every caller, but there is one caller so far.

In the latest push I moved this logic to the caller.

cade8e833197a202dbb03c50359396ecd437ac8c

~optional, could use a comment explaining the priority metric. Looks like transactions with fewer broadcasts or older broadcasts are prioritized. A unit test would help make that clear as well.~

nm, added later

b81c3cc19ec8055d04b38a72fdfabbfcb2ed2358

Why isn't this at the top of the super long function (and include VERACK also?)

Because we want to ignore incoming messages after a successful handshake. Ignoring the VERACK would disturb the handshake.

b81c3cc19ec8055d04b38a72fdfabbfcb2ed2358

Is it ok to erase from this map now? What if none of our connections are successful ?

Note that this would only be reached if m_by_priority contains txid that does not have an entry in m_by_txid which is a gross bug in the PrivateBroadast class. IIRC this was an assert before but somebody suggested to use Assume() instead.

b81c3cc19ec8055d04b38a72fdfabbfcb2ed2358

Isn't this GetTime() deprecated?

https://github.com/bitcoin/bitcoin/blob/7a7e7d189dfe5efa6ab3c644f55f410412163503/src/util/time.h#L95-L100

Replaced with NodeClock::now().

3f5a4187a9e5d2bbb65c06c1799155fd7dd0d132

This made me wonder, if the tx gets confirmed in a block before we get it via relay, will it ever get removed?

update: looks like this will happen in ReattemptPrivateBroadcast() when we call m_chainman.ProcessTransaction() ?

Yes, it will return something other than MempoolAcceptResult::ResultType::VALID. Would happen as well if a conflicting transaction is mined in the meantime.

3f5a4187a9e5d2bbb65c06c1799155fd7dd0d132

Feels like this should be part of Remove(), which I think highlights some possible confusion between the two class PrivateBroadcast you have. One in ConnMan and one in its own unit, used in PeerManager.

I dunno if there's a cleaner approach available, to ensure that the two objects don't get out of sync? Like instead of m_num_to_open in ConnMan, could you refer to the size of the ByNodeId map?

There is some layer separation between net/CConnman and net_processing/PeerManager which I am trying not to blur too much. The lower net level only knows about connections and things like "I need to open N connections of type X". It does not know anything about transactions, or P2P messages which are handled by the net_processing layer.

The constant NUM_PRIVATE_BROADCAST_PER_TX (send a transaction to this many peers) belongs to the net_processing layer. Calling this NumToOpenSub() is only done if Remove() succeeds and NUM_PRIVATE_BROADCAST_PER_TX > num_broadcasted.value() holds. So, no way to do it in CConnman because that would mean accessing NUM_PRIVATE_BROADCAST_PER_TX from CConnman.

could you refer to the size of the ByNodeId map?

That is a "transaction id mapped by node id". Couldn't deal with transactions from within CConnman.

what about m_by_nodeid?

m_by_nodeid is managed by PrivateBroadcast::PushedToNode() and PrivateBroadcast::FinishBroadcast().

a86785dc29faa2dcff301641e2f823b73089dae2

should 1min be defined as a constant in the header file? This is what we consider stale?

Yes, moved to a constant and added a comment to it. Placed the constant in the .cpp file since it is only used in that .cpp file, to limit its scope. It is not part of the "public" private_broadcast.h/cpp interface

5f49e61ffa576aa6c27de30a7db51d17c4f5c516

How could this be condition be different now than when it is checked in init.cpp?

The Tor connectivity could be "off" during startup and become "on" later at runtime, when we connect to the Tor daemon and it tells us the Tor proxy address. This is why the startup condition is a bit different, it has the onion_may_become_reachable variable.

0a6797ffb3eb2d45bce1dc68e830efd89ac2a442

Teleporting the comment from #29420 (review)

Why didn't we need to do this on master?

The old log message was Connected & Listening: 127.0.0.1:15689 not Connected & Listening: 0:0

I replied to that thread to keep the discussion in one place.