Use util::Result in for calculating mempool ancestors

stickies-v commented at 8:55 pm on October 10, 2022: contributor

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 commented at 11:50 pm on October 10, 2022: contributor

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.

fanquake requested review from glozow on Oct 11, 2022

fanquake added the label Mempool on Oct 11, 2022

stickies-v force-pushed on Oct 11, 2022

w0xlt commented at 2:04 pm on October 18, 2022: contributor

Approach ACK

glozow commented at 9:32 am on October 19, 2022: member

Concept ACK, definitely prefer this over out-params.

theStack commented at 10:08 pm on October 20, 2022: contributor

Concept ACK

aureleoules approved

aureleoules commented at 8:41 am on October 21, 2022: member

ACK a425537fbfc20d87c325b5ae2f06f2597a31cf0e - LGTM and no behavior change

in src/node/miner.cpp:398 in a425537fbf outdated

394@@ -394,9 +395,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
395             continue;
396         }
397 
398-        CTxMemPool::setEntries ancestors;
399-        std::string dummy;
400-        mempool.CalculateMemPoolAncestors(*iter, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false);
401+        auto ancestors{*Assert(mempool.CalculateMemPoolAncestors(*iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false))};

furszy commented at 1:47 pm on October 21, 2022:

Even when this currently cannot fail (because of the no limits and search parents args), I would be more conservative and catch + log any possible result error instead of assert the result.

In this way, we keep the current behavior, and use an empty ancestors set instead of aborting in production if something gets changed inside CalculateMemPoolAncestors and we forget to update this call.

stickies-v commented at 11:25 am on October 24, 2022:

Interesting. I see your point re robustness, but generally, I really don’t like silent failures - even when logged. I’d much rather they throw so we can more easily detect and fix unexpected behaviour as early on as possible. There are 7 instances in this PR where we do this kind of assertion - would you only want to update it here or also in the other instances? I also understand that the current behaviour is that of silent failure, so I wouldn’t want to break anything with this change - but as you mention, I don’t think it does, given the NoLimits() parameter in all of these instances.

Cc’ing the other reviewers in case anyone else has a view (@w0xlt @glozow @theStack @aureleoules)

 0diff --git a/src/node/miner.cpp b/src/node/miner.cpp
 1index 0dc50c75c..c6838aee0 100644
 2--- a/src/node/miner.cpp
 3+++ b/src/node/miner.cpp
 4@@ -395,7 +395,13 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
 5             continue;
 6         }
 7 
 8-        auto ancestors{*Assert(mempool.CalculateMemPoolAncestors(*iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false))};
 9+        auto ancestors_result{Assume(mempool.CalculateMemPoolAncestors(*iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false))};
10+        if (!ancestors_result) {
11+            LogPrintLevel(BCLog::MEMPOOL, BCLog::Level::Error, 
12+                          "%s: CalculateMemPoolAncestors failed unexpectedly, continuing with empty ancestor set (%s)",
13+                          __func__, util::ErrorString(ancestors_result));
14+        }
15+        auto ancestors{ancestors_result.value_or(CTxMemPool::setEntries{})}; // for robustness, continue with empty ancestors if the calculation failed
16 
17         onlyUnconfirmed(ancestors);
18         ancestors.insert(iter);

edit: now using Assume() in the diff as per MarcoFalke’s suggestion

maflcko commented at 11:33 am on October 24, 2022:

At least you’ll still need to use Assume to abort in debug builds

furszy commented at 1:51 pm on October 24, 2022:

Interesting. I see your point re robustness, but generally, I really don’t like silent failures - even when logged. I’d much rather they throw so we can more easily detect and fix unexpected behaviour as early on as possible

Well, at least on the mempool and validation areas, I’m on the totally opposite position. If a result can have different meanings, then I would forcibly handle every possible outcome. We want robustness there.

Generally speaking: as the node’s mempool state is built from the network, if there is a way to trigger something like this from an external source and we overseeing it (which could happen because we are humans), then an attacker could crash nodes remotely. And once that happens, there isn’t a quick fix. Core isn’t a common piece of software, any new version distribution and update coordination is hard.

