Merge #18807: [doc / test / mempool] unbroadcast follow-ups

9e1cb1adf1800efe429e348650931f2669b0d2c0 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67166a6ab7c0f33f7ec1990d3c31761e [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f29c63d57af05bfbdd6035bb9c965de2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5833a5c5fc735ef00c0a80f5fab7a2c [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c744a103b633c1051e8fbc01e612097dc [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb0ec73f10b196e79b637aa601c0a6b7 [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba5498318233ab81decbc585e9619d8ffe2df1b0 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b4e5ae249b8011360c6b0f7dc731581 [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15de762fdaf0937a0877d17b0c2bce16e [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab5a5a41685f437db9081fa7b395fa73 [docs] add release notes (Amiti Uttarwar)

Pull request description:

  This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.

  #18895 is another follow up to that addresses other functionality updates.

  Background context:
  The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.

ACKs for top commit:
  MarcoFalke:
    ACK 9e1cb1adf1 👁
  gzhao408:
    ACK [`9e1cb1a`](9e1cb1adf1)

Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
This commit is contained in:
MarcoFalke 2020-05-30 12:22:03 -04:00 committed by PastaPastaPasta
parent f1885c2221
commit be6a045b8c
9 changed files with 74 additions and 20 deletions

View File

@ -0,0 +1,33 @@
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.
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)

View File

@ -965,7 +965,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);
}

View File

@ -494,7 +494,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
{RPCResult{RPCResult::Type::STR_HEX, "transactionid", "parent transaction id"}}},
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, "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)"},
RPCResult{RPCResult::Type::BOOL, "time", "True if this transaction was locked via InstantSend"}
};}

View File

@ -750,7 +750,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);
}
@ -760,12 +760,12 @@ public:
void RemoveUnbroadcastTx(const uint256& txid, const bool unchecked = false);
/** Returns transactions in unbroadcast set */
const std::set<uint256> GetUnbroadcastTxs() const {
std::set<uint256> GetUnbroadcastTxs() const {
LOCK(cs);
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);

View File

@ -5403,12 +5403,18 @@ bool LoadMempool(CTxMemPool& pool)
pool.PrioritiseTransaction(i.first, i.second);
}
std::set<uint256> unbroadcast_txids;
file >> unbroadcast_txids;
unbroadcast = unbroadcast_txids.size();
// TODO: remove this try except in v0.22
try {
std::set<uint256> 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) {

View File

@ -79,7 +79,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.
self.disconnect_nodes(0, 2)
self.disconnect_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"))
self.connect_nodes(0, 2)
@ -152,8 +154,10 @@ class MempoolPersistTest(BitcoinTestFramework):
# clear out mempool
node0.generate(1)
# disconnect nodes to make a txn that remains in the unbroadcast set.
self.disconnect_nodes(0, 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

View File

@ -14,6 +14,7 @@ from test_framework.util import (
create_confirmed_utxos,
)
MAX_INITIAL_BROADCAST_DELAY = 15 * 60 # 15 minutes in seconds
class MempoolUnbroadcastTest(BitcoinTestFramework):
def set_test_params(self):
@ -70,7 +71,7 @@ class MempoolUnbroadcastTest(BitcoinTestFramework):
self.connect_nodes(0, 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
@ -84,15 +85,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)
self.disconnect_nodes(0, 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]
self.disconnect_nodes(0, 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.

View File

@ -740,6 +740,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())

View File

@ -52,10 +52,16 @@ 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)
# 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(self.mocktime + twelve_hrs - two_min)
self.mocktime = self.mocktime + 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 2 hours in the future,