Mempool Update Cut-Through Optimization #21464

JeremyRubin · 2021-03-17T21:13:56Z

Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise.

There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go.

jonatack · 2021-03-18T19:26:08Z

Interesting on first look, will review.

JeremyRubin · 2021-04-07T00:20:15Z

pushed an update to simplify setRemove to hold txids instead of tx reference_wrappers, which eliminated the confusing ownership thing. should make review simpler.

instagibbs

some comments, logic looks right from manual inspection of other limits code

will test

src/txmempool.cpp

instagibbs · 2021-04-12T06:58:47Z

utACK e8dafeb

going to dive into tests next to upgrade to tACK

glozow

ACK e8dafeb Code looks correct to me. I'm mostly just suggesting some docs if you retouch.

I wonder if (in addition to this) it would be worth changing ATMP to take a bool check_for_children param where, if true, looks for descendants in mempool and includes them when applying ancestor/descendant limits. Technically, ATMP is currently not enforcing these rules properly for reorgs by assuming no children in the mempool. That way we can probably remove them in UpdateMempoolForReorg() before even calling UpdateTransactionsFromBlock().

src/txmempool.h

src/txmempool.cpp

rebroad · 2021-04-16T11:06:36Z

@JeremyRubin could you explain what this does in more layman's terms please? Is this pull anything to do with helping to fill the mempool in a way that reduces filling it with TXs that will get deleted later (due to mempool becoming full)?

instagibbs · 2021-04-16T11:09:19Z

This is an optimization for the case where a reorg puts an ancestor transaction(package) back into the mempool, thus temporarily busting the transaction chain limits of the mempool.

jnewbery

Looks good. A few style comments inline, including a suggested change to minimize the diff in the CTxMemPool interface and in the calling code.

I think the two commits can be squashed.

src/txmempool.h

src/txmempool.cpp

DrahtBot · 2021-05-03T09:33:16Z

🕵️ @hebasto @sipa have been requested to review this pull request as specified in the REVIEWERS file.

ariard

Concept ACK

Looking to test this a bit more.

There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go.

I don't have an opinion on patch ordering but I would be glad if we can land the Epoch changes (#17628) at some point. Either as it is, or after a bit of txmempool internals cleaning/encapsulation :)

src/txmempool.cpp

src/validation.cpp

DrahtBot · 2021-07-10T14:49:21Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23962 (Use int32_t type for transaction size/weight consistently by hebasto)
#21436 (doc: Fix several references in txmempool comments by kiminuo)
#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

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.

JeremyRubin · 2021-07-10T21:09:06Z

Addressed style/doc stuff in a separate commit to preserve ACKs (squashable)

I also introcuded a new removal reason in a separate commit, this should not be squashed as it is ortho to the main change.

@laanwj anything else you'd like to see here?

jnewbery · 2021-07-12T11:47:15Z

Addressed style/doc stuff in a separate commit to preserve ACKs (squashable)

I don't understand this. The head commit is what needs ACKs, and that's changed now that there are new commits. I'm very happy to re-review once you've squashed commits and rebased on master.

JeremyRubin · 2021-07-15T23:02:02Z

Ok, made changes requested by @jnewbery and squashed. Will open a followup PR with the validation interface after as requested by @ariard. Thanks.

src/txmempool.h

darosior

ACK 2820b04

jnewbery

I think it'd be better to squash the test commit into the first commit, so the changes are atomic.

src/txmempool.h

src/txmempool.cpp

src/txmempool.h

JeremyRubin · 2021-09-12T20:44:03Z

Addressed all feedback; keeping tests separate from changes for easier A/B comparison. However, I have reordered them so Tests come first which is more practical.

Talkless · 2021-11-06T20:39:27Z

Weak ACK 81efedb. Weak because of my limited mempool knowledge, though code looks OK, and I successfully ran unit and extended functional tests.

mjdietzx · 2021-11-15T17:15:18Z

When I read the commit:

Often when we're updating mempool entries we update entries that we
ultimately end up removing the updated entries shortly thereafter. This
patch makes it so that we filter for such entries a bit earlier in
processing

based on the wording, I assumed that we were "filtering" these entries out before adding them to the mempool. However, after reviewing the PR it seems that these entries are still added to the mempool, state is still calculated/updated and cached, it's just that we are now (pre-emptively) removing descendants_to_remove right away at the end of UpdateTransactionsFromBlock

Is my understanding correct? If so, it makes me wonder where the performance improvement is coming from. To me it seems all the same work is done before/after this PR, just earlier.

Re: my review comment 9264766, mempool_updatefromblock.py (which I agree is a great benchmark test for this optimization) is running in the ~same time before/after this PR. So I am curious if we are actually seeing any speedup from this optimization

laanwj · 2021-12-02T15:39:28Z

Code review ACK 81efedb
Thanks for clarifying doc comments to UpdateTransactionsFromBlock and UpdateForDescendants.

laanwj · 2021-12-02T15:43:10Z