Thus why I would rather be more conservative in terms of using assertions there. And instead, I would actually force us to not ignore errors paths (we could extend our result class to work similarly to the Rust result type, so we are always forced to handle every possible outcome by default) or to rewrite code so it cannot fail in places that we don’t expect failures.

This all comes from very bad experiences that I had in the past..

At least you’ll still need to use Assume to abort in debug builds @MarcoFalke that wouldn’t work, the result class internally asserts that the value exists when the operator*() is called (the upper Assert() call is actually redundant).

maflcko commented at 1:56 pm on October 24, 2022:

@MarcoFalke that wouldn’t work, the result class internally asserts that the value exists when the operator*() is called (the upper Assert() call is actually redundant).

Sure, but not when * is replaced with value_or.

furszy commented at 2:08 pm on October 24, 2022:

@MarcoFalke that wouldn’t work, the result class internally asserts that the value exists when the operator*() is called (the upper Assert() call is actually redundant).

Sure, but not when * is replaced with value_or.

Yeah, in that way we would retain the current behavior and add the assertion for debug builds only.

Still, future thing, I’m more inclined to rewrite the code so the function cannot fail or.., at least, add the error handler so it becomes clear what we do if it fails on places that we don’t expect failures.

stickies-v commented at 7:24 pm on October 25, 2022:

(the upper Assert() call is actually redundant)

I don’t think that’s true, Assert does a boolean evaluation, so Result’s operator*() is not called.

… then an attacker could crash nodes remotely…

Point taken. I’ve reorganized (force pushing soon) the PR so that the first 2 commits keep the current (robust) behaviour but just using util::Result. In 2 subsequent commits I introduce and use a helper fn AssumeCalculateMemPoolAncestors to call CalculateMemPoolAncestors and always get a setEntries back, but for non-debug builds log the error in case of failure, or halt the application for debug builds. I think that strikes a nice balance between robustness in production and increasing visibility (of which currently we have 0 in master, for this issue) in development - hope you agree?

glozow commented at 12:07 pm on October 28, 2022:

Using value_or and Assume() seems like a good balance between handling every case and avoiding asserts in p2p-reachable codepaths. It’s not great that some callsites ignore the return value of CMPA and continue using the ancestor set on master - I was going to comment about it until I saw the last couple commits. Agree adding an Assume() to catch in debug mode + logging is a better approach than halting in production.

stickies-v commented at 1:18 pm on October 28, 2022:

I think everyone’s concerns are now addressed so I’ll mark this as resolved, thanks for the input everyone.

luke-jr commented at 3:11 pm on October 25, 2022: member

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

stickies-v force-pushed on Oct 25, 2022

stickies-v commented at 7:50 pm on October 25, 2022: contributor

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 marked this as a draft on Oct 25, 2022

stickies-v commented at 8:48 pm on October 25, 2022: contributor

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

stickies-v force-pushed on Oct 26, 2022

stickies-v marked this as ready for review on Oct 26, 2022

stickies-v commented at 12:36 pm on October 26, 2022: contributor

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 approved

aureleoules commented at 1:36 pm on October 27, 2022: member

reACK 2f832f0c45aae3b51353db72691b4bc6450a7e9a

in src/txmempool.cpp:269 in c25c6a6dc7 outdated

