From 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sun, 28 Apr 2019 16:26:31 -0500 Subject: [validation] Remove fMissingInputs from AcceptToMemoryPool() Handle this failure in the same way as all other failures: call Invalid() with the reasons for the failure. --- src/bench/block_assemble.cpp | 2 +- src/consensus/validation.h | 7 +------ src/net_processing.cpp | 10 ++++------ src/node/transaction.cpp | 12 +++++------- src/rpc/rawtransaction.cpp | 11 ++++++----- src/test/txvalidation_tests.cpp | 1 - src/test/txvalidationcache_tests.cpp | 2 +- src/test/validation_block_tests.cpp | 1 - src/validation.cpp | 23 +++++++---------------- src/validation.h | 2 +- 10 files changed, 26 insertions(+), 45 deletions(-) (limited to 'src') diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index a3a44e5dbc..2f47398d99 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -39,7 +39,7 @@ static void AssembleBlock(benchmark::State& state) for (const auto& txr : txs) { TxValidationState state; - bool ret{::AcceptToMemoryPool(::mempool, state, txr, nullptr /* pfMissingInputs */, nullptr /* plTxnReplaced */, false /* bypass_limits */, /* nAbsurdFee */ 0)}; + bool ret{::AcceptToMemoryPool(::mempool, state, txr, nullptr /* plTxnReplaced */, false /* bypass_limits */, /* nAbsurdFee */ 0)}; assert(ret); } } diff --git a/src/consensus/validation.h b/src/consensus/validation.h index cdc3aecc1e..e602b9d5f3 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -27,12 +27,7 @@ enum class TxValidationResult { */ TX_RECENT_CONSENSUS_CHANGE, TX_NOT_STANDARD, //!< didn't meet our local policy rules - /** - * transaction was missing some of its inputs - * TODO: ATMP uses fMissingInputs and a valid ValidationState to indicate missing inputs. - * Change ATMP to use TX_MISSING_INPUTS. - */ - TX_MISSING_INPUTS, + TX_MISSING_INPUTS, //!< transaction was missing some of its inputs TX_PREMATURE_SPEND, //!< transaction spends a coinbase too early, or violates locktime/sequence locks /** * Transaction might be missing a witness, have a witness prior to SegWit diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6c9cf1ccb6..8b1f18398d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1836,14 +1836,13 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se const CTransactionRef porphanTx = orphan_it->second.tx; const CTransaction& orphanTx = *porphanTx; NodeId fromPeer = orphan_it->second.fromPeer; - bool fMissingInputs2 = false; // Use a new TxValidationState because orphans come from different peers (and we call // MaybePunishNodeForTx based on the source peer from the orphan map, not based on the peer // that relayed the previous transaction). TxValidationState orphan_state; if (setMisbehaving.count(fromPeer)) continue; - if (AcceptToMemoryPool(mempool, orphan_state, porphanTx, &fMissingInputs2, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { + if (AcceptToMemoryPool(mempool, orphan_state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, *connman); for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { @@ -1856,7 +1855,7 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se } EraseOrphanTx(orphanHash); done = true; - } else if (!fMissingInputs2) { + } else if (orphan_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (orphan_state.IsInvalid()) { // Punish peer that gave us an invalid orphan tx if (MaybePunishNodeForTx(fromPeer, orphan_state)) { @@ -2492,7 +2491,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr LOCK2(cs_main, g_cs_orphans); - bool fMissingInputs = false; TxValidationState state; CNodeState* nodestate = State(pfrom->GetId()); @@ -2503,7 +2501,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr std::list lRemovedTxn; if (!AlreadyHave(inv) && - AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { + AcceptToMemoryPool(mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { mempool.check(&::ChainstateActive().CoinsTip()); RelayTransaction(tx.GetHash(), *connman); for (unsigned int i = 0; i < tx.vout.size(); i++) { @@ -2525,7 +2523,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // Recursively process any orphan transactions that depended on this one ProcessOrphanTx(connman, pfrom->orphan_work_set, lRemovedTxn); } - else if (fMissingInputs) + else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected for (const CTxIn& txin : tx.vin) { diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 4645baa3d7..8989a77e57 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -37,17 +37,15 @@ TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err if (!mempool.exists(hashTx)) { // Transaction is not already in the mempool. Submit it. TxValidationState state; - bool fMissingInputs; - if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs, + if (!AcceptToMemoryPool(mempool, state, std::move(tx), nullptr /* plTxnReplaced */, false /* bypass_limits */, max_tx_fee)) { + err_string = FormatStateMessage(state); if (state.IsInvalid()) { - err_string = FormatStateMessage(state); - return TransactionError::MEMPOOL_REJECTED; - } else { - if (fMissingInputs) { + if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { return TransactionError::MISSING_INPUTS; } - err_string = FormatStateMessage(state); + return TransactionError::MEMPOOL_REJECTED; + } else { return TransactionError::MEMPOOL_ERROR; } } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 68024fbe2f..48b26d2a6f 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -894,19 +894,20 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) result_0.pushKV("txid", tx_hash.GetHex()); TxValidationState state; - bool missing_inputs; bool test_accept_res; { LOCK(cs_main); - test_accept_res = AcceptToMemoryPool(mempool, state, std::move(tx), &missing_inputs, + test_accept_res = AcceptToMemoryPool(mempool, state, std::move(tx), nullptr /* plTxnReplaced */, false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true); } result_0.pushKV("allowed", test_accept_res); if (!test_accept_res) { if (state.IsInvalid()) { - result_0.pushKV("reject-reason", strprintf("%s", state.GetRejectReason())); - } else if (missing_inputs) { - result_0.pushKV("reject-reason", "missing-inputs"); + if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { + result_0.pushKV("reject-reason", "missing-inputs"); + } else { + result_0.pushKV("reject-reason", strprintf("%s", state.GetRejectReason())); + } } else { result_0.pushKV("reject-reason", state.GetRejectReason()); } diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 1f781ca83a..391ebfadfb 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -39,7 +39,6 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) BOOST_CHECK_EQUAL( false, AcceptToMemoryPool(mempool, state, MakeTransactionRef(coinbaseTx), - nullptr /* pfMissingInputs */, nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */)); diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index ea2ef5546f..144230b114 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -23,7 +23,7 @@ ToMemPool(const CMutableTransaction& tx) LOCK(cs_main); TxValidationState state; - return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx), nullptr /* pfMissingInputs */, + return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx), nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */); } diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index cc1406c062..aca9f475ac 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -285,7 +285,6 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) ::mempool, state, tx, - /* pfMissingInputs */ &ignored, &plTxnReplaced, /* bypass_limits */ false, /* nAbsurdFee */ 0)); diff --git a/src/validation.cpp b/src/validation.cpp index eaf1cfe23d..ca6d2176b3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -365,7 +365,7 @@ static void UpdateMempoolForReorg(DisconnectedBlockTransactions& disconnectpool, // ignore validation errors in resurrected transactions TxValidationState stateDummy; if (!fAddToMempool || (*it)->IsCoinBase() || - !AcceptToMemoryPool(mempool, stateDummy, *it, nullptr /* pfMissingInputs */, + !AcceptToMemoryPool(mempool, stateDummy, *it, nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */)) { // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). @@ -442,7 +442,6 @@ public: struct ATMPArgs { const CChainParams& m_chainparams; TxValidationState &m_state; - bool* m_missing_inputs; const int64_t m_accept_time; std::list* m_replaced_transactions; const bool m_bypass_limits; @@ -538,7 +537,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Copy/alias what we need out of args TxValidationState &state = args.m_state; - bool* pfMissingInputs = args.m_missing_inputs; const int64_t nAcceptTime = args.m_accept_time; const bool bypass_limits = args.m_bypass_limits; const CAmount& nAbsurdFee = args.m_absurd_fee; @@ -554,10 +552,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) CAmount& nConflictingFees = ws.m_conflicting_fees; size_t& nConflictingSize = ws.m_conflicting_size; - if (pfMissingInputs) { - *pfMissingInputs = false; - } - if (!CheckTransaction(tx, state)) return false; // state filled in by CheckTransaction @@ -647,10 +641,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } // Otherwise assume this might be an orphan tx for which we just haven't seen parents yet - if (pfMissingInputs) { - *pfMissingInputs = true; - } - return false; // fMissingInputs and !state.IsInvalid() is used to detect this condition, don't set state.Invalid() + return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent"); } } @@ -1047,11 +1038,11 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs /** (try to) add transaction to memory pool with a specified acceptance time **/ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, - bool* pfMissingInputs, int64_t nAcceptTime, std::list* plTxnReplaced, + int64_t nAcceptTime, std::list* plTxnReplaced, bool bypass_limits, const CAmount nAbsurdFee, bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::vector coins_to_uncache; - MemPoolAccept::ATMPArgs args { chainparams, state, pfMissingInputs, nAcceptTime, plTxnReplaced, bypass_limits, nAbsurdFee, coins_to_uncache, test_accept }; + MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, plTxnReplaced, bypass_limits, nAbsurdFee, coins_to_uncache, test_accept }; bool res = MemPoolAccept(pool).AcceptSingleTransaction(tx, args); if (!res) { // Remove coins that were not present in the coins cache before calling ATMPW; @@ -1069,11 +1060,11 @@ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPo } bool AcceptToMemoryPool(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, - bool* pfMissingInputs, std::list* plTxnReplaced, + std::list* plTxnReplaced, bool bypass_limits, const CAmount nAbsurdFee, bool test_accept) { const CChainParams& chainparams = Params(); - return AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, pfMissingInputs, GetTime(), plTxnReplaced, bypass_limits, nAbsurdFee, test_accept); + return AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, GetTime(), plTxnReplaced, bypass_limits, nAbsurdFee, test_accept); } /** @@ -4969,7 +4960,7 @@ bool LoadMempool(CTxMemPool& pool) TxValidationState state; if (nTime + nExpiryTimeout > nNow) { LOCK(cs_main); - AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, nullptr /* pfMissingInputs */, nTime, + AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, nTime, nullptr /* plTxnReplaced */, false /* bypass_limits */, 0 /* nAbsurdFee */, false /* test_accept */); if (state.IsValid()) { diff --git a/src/validation.h b/src/validation.h index a8d7e76a31..7f9582adfd 100644 --- a/src/validation.h +++ b/src/validation.h @@ -273,7 +273,7 @@ void PruneBlockFilesManual(int nManualPruneHeight); /** (try to) add transaction to memory pool * plTxnReplaced will be appended to with all transactions replaced from mempool **/ bool AcceptToMemoryPool(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx, - bool* pfMissingInputs, std::list* plTxnReplaced, + std::list* plTxnReplaced, bool bypass_limits, const CAmount nAbsurdFee, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Get the BIP9 state for a given deployment at the current tip. */ -- cgit v1.2.3