validation: return more helpful results for reconsiderable fee failures and skipped transactions

glozow commented at 4:31 pm on November 3, 2023: member

Split off from #26711 (suggested in #26711 (comment)). This is part of #27463.

Add 2 new TxValidationResults
- TX_RECONSIDERABLE helps us encode transactions who have failed fee checks that can be bypassed using package validation. This is distinguished from TX_MEMPOOL_POLICY so that we re-validate a transaction if and only if it is eligible for package CPFP. In the future, we will have a separate cache for reconsiderable rejects so these transactions don’t go in m_recent_rejects.
- TX_UNKNOWN helps us communicate that we aborted package validation and didn’t finish looking at this transaction: it’s not valid but it’s also not invalid (i.e. don’t cache it as a rejected tx)
Return effective feerate and the wtxids of transactions used to calculate that effective feerate when the error is TX_SINGLE_FAILURE. Previously, we would only provide this information if the transaction passed. Now that we have package validation, it’s much more helpful to the caller to know how the failing feerate was calculated. This can also be used to improve our submitpackage RPC result (which is currently a bit unhelpful when things fail).
Use the newly added CheckPackageMempoolAcceptResult for existing package validation tests. This increases test coverage and helps test the changes made in this PR.

glozow added the label Validation on Nov 3, 2023

DrahtBot commented at 4:31 pm on November 3, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, achow101, murchandamus, ismaelsadeeq

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:

#28345 (Bugfix: Package relay / bytespersigop checks by luke-jr)
#26451 (Enforce incentive compatibility for all RBF replacements by sdaftuar)

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.

instagibbs commented at 4:33 pm on November 3, 2023: member

so that we re-validate a transaction if and only if it is eligible for package CPFP.

and in the future we’ll use this for making sure we’d re-request this transaction as part of another package at p2p, correct?

glozow commented at 4:35 pm on November 3, 2023: member

and in the future we’ll use this for making sure we’d re-request this transaction as part of another package at p2p, correct?

Correct :+1:

instagibbs commented at 5:28 pm on November 3, 2023: member

think you’ll have to touch CheckATMPInvariants in the tx_pool fuzz test to add TX_SINGLE_FAILURE case

glozow force-pushed on Nov 3, 2023

DrahtBot added the label CI failed on Nov 3, 2023

in src/validation.cpp:1151 in ba9e2ae9e0 outdated