265@@ -266,6 +266,7 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
266                                            std::string &errString,
267                                            bool fSearchForParents /* = true */) const
268 {
269+    setAncestors.clear();

maflcko commented at 8:48 am on October 28, 2022:

can you explain why this is a bugfix? There is currently no code path that doesn’t clear the set before calling this function.

maflcko commented at 8:53 am on October 28, 2022:

Ok, I see it is only in the package acceptance code path?

stickies-v commented at 10:06 am on October 28, 2022:

it is only in the package acceptance code path?

That’s my understanding, yes. MemPoolAccept::PreChecks and MemPoolAccept::SubmitPackage are the only functions where setAncestors isn’t (always) cleared before calling CalculateMemPoolAncestors. In MemPoolAccept::AcceptMultipleTransactions(), we first call PreChecks() and then SubmitPackage() with the same Workspace. PreChecks always seems to be called with a fresh Workspace, so I’d say SubmitPackage is the only issue. Given that this is currently only used in testmempoolaccept and submitpackage RPCs, it shouldn’t be a particularly impactful bug.

I’ll update the PR description a bit more now, and if I have to retouch, I’ll update the bugfix commit message to add some more background.

glozow commented at 10:13 am on October 28, 2022:

Correct, the only path is in SubmitPackage. It’s not problematic to pass in ws.m_ancestors that is already populated because the set calculated in SubmitPackage should be a superset of the set calculated in PreChecks. If not, that would be a bug (hence the Assume(false) case). I agree CMPA should always start with an empty ancestor set. ~~Feel free to call it a bug or unexpected behavior etc.~~ Maybe I wouldn’t call it bug, but brittle code that is worth improving.

stickies-v commented at 10:57 am on October 28, 2022:

I wrote a quick check on master to verify that ancestors passed into CMPA are indeed always a subset of what is returned at the end of CMPA. At least as far as the scenarios covered in the unit and functional tests, that seems to be the case, since with this patch all tests complete without throwing.

 0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
 1index 84ed2e9ef..e024ebe81 100644
 2--- a/src/txmempool.cpp
 3+++ b/src/txmempool.cpp
 4@@ -190,6 +190,8 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
 5                                                   const Limits& limits,
 6                                                   std::string &errString) const
 7 {
 8+    setEntries incoming_ancestors{setAncestors.begin(), setAncestors.end()};
 9+    setAncestors.clear();
10     size_t totalSizeWithAncestors = entry_size;
11 
12     while (!staged_ancestors.empty()) {
13@@ -226,6 +228,9 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
14         }
15     }
16 
17+    for (auto& ancestor : incoming_ancestors){
18+        if (setAncestors.find(ancestor) == setAncestors.end()) throw std::runtime_error("incoming ancestor not found");
19+    }
20     return true;
21 }

glozow commented at 11:51 am on October 28, 2022:

Thank you for checking!

So this is an example of brittle code that hasn’t resulted in an actual bug (yet) but is worth improving. See also #23621 which is a similar situation.

willcl-ark commented at 2:40 pm on November 29, 2022:

Why is this bugfix added here and then removed later?

edit: Ah I think I see. Is the idea that this commit will be cherry-picked for backport?

stickies-v commented at 2:53 pm on November 29, 2022:

I’m unsure if we should keep the bugfix commit. As glozow says, the interface was brittle and could have been used in a buggy way, but that currently doesn’t appear to be the case (as per my own verification too). With that understanding, we probably don’t want to backport this? Alternatively, the separate bugfix commit could make it easier for downstream projects to patch this (potential) issue without having to adopt the full changeset? But I’m not sure that’s how we usually do things.

cc @luke-jr - any further thoughts on this after your earlier comment and glozow’s explanation?

glozow commented at 12:05 pm on December 5, 2022:

I didn’t care about the name very much before but given it has confused multiple people, maybe rename the commit as a “making explicit that the interface is changing” or drop it?

stickies-v commented at 6:20 pm on December 12, 2022:

I’ve dropped the bugfix commit in the latest force push

in src/txmempool.cpp:20 in 643ea3a2f8 outdated

16@@ -17,8 +17,10 @@
17 #include <util/check.h>
18 #include <util/moneystr.h>
19 #include <util/overflow.h>
20+#include <util/result.h>

glozow commented at 10:49 am on October 28, 2022:

in 643ea3a2f8c12b538c5a2a3994f383ef0ed372fa:

is this necessary if it’s already included in txmempool.h?

stickies-v commented at 1:14 pm on October 28, 2022:

Not necessary, but developer notes state:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

I’m not sure if just listing it as a return type classifies as “uses”, but either way it seems like a sane thing to do?

glozow commented at 3:35 pm on October 28, 2022:

Oops, good point :)

JeremyRubin commented at 4:00 pm on October 28, 2022: contributor

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 commented at 4:50 pm on November 1, 2022: member

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?

DrahtBot added the label Needs rebase on Nov 22, 2022

stickies-v force-pushed on Nov 22, 2022

stickies-v commented at 3:19 pm on November 22, 2022: contributor

Rebased to address the merge conflict from - no other changes:

0git range-diff 85892f77c98c7a08834a06d52af3eb474275afd8 2f832f0 1f89af219a89da155d5302ca4fdc4f8f1f8612aa

@JeremyRubin I’m not sure I understand your question either, would appreciate if you could elaborate a bit? Thanks!

