validation, bugfix: provide more info in *MempoolAcceptResult #26646

glozow commented at 10:43 AM on December 6, 2022: member

This PR fixes a bug and improves the mempool accept interface to return information more predictably.

Bug: In package validation, we first try the transactions individually (see doc/policy/packages.md for more explanation) and, if they all failed for missing inputs and policy-related (i.e. fee) reasons, we'll try package validation. Otherwise, we'll just "quit early" since, for example, if a transaction had an invalid signature, adding a child will not help make it valid. Currently, when we quit early, we're not setting the package_state to be invalid, so the caller might think it succeeded. Also, we're returning no results - it makes more sense to return the individual transaction failure. Thanks instagibbs for catching #25038 (review)!

Also, make the package results interface generally more useful/predictable:

Always return the feerate at which a transaction was considered for CheckFeeRate in MempoolAcceptResult::m_effective_feerate when it was successful. This can replace the current PackageMempoolAcceptResult::m_package_feerate, which only sometimes exists.
Always provide an entry for every transaction in PackageMempoolAcceptResult::m_tx_results when the error is PCKG_TX.

glozow added the label Bug on Dec 6, 2022

glozow added the label Validation on Dec 6, 2022

DrahtBot commented at 10:43 AM on December 6, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, achow101, naumenkogs
Approach ACK	stickies-v
Stale ACK	vincenzopalazzo

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26711 ([WIP] validate package transactions with their in-package ancestor sets by glozow)
#25429 (refactor: Avoid UniValue copies by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/validation.cpp:1352 in 139e0e10d9 outdated

1343 | @@ -1343,6 +1344,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1344 |      // the new transactions. This ensures we don't double-count transaction counts and sizes when
1345 |      // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy.
1346 |      ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args);
1347 | +    // Results from individual validation. "Mutable" because if a transaction fails by itself but
1348 | +    // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not
1349 | +    // reflected in this map). If a transaction fails more than once, we want to return the first
1350 | +    // result, when it was considered on its own. So changes will only be from invalid -> valid.
1351 | +    std::map<uint256, MempoolAcceptResult> individual_results_mutable;

instagibbs commented at 3:52 PM on December 6, 2022:

call this "nonfinal" or something seems clearer to me than mutable

glozow commented at 12:36 PM on December 7, 2022:

renamed to individual_results_nonfinal

instagibbs commented at 2:27 PM on December 7, 2022:

name is still the same as before

stickies-v commented at 4:31 PM on December 12, 2022:

alternative suggestion: "preliminary", but happy with "nonfinal" too.

glozow commented at 1:03 PM on January 11, 2023:

I don't like "preliminary" as much as it's not guaranteed there's another round :shrug:

in src/validation.cpp:1383 in c1cd5e340e outdated

1379 | @@ -1380,6 +1380,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1380 |                  // future.  Continue individually validating the rest of the transactions, because
1381 |                  // some of them may still be valid.
1382 |                  quit_early = true;
1383 | +                package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");

instagibbs commented at 4:08 PM on December 6, 2022:

at 139e0e10d96a53231b8c5179def33627aed48d3e commenting out this line doesn't seem to fail any tests, but it does for this particular commit.

glozow commented at 9:28 PM on December 6, 2022:

I see what you mean yeah. Will tweak the test.

glozow commented at 12:35 PM on December 7, 2022:

Ah, the problem was the same line exists in the non-quit-early case as well. Deleted that as it's incorrect - if we're not quitting early then package_state won't be used. The non-quit-early case shouldn't be modifying package_state. Also added a comment/rename to make this more clear.

This also has the effect of making the test fail if you comment out this line on the last commit.

in src/validation.cpp:1409 in 139e0e10d9 outdated

