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

glozow · 2022-12-06T10:43:42Z

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 (comment)!

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.

DrahtBot · 2022-12-06T10:43:45Z

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.

instagibbs

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

src/validation.cpp

instagibbs

ACK c5ca075 naming nit left aside

src/validation.cpp

sdaftuar · 2022-12-08T21:37:58Z

src/validation.cpp

+        /** If we're doing package validation (i.e. m_package_feerates=true), the "effective"
+         * package feerate of this transaction, which may include fee and vsize of its ancestors
+         * and/or descendants. */
+        CFeeRate m_package_feerate{0};


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.

Does this comment equally apply to package submission in master?

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

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).

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

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.

@sdaftuar comfortable with resolving for this PR?

stickies-v

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

A few more general comments:

commit message in b00bebb 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: a723541 could be split in two commits: one for rename, and one for the actual change

src/test/txpackage_tests.cpp

src/validation.cpp

src/rpc/mempool.cpp

test/functional/rpc_packages.py

src/validation.cpp

instagibbs

reACK 0d383a6

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

src/validation.cpp

src/rpc/mempool.cpp

vincenzopalazzo

ACK 0d383a6

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 :)

vincenzopalazzo

reACK 48f81f2

instagibbs · 2022-12-19T18:26:15Z

ACK 48f81f2

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

stickies-v

Approach ACK 48f81f2 - 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.

src/validation.cpp

src/rpc/mempool.cpp

naumenkogs · 2022-12-21T10:06:39Z

ACK 48f81f2

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

src/validation.cpp

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

…inputs failure

glozow · 2023-01-06T17:57:50Z

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 · 2023-01-09T07:42:16Z

reACK ada89cb

instagibbs

reACK ada89cb

src/validation.cpp

achow101

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

achow101 · 2023-01-09T19:47:49Z

src/validation.cpp

+            Assume(!args.m_package_feerates);
+            const std::vector<uint256> single_wtxid{ws.m_ptx->GetWitnessHash()};


In 92d789b "[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.

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.

achow101 · 2023-01-09T19:58:07Z

test/functional/rpc_packages.py

-            tx_result = submitpackage_result["tx-results"][tx.getwtxid()]
-            assert_equal(tx_result, {
-                "txid": package_txn["txid"],
-                "vsize": tx.get_vsize(),
-                "fees": {
-                    "base": DEFAULT_FEE,
-                }
-            })
+            wtxid = tx.getwtxid()
+            assert wtxid in submitpackage_result["tx-results"]
+            tx_result = submitpackage_result["tx-results"][wtxid]
+            assert_equal(tx_result["txid"], tx.rehash())
+            assert_equal(tx_result["vsize"], tx.get_vsize())
+            assert_equal(tx_result["fees"]["base"], DEFAULT_FEE)


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

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

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

achow101 · 2023-01-09T20:14:16Z

src/rpc/mempool.cpp

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


In 57b56b7 "[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.

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).

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.

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

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.

glozow

Thanks @achow101, agree with your comments, addressed.

glozow · 2023-01-10T10:40:41Z

src/rpc/mempool.cpp

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


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).

glozow · 2023-01-10T10:44:57Z

test/functional/rpc_packages.py

-            tx_result = submitpackage_result["tx-results"][tx.getwtxid()]
-            assert_equal(tx_result, {
-                "txid": package_txn["txid"],
-                "vsize": tx.get_vsize(),
-                "fees": {
-                    "base": DEFAULT_FEE,
-                }
-            })
+            wtxid = tx.getwtxid()
+            assert wtxid in submitpackage_result["tx-results"]
+            tx_result = submitpackage_result["tx-results"][wtxid]
+            assert_equal(tx_result["txid"], tx.rehash())
+            assert_equal(tx_result["vsize"], tx.get_vsize())
+            assert_equal(tx_result["fees"]["base"], DEFAULT_FEE)


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

glozow · 2023-01-10T10:46:14Z

src/validation.cpp

+            Assume(!args.m_package_feerates);
+            const std::vector<uint256> single_wtxid{ws.m_ptx->GetWitnessHash()};


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.

instagibbs · 2023-01-10T15:24:54Z

reACK 264f9ef

instagibbs

micro nit

instagibbs · 2023-01-10T15:22:57Z

src/rpc/mempool.cpp

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


Suggested change

// Effective feerate is not provided for MEMPOOL_ENTRY transactions even

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

achow101 · 2023-01-10T22:37:21Z

ACK 264f9ef

naumenkogs · 2023-01-11T07:36:17Z

reACK 264f9ef

…lAcceptResult 264f9ef [validation] return MempoolAcceptResult for every tx on PCKG_TX failure (glozow) dae81e0 [refactor] rename variables in AcceptPackage for clarity (glozow) da484bc [doc] release note effective-feerate and effective-includes RPC results (glozow) 5eab397 [validation] remove PackageMempoolAcceptResult::m_package_feerate (glozow) 601bac8 [rpc] return effective-includes in testmempoolaccept and submitpackage (glozow) 1691eaa [rpc] return effective-feerate in testmempoolaccept and submitpackage (glozow) d6c7b78 [validation] return wtxids of other transactions whose fees were used (glozow) 1605886 [validation] return effective feerate from mempool validation (glozow) 5d35b4a [test] package validation quits early due to non-policy, non-missing-inputs failure (glozow) be2e4d9 [validation] when quitting early in AcceptPackage, set package_state and tx result (glozow) Pull request description: 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 bitcoin#25038 (comment)! 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`. ACKs for top commit: instagibbs: reACK bitcoin@264f9ef achow101: ACK 264f9ef naumenkogs: reACK 264f9ef Tree-SHA512: ce7fd9927a80030317cc6157822596e85a540feff5dbf5eea7c62da2eb50c917cdddc9da1e2ff62cc18b98b27d360151811546bd9d498859679a04bbee090837