DrahtBot removed the label Needs rebase on Nov 22, 2022

JeremyRubin commented at 6:05 pm on November 22, 2022: contributor

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. You can decrease the limit by another ancestor count if necessary.

stickies-v commented at 7:14 pm on November 22, 2022: contributor

Thanks for clearing it up, you’re talking about CTxMemPoolEntry::Parents staged_ancestors, right? @glozow suggested me that improvement (offline) earlier as well, I agree that it looks like that can be improved with an epoch approach as you say. I’ve started working on it but not yet ready for PR. I’d say it’s pretty orthogonal to the changes proposed here (we don’t really touch staged_ancestors, focus is on the returned set of ancestors), so in my view it shouldn’t be a hold up for making progress here, if you agree?

JeremyRubin commented at 6:02 pm on November 23, 2022: contributor

sort of orthogonal, sort of not. if you do the refactor mentioned, i think you would end up not needing this refactor, so the two overlap.

stickies-v commented at 6:11 pm on November 23, 2022: contributor

i think you would end up not needing this refactor, so the two overlap.

I don’t think I agree. The epoch refactoring would mostly target/remove staged_ancestors, whereas this PR targets the setAncestors that is passed into CalculateMemPoolAncestors as an in,out parameter. I don’t see how using epoch affects CalculateMemPoolAncestors properly returning the ancestors as opposed adding them to the setAncestors reference?

JeremyRubin commented at 7:52 pm on November 23, 2022: contributor

i’d need to look more detailed into the specifics, but no, the epoch patches could eliminate setAncestors and not the staged ancestors. Staging is still required since it is the work queue.

They way to look at it is in terms of where the setAncestors is consumed – afaict, it’s eventually transformed into just a count, so it’s possible we can just keep the count directly if we use epochs to serve as a de-facto set. We still require staging to track what to expand next.

stickies-v commented at 8:15 pm on November 23, 2022: contributor

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 commented at 12:20 pm on November 29, 2022: member

@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 https://github.com/bitcoin/bitcoin/blob/a035b6a0c418d0b720707df69559028bd662fa70/src/policy/rbf.cpp#L42-L48

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 approved

willcl-ark commented at 3:15 pm on November 29, 2022: contributor

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.

in src/validation.cpp:897 in 700bb2e2cd outdated

898-        }
899+
900+        auto invalid_too_long{state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", util::ErrorString(ancestors).original)};
901+        if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) return invalid_too_long;
902+        ancestors = m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits);
903+        if (!ancestors) return invalid_too_long;

glozow commented at 2:54 pm on December 12, 2022:

in 700bb2e2cdc083777eaea8699935d2e4ebf0d24d:

Here, by instantiating invalid_too_long outside of the if condition, you are mutating state.m_mode even if the CPFP carve out succeeds. I don’t think this causes the error to be returned when it shouldn’t be, but I don’t think it’s correct to do this. I’d suggest keeping the state.Invalid() line inside the if condition.

glozow commented at 3:00 pm on December 12, 2022:

To reproduce - we probably can’t test this since we can’t call PreChecks() in isolation, but if you add a Assume(ws.m_state.IsValid()) sanity check right before addUnchecked() and run test/functional/mempool_package_onemore.py it will hit.

stickies-v commented at 6:19 pm on December 12, 2022:

Oh great catch, thanks! I hadn’t considered the side effects. Fixed in 66e028f7399b6511f9b73b1cef54b6a6ac38a024.

mempool: use util::Result for CalculateAncestorsAndCheckLimits

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

66e028f739

stickies-v force-pushed on Dec 12, 2022

stickies-v commented at 6:25 pm on December 12, 2022: contributor

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

stickies-v requested review from aureleoules on Dec 12, 2022

stickies-v removed review request from aureleoules on Dec 12, 2022

stickies-v requested review from willcl-ark on Dec 12, 2022

stickies-v removed review request from willcl-ark on Dec 12, 2022

stickies-v requested review from aureleoules on Dec 12, 2022

aureleoules commented at 6:44 pm on December 12, 2022: member

CI is red @stickies-v

stickies-v force-pushed on Dec 12, 2022

stickies-v commented at 7:27 pm on December 12, 2022: contributor

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.