1405 |              }
1406 |          }
1407 |      }
1408 |  
1409 |      // Nothing to do if the entire package has already been submitted.
1410 |      if (quit_early || txns_new.empty()) {

instagibbs commented at 4:12 PM on December 6, 2022:

ever slightly so meta, but I think txns_new should instead be called package_eval_txns or something, since there are other new transactions that have been not included in this set, due to being accepted individually.

glozow commented at 12:37 PM on December 7, 2022:

renamed, no problem. 3line diff

stickies-v commented at 5:35 PM on December 12, 2022:

Sorry to bikeshed, but txns_package_eval is hard to understand imo. Would txns_to_evaluate (or package_txns_to_evaluate, but I think it's too verbose) work?

instagibbs commented at 4:13 PM on December 6, 2022: member

Liking the new interfaces significantly more so far, just naming qualms for now

glozow force-pushed on Dec 7, 2022

instagibbs commented at 2:34 PM on December 7, 2022: member

ACK c5ca0755273597cf1d0f1d10b6b914b13a8bb37f naming nit left aside

fanquake requested review from sdaftuar on Dec 7, 2022

in src/validation.cpp:589 in 70d1af27cf outdated

 581 | @@ -582,6 +582,11 @@ class MemPoolAccept
 582 |          /** Total virtual size of all transactions being replaced. */
 583 |          size_t m_conflicting_size{0};
 584 |  
 585 | +        /** If we're doing package validation (i.e. m_package_feerates=true), the "effective"
 586 | +         * package feerate of this transaction, which may include fee and vsize of its ancestors
 587 | +         * and/or descendants. */
 588 | +        CFeeRate m_package_feerate{0};

sdaftuar commented at 9:37 PM on December 8, 2022:

I'm wondering how well-defined this value is. Am I right in thinking that the calculation here is "sum(fees_of_package_transactions)/sum(size_of_package_transactions)"?

What happens in this case: we have 3 transactions A -> B, C, where C is a child of both A and B.

A has a low feerate and cannot make it in to the mempool by itself. (A+B) has a high enough feerate for acceptance, but we receive for validation (A+B+C) instead. B cannot make it in by itself, since it would be missing a parent, and we never try A+B, right? So the package feerate for C would reflect the fees and feerate of B (which could be higher than C's own feerate, I think?).

Do we require that C pay a higher feerate than any of its parents when doing package acceptance? If not, then I'm not sure this value will always make sense, ie the package feerate of a transaction with no children ought never be greater than its own feerate.

instagibbs commented at 9:44 PM on December 8, 2022:

Does this comment equally apply to package submission in master?

sdaftuar commented at 10:17 PM on December 8, 2022:

I think so, but I was about to write a test using the example above, except where C is 0-fee, and see if it makes it in...

glozow commented at 11:12 AM on December 9, 2022:

Yes, master does not currently handle this case because we're currently doing A, B, C individually, so we'd get 1 "too low fee" and 2 "missing inputs," and then reject all of them. When I first tried to implement for 1-child-with-parents, I had incorrectly assumed this was okay and the code made it in before I realized - my apologies for that. The intention is definitely to fix it.

Requiring C pay a higher feerate than its parents is ok in the child-with-parents scenario but not generic ancestor package scenario (imagine low fee grandparent, lower fee parent, high fee child). I think we've discussed "for each tx in topological order, calculate its in-package ancestor set that hasn't made it in yet, and submit that as a package" - I have drafted this here.

Btw this test case is implemented here (it should fail on master as you expect).

instagibbs commented at 3:12 PM on December 9, 2022:

Can confirm the patch fails as expected: the entire triangle package makes it in the mempool

glozow commented at 11:28 AM on December 16, 2022:

Re: "I'm wondering how well-defined this value is." Last push adds the wtxids of which transactions are included in the feerate, to both MempoolAcceptResult and the testmempoolaccept/submitpackage RPCs.

Re: the problem where we don't always end up with the right transactions, might be better for another PR. I've opened #26711 to do this.

glozow commented at 10:22 AM on December 21, 2022:

@sdaftuar comfortable with resolving for this PR?

in src/test/txpackage_tests.cpp:284 in c5ca075527 outdated

 280 | +    // Parent and child package where transactions are invalid for reasons other than fee and
 281 | +    // missing inputs, so the package validation isn't expected to happen.
 282 | +    {
 283 | +        CScriptWitness bad_witness;
 284 | +        bad_witness.stack.push_back(std::vector<unsigned char>(1));
 285 | +        CMutableTransaction mtx_parent_invalid = mtx_parent;

stickies-v commented at 2:14 PM on December 12, 2022:

nit: list initialization (and in a few more places down below)

        CMutableTransaction mtx_parent_invalid{mtx_parent};

glozow commented at 11:28 AM on December 16, 2022:

Done

in src/validation.cpp:1189 in c5ca075527 outdated

1181 | @@ -1176,16 +1182,17 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
1182 |  
1183 |      if (!ConsensusScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state);
1184 |  
1185 | +    const CFeeRate effective_feerate(ws.m_modified_fees, ws.m_vsize);

stickies-v commented at 3:24 PM on December 12, 2022:

nit

    const CFeeRate effective_feerate{ws.m_modified_fees, ws.m_vsize};

glozow commented at 12:06 PM on December 14, 2022:

In the case of virtual sizes and CFeeRate, we do a narrowing conversion (the CFeeRate ctor takes a uint32_t, while we use int64_t for Workspace::m_vsize), so the compiler complains. This is exactly what braced initialization is good for imo, but we should really fix them. This is an annoying thing about CFeeRate in general since we use various types for vsize around the codebase. I'd probably save this for a "unify types used to represent virtual size" PR.

stickies-v commented at 12:21 PM on December 16, 2022:

Aha, good point, can't just go blindly nitting to do list initialization everywhere. However, would it then be a good starting point to just make the conversion explicit instead?

    const CFeeRate effective_feerate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)};

glozow commented at 4:23 PM on December 19, 2022:

applied across the effective feerate commit

in src/rpc/mempool.cpp:127 in c5ca075527 outdated

 123 | @@ -124,6 +124,7 @@ static RPCHelpMan testmempoolaccept()
 124 |                      {RPCResult::Type::OBJ, "fees", /*optional=*/true, "Transaction fees (only present if 'allowed' is true)",
 125 |                      {
 126 |                          {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT},
 127 | +                        {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/true, "if something other than base feerate was used, the effective feerate in " + CURRENCY_UNIT + " per KvB. For example, the package feerate and/or feerate with modified fees from prioritisetransaction."},

stickies-v commented at 3:54 PM on December 12, 2022:

nit: since this is a member of "fees", I think "effective-rate" can be a less verbose alternative (and also consistent with "base")

                        {RPCResult::Type::STR_AMOUNT, "effective-rate", /*optional=*/true, "if something other than base feerate was used, the effective feerate in " + CURRENCY_UNIT + " per KvB. For example, the package feerate and/or feerate with modified fees from prioritisetransaction."},

stickies-v commented at 3:56 PM on December 12, 2022:

Have you considered always including this field? Besides the minimal data overhead, I don't see a reason to not always include it. Simplifies the logic for both server and client?

glozow commented at 11:37 AM on December 13, 2022:

Have you considered always including this field?

We don't always know it. We could have quit validation before looking up inputs, e.g. because CheckTransaction() failed. It's also possible we couldn't find the inputs at all.

stickies-v commented at 10:23 PM on December 20, 2022:

But I think we always know it when allowed=true? For fees, the docs already state that "(only present if 'allowed' is true)". So I think, consistent with base (which isn't optional), we can always (when allowed=true) return effective-feerate. This makes for a more consistent API imo.

I suppose this also comes down to personal preference, so I'm okay leaving it as is too if you/others prefer.

glozow commented at 5:58 PM on January 6, 2023:

Changed to non-optional now

in test/functional/rpc_packages.py:301 in c5ca075527 outdated

 297 | @@ -298,27 +298,19 @@ def test_submit_child_with_parents(self, num_parents, partial_submit):
 298 |          submitpackage_result = node.submitpackage(package=[tx["hex"] for tx in package_txns])
 299 |  
 300 |          # Check that each result is present, with the correct size and fees
 301 | -        for package_txn in package_txns:
 302 | -            tx = package_txn["tx"]
 303 | +        for i in range(num_parents + 1):

stickies-v commented at 4:04 PM on December 12, 2022:

imo this is less readable than the previous for package_txn in package_txns:

(I think the rest of the changes in this loop are an improvement)

glozow commented at 12:38 PM on December 14, 2022:

Changed back

in src/validation.cpp:1412 in c5ca075527 outdated

1409 |      // Nothing to do if the entire package has already been submitted.
1410 | -    if (quit_early || txns_new.empty()) {
1411 | +    if (quit_early || txns_package_eval.empty()) {
1412 | +        for (const auto& [wtxid, mempoolaccept_res] : individual_results_mutable) {
1413 | +            Assume(results.find(wtxid) == results.end());
1414 | +            results.emplace(wtxid, mempoolaccept_res);

stickies-v commented at 4:47 PM on December 12, 2022:

Additional lookup can be avoided:

            Assume(results.emplace(wtxid, mempoolaccept_res).second);

glozow commented at 12:37 PM on December 14, 2022:

Thanks, taken

in src/validation.cpp:1432 in c5ca075527 outdated

1433 | +        // Include individual previous failure reasons if they aren't provided.
1434 | +        for (const auto& [wtxid, mempoolaccept_res] : individual_results_mutable) {
1435 | +            if (submission_result.m_tx_results.find(wtxid) == submission_result.m_tx_results.end()) {
1436 | +                submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res);
1437 | +            }
1438 | +        }

stickies-v commented at 5:11 PM on December 12, 2022:

std::map::insert doesn't overwrite if key already exists, so since we're not using rvalue refs I think this can be simplified without performance degradation:

        // insert doesn't overwrite existing keys
        submission_result.m_tx_results.insert(individual_results_mutable.begin(), individual_results_mutable.end());

glozow commented at 12:38 PM on December 14, 2022:

Ah thanks, taken

in src/validation.cpp:1427 in c5ca075527 outdated

1428 |          submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res);
1429 |      }
1430 | -    if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value());
1431 | +    if (submission_result.m_state.GetResult() == PackageValidationResult::PCKG_TX) {
1432 | +        // Package validation failed because one or more transactions failed.
1433 | +        // Include individual previous failure reasons if they aren't provided.

stickies-v commented at 5:39 PM on December 12, 2022:

nit: could make "previous" more explicit

        // Include individual failure reasons if not already provided by `AcceptSingleTransaction()`.

glozow commented at 12:37 PM on December 14, 2022:

Clarified this comment

in src/validation.cpp:1408 in c5ca075527 outdated

1404 | +                txns_package_eval.push_back(tx);
1405 |              }
1406 |          }
1407 |      }
1408 |  
1409 |      // Nothing to do if the entire package has already been submitted.

stickies-v commented at 5:42 PM on December 12, 2022:

Not specific to this PR, but I think this docstring is out of date? In addition to entire package already being submitted, this block also handles the case of package containing invalid txns. Perhaps good to update now?

glozow commented at 12:37 PM on December 14, 2022:

Updated

in src/validation.cpp:1414 in c5ca075527 outdated

1411 | +    if (quit_early || txns_package_eval.empty()) {
1412 | +        for (const auto& [wtxid, mempoolaccept_res] : individual_results_mutable) {
1413 | +            Assume(results.find(wtxid) == results.end());
1414 | +            results.emplace(wtxid, mempoolaccept_res);
1415 | +        }
1416 |          // No package feerate when no package validation was done.

stickies-v commented at 5:43 PM on December 12, 2022:

This comment can now be removed I think

glozow commented at 12:37 PM on December 14, 2022:

Removed, thanks

stickies-v commented at 6:01 PM on December 12, 2022: contributor

Concept ACK - fixes the bug as specified and imo makes the interface more clear

A few more general comments:

commit message in b00bebbb84af10042ee623116118207b95194a02 I think should be "remove PackageMempoolAcceptResult::m_package_feerate" instead of "remove PackageMempoolAccept::m_package_feerate"
requires a release notes file highlighting the RPC changes
the docstring of assert_equal_package_results (rpc_packages.py) still mentions package feerate which I think is now out of date
nit: a72354186f9328d3f56bdd4a41959ff56a774653 could be split in two commits: one for rename, and one for the actual change

glozow force-pushed on Dec 14, 2022

in src/validation.cpp:1350 in 0d383a65ee outdated

1347 |      // coins_to_uncache. The backend will be connected again when needed in PreChecks.
1348 |      m_view.SetBackend(m_dummy);
1349 |  
1350 |      LOCK(m_pool.cs);
1351 | +    // Stores final results that will not change.
1352 |      std::map<const uint256, const MempoolAcceptResult> results;

instagibbs commented at 3:02 PM on December 14, 2022:

rename to results_final to be self documenting later?

glozow commented at 4:11 PM on December 19, 2022:

done

in src/validation.cpp:1440 in 0d383a65ee outdated

1436 | +    // own PackageValidationState; package_state is unused past this point.
1437 | +    auto submission_result = AcceptMultipleTransactions(txns_package_eval, args);
1438 |      // Include already-in-mempool transaction results in the final result.
1439 |      for (const auto& [wtxid, mempoolaccept_res] : results) {
1440 | -        submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res);
1441 | +        Assume(submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res).second);

instagibbs commented at 3:07 PM on December 14, 2022:

Could also assert that m_result_type != MempoolAcceptResult::ResultType::INVALID to catch possible regressions

glozow commented at 4:08 PM on December 19, 2022:

Good idea, added this + an Assume that the individual_results_nonfinal are all ResultType::INVALID above.

instagibbs approved

instagibbs commented at 3:12 PM on December 14, 2022: member

reACK https://github.com/bitcoin/bitcoin/pull/26646/commits/0d383a65ee01eb58c011c4c056523b9a0f20d96f

Doc changes, and the returning of wtxids considered for the package feerate being the major changes.

in src/rpc/mempool.cpp:222 in 0d383a65ee outdated

 218 | @@ -215,6 +219,15 @@ static RPCHelpMan testmempoolaccept()
 219 |                          result_inner.pushKV("vsize", virtual_size);
 220 |                          UniValue fees(UniValue::VOBJ);
 221 |                          fees.pushKV("base", ValueFromAmount(fee));
 222 | +                        CHECK_NONFATAL(tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID);

vincenzopalazzo commented at 8:35 PM on December 17, 2022:

This check looks redundant? are you in an if with the condition tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID and tx_result is never modified so this confused me while I was reading the code.

I'm missing anything that makes this observation a dum observation?

glozow commented at 3:57 PM on December 19, 2022:

Good point, this is redundant!

in src/rpc/mempool.cpp:883 in 0d383a65ee outdated

 879 | @@ -864,6 +880,15 @@ static RPCHelpMan submitpackage()
 880 |                      result_inner.pushKV("vsize", int64_t{it->second.m_vsize.value()});
 881 |                      UniValue fees(UniValue::VOBJ);
 882 |                      fees.pushKV("base", ValueFromAmount(it->second.m_base_fees.value()));
 883 | +                    if (it->second.m_result_type == MempoolAcceptResult::ResultType::VALID &&

vincenzopalazzo commented at 8:46 PM on December 17, 2022:

                    if (tx_resultm_result_type == MempoolAcceptResult::ResultType::VALID &&

This is a very side comment that does not point to be an annoying comment, just It is just a side note also because I noted that you use this in testmempoolaccept, I think it is just a historical reason?

diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 6f08ac5e4..bc30b9a20 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -871,27 +871,28 @@ static RPCHelpMan submitpackage()
                 auto it = package_result.m_tx_results.find(tx->GetWitnessHash());
                 CHECK_NONFATAL(it != package_result.m_tx_results.end());
                 UniValue result_inner{UniValue::VOBJ};
+                const auto& tx_result = it->second;
                 result_inner.pushKV("txid", tx->GetHash().GetHex());
-                if (it->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) {
-                    result_inner.pushKV("other-wtxid", it->second.m_other_wtxid.value().GetHex());
+                if (tx_result.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) {
+                    result_inner.pushKV("other-wtxid", tx_result.m_other_wtxid.value().GetHex());
                 }
-                if (it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
-                    it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY) {
-                    result_inner.pushKV("vsize", int64_t{it->second.m_vsize.value()});
+                if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID ||
+                    tx_result.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY) {
+                    result_inner.pushKV("vsize", int64_t{tx_result.m_vsize.value()});
                     UniValue fees(UniValue::VOBJ);
-                    fees.pushKV("base", ValueFromAmount(it->second.m_base_fees.value()));
-                    if (it->second.m_result_type == MempoolAcceptResult::ResultType::VALID &&
-                        it->second.m_effective_feerate.value().GetFee(it->second.m_vsize.value()) != it->second.m_base_fees.value()) {
-                        fees.pushKV("effective-feerate", ValueFromAmount(it->second.m_effective_feerate.value().GetFeePerK()));
+                    fees.pushKV("base", ValueFromAmount(tx_result.m_base_fees.value()));
+                    if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID &&
+                        tx_result.m_effective_feerate.value().GetFee(tx_result.m_vsize.value()) != tx_result.m_base_fees.value()) {
+                        fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK()));
                         UniValue effective_includes_res(UniValue::VARR);
-                        for (const auto& wtxid : it->second.m_wtxids_fee_calculations.value()) {
+                        for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) {
                             effective_includes_res.push_back(wtxid.ToString());
                         }
                         fees.pushKV("effective-includes", effective_includes_res);
                     }
                     result_inner.pushKV("fees", fees);
-                    if (it->second.m_replaced_transactions.has_value()) {
-                        for (const auto& ptx : it->second.m_replaced_transactions.value()) {
+                    if (tx_result.m_replaced_transactions.has_value()) {
+                        for (const auto& ptx : tx_result.m_replaced_transactions.value()) {
                             replaced_txids.insert(ptx->GetHash());
                         }
                     }

glozow commented at 4:52 PM on December 19, 2022:

Applied this change to the lines touched in this PR

vincenzopalazzo approved

vincenzopalazzo commented at 8:52 PM on December 17, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/26646/commits/0d383a65ee01eb58c011c4c056523b9a0f20d96f

My comments are just comments that included just some considerations where I do not have an answer on why it is done, I do not want to appear like an annoying person :)

in src/validation.cpp:1157 in 0d383a65ee outdated

1152 | +    std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids),
1153 | +                   [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); });
1154 |      // Find the wtxids of the transactions that made it into the mempool. Allow partial submission,
1155 |      // but don't report success unless they all made it into the mempool.
1156 |      for (Workspace& ws : workspaces) {
1157 | +        const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : CFeeRate(ws.m_modified_fees, ws.m_vsize);

stickies-v commented at 4:15 PM on December 19, 2022:

nit: I think this can be moved inside the if loop?

glozow force-pushed on Dec 19, 2022

vincenzopalazzo approved

vincenzopalazzo commented at 6:24 PM on December 19, 2022: none

reACK https://github.com/bitcoin/bitcoin/pull/26646/commits/48f81f2b5e4052717989a30db942c378d369215b

instagibbs commented at 6:26 PM on December 19, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/26646/commits/48f81f2b5e4052717989a30db942c378d369215b

changes are: removing redundant nonfatal check giving it->second a variable for ease of reading static case of m_vsize to uint32_t doc and suggested variable renamings

in src/validation.cpp:1152 in 48f81f2b5e outdated

1147 | @@ -1143,12 +1148,18 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
1148 |      // make sure we haven't exceeded max mempool size.
1149 |      LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
1150 |  
1151 | +    std::vector<uint256> all_package_wtxids;

stickies-v commented at 7:25 PM on December 19, 2022:

nit: could preallocate memory of workspaces.size()

instagibbs commented at 2:34 PM on January 9, 2023:

note this was accomplished on line below

fanquake requested review from sdaftuar on Dec 20, 2022

in src/validation.cpp:1278 in 48f81f2b5e outdated

1276 |              // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks).
1277 | +            // Note that when test_accept=true, package feerate is not used, so we only include the
1278 | +            // 1 transaction's wtxid in MempoolAcceptResult::wtxids_fee_calculations. This should be
1279 | +            // updated if that changes.
1280 | +            Assume(!args.m_package_feerates);
1281 | +            const std::vector<uint256> single_wtxid({ws.m_ptx->GetWitnessHash()});

stickies-v commented at 10:12 PM on December 20, 2022:

nit

            const std::vector<uint256> single_wtxid{ws.m_ptx->GetWitnessHash()};

glozow commented at 5:58 PM on January 6, 2023:

Done

in src/validation.cpp:1191 in 48f81f2b5e outdated

1186 | @@ -1176,16 +1187,20 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
1187 |  
1188 |      if (!ConsensusScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state);
1189 |  
1190 | +    const CFeeRate effective_feerate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)};
1191 | +    const std::vector<uint256> single_wtxid({ws.m_ptx->GetWitnessHash()});

stickies-v commented at 10:12 PM on December 20, 2022:

nit

    const std::vector<uint256> single_wtxid{ws.m_ptx->GetWitnessHash()};

stickies-v commented at 10:46 PM on December 20, 2022: contributor

Approach ACK 48f81f2b5 - not feeling super comfortable yet with my review so want to mull it over a bit more.

Mostly a few nits, and also I think effective-feerate being optional in the RPC isn't ideal (but I can move past it). Additionally, I think the last 3 comments from my previous review haven't been addressed yet? The missing RPC docs/release notes is probably the most important one there.

naumenkogs commented at 10:06 AM on December 21, 2022: member

ACK 48f81f2b5e4052717989a30db942c378d369215b

I don't have a big picture about package relay yet, but these changes are good on their own.

in src/validation.cpp:1162 in 0d17b04977 outdated

1158 |              CFeeRate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)};
1159 |          if (m_pool.exists(GenTxid::Wtxid(ws.m_ptx->GetWitnessHash()))) {
1160 |              results.emplace(ws.m_ptx->GetWitnessHash(),
1161 | -                MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate));
1162 | +                MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize,
1163 | +                                             ws.m_base_fees, effective_feerate, all_package_wtxids));

