Use util::Result in for calculating mempool ancestors #26289

stickies-v · 2022-10-10T20:55:08Z

Upon reviewing the documentation for CTxMemPool::CalculateMemPoolAncestors, I noticed setAncestors was meant to be an out parameter but actually is an in,out parameter, as can be observed by adding assert(setAncestors.empty()); as the first line in the function and running make check. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear.

Unexpected behaviour

This behaviour occurs only in the package acceptance path, currently only triggered by testmempoolaccept and submitpackage RPCs.

In MemPoolAccept::AcceptMultipleTransactions(), we first call PreChecks() and then SubmitPackage() with the same Workspace ws reference. PreChecks leaves ws.m_ancestors in a potentially non-empty state, before it is passed on to MemPoolAccept::SubmitPackage. SubmitPackage is the only place where setAncestors isn't guaranteed to be empty before calling CalculateMemPoolAncestors. The most straightforward fix is to just forcefully clear setAncestors at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit.

Improvements

Return value instead of out-parameters

This PR updates the function signatures for CTxMemPool::CalculateMemPoolAncestors and CTxMemPool::CalculateAncestorsAndCheckLimits to use a util::Result return type and eliminate both the setAncestors in,out-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit.

Observability

There are 7 instances where we currently call CalculateMemPoolAncestors without actually checking if the function succeeded because we assume that it can't fail, such as in miner.cpp. This PR adds a new wrapper AssumeCalculateMemPoolAncestors function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here.

DrahtBot · 2022-10-10T23:50:52Z

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	w0xlt, glozow, furszy, aureleoules, achow101
Concept ACK	theStack
Stale ACK	willcl-ark

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:

#26531 (mempool: Add mempool tracepoints by virtu)

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.

w0xlt

Approach ACK

glozow

Concept ACK, definitely prefer this over out-params.

theStack · 2022-10-20T22:08:38Z

Concept ACK

aureleoules

ACK a425537fbfc20d87c325b5ae2f06f2597a31cf0e - LGTM and no behavior change

src/node/miner.cpp

luke-jr · 2022-10-25T15:11:27Z

If there's an actual bug, it should be fixed before/independently of the refactor (and only that fix backported).

stickies-v · 2022-10-25T19:50:53Z

Force pushed to address review comments.

If there's an actual bug, it should be fixed before/independently of the refactor (and only that fix backported)

I added c25c6a6

Most of the changes address furszy's comments. In summary: ancestors/descendant limits unexpectedly getting hit no longer result in an assertion error (unless on debug builds) but in an error log message instead to avoid the node from crashing in case of a (non-fatal) unexpected error.

stickies-v · 2022-10-25T20:48:16Z

Marking as draft until I've resolved the CI issues.

stickies-v · 2022-10-26T12:36:17Z

PR is now ready for re-review - CI issues fixed. Looks like the argument forwarding was causing issues on some platforms (ARM, Win64) - so I've just updated it to not use forwarding, since debugging it was going too slow and this approach is clean enough.

aureleoules

reACK 2f832f0c45aae3b51353db72691b4bc6450a7e9a

src/txmempool.cpp

src/node/miner.cpp

JeremyRubin · 2022-10-28T16:00:24Z

Would there be any merit to an alternative approach of logging how many current ancestors were passed in, and then subtracting that for the invariant check? That way you allocate less memory in the package code?

glozow · 2022-11-01T16:50:59Z

Would there be any merit to an alternative approach of logging how many current ancestors were passed in, and then subtracting that for the invariant check? That way you allocate less memory in the package code?

Having trouble understanding the question, can you elaborate on where the subtraction would be and which memory allocation you're referring to?

stickies-v · 2022-11-23T20:15:13Z

where the setAncestors is consumed -- afaict, it's eventually transformed into just a count

For example in CTxMemPool::PrioritiseTransaction and CTxMemPool::UpdateForRemoveFromMempool we actually use the calculated ancestors in a way that doesn't seem trivial to refactor out.

glozow · 2022-11-29T12:20:20Z

@JeremyRubin

I think I was mostly observing that the ancestors tracking is mostly being used for limits checking here, so there is an opportunity to (I think) not actually build a ancestor set and just use epochs to count the number of distinct ancestors directly where the setAncestors is consumed

