From dab298d9ab5a5a41685f437db9081fa7b395fa73 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Apr 2020 10:12:25 -0700 Subject: [PATCH 01/10] [docs] add release notes --- doc/release-notes.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/doc/release-notes.md b/doc/release-notes.md index 0d668a6302f..0a2d0fbfc3b 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -65,9 +65,28 @@ Notable changes P2P and network changes ----------------------- +- The mempool now tracks whether transactions submitted via the wallet or RPCs + have been successfully broadcast. Every 10-15 minutes, the node will try to + announce unbroadcast transactions until a peer requests it via a `getdata` + message or the transaction is removed from the mempool for other reasons. + The node will not track the broadcast status of transactions submitted to the + node using P2P relay. This version reduces the initial broadcast guarantees + for wallet transactions submitted via P2P to a node running the wallet. (#18038) + Updated RPCs ------------ +- `getmempoolinfo` now returns an additional `unbroadcastcount` field. The + mempool tracks locally submitted transactions until their initial broadcast + is acknowledged by a peer. This field returns the count of transactions + waiting for acknowledgement. + +- Mempool RPCs such as `getmempoolentry` and `getrawmempool` with + `verbose=true` now return an additional `unbroadcast` field. This indicates + whether initial broadcast of the transaction has been acknowledged by a + peer. `getmempoolancestors` and `getmempooldescendants` are also updated. + + Changes to Wallet or GUI related RPCs can be found in the GUI or Wallet section below. New RPCs @@ -87,6 +106,13 @@ New settings Wallet ------ +- To improve wallet privacy, the frequency of wallet rebroadcast attempts is + reduced from approximately once every 15 minutes to once every 12-36 hours. + To maintain a similar level of guarantee for initial broadcast of wallet + transactions, the mempool tracks these transactions as a part of the newly + introduced unbroadcast set. See the "P2P and network changes" section for + more information on the unbroadcast set. (#18038) + #### Wallet RPC changes - The `upgradewallet` RPC replaces the `-upgradewallet` command line option. From bd093ca15de762fdaf0937a0877d17b0c2bce16e Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Apr 2020 10:17:42 -0700 Subject: [PATCH 02/10] [test] updates to unbroadcast test - add () to function to actually disconnect from p2pconn - extract max interval into a constant - disconnect at the end of a subtest rather than start of next --- test/functional/mempool_unbroadcast.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/functional/mempool_unbroadcast.py b/test/functional/mempool_unbroadcast.py index dedf5b8a476..365d0111579 100755 --- a/test/functional/mempool_unbroadcast.py +++ b/test/functional/mempool_unbroadcast.py @@ -16,6 +16,7 @@ from test_framework.util import ( disconnect_nodes, ) +MAX_INITIAL_BROADCAST_DELAY = 15 * 60 # 15 minutes in seconds class MempoolUnbroadcastTest(BitcoinTestFramework): def set_test_params(self): @@ -72,7 +73,7 @@ class MempoolUnbroadcastTest(BitcoinTestFramework): connect_nodes(node, 1) # fast forward into the future & ensure that the second node has the txns - node.mockscheduler(15 * 60) # 15 min in seconds + node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY) self.sync_mempools(timeout=30) mempool = self.nodes[1].getrawmempool() assert rpc_tx_hsh in mempool @@ -86,15 +87,16 @@ class MempoolUnbroadcastTest(BitcoinTestFramework): self.log.info("Add another connection & ensure transactions aren't broadcast again") conn = node.add_p2p_connection(P2PTxInvStore()) - node.mockscheduler(15 * 60) - time.sleep(5) + node.mockscheduler(MAX_INITIAL_BROADCAST_DELAY) + time.sleep(2) # allow sufficient time for possibility of broadcast assert_equal(len(conn.get_invs()), 0) + disconnect_nodes(node, 1) + node.disconnect_p2ps() + def test_txn_removal(self): self.log.info("Test that transactions removed from mempool are removed from unbroadcast set") node = self.nodes[0] - disconnect_nodes(node, 1) - node.disconnect_p2ps # since the node doesn't have any connections, it will not receive # any GETDATAs & thus the transaction will remain in the unbroadcast set. From 00d44a534b4e5ae249b8011360c6b0f7dc731581 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Apr 2020 10:18:36 -0700 Subject: [PATCH 03/10] [test] P2P connection behavior should meet expectations - P2PTxInvStore should supplement `on_inv` behavior of parent, not overwrite. --- test/functional/test_framework/mininode.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py index 95a63717d60..83149ead54b 100755 --- a/test/functional/test_framework/mininode.py +++ b/test/functional/test_framework/mininode.py @@ -652,6 +652,8 @@ class P2PTxInvStore(P2PInterface): # save txid self.tx_invs_received[i.hash] += 1 + super().on_inv(message) + def get_invs(self): with mininode_lock: return list(self.tx_invs_received.keys()) From ba5498318233ab81decbc585e9619d8ffe2df1b0 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Apr 2020 10:20:16 -0700 Subject: [PATCH 04/10] [test] Test that wallet transactions aren't rebroadcast before 12 hours --- .../functional/wallet_resendwallettransactions.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index b384998d562..3417616d77d 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -49,16 +49,21 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): block.solve() node.submitblock(ToHex(block)) - # Transaction should not be rebroadcast node.syncwithvalidationinterfacequeue() - node.p2ps[1].sync_with_ping() - assert_equal(node.p2ps[1].tx_invs_received[txid], 0) + now = int(time.time()) + + # Transaction should not be rebroadcast within first 12 hours + # Leave 2 mins for buffer + twelve_hrs = 12 * 60 * 60 + two_min = 2 * 60 + node.setmocktime(now + twelve_hrs - two_min) + time.sleep(2) # ensure enough time has passed for rebroadcast attempt to occur + assert_equal(txid in node.p2ps[1].get_invs(), False) self.log.info("Bump time & check that transaction is rebroadcast") # Transaction should be rebroadcast approximately 24 hours in the future, # but can range from 12-36. So bump 36 hours to be sure. - rebroadcast_time = int(time.time()) + 36 * 60 * 60 - node.setmocktime(rebroadcast_time) + node.setmocktime(now + 36 * 60 * 60) wait_until(lambda: node.p2ps[1].tx_invs_received[txid] >= 1, lock=mininode_lock) From 9c8a55d9cb0ec73f10b196e79b637aa601c0a6b7 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Apr 2020 10:42:01 -0700 Subject: [PATCH 05/10] [mempool] Don't throw expected error message when upgrading --- src/validation.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index dbdf5028fd5..396fc0a1b50 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5067,12 +5067,18 @@ bool LoadMempool(CTxMemPool& pool) pool.PrioritiseTransaction(i.first, i.second); } - std::set unbroadcast_txids; - file >> unbroadcast_txids; - unbroadcast = unbroadcast_txids.size(); + // TODO: remove this try except in v0.22 + try { + std::set unbroadcast_txids; + file >> unbroadcast_txids; + unbroadcast = unbroadcast_txids.size(); - for (const auto& txid : unbroadcast_txids) { + for (const auto& txid : unbroadcast_txids) { pool.AddUnbroadcastTx(txid); + } + } catch (const std::exception&) { + // mempool.dat files created prior to v0.21 will not have an + // unbroadcast set. No need to log a failure if parsing fails here. } } catch (const std::exception& e) { From 1f94bb0c744a103b633c1051e8fbc01e612097dc Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Apr 2020 14:40:05 -0700 Subject: [PATCH 06/10] [doc] Provide rationale for randomization in scheduling. --- src/net_processing.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d0e345c30ec..e0eb092f1d3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -825,7 +825,8 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const } } - // schedule next run for 10-15 minutes in the future + // Schedule next run for 10-15 minutes in the future. + // We add randomness on every cycle to avoid the possibility of P2P fingerprinting. const std::chrono::milliseconds delta = std::chrono::minutes{10} + GetRandMillis(std::chrono::minutes{5}); scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta); } From fa32e676e5833a5c5fc735ef00c0a80f5fab7a2c Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 1 May 2020 14:03:51 -0700 Subject: [PATCH 07/10] [test] Manage node connections better in mempool persist test there were two calls to disconnect_nodes that were no-ops. fixed one & removed the other & added assertions to confirm node has no connections when creating the unbroadcast transaction. --- test/functional/mempool_persist.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/functional/mempool_persist.py b/test/functional/mempool_persist.py index 3969da2eb0c..5d00648aed5 100755 --- a/test/functional/mempool_persist.py +++ b/test/functional/mempool_persist.py @@ -84,7 +84,9 @@ class MempoolPersistTest(BitcoinTestFramework): assert_greater_than_or_equal(tx_creation_time_higher, tx_creation_time) # disconnect nodes & make a txn that remains in the unbroadcast set. - disconnect_nodes(self.nodes[0], 2) + disconnect_nodes(self.nodes[0], 1) + assert(len(self.nodes[0].getpeerinfo()) == 0) + assert(len(self.nodes[0].p2ps) == 0) self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), Decimal("12")) connect_nodes(self.nodes[0], 2) @@ -157,8 +159,10 @@ class MempoolPersistTest(BitcoinTestFramework): # clear out mempool node0.generate(1) - # disconnect nodes to make a txn that remains in the unbroadcast set. - disconnect_nodes(node0, 1) + # ensure node0 doesn't have any connections + # make a transaction that will remain in the unbroadcast set + assert(len(node0.getpeerinfo()) == 0) + assert(len(node0.p2ps) == 0) node0.sendtoaddress(self.nodes[1].getnewaddress(), Decimal("12")) # shutdown, then startup with wallet disabled From 750456d6f29c63d57af05bfbdd6035bb9c965de2 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 20 May 2020 13:50:04 -0700 Subject: [PATCH 08/10] [trivial] Remove misleading 'const' Returning by const value is only meaningful in a specific circumstance around user defined types. In this case, the const is not enforcing any restrictions on the call site, so is misleading. --- src/txmempool.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/txmempool.h b/src/txmempool.h index 4568eb928d9..fd762565e80 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -714,7 +714,7 @@ public: void RemoveUnbroadcastTx(const uint256& txid, const bool unchecked = false); /** Returns transactions in unbroadcast set */ - const std::set GetUnbroadcastTxs() const { + std::set GetUnbroadcastTxs() const { LOCK(cs); return m_unbroadcast_txids; } From 8f30260a67166a6ab7c0f33f7ec1990d3c31761e Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 23 May 2020 11:02:53 -0700 Subject: [PATCH 09/10] [doc] Update unbroadcast description in RPC results --- src/rpc/blockchain.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 717ca308ac1..287b73b7b00 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -414,7 +414,7 @@ static std::vector MempoolEntryDescription() { return { RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction", {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}}, RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee)"}, - RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet confirmed)"}, + RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet acknowledged by any peers)"}, };} static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPoolEntry& e) EXCLUSIVE_LOCKS_REQUIRED(pool.cs) From 9e1cb1adf1800efe429e348650931f2669b0d2c0 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 23 May 2020 12:23:22 -0700 Subject: [PATCH 10/10] [trivial/doc] Fix comment type --- src/txmempool.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index fd762565e80..583f7614b71 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -704,7 +704,7 @@ public: /** Adds a transaction to the unbroadcast set */ void AddUnbroadcastTx(const uint256& txid) { LOCK(cs); - /** Sanity Check: the transaction should also be in the mempool */ + // Sanity Check: the transaction should also be in the mempool if (exists(txid)) { m_unbroadcast_txids.insert(txid); } @@ -719,7 +719,7 @@ public: return m_unbroadcast_txids; } - // Returns if a txid is in the unbroadcast set + /** Returns whether a txid is in the unbroadcast set */ bool IsUnbroadcastTx(const uint256& txid) const { LOCK(cs); return (m_unbroadcast_txids.count(txid) != 0);