achow101 commented at 8:07 PM on January 4, 2023:

In 0d17b04977d6882e8dba6cf6c192a825a446f4ae "[validation] return wtxids of other transactions whose fees were used"

My understanding is that effective_feerate is computed using all of the package's txs when args.m_package_feerates == true, but only the workspace's single tx when that parameter is false. So I think it is incorrect to give all_package_wtxids in the latter case?

glozow commented at 11:39 AM on January 5, 2023:

We include MempoolAcceptResult::m_effective_feerate and m_wtxids_fee_calculations whenever the transaction is successful i.e. ResultType::VALID. I think it's still meaningful for single transactions, as it can include modified fees from prioritisetransaction. Alternatively we could return a separate result for modified fees.

My understanding is that effective_feerate is computed using all of the package's txs when args.m_package_feerates == true, but only the workspace's single tx when that parameter is false.

This is not 100% accurate - it's all of the transactions within AcceptMultipleTransactions, but that's not equivalent to the entire package. During a package validation (see AcceptPackage) we may split up a package into multiple groups for fee calculations (i.e. right now transactions can be submitted individually and with #26711 various subgroups). The hope here is for the caller to know exactly which transactions were included in the "package feerate" for a transaction.

achow101 commented at 4:57 PM on January 5, 2023:

Right, what I mean is that there is a mismatch between m_effective_feerate and m_wtxid_fee_calculations depending on args.m_package_feerates. When that parameter is true, we would correctly report m_effective_feerate as the package feerate and m_wtxids_fee_calculations as all of the wtxids in the workspaces. But when it is false, m_effective_feerate is the modified feerate of just the single transaction in the workspace, whereas m_wtxid_fee_calculations is still all of the wtxids in all of the workspaces. That is what I think is incorrect. It should be the workspace's single wtxid, rather than all of them.

It looks like the only paths to get to this code are either args.m_package_feerates = true or there's a single transaction. So it doesn't really matter, just feels a bit wrong.

glozow commented at 5:57 PM on January 6, 2023:

Ohh I see, yeah that's a bit weird. Changed now.

[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 <gsanders87@gmail.com>

be2e4d94e5

[test] package validation quits early due to non-policy, non-missing-inputs failure 5d35b4a7de

[validation] return effective feerate from mempool validation 1605886380

glozow force-pushed on Jan 6, 2023

glozow commented at 5:57 PM on January 6, 2023: member

Sorry for missing a few of your review comments @stickies-v!

requires a release notes file highlighting the RPC changes

Added for testmempoolaccept, submitpackage is regtest-only.

the docstring of assert_equal_package_results (rpc_packages.py) still mentions package feerate which I think is now out of date

This refers to the "package feerate" concept documented here, rather than the RPC result "package-feerate". So left as is, sorry for that being a bit confusing.

nit: a723541 could be split in two commits: one for rename, and one for the actual change

Done

naumenkogs commented at 7:42 AM on January 9, 2023: member

reACK ada89cb

fanquake requested review from instagibbs on Jan 9, 2023

instagibbs approved

instagibbs commented at 2:39 PM on January 9, 2023: member

reACK ada89cbd4249fad5eaedfa0a173dfc911f188084

in src/validation.cpp:1282 in 92d789b090 outdated

1274 | @@ -1265,9 +1275,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1275 |                  CFeeRate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)};
1276 |              // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are
1277 |              // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks).
1278 | +            // Note that when test_accept=true, package feerate is not used, so we only include the
1279 | +            // 1 transaction's wtxid in MempoolAcceptResult::wtxids_fee_calculations. This should be
1280 | +            // updated if that changes.
1281 | +            Assume(!args.m_package_feerates);
1282 | +            const std::vector<uint256> single_wtxid{ws.m_ptx->GetWitnessHash()};