in src/node/miner.cpp:398 in 9481b62101 outdated

393@@ -394,9 +394,8 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
394             continue;
395         }
396 
397-        CTxMemPool::setEntries ancestors;
398-        std::string dummy;
399-        mempool.CalculateMemPoolAncestors(*iter, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false);
400+        auto ancestors_result{mempool.CalculateMemPoolAncestors(*iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)};
401+        auto ancestors{ancestors_result.value_or(CTxMemPool::setEntries{})};

glozow commented at 11:54 am on December 13, 2022:

question in 9481b62101a353c085e62e6635a1700685d9f6ca: does this make a copy of the set in ancestors_result ?

stickies-v commented at 4:04 pm on December 13, 2022:

Great question - and it pushed me quite deep down an interesting rabbit hole. I hadn’t considered this. I think you’re right that (without compiler optimization*) this (and all the other similar instances in this PR) would lead to a copy of the set, because we’re calling value_or() on an lvalue so the const& qualified value_or() method is used, which returns by value and doesn’t benefit from move semantics.

By turning ancestors_result into an rvalue ref with std::move, we can benefit from the move semantics that std::set has implemented since now we’ll be using the && qualified value_or() method.

I’ve force pushed a change to replace all instances of ancestors_result into std::move(ancestors_result) as well as update AssumeCalculateMemPoolAncestors() to be explicit about using move semantics. (note: most of these changes are undone in a later commit where we start using the newly introduced AssumeCalculateMemPoolAncestors() function)

*I think, in practice, in most of the affected instances in this PR the copy operation actually would have been optimized away by the compiler (because I think ancestors_result just would have been inlined anyway, making them rvalues), but at the very least the code is now more explicit about intent, and potentially more performant - and I could be wrong about my optimization assumptions.

Thank you for raising this!

mempool: use util::Result for CalculateMemPoolAncestors

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

f911bdfff9

mempool: add AssumeCalculateMemPoolAncestors helper function

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

5481f65849

mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly

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.

47c4b1f52a

stickies-v force-pushed on Dec 13, 2022

stickies-v commented at 4:06 pm on December 13, 2022: contributor

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

w0xlt approved

w0xlt commented at 6:05 pm on December 13, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26289/commits/47c4b1f52ab8d95d7deef83050bad49d1e3e5990

in src/validation.cpp:1118 in f911bdfff9 outdated

1122-            Assume(false);
1123-            all_submitted = false;
1124-            package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
1125-                                  strprintf("BUG! Mempool ancestors or descendants were underestimated: %s",
1126-                                            ws.m_ptx->GetHash().ToString()));
1127+        {

glozow commented at 11:31 am on December 19, 2022:

question in f911bdfff9: why add this scope?

stickies-v commented at 1:24 pm on December 19, 2022:

Because we’re using move semantics in https://github.com/bitcoin/bitcoin/pull/26289/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1129, so I wanted to be explicit about having ancestors go out of scope right after. I think that’s worth the extra indentation?

glozow commented at 12:02 pm on December 19, 2022: member

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

glozow requested review from willcl-ark on Dec 20, 2022

in src/txmempool.cpp:260 in 47c4b1f52a

258+}
259+
260+CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors(
261+    std::string_view calling_fn_name,
262+    const CTxMemPoolEntry &entry,
263+    const Limits& limits,

furszy commented at 9:47 pm on December 30, 2022:

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.

in src/txmempool.cpp:258 in 47c4b1f52a

256+    return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, staged_ancestors,
257+                                            limits);
258+}
259+
260+CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors(
261+    std::string_view calling_fn_name,

furszy commented at 9:52 pm on December 30, 2022:

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.

furszy commented at 9:57 pm on December 30, 2022: member

light code review ACK 47c4b1f5

left two comments, nothing blocking.

aureleoules approved

aureleoules commented at 11:13 am on January 2, 2023: member

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

achow101 commented at 9:10 pm on January 3, 2023: member

ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990

achow101 merged this on Jan 3, 2023

achow101 closed this on Jan 3, 2023

sidhujag referenced this in commit df69476bfe on Jan 4, 2023

bitcoin locked this on Jan 3, 2024

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

Unexpected behaviour

Improvements

Return value instead of out-parameters

Observability

Reviews

Conflicts