validation: return more helpful results for reconsiderable fee failures and skipped transactions #28785
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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 👍 |
|
think you'll have to touch |
glozow
force-pushed
the
2023-11-txresult-fee
branch
from
7c92623
to
6b0dc66
Compare
glozow
force-pushed
the
2023-11-txresult-fee
branch
2 times, most recently
from
a291c4a
to
0f65d69
Compare
glozow
force-pushed
the
2023-11-txresult-fee
branch
from
0f65d69
to
e9f9772
Compare
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.
glozow
force-pushed
the
2023-11-txresult-fee
branch
from
e9f9772
to
0150e86
Compare
|
ACK 0150e86 suggested changes plus |
There was a problem hiding this comment.
crACK 0150e86 with grain of salt:
I’m pretty new to reviewing mempool code:
- The introduction of an explicit class for
Wtxidmakes sense to me - The introduction of
TX_RECONSIDERABLEmakes 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_RECONSIDERABLEand 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
the
2023-11-txresult-fee
branch
2 times, most recently
from
2dd76f2
to
3979f1a
Compare
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.
|
Addressed #28785 (review) and fixed the ci error |
|
reACK 1147e00 took reconsiderable variable renames, tests using CheckPackageMempoolAcceptResult more, unsure why ci was failing though :) |
|
ACK 1147e00 |
| 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)}) { |
There was a problem hiding this comment.
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.
Split off from #26711 (suggested in #26711 (comment)). This is part of #27463.
TX_RECONSIDERABLEhelps us encode transactions who have failed fee checks that can be bypassed using package validation. This is distinguished fromTX_MEMPOOL_POLICYso 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 inm_recent_rejects.TX_UNKNOWNhelps 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)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).CheckPackageMempoolAcceptResultfor existing package validation tests. This increases test coverage and helps test the changes made in this PR.