From be2e4d94e59510e0a98408313feb27e97321b16e Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 3 Nov 2022 15:01:17 +0000 Subject: [PATCH] [validation] when quitting early in AcceptPackage, set package_state and tx result Bug: not setting package_state means package_state.IsValid() == true and the caller does not know that this failed. We won't be validating this transaction again, so it makes sense to return this failure to the caller. Rename package_state to package_state_quit_early to make it more clear what this variable is used for and what its scope is. Co-authored-by: Greg Sanders --- src/validation.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e24d39170e1..9845e8ddc95 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1273,19 +1273,20 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args) { AssertLockHeld(cs_main); - PackageValidationState package_state; + // Used if returning a PackageMempoolAcceptResult directly from this function. + PackageValidationState package_state_quit_early; // Check that the package is well-formed. If it isn't, we won't try to validate any of the // transactions and thus won't return any MempoolAcceptResults, just a package-wide error. // Context-free package checks. - if (!CheckPackage(package, package_state)) return PackageMempoolAcceptResult(package_state, {}); + if (!CheckPackage(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {}); // All transactions in the package must be a parent of the last transaction. This is just an // opportunity for us to fail fast on a context-free check without taking the mempool lock. if (!IsChildWithParents(package)) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); - return PackageMempoolAcceptResult(package_state, {}); + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); } // IsChildWithParents() guarantees the package is > 1 transactions. @@ -1317,8 +1318,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout); }; if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); - return PackageMempoolAcceptResult(package_state, {}); + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); } // Protect against bugs where we pull more inputs from disk that miss being added to // coins_to_uncache. The backend will be connected again when needed in PreChecks. @@ -1381,6 +1382,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // future. Continue individually validating the rest of the transactions, because // some of them may still be valid. quit_early = true; + package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + results.emplace(wtxid, single_res); } else { txns_new.push_back(tx); } @@ -1390,7 +1393,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Nothing to do if the entire package has already been submitted. if (quit_early || txns_new.empty()) { // No package feerate when no package validation was done. - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state_quit_early, std::move(results)); } // Validate the (deduplicated) transactions as a package. auto submission_result = AcceptMultipleTransactions(txns_new, args);