sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-25 03:52:49 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#22261: [p2p/mempool] Two small fixes to node broadcast logic

Browse Source 
5a77abd4e657458852875a07692898982f4b1db5 [style] Clean up BroadcastTransaction() (John Newbery)
7282d4c0363ab5152baa34af626cb49afbfddc32 [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow)
cd48372b67d961fe661990a2c6d3cc3d91478924 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery)
847b6ed48d7bacec9024618922e9b339d2d97676 [test] Test transactions are not re-added to unbroadcast set (Duncan Dean)
2837a9f1eaa2c6bf402d1d9891d9aa84c4a56033 [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery)

Pull request description:

  1. Only add a transaction to the unbroadcast set when it's added to the mempool

      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.

  2. Allow rebroadcast for same-txid-different-wtxid transactions

      There is 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.

  The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.

ACKs for top commit:
  duncandean:
    reACK 5a77abd
  naumenkogs:
    ACK 5a77abd4e657458852875a07692898982f4b1db5
  theStack:
    re-ACK 5a77abd4e657458852875a07692898982f4b1db5
  lsilva01:
    re-ACK 5a77abd4e6

Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516
This commit is contained in:
fanquake 2021-07-20 20:39:57 +08:00 committed by Konstantin Akimov
parent 1430897fc4
commit facf685285
No known key found for this signature in database
GPG Key ID: 2176C4A5D01EA524
2 changed files with 65 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

109
src/node/transaction.cpp
View File

@ -31,34 +31,42 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str
TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, bilingual_str& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits) TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, bilingual_str& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, bool bypass_limits)
{ {
// BroadcastTransaction can be called by either sendrawtransaction RPC or wallet RPCs. // BroadcastTransaction can be called by either sendrawtransaction RPC or the wallet.
// node.peerman is assigned both before chain clients and before RPC server is accepting calls, // chainman, mempool and peerman are initialized before the RPC server and wallet are started
// and reset after chain clients and RPC sever are stopped. node.peerman should never be null here. // and reset after the RPC sever and wallet are stopped.
assert(node.peerman); assert(node.chainman);
assert(node.mempool); assert(node.mempool);
assert(node.peerman);
std::promise<void> promise; std::promise<void> promise;
uint256 hashTx = tx->GetHash(); uint256 txid = tx->GetHash();
bool callback_set = false; bool callback_set = false;
{ // cs_main scope {
assert(node.chainman); LOCK(cs_main);
LOCK(cs_main);
// If the transaction is already confirmed in the chain, don't do anything // If the transaction is already confirmed in the chain, don't do anything
// and return early. // and return early.
CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip(); CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip();
for (size_t o = 0; o < tx->vout.size(); o++) { for (size_t o = 0; o < tx->vout.size(); o++) {
const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o)); const Coin& existingCoin = view.AccessCoin(COutPoint(txid, o));
// IsSpent does not mean the coin is spent, it means the output does not exist. // IsSpent does not mean the coin is spent, it means the output does not exist.
// So if the output does exist, then this transaction exists in the chain. // So if the output does exist, then this transaction exists in the chain.
if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN; if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_CHAIN;
} }
if (!node.mempool->exists(hashTx)) { if (auto mempool_tx = node.mempool->get(txid); mempool_tx) {
// Transaction is not already in the mempool. // There's already a transaction in the mempool with this txid. Don't
if (max_tx_fee > 0) { // try to submit this transaction to the mempool (since it'll be
// First, call ATMP with test_accept and check the fee. If ATMP // rejected as a TX_CONFLICT), but do attempt to reannounce the mempool
// fails here, return error immediately. // transaction if relay=true.
const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, //
bypass_limits, true /* test_accept */); } else {
// Transaction is not already in the mempool.
if (max_tx_fee > 0) {
// First, call ATMP with test_accept and check the fee. If ATMP
// fails here, return error immediately.
const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx,
bypass_limits, true /* test_accept */);
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
return HandleATMPError(result.m_state, err_string.original); return HandleATMPError(result.m_state, err_string.original);
} else if (result.m_base_fees.value() > max_tx_fee) { } else if (result.m_base_fees.value() > max_tx_fee) {
@ -68,28 +76,33 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
// Try to submit the transaction to the mempool. // Try to submit the transaction to the mempool.
const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx,
bypass_limits, false /* test_accept */); bypass_limits, false /* test_accept */);
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
return HandleATMPError(result.m_state, err_string.original); return HandleATMPError(result.m_state, err_string.original);
}
// Transaction was accepted to the mempool.
if (relay) {
// the mempool tracks locally submitted transactions to make a
// best-effort of initial broadcast
node.mempool->AddUnbroadcastTx(txid);
}
if (wait_callback) {
// For transactions broadcast from outside the wallet, make sure
// that the wallet has been notified of the transaction before
// continuing.
//
// This prevents a race where a user might call sendrawtransaction
// with a transaction to/from their wallet, immediately call some
// wallet RPC, and get a stale result because callbacks have not
// yet been processed.
CallFunctionInValidationInterfaceQueue([&promise] {
promise.set_value();
});
callback_set = true;
}
} }
// Transaction was accepted to the mempool.
if (wait_callback) {
// For transactions broadcast from outside the wallet, make sure
// that the wallet has been notified of the transaction before
// continuing.
//
// This prevents a race where a user might call sendrawtransaction
// with a transaction to/from their wallet, immediately call some
// wallet RPC, and get a stale result because callbacks have not
// yet been processed.
CallFunctionInValidationInterfaceQueue([&promise] {
promise.set_value();
});
callback_set = true;
}
}
} // cs_main } // cs_main
if (callback_set) { if (callback_set) {
@ -99,12 +112,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
} }
if (relay) { if (relay) {
// the mempool tracks locally submitted transactions to make a
// best-effort of initial broadcast
node.mempool->AddUnbroadcastTx(hashTx);
LOCK(cs_main); LOCK(cs_main);
node.peerman->RelayTransaction(hashTx); node.peerman->RelayTransaction(txid);
} }
return TransactionError::OK; return TransactionError::OK;

6
test/functional/mempool_unbroadcast.py
View File

@ -92,6 +92,12 @@ class MempoolUnbroadcastTest(BitcoinTestFramework):
self.disconnect_nodes(0, 1) self.disconnect_nodes(0, 1)
node.disconnect_p2ps() node.disconnect_p2ps()
self.log.info("Rebroadcast transaction and ensure it is not added to unbroadcast set when already in mempool")
rpc_tx_hsh = node.sendrawtransaction(txFS["hex"])
mempool = node.getrawmempool(True)
assert rpc_tx_hsh in mempool
assert not mempool[rpc_tx_hsh]['unbroadcast']
def test_txn_removal(self): def test_txn_removal(self):
self.log.info("Test that transactions removed from mempool are removed from unbroadcast set") self.log.info("Test that transactions removed from mempool are removed from unbroadcast set")
node = self.nodes[0] node = self.nodes[0]