c28d461 doc: move errant release note to doc/ (fanquake) Pull request description: Release note from #26646 should be in doc/. Top commit has no ACKs. Tree-SHA512: 741323c0bc526163d65e441c9e677ee3613ed0c55b1880b99fb63b0f3306aeca0cad83dadef05c23def8d0fc6a224956b27ae1bf4b1ff980a1be2d31feef7e3d

c28d461 doc: move errant release note to doc/ (fanquake) Pull request description: Release note from bitcoin#26646 should be in doc/. Top commit has no ACKs. Tree-SHA512: 741323c0bc526163d65e441c9e677ee3613ed0c55b1880b99fb63b0f3306aeca0cad83dadef05c23def8d0fc6a224956b27ae1bf4b1ff980a1be2d31feef7e3d

glozow added Bug Validation labels Dec 6, 2022

glozow mentioned this pull request Dec 6, 2022

policy: nVersion=3 and Package RBF #25038

Closed

instagibbs reviewed Dec 6, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

glozow force-pushed the package-single-tx-result branch from 139e0e1 to c5ca075 Compare December 7, 2022 12:34

instagibbs reviewed Dec 7, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

fanquake requested a review from sdaftuar December 7, 2022 14:43

sdaftuar reviewed Dec 8, 2022

View reviewed changes

stickies-v reviewed Dec 12, 2022

View reviewed changes

glozow force-pushed the package-single-tx-result branch from c5ca075 to 0d383a6 Compare December 14, 2022 12:40

instagibbs approved these changes Dec 14, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

glozow mentioned this pull request Dec 16, 2022

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

Closed

vincenzopalazzo reviewed Dec 17, 2022

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Dec 17, 2022

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

vincenzopalazzo approved these changes Dec 17, 2022

View reviewed changes

glozow force-pushed the package-single-tx-result branch from 0d383a6 to 48f81f2 Compare December 19, 2022 16:37

vincenzopalazzo approved these changes Dec 19, 2022

View reviewed changes

fanquake requested a review from sdaftuar December 20, 2022 08:22

stickies-v reviewed Dec 20, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 22, 2022

refactor: Avoid UniValue copies #25429

Closed

achow101 reviewed Jan 4, 2023

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

glozow and others added 3 commits January 6, 2023 17:37

[test] package validation quits early due to non-policy, non-missing-…

5d35b4a

…inputs failure

[validation] return effective feerate from mempool validation

1605886

glozow force-pushed the package-single-tx-result branch from 48f81f2 to ada89cb Compare January 6, 2023 17:55

fanquake requested a review from instagibbs January 9, 2023 09:58

instagibbs approved these changes Jan 9, 2023

View reviewed changes

src/validation.cpp Show resolved Hide resolved

achow101 reviewed Jan 9, 2023

View reviewed changes

glozow added 7 commits January 10, 2023 10:36

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

d6c7b78

[rpc] return effective-feerate in testmempoolaccept and submitpackage

1691eaa

[rpc] return effective-includes in testmempoolaccept and submitpackage

601bac8

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

da484bc

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

[refactor] rename variables in AcceptPackage for clarity

dae81e0

glozow force-pushed the package-single-tx-result branch from ada89cb to 264f9ef Compare January 10, 2023 11:11

glozow commented Jan 10, 2023

View reviewed changes

instagibbs reviewed Jan 10, 2023

View reviewed changes

glozow merged commit 2600257 into bitcoin:master Jan 11, 2023

glozow deleted the package-single-tx-result branch January 11, 2023 13:26

fanquake mentioned this pull request Jan 12, 2023

doc: move errant release note to doc/ #26877

Merged

glozow mentioned this pull request Sep 27, 2023

Package Relay Project Tracking #27463

Open

54 tasks

bitcoin locked and limited conversation to collaborators Jan 11, 2024

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

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

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

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

Conversation

glozow commented Dec 6, 2022 • edited Loading

DrahtBot commented Dec 6, 2022 • edited Loading

Reviews

Conflicts

instagibbs left a comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment • edited Loading

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

instagibbs commented Dec 19, 2022

stickies-v left a comment

Choose a reason for hiding this comment

naumenkogs commented Dec 21, 2022

glozow commented Jan 6, 2023

naumenkogs commented Jan 9, 2023

instagibbs left a comment

Choose a reason for hiding this comment

achow101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Jan 10, 2023

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Jan 10, 2023

naumenkogs commented Jan 11, 2023

glozow commented Dec 6, 2022 •

edited

Loading

DrahtBot commented Dec 6, 2022 •

edited

Loading

vincenzopalazzo left a comment •

edited

Loading