Looks like there has been a silent merge conflict though. Local compilation results in:

/…/bitcoin/src/validation.cpp:351:79: error: reference to type 'const std::string' (aka 'const basic_string<char>') could not bind to an lvalue of type 'const unsigned int'
    const uint64_t ancestor_count_limit = gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
                                                                              ^~~~~~~~~~~~~~~~~~~~~~
/…/bitcoin/src/util/system.h:327:70: note: passing argument to parameter 'strDefault' here
    std::string GetArg(const std::string& strArg, const std::string& strDefault) const;
                                                                     ^
/…/bitcoin/src/validation.cpp:351:20: error: no viable conversion from 'std::string' (aka 'basic_string<char>') to 'const uint64_t' (aka 'const unsigned long')
    const uint64_t ancestor_count_limit = gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
                   ^                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/basic_string.h:816:7: note: candidate function
      operator __sv_type() const noexcept
      ^
/…/bitcoin/src/validation.cpp:352:77: error: reference to type 'const std::string' (aka 'const basic_string<char>') could not bind to an lvalue of type 'const unsigned int'
    const uint64_t ancestor_size_limit = gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000;
                                                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/…/bitcoin/src/util/system.h:327:70: note: passing argument to parameter 'strDefault' here
    std::string GetArg(const std::string& strArg, const std::string& strDefault) const;
                                                                     ^
/…/bitcoin/src/validation.cpp:352:106: error: invalid operands to binary expression ('std::string' (aka 'basic_string<char>') and 'int')
    const uint64_t ancestor_size_limit = gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * 1000;
                                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/complex:391:5: note: candidate template ignored: could not match 'complex' against 'basic_string'
    operator*(const complex<_Tp>& __x, const complex<_Tp>& __y)
    ^
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/complex:400:5: note: candidate template ignored: could not match 'complex' against 'basic_string'
    operator*(const complex<_Tp>& __x, const _Tp& __y)
    ^
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/complex:409:5: note: candidate template ignored: could not match 'complex<type-parameter-0-0>' against 'int'
    operator*(const _Tp& __x, const complex<_Tp>& __y)
    ^

I think it's simply a matter of changing GetArg to GetIntArg for these.

…e improved algorithm

Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise.

JeremyRubin · 2021-12-10T23:41:24Z