achow101 commented at 7:47 PM on January 9, 2023:

In 92d789b090a166ea6deebbfcbf2c1069fa8cc693 "[validation] return wtxids of other transactions whose fees were used"

Since the previous commit adds the calculation for effective_feerate here depending on m_package_feerates, I would expect that the calculation for the involved wtxids would do that as well. Or the previous commit could add this Assume and just do the single tx feerate calculation. The behavior for setting the effective feerate and the involved txids should be unified to avoid bugs.

glozow commented at 10:46 AM on January 10, 2023:

Agree this is more bug-preventative. The downside is we end up constructing all_wtxids without using it when doing a test package mempool accept, but seems alright.

in test/functional/rpc_packages.py:316 in 57b56b7ce9 outdated

 313 | +            wtxid = tx.getwtxid()
 314 | +            assert wtxid in submitpackage_result["tx-results"]
 315 | +            tx_result = submitpackage_result["tx-results"][wtxid]
 316 | +            assert_equal(tx_result["txid"], tx.rehash())
 317 | +            assert_equal(tx_result["vsize"], tx.get_vsize())
 318 | +            assert_equal(tx_result["fees"]["base"], DEFAULT_FEE)

achow101 commented at 7:58 PM on January 9, 2023:

In 57b56b7ce9ebef646d3fe3e706bf994c17864f3c "[rpc] return effective-feerate in testmempoolaccept and submitpackage"