1146@@ -1139,7 +1147,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
1147     if (!args.m_package_submission && !bypass_limits) {
1148         LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
1149         if (!m_pool.exists(GenTxid::Txid(hash)))
1150-            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full");
1151+            return state.Invalid(TxValidationResult::TX_SINGLE_FAILURE, "mempool full");

instagibbs commented at 7:32 pm on November 3, 2023:

Might make sense to add a comment to the effect that we don’t want to reject fetching this same tx in a future package

glozow commented at 1:56 pm on November 6, 2023:

Added comment

in src/validation.cpp:1240 in 6b0dc66988 outdated

1234@@ -1227,7 +1235,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
1235 
1236     Workspace ws(ptx);
1237 
1238-    if (!PreChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state);
1239+    if (!PreChecks(args, ws)) {
1240+        if (ws.m_state.GetResult() == TxValidationResult::TX_SINGLE_FAILURE) {
1241+            // Failed for fee reasons. Provide the effective feerate and which txns were included.

instagibbs commented at 7:49 pm on November 3, 2023:

0            // Failed for fee reasons. Provide the effective feerate and which txn was included.

glozow commented at 1:56 pm on November 6, 2023:

Taken

in src/validation.h:181 in 6b0dc66988 outdated

179         return MempoolAcceptResult(state);
180     }
181 
182+    static MempoolAcceptResult FeeFailure(TxValidationState state,
183+                                          CFeeRate effective_feerate,
184+                                          const std::vector<uint256>& wtxids_fee_calculations) {

instagibbs commented at 7:50 pm on November 3, 2023:

Wtxid? :point_right: :point_left:

glozow commented at 1:56 pm on November 6, 2023:

Changed :+1:

instagibbs commented at 7:52 pm on November 3, 2023: member

LGTM with a few suggestions https://github.com/bitcoin/bitcoin/pull/28785/commits/6b0dc669886c07a81fdfbf2d1a71a77ca3bac1c3

glozow force-pushed on Nov 6, 2023

[refactor] use Wtxid for m_wtxids_fee_calculations 5c786a026a

in src/consensus/validation.h:57 in 8bad2718da outdated

53@@ -54,6 +54,8 @@ enum class TxValidationResult {
54     TX_CONFLICT,
55     TX_MEMPOOL_POLICY,        //!< violated mempool's fee/size/descendant/RBF/etc limits
56     TX_NO_MEMPOOL,            //!< this node does not have a mempool so can't validate the transaction
57+    TX_SINGLE_FAILURE,        //!< fee was insufficient to meet some policy, but can change if submitted in a (different) package

murchandamus commented at 2:32 pm on November 6, 2023:

It was not obvious to me what the motivation for SINGLE in the constant name here is. After discussing with @glozow, maybe:

0    TX_RECONSIDERABLE,        //!< mining score was insufficient to meet some policy, but transaction might be acceptable if submitted in a (different) package

glozow commented at 2:47 pm on November 6, 2023:

Yeah renamed, I think it’ll help make the connection with m_reconsiderable_rejects filter later as well.

glozow force-pushed on Nov 6, 2023

[validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWN

With package validation rules, transactions that fail individually may
sometimes be eligible for reconsideration if submitted as part of a
(different) package. For now, that includes trasactions that failed for
being too low feerate.  Add a new TxValidationResult type to distinguish
these failures from others.  In the next commits, we will abort package
validation if a tx fails for any other reason. In the future, we will
also decide whether to cache failures in recent_rejects based on this
result (we won't want to reject a package containing a transaction that
was rejected previously for being low feerate).

Package validation also sometimes elects to skip some transactions when
it knows the package will not be submitted in order to quit sooner. Add
a result to specify this situation; we also don't want to cache these
as rejections.

3979f1afcb

glozow force-pushed on Nov 6, 2023

instagibbs commented at 2:49 pm on November 6, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/28785/commits/0150e860863f95b18448e7b67f5db27017660670

suggested changes plus TX_RECONSIDERABLE renaming which makes sense

in src/test/fuzz/tx_pool.cpp:158 in 0150e86086 outdated

153@@ -154,12 +154,15 @@ void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, b
154         // It may be already in the mempool since in ATMP cases we don't set MEMPOOL_ENTRY or DIFFERENT_WITNESS
155         Assert(!res.m_state.IsValid());
156         Assert(res.m_state.IsInvalid());
157+
158+        const bool is_single_failure{res.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE};

murchandamus commented at 9:16 pm on November 6, 2023:

After the name change, perhaps:

0        const bool is_reconsiderable{res.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE};

ismaelsadeeq commented at 9:37 pm on November 6, 2023:

PR description also needs to be updated to TX_RECONSIDERABLE

glozow commented at 11:27 am on November 7, 2023:

taken

in src/test/util/txmempool.cpp:93 in 0150e86086 outdated

88@@ -89,11 +89,14 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
89         }
90 
91         // m_effective_feerate and m_wtxids_fee_calculations should exist iff the result was valid
92-        if (atmp_result.m_effective_feerate.has_value() != valid) {
93+        // or if the failure was TX_RECONSIDERABLE
94+        const bool valid_or_single_failure{atmp_result.m_result_type == MempoolAcceptResult::ResultType::VALID ||

murchandamus commented at 9:20 pm on November 6, 2023:

Perhaps:

0        const bool valid_or_reconsiderable{atmp_result.m_result_type == MempoolAcceptResult::ResultType::VALID ||

glozow commented at 11:27 am on November 7, 2023:

taken

murchandamus commented at 9:35 pm on November 6, 2023: contributor

crACK 0150e860863f95b18448e7b67f5db27017660670 with grain of salt:

I’m pretty new to reviewing mempool code:

The introduction of an explicit class for Wtxid makes sense to me
The introduction of TX_RECONSIDERABLE makes sense to me in the context of transactions being tested in the context of packages where they might be resubmitted with a higher resulting mining score
It makes sense to me that some mempool acceptance tests would now fail with TX_RECONSIDERABLE and the ones that do look reasonable to me
The code changes look reasonable small and targeted to the context

I cannot tell whether the changes are complete and whether they are being comprehensively tested.

glozow force-pushed on Nov 7, 2023

[test] use CheckPackageMempoolAcceptResult in previous tests

Increases test coverage (check every result field) and makes it easier
to test the changes in the next commit.

10dd9f2441

[validation] change package-fee-too-low, return wtxid(s) and effective feerate

With subpackage evaluation and de-duplication, it's not always the
entire package that is used in CheckFeerate. To be more helpful to the
caller, specify which transactions were included in the evaluation and
what the feerate was.

Instead of PCKG_POLICY (which is supposed to be for package-wide
errors), use PCKG_TX.

1147e00e59

glozow commented at 12:03 pm on November 7, 2023: member

Addressed #28785#pullrequestreview-1716255880 and fixed the ci error

DrahtBot removed the label CI failed on Nov 7, 2023

instagibbs commented at 12:14 pm on November 7, 2023: member

reACK https://github.com/bitcoin/bitcoin/pull/28785/commits/1147e00e59e47f27024ec96629993c66a3ce4ef0

took reconsiderable variable renames, tests using CheckPackageMempoolAcceptResult more, unsure why ci was failing though :)

DrahtBot requested review from murchandamus on Nov 7, 2023

achow101 commented at 9:36 pm on November 7, 2023: member

ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0

in src/test/txpackage_tests.cpp:138 in 1147e00e59

151-    BOOST_CHECK(it_child->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child)) == COIN);
152-    BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1);
153-    BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash());
154+    Package package_parent_child{tx_parent, tx_child};
155+    const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_parent_child, /*test_accept=*/true);
156+    if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_parent_child, result_parent_child, /*expect_valid=*/true, nullptr)}) {

murchandamus commented at 9:56 pm on November 7, 2023:

I was very confused by the changes in this vein, until I realized that CheckPackageMempoolAcceptResult(…) returns an error in case of a CPMA failure, but returns nothing in the case of a success. Per the name of this function my initial expectation was that we would expect a truthy return value in case of successful acceptance.

murchandamus commented at 10:04 pm on November 7, 2023: contributor

reACK 1147e00e59e47f27024ec96629993c66a3ce4ef0

ismaelsadeeq approved

ismaelsadeeq commented at 8:17 am on November 8, 2023: member

ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0

looks good to me.

fanquake merged this on Nov 8, 2023

fanquake closed this on Nov 8, 2023

glozow deleted the branch on Nov 9, 2023

validation: return more helpful results for reconsiderable fee failures and skipped transactions #28785

Code Coverage

Reviews

Conflicts