For example in CTxMemPool::PrioritiseTransaction and CTxMemPool::UpdateForRemoveFromMempool we actually use the calculated ancestors in a way that doesn't seem trivial to refactor out.

Agree with @stickies-v, we definitely require the actual entries themselves at several CalculateMemPoolAncestors callsites externally as well, see

bitcoin/src/policy/rbf.cpp

Lines 42 to 48 in a035b6a

    
           pool.CalculateMemPoolAncestors(entry, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false); 
        
           for (CTxMemPool::txiter it : ancestors) { 
        
               if (SignalsOptInRBF(it->GetTx())) { 
        
                   return RBFTransactionState::REPLACEABLE_BIP125; 
        
               } 
        
           }

And some only require that the limits are checked - we could split those into 2 different functions e.g. GetAncestorEntries() and CheckAncestorDescendantLimits() in the future, but in any case this refactor makes sense for a GetAncestorEntries() interface.

willcl-ark

ACK 1f89af219a89da155d5302ca4fdc4f8f1f8612aa

I am unsure about the benefit of the first commit which is later removed, but it seems this might still be up for discussion.

The larger refactor related to removing out params and using Result looks good.

src/txmempool.cpp

src/validation.cpp

Avoid using setAncestors outparameter, simplify function signatures and avoid creating unused dummy strings.

stickies-v

Force pushed to remove the bugfix commit and address review comments re instantiating invalid_too_long making the state invalid even when the failure path isn't hit

src/validation.cpp

src/txmempool.cpp

aureleoules · 2022-12-12T18:44:53Z

CI is red @stickies-v

stickies-v · 2022-12-12T19:27:36Z

CI is red @stickies-v

Sorry, fixed now by capturing the original error message in a variable, so we don't show the error message from trying the cpfp carve-out.

src/node/miner.cpp

Avoid using setAncestors outparameter, simplify function signatures and avoid creating unused dummy strings.

There are quite a few places that assume CalculateMemPoolAncestors will return a value without raising an error. This helper function adds logging (and Assume for debug builds) that ensures robustness but increases visibility in case of unexpected failures

When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds ancestor/descendant limits even though we expect no limits to be applied), add an error log entry for increased visibility. For debug builds, the application will even halt completely since this is not supposed to happen.

stickies-v · 2022-12-13T16:06:03Z

Force pushed to avoid set copy operations when calling value_or() on results, see this comment for more background

w0xlt

ACK 47c4b1f

glozow

ACK 47c4b1f
code review to verify no change in behavior, and bench via ComplexMemPool and #26695 suggests no change in performance

src/validation.cpp

furszy

light code review ACK 47c4b1f

left two comments, nothing blocking.

furszy · 2022-12-30T21:47:07Z

src/txmempool.cpp

+CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors(
+    std::string_view calling_fn_name,
+    const CTxMemPoolEntry &entry,
+    const Limits& limits,


Non-blocking point:

Could remove the limits arg. All the callers use CTxMemPool::Limits::NoLimits() and the only way CalculateMemPoolAncestors fail is due a limit restriction.

furszy · 2022-12-30T21:52:19Z

src/txmempool.cpp

+CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors(
+    std::string_view calling_fn_name,


Non-blocking nit:

This should be a helper/util static function rather than a class method. It's weird for a class method to receive the caller's function name just to log it internally.

aureleoules

ACK 47c4b1f
Great refactor that fixes a confusing function signature.

achow101 · 2023-01-03T21:10:34Z

ACK 47c4b1f

…stors

fanquake requested a review from glozow October 11, 2022 07:53

fanquake added the Mempool label Oct 11, 2022

stickies-v force-pushed the mempool-use-result branch from 425ae6d to a425537 Compare October 11, 2022 16:01

This was referenced Oct 11, 2022

p2p: remove adjusted time #25908

Closed

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

Use int32_t type for most transaction size/weight values #23962

Merged

w0xlt reviewed Oct 18, 2022

View reviewed changes

glozow reviewed Oct 19, 2022

View reviewed changes

aureleoules approved these changes Oct 21, 2022

View reviewed changes

furszy reviewed Oct 21, 2022

View reviewed changes

src/node/miner.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Oct 24, 2022

test: Use type-safe NodeSeconds for TestMemPoolEntryHelper #26376

Merged

stickies-v force-pushed the mempool-use-result branch from a425537 to c7f8236 Compare October 25, 2022 19:43

stickies-v marked this pull request as draft October 25, 2022 20:47

DrahtBot mentioned this pull request Oct 26, 2022

Bump unconfirmed ancestor transactions to target feerate #26152

Merged

stickies-v force-pushed the mempool-use-result branch from c7f8236 to 2f832f0 Compare October 26, 2022 11:43

stickies-v marked this pull request as ready for review October 26, 2022 12:36

aureleoules approved these changes Oct 27, 2022

View reviewed changes

maflcko reviewed Oct 28, 2022

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

glozow reviewed Oct 28, 2022

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/node/miner.cpp Outdated Show resolved Hide resolved

This was referenced Nov 4, 2022

policy: Ephemeral anchors #26403

Closed

policy: nVersion=3 and Package RBF #25038

Closed

DrahtBot mentioned this pull request Nov 19, 2022

mempool: Add mempool tracepoints #26531

Merged

willcl-ark approved these changes Nov 29, 2022

View reviewed changes

glozow reviewed Dec 12, 2022

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

mempool: use util::Result for CalculateAncestorsAndCheckLimits

66e028f

Avoid using setAncestors outparameter, simplify function signatures and avoid creating unused dummy strings.

stickies-v force-pushed the mempool-use-result branch from 1f89af2 to 4f1e03c Compare December 12, 2022 18:24

stickies-v commented Dec 12, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

stickies-v requested review from aureleoules and willcl-ark and removed request for aureleoules and willcl-ark December 12, 2022 18:26

stickies-v force-pushed the mempool-use-result branch from 4f1e03c to 3cbed28 Compare December 12, 2022 19:24

glozow reviewed Dec 13, 2022

View reviewed changes

src/node/miner.cpp Outdated Show resolved Hide resolved

stickies-v added 3 commits December 13, 2022 15:42

mempool: use util::Result for CalculateMemPoolAncestors

f911bdf

Avoid using setAncestors outparameter, simplify function signatures and avoid creating unused dummy strings.

stickies-v force-pushed the mempool-use-result branch from 3cbed28 to 47c4b1f Compare December 13, 2022 15:49

w0xlt approved these changes Dec 13, 2022

View reviewed changes

glozow reviewed Dec 19, 2022

View reviewed changes

src/validation.cpp Show resolved Hide resolved

glozow requested a review from willcl-ark December 20, 2022 14:10

furszy reviewed Dec 30, 2022

View reviewed changes

aureleoules approved these changes Jan 2, 2023

View reviewed changes

achow101 merged commit 80fc1af into bitcoin:master Jan 3, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 4, 2023

Merge bitcoin#26289: Use util::Result in for calculating mempool ance…

df69476

…stors

bitcoin locked and limited conversation to collaborators Jan 3, 2024

		CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors(
		std::string_view calling_fn_name,

Use util::Result in for calculating mempool ancestors #26289

Use util::Result in for calculating mempool ancestors #26289

Uh oh!

Conversation

stickies-v commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unexpected behaviour

Improvements

Return value instead of out-parameters

Observability

Uh oh!

DrahtBot commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Oct 20, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luke-jr commented Oct 25, 2022

Uh oh!

stickies-v commented Oct 25, 2022

Uh oh!

stickies-v commented Oct 25, 2022

Uh oh!

stickies-v commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeremyRubin commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Nov 1, 2022

Uh oh!

stickies-v commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Nov 29, 2022

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aureleoules commented Dec 12, 2022

Uh oh!

stickies-v commented Dec 12, 2022

Uh oh!

Uh oh!

stickies-v commented Dec 13, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

stickies-v commented Oct 10, 2022 •

edited

Loading

DrahtBot commented Oct 10, 2022 •

edited

Loading

stickies-v commented Oct 26, 2022 •

edited

Loading

JeremyRubin commented Oct 28, 2022 •

edited

Loading

stickies-v commented Nov 23, 2022 •

edited

Loading

furszy Dec 30, 2022 •

edited

Loading