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

glozow · 2023-11-03T16:31:06Z

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.

DrahtBot · 2023-11-03T16:31:10Z

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 · 2023-11-03T16:33:59Z

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 · 2023-11-03T16:35:36Z

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 👍

instagibbs · 2023-11-03T17:28:33Z

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

instagibbs

LGTM with a few suggestions 6b0dc66

src/validation.cpp

src/validation.h

src/consensus/validation.h

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.

instagibbs · 2023-11-06T14:49:25Z

ACK 0150e86

suggested changes plus TX_RECONSIDERABLE renaming which makes sense

murchandamus

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.

src/test/fuzz/tx_pool.cpp

src/test/util/txmempool.cpp

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

…e 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.

glozow · 2023-11-07T12:03:15Z

Addressed #28785 (review) and fixed the ci error

instagibbs · 2023-11-07T12:14:24Z

reACK 1147e00

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

achow101 · 2023-11-07T21:36:27Z

ACK 1147e00

murchandamus

reACK 1147e00

murchandamus · 2023-11-07T21:56:05Z

src/test/txpackage_tests.cpp

-    BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash());
+    Package package_parent_child{tx_parent, tx_child};
+    const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_parent_child, /*test_accept=*/true);
+    if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_parent_child, result_parent_child, /*expect_valid=*/true, nullptr)}) {


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.

ismaelsadeeq

ACK 1147e00

looks good to me.

Summary: Deduplicate code that exists in both tx processing and ProcessOrphanTx. Additionally, this can be reused in a function that handles multiple MempoolAcceptResults from package submission. refactor: consolidate MempoolAcceptResult processing ``` Every time we try to ProcessTransaction (i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached in m_recent_rejects so we don't try to download/validate it again. There are 2 current places and at least 1 future place where we need to process MempoolAcceptResult [...] Consolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR. ``` Backport of [[bitcoin/bitcoin#29619 | core#29619]]. Refactor only, there is no change in behavior. Backport note: the TX_UNKNOWN validation state has not been backported and is skipped in the net_processing check for invalid txs. To this date this enum value is never used upstream so we're not missing anything (introduced in [[bitcoin/bitcoin#28785 | core#28785]]). Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D16346

…es and skipped transactions Summary: ``` - 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. ``` Backport of [[bitcoin/bitcoin#28785 | core#28785]], [[bitcoin/bitcoin#28764 | core#28764]] and [[bitcoin/bitcoin#28788 | core#28788]] (last too are only the test function). Test Plan: ninja all check-all Reviewers: #bitcoin_abc, roqqit Reviewed By: roqqit Differential Revision: https://reviews.bitcoinabc.org/D16439

Summary: Deduplicate code that exists in both tx processing and ProcessOrphanTx. Additionally, this can be reused in a function that handles multiple MempoolAcceptResults from package submission. refactor: consolidate MempoolAcceptResult processing ``` Every time we try to ProcessTransaction (i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached in m_recent_rejects so we don't try to download/validate it again. There are 2 current places and at least 1 future place where we need to process MempoolAcceptResult [...] Consolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR. ``` Backport of [[bitcoin/bitcoin#29619 | core#29619]]. Refactor only, there is no change in behavior. Backport note: the TX_UNKNOWN validation state has not been backported and is skipped in the net_processing check for invalid txs. To this date this enum value is never used upstream so we're not missing anything (introduced in [[bitcoin/bitcoin#28785 | core#28785]]). Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D16346

…es and skipped transactions Summary: ``` - 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. ``` Backport of [[bitcoin/bitcoin#28785 | core#28785]], [[bitcoin/bitcoin#28764 | core#28764]] and [[bitcoin/bitcoin#28788 | core#28788]] (last too are only the test function). Test Plan: ninja all check-all Reviewers: #bitcoin_abc, roqqit Reviewed By: roqqit Differential Revision: https://reviews.bitcoinabc.org/D16439

glozow added the Validation label Nov 3, 2023

glozow mentioned this pull request Nov 3, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

glozow mentioned this pull request Nov 3, 2023

Package Relay Project Tracking #27463

Open

57 tasks

glozow force-pushed the 2023-11-txresult-fee branch from 7c92623 to 6b0dc66 Compare November 3, 2023 17:34

DrahtBot added the CI failed label Nov 3, 2023

instagibbs reviewed Nov 3, 2023

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

This was referenced Nov 3, 2023

Bugfix: Package relay / bytespersigop checks #28345

Closed

Enforce incentive compatibility for all RBF replacements #26451

Closed

glozow force-pushed the 2023-11-txresult-fee branch 2 times, most recently from a291c4a to 0f65d69 Compare November 6, 2023 14:12

[refactor] use Wtxid for m_wtxids_fee_calculations

5c786a0

glozow force-pushed the 2023-11-txresult-fee branch from 0f65d69 to e9f9772 Compare November 6, 2023 14:35

murchandamus reviewed Nov 6, 2023

View reviewed changes

src/consensus/validation.h Outdated Show resolved Hide resolved

glozow force-pushed the 2023-11-txresult-fee branch from e9f9772 to 0150e86 Compare November 6, 2023 14:45

murchandamus reviewed Nov 6, 2023

View reviewed changes

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

src/test/util/txmempool.cpp Outdated Show resolved Hide resolved

glozow force-pushed the 2023-11-txresult-fee branch 2 times, most recently from 2dd76f2 to 3979f1a Compare November 7, 2023 10:16

glozow added 2 commits November 7, 2023 11:23

[test] use CheckPackageMempoolAcceptResult in previous tests

10dd9f2

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

DrahtBot removed the CI failed label Nov 7, 2023

DrahtBot requested a review from murchandamus November 7, 2023 12:14

murchandamus reviewed Nov 7, 2023

View reviewed changes

ismaelsadeeq approved these changes Nov 8, 2023

View reviewed changes

fanquake merged commit d690f89 into bitcoin:master Nov 8, 2023

glozow deleted the 2023-11-txresult-fee branch November 9, 2023 09:36

bitcoin locked and limited conversation to collaborators Nov 8, 2024

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

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

Uh oh!

Conversation

glozow commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

instagibbs commented Nov 3, 2023

Uh oh!

glozow commented Nov 3, 2023

Uh oh!

instagibbs commented Nov 3, 2023

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Nov 6, 2023

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow commented Nov 7, 2023

Uh oh!

instagibbs commented Nov 7, 2023

Uh oh!

achow101 commented Nov 7, 2023

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

glozow commented Nov 3, 2023 •

edited

Loading

DrahtBot commented Nov 3, 2023 •

edited

Loading