AcceptPackage fixups

No behavior changes, just clarifications.

Author: glozow

src/validation.cpp

@@ -596,10 +596,10 @@ class MemPoolAccept
 
     // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script
     // cache - should only be called after successful validation of all transactions in the package.
-    // The package may end up partially-submitted after size limitting; returns true if all
+    // The package may end up partially-submitted after size limiting; returns true if all
     // transactions are successfully added to the mempool, false otherwise.
-    bool FinalizePackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, PackageValidationState& package_state,
-                         std::map<const uint256, const MempoolAcceptResult>& results)
+    bool SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, PackageValidationState& package_state,
+                       std::map<const uint256, const MempoolAcceptResult>& results)
          EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
 
     // Compare a package's feerate against minimum allowed.
@@ -1038,12 +1038,17 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
     return true;
 }
 
-bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace>& workspaces,
-                                    PackageValidationState& package_state,
-                                    std::map<const uint256, const MempoolAcceptResult>& results)
+bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& workspaces,
+                                  PackageValidationState& package_state,
+                                  std::map<const uint256, const MempoolAcceptResult>& results)
 {
     AssertLockHeld(cs_main);
     AssertLockHeld(m_pool.cs);
+    // Sanity check: none of the transactions should be in the mempool, and none of the transactions
+    // should have a same-txid-different-witness equivalent in the mempool.
+    assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws){
+        return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); }));
+
     bool all_submitted = true;
     // ConsensusScriptChecks adds to the script cache and is therefore consensus-critical;
     // CheckInputsFromMempoolAndCache asserts that transactions only spend coins available from the
@@ -1058,10 +1063,10 @@ bool MemPoolAccept::FinalizePackage(const ATMPArgs& args, std::vector<Workspace>
 
         // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the
         // last calculation done in PreChecks, since package ancestors have already been submitted.
-        std::string err_string;
+        std::string unused_err_string;
         if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limit_ancestors,
                                              m_limit_ancestor_size, m_limit_descendants,
-                                             m_limit_descendant_size, err_string)) {
+                                             m_limit_descendant_size, unused_err_string)) {
             results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
             // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail.
             all_submitted = Assume(false);
@@ -1188,7 +1193,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 
     if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results));
 
-    if (!FinalizePackage(args, workspaces, package_state, results)) {
+    if (!SubmitPackage(args, workspaces, package_state, results)) {
         package_state.Invalid(PackageValidationResult::PCKG_TX, "submission failed");
         return PackageMempoolAcceptResult(package_state, std::move(results));
     }
@@ -1214,11 +1219,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
         return PackageMempoolAcceptResult(package_state, {});
     }
 
-    const auto& child = package[package.size() - 1];
+    // IsChildWithParents() guarantees the package is > 1 transactions.
+    assert(package.size() > 1);
     // The package must be 1 child with all of its unconfirmed parents. The package is expected to
     // be sorted, so the last transaction is the child.
+    const auto& child = package.back();
     std::unordered_set<uint256, SaltedTxidHasher> unconfirmed_parent_txids;
-    std::transform(package.cbegin(), package.end() - 1,
+    std::transform(package.cbegin(), package.cend() - 1,
                    std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()),
                    [](const auto& tx) { return tx->GetHash(); });
 
@@ -1348,12 +1355,12 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx
     }();
 
     // Uncache coins pertaining to transactions that were not submitted to the mempool.
-    // Ensure the coins cache is still within limits.
     if (test_accept || result.m_state.IsInvalid()) {
         for (const COutPoint& hashTx : coins_to_uncache) {
             active_chainstate.CoinsTip().Uncache(hashTx);
         }
     }
+    // Ensure the coins cache is still within limits.
     BlockValidationState state_dummy;
     active_chainstate.FlushStateToDisk(state_dummy, FlushStateMode::PERIODIC);
     return result;