This change doesn't seem to be meaningful? The test passes either way.

glozow commented at 10:44 AM on January 10, 2023:

Ended up keeping it in latest push, as the result now includes optional fields and a feerate so we need to use assert_fee_amount

in src/rpc/mempool.cpp:221 in 57b56b7ce9 outdated

 217 | @@ -217,6 +218,9 @@ static RPCHelpMan testmempoolaccept()
 218 |                          result_inner.pushKV("vsize", virtual_size);
 219 |                          UniValue fees(UniValue::VOBJ);
 220 |                          fees.pushKV("base", ValueFromAmount(fee));
 221 | +                        if (fee != tx_result.m_effective_feerate.value().GetFee(virtual_size)) {

achow101 commented at 8:14 PM on January 9, 2023:

In 57b56b7ce9ebef646d3fe3e706bf994c17864f3c "[rpc] return effective-feerate in testmempoolaccept and submitpackage"

For here and in submitpackage, it's not clear to me why effective-feerate and effective-includes should be guarded by whether it is different from the base fees. The presence (or lack thereof) of this field indicates whether this transaction was validated as part of the package, but that is also more explicitly represented by effective-includes. I don't think it's harmful to provide this information for all transactions.

glozow commented at 10:40 AM on January 10, 2023:

Ah thanks, I think I agree. For submitpackage we should continue to gate on whether the tx was in the mempool already - in that case we wouldn't know whether package feerate was used so I wouldn't want to report a potentially inaccurate feerate. Added a comment about this as well. Since testmempoolaccept doesn't allow already-in-mempool transactions, it should always have this for successful transactions.

Base != effective is no longer a gate. Edited the optional bool for these results to reflect the status. Some tests needed to be changed (just dropping the effective-* results there because they aren't relevant in those tests and comparing feerates isn't handled properly in assert_equal).

achow101 commented at 8:40 PM on January 9, 2023: member

A few not-necessarily blocking comments, but would like to get some rationale for some decisions.

[validation] return wtxids of other transactions whose fees were used d6c7b78ef2

[rpc] return effective-feerate in testmempoolaccept and submitpackage 1691eaa818

[rpc] return effective-includes in testmempoolaccept and submitpackage 601bac88cb

[validation] remove PackageMempoolAcceptResult::m_package_feerate

This value creates an extremely confusing interface as its existence is
dependent upon implementation details (whether something was submitted
on its own, etc). MempoolAcceptResult::m_effective_feerate is much more
helpful, as it always exists for submitted transactions.

5eab397b98

[doc] release note effective-feerate and effective-includes RPC results

No release note for submitpackage because it is regtest-only.

da484bc738

[refactor] rename variables in AcceptPackage for clarity dae81e01e8

[validation] return MempoolAcceptResult for every tx on PCKG_TX failure

This makes the interface more predictable and useful. The caller
understands one or more transactions failed, and can learn what happened
with each transaction. We already have this information, so we might as
well return it.

It doesn't make sense to do this for other PackageValidationResult
values because:
- PCKG_RESULT_UNSET: this means everything succeeded, so the individual
  failures are no longer accurate.
- PCKG_MEMPOOL_ERROR: something went wrong with the mempool logic;
  transaction failures might not be meaningful.
- PCKG_POLICY: this means something was wrong with the package as a
  whole. The caller should use the PackageValidationState to find the
  error, rather than looking at individual MempoolAcceptResults.

264f9ef17f

glozow force-pushed on Jan 10, 2023

glozow commented at 11:20 AM on January 10, 2023: member

Thanks @achow101, agree with your comments, addressed.

in src/rpc/mempool.cpp:882 in 264f9ef17f

 877 | @@ -864,6 +878,17 @@ static RPCHelpMan submitpackage()
 878 |                      result_inner.pushKV("vsize", int64_t{it->second.m_vsize.value()});
 879 |                      UniValue fees(UniValue::VOBJ);
 880 |                      fees.pushKV("base", ValueFromAmount(it->second.m_base_fees.value()));
 881 | +                    if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
 882 | +                        // Effective feerate is not provided for MEMPOOL_ENTRY transactions even

instagibbs commented at 3:22 PM on January 10, 2023:

                        // Effective feerate is not provided for MEMPOOL_ENTRY(already-in-mempool) transactions even

instagibbs commented at 3:24 PM on January 10, 2023: member

reACK https://github.com/bitcoin/bitcoin/pull/26646/commits/264f9ef17f650035882d24083fb419845942a3ac

instagibbs commented at 3:25 PM on January 10, 2023: member

micro nit

achow101 commented at 10:37 PM on January 10, 2023: member

ACK 264f9ef17f650035882d24083fb419845942a3ac

naumenkogs commented at 7:36 AM on January 11, 2023: member

reACK 264f9ef17f650035882d24083fb419845942a3ac

glozow merged this on Jan 11, 2023

glozow closed this on Jan 11, 2023

glozow deleted the branch on Jan 11, 2023

sidhujag referenced this in commit f5d8392ffe on Jan 11, 2023

maflcko referenced this in commit bd74004532 on Jan 12, 2023

sidhujag referenced this in commit a2e2fda9b5 on Jan 12, 2023

bitcoin locked this on Jan 11, 2024