rebased, test failures are unrelated it looks like (this PR doesn't touch secp?)

maflcko · 2022-01-03T16:00:37Z

reran ci (was unrelated failure)

JeremyRubin · 2022-01-03T19:04:33Z

requesting re-acks from:

@instagibbs
@glozow
@ariard
@laanwj
@Talkless

src/validation.cpp

instagibbs · 2022-01-04T05:18:16Z

reACK c5b36b1

sipa · 2022-01-07T20:23:19Z

utACK c5b36b1

mzumsande

Code Review ACK c5b36b1
The change looks correct to me, the doc rework for the touched functions is an improvement.

instagibbs · 2022-01-24T23:30:35Z

RFM?

c5b36b1 Mempool Update Cut-Through Optimization (Jeremy Rubin) c49daf9 [TESTS] Increase limitancestorcount in tournament RPC test to showcase improved algorithm (Jeremy Rubin) Pull request description: Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise. There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go. ACKs for top commit: instagibbs: reACK c5b36b1 sipa: utACK c5b36b1 mzumsande: Code Review ACK c5b36b1 Tree-SHA512: 78b16864f77a637d8a68a65e23c019a9757d8b2243486728ef601d212ae482f6084cf8e69d810958c356f1803178046e4697207ba40d6d10529ca57de647fae6

Summary: Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise. Increase limitancestorcount in tournament RPC test to showcase improved algorithm This is a backport of [[bitcoin/bitcoin#21464 | core#21464]] Notes: - I did not see a significant gain for this functional test. If there is any improvement, it is less than the usual variability (+- 10 seconds for a test that takes 220 seconds). - the `policy/mempool.h` include is needed for `DEFAULT_ANCESTOR_LIMIT` and `DEFAULT_ANCESTOR_SIZE_LIMIT` Test Plan: `ninja && ninja check check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12589

JeremyRubin added this to Epoch MemPool in Mempool improvements Mar 17, 2021

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch from c821ada to d4a5796 Compare March 17, 2021 21:24

DrahtBot added Mempool Validation labels Mar 17, 2021

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch from 5f98973 to a4aef7a Compare April 6, 2021 22:29

laanwj added this to Blockers in High-priority for review Apr 7, 2021

instagibbs reviewed Apr 12, 2021

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch 2 times, most recently from 2be4f77 to e8dafeb Compare April 12, 2021 02:00

glozow reviewed Apr 14, 2021

View reviewed changes

src/txmempool.h Outdated Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

src/txmempool.cpp Show resolved Hide resolved

jnewbery reviewed Apr 28, 2021

View reviewed changes

ariard reviewed May 29, 2021

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 10, 2021

Make m_mempool optional in CChainState #22415

Merged

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch from 03d1cae to f0d8236 Compare July 10, 2021 19:35

DrahtBot added the Needs rebase label Jul 15, 2021

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch from f0d8236 to 2820b04 Compare July 15, 2021 22:59

DrahtBot removed the Needs rebase label Jul 16, 2021

glozow reviewed Jul 16, 2021

View reviewed changes

src/txmempool.h Show resolved Hide resolved

darosior approved these changes Jul 19, 2021

View reviewed changes

jnewbery reviewed Jul 26, 2021

View reviewed changes

src/txmempool.h Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

src/txmempool.cpp Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch from 2820b04 to 81efedb Compare September 12, 2021 18:50

DrahtBot mentioned this pull request Sep 26, 2021

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

DrahtBot mentioned this pull request Oct 4, 2021

Add ChainstateManager::ProcessTransaction #23173

Merged

ryihan approved these changes Nov 6, 2021

View reviewed changes

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch from 81efedb to a1b6371 Compare December 7, 2021 16:41

JeremyRubin added 2 commits December 8, 2021 23:07

[TESTS] Increase limitancestorcount in tournament RPC test to showcas…

c49daf9

…e improved algorithm

JeremyRubin force-pushed the epoch-mempool-cut-through-optimized branch from a1b6371 to c5b36b1 Compare December 9, 2021 07:08

DrahtBot mentioned this pull request Jan 3, 2022

Remove txmempool implicit-integer-sign-change sanitizer suppressions #23957

Closed

instagibbs reviewed Jan 4, 2022

View reviewed changes

src/validation.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 4, 2022

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

Merged

mzumsande reviewed Jan 24, 2022

View reviewed changes

fanquake merged commit 0147278 into bitcoin:master Jan 25, 2022

fanquake removed this from Blockers in High-priority for review Jan 25, 2022

ziggie1984 mentioned this pull request Jan 25, 2022

[mempool] allow tx replacement by smaller witness #24007

Closed

JeremyRubin mentioned this pull request Jan 26, 2022

Optimize Mempool Reorg logic using Epochs, improving memory usage and runtime. #24158

Closed

bitcoin locked and limited conversation to collaborators Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mempool Update Cut-Through Optimization #21464

Mempool Update Cut-Through Optimization #21464

JeremyRubin commented Mar 17, 2021

jonatack commented Mar 18, 2021

JeremyRubin commented Apr 7, 2021

instagibbs left a comment

instagibbs commented Apr 12, 2021

glozow left a comment •

edited

rebroad commented Apr 16, 2021

instagibbs commented Apr 16, 2021

jnewbery left a comment

DrahtBot commented May 3, 2021

ariard left a comment

DrahtBot commented Jul 10, 2021 •

edited

JeremyRubin commented Jul 10, 2021

jnewbery commented Jul 12, 2021

JeremyRubin commented Jul 15, 2021

darosior left a comment

jnewbery left a comment

JeremyRubin commented Sep 12, 2021

Talkless commented Nov 6, 2021

mjdietzx commented Nov 15, 2021 •

edited

laanwj commented Dec 2, 2021

laanwj commented Dec 2, 2021

JeremyRubin commented Dec 10, 2021

maflcko commented Jan 3, 2022

JeremyRubin commented Jan 3, 2022

instagibbs commented Jan 4, 2022

sipa commented Jan 7, 2022

mzumsande left a comment

instagibbs commented Jan 24, 2022

Mempool Update Cut-Through Optimization #21464

Mempool Update Cut-Through Optimization #21464

Conversation

JeremyRubin commented Mar 17, 2021

jonatack commented Mar 18, 2021

JeremyRubin commented Apr 7, 2021

instagibbs left a comment

Choose a reason for hiding this comment

instagibbs commented Apr 12, 2021

glozow left a comment • edited

Choose a reason for hiding this comment

rebroad commented Apr 16, 2021

instagibbs commented Apr 16, 2021

jnewbery left a comment

Choose a reason for hiding this comment

DrahtBot commented May 3, 2021

ariard left a comment

Choose a reason for hiding this comment

DrahtBot commented Jul 10, 2021 • edited

Conflicts

JeremyRubin commented Jul 10, 2021

jnewbery commented Jul 12, 2021

JeremyRubin commented Jul 15, 2021

darosior left a comment

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

JeremyRubin commented Sep 12, 2021

Talkless commented Nov 6, 2021

mjdietzx commented Nov 15, 2021 • edited

laanwj commented Dec 2, 2021

laanwj commented Dec 2, 2021

JeremyRubin commented Dec 10, 2021

maflcko commented Jan 3, 2022

JeremyRubin commented Jan 3, 2022

instagibbs commented Jan 4, 2022

sipa commented Jan 7, 2022

mzumsande left a comment

Choose a reason for hiding this comment

instagibbs commented Jan 24, 2022

glozow left a comment •

edited

DrahtBot commented Jul 10, 2021 •

edited

mjdietzx commented Nov 15, 2021 •

edited