Make m_mempool optional in CChainState by jamesob · Pull Request #22415 · bitcoin/bitcoin

jamesob · 2021-07-07T19:55:52Z

Make CChainState::m_mempool optional by making it a pointer instead of a reference. This will allow a simplification to assumeutxo semantics (see #15606 (review)) and help facilitate the -nomempool option.

maflcko

Concept ACK

src/validation.cpp

src/validation.h

ryanofsky · 2021-07-07T23:24:44Z

Concept ACK. I think you can drop the second commit and add a LOCK_RETURNED helper method to be able to keep using lock annotations when m_mempool is null:

RecursiveMutex* MempoolMutex() const LOCK_RETURNED(m_mempool->cs) { return m_mempool ? &m_mempool->cs : nullptr; }

Seems to work for me in commit 8db5953

DrahtBot · 2021-07-08T00:48:00Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Move pblocktree global to BlockManager #22371 by MarcoFalke
validation: UpdateTip/CheckBlockIndex assumeutxo support #21526 by jamesob
Mempool Update Cut-Through Optimization #21464 by JeremyRubin

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.

jamesob · 2021-07-08T02:05:13Z

Rebased to remove the second commit per Russ' suggestion, which also addresses most of Marco's feedback.

Seems to work for me in commit 8db5953

Wow, that was wizardly. Thanks @ryanofsky!

I think std::variant can do this

I'll look into this tomorrow.

src/validation.h

naumenkogs · 2021-07-08T09:46:15Z

Light code review ACK b17bb4c37d58ca49e877983a632bfcaa493be993

maflcko

Nice solution with the LOCK_RETURNED

src/validation.h

jonatack · 2021-07-08T14:52:44Z

Concept ACK, will review on next push.

ryanofsky

Code review ACK b17bb4c37d58ca49e877983a632bfcaa493be993

src/init.cpp

ryanofsky · 2021-07-08T14:56:05Z

src/validation.cpp

In commit "validation: make CChainState::m_mempool optional" (b17bb4c37d58ca49e877983a632bfcaa493be993)

Note: It could be nice to be able to write AssertLockHeld(MempoolMutex() for consistency with LOCK(MempoolMutex()) and to avoid the if statement. But it'd be beyond scope of this PR since it'd require changing AssertLockHeld and there are other issues with that function anyway.

ryanofsky · 2021-07-08T14:59:59Z

src/validation.cpp

In commit "validation: make CChainState::m_mempool optional" (b17bb4c37d58ca49e877983a632bfcaa493be993)

Note: Naively I'd expect the mempool pointer for the activated snapshot to be non-null, but I guess it could be set later when the snapshot is fully initialized.

I was also surprised by this. Is the snapshot chainstate the one that eventually becomes our tip? In that case, don't we want it to have a mempool?

Yeah, this is a little confusing absent context, but basically the idea per @ryanofsky's feedback is that mempool presence should be used to indicate chainstate activeness, so we don't want to give the snapshot chainstate a mempool until we consider it active.

jamesob · 2021-07-08T16:13:08Z

Rebased.

Note: Naively I'd expect the mempool pointer for the activated snapshot to be non-null, but I guess it could be set later when the snapshot is fully initialized.

This is a good point. I'd come up with a commit that implements the std::variant feedback (and am glad to have for learning about that), but @ryanofsky's point here makes that change inappropriate: with assumeutxo, we may actually want the snapshot chain to have a non-null mempool if we're in, say, init.cpp and loading a snapshot chainstate that is active but not yet at tip.

I've covered all of the other feedback aside from the lock assertion for the reasons Russ mentioned; maybe we can get around to it later.

ryanofsky

Code review ACK 8511429f3bc480ddeef06c23810b3c47fa8513ee. Just some minor tweaks since last review: whitespace, unique_ptr assert, null mempool args

jonatack

ACK 8511429f3bc480ddeef06c23810b3c47fa8513ee code review, debug build, running bitcoind on signet as a sanity check, operation nominal

src/validation.cpp

src/validation.h

jnewbery

Concept ACK. A few non-blocking suggestions inline.

src/validation.cpp

jamesob · 2021-07-09T17:24:33Z

Thanks for the prompt attention, good people. I've pushed a rebase that includes pretty much all of @jnewbery's feedback. This widens the scope of the PR to encompass a bit of refactoring, but I think it's all reasonable stuff to include here.

ryanofsky

Code review ACK 29acd850fb0539b8358a451089ef7b1c583dd1a1. Simplifications from John make the change nicer.

src/validation.cpp

jamesob · 2021-07-09T18:50:15Z

Rebase to incorporate minor changes from @ryanofsky's feedback (#22415 (comment)).

src/validation.cpp

src/validation.h

jnewbery · 2021-07-12T11:09:24Z

src/validation.cpp

I was also surprised by this. Is the snapshot chainstate the one that eventually becomes our tip? In that case, don't we want it to have a mempool?

jnewbery

Looking good. Just a few small comments inline.

src/validation.cpp

src/validation.h

ryanofsky

Code review ACK c13e832af4a044a5c4beb4694022df53fa2eaf6d

src/validation.h

src/validation.cpp

src/validation.h

src/validation.cpp

src/test/validation_flush_tests.cpp

jamesob · 2021-07-13T13:29:39Z

Anyone have any idea what the deal is with libsecp tests failing on ARM here?

This doesn't look like a spurious failure.

ryanofsky · 2021-07-13T14:22:51Z

@MarcoFalke would probably be most knowledgeable about the qemu-arm coredump #22415 (comment) https://cirrus-ci.com/task/6754077301276672?logs=ci#L3494, I was curious about that too. It seems like it is happening in a libsecp256k1 test so not related? But maybe there are ways to debug.

(I don't want to bikeshed the style issues, since they don't actually matter at all. But just for fun I'll say I do agree with John in not liking mostly spurious const. Also would drop Try prefix or replace it with Maybe since Try suggests some kind of failure would be expected. I do think having this-> prefixes is more readable in current code to distinguish method calls from function calls, but in non-legacy code it'd be cleaner to use lowerCaseMethod() and UpperCaseFunction() names to avoid the ambiguity.)

maflcko · 2021-07-13T14:40:45Z

The (~x[m][shift]) << (64 - (1 << usebits))) == 0 failure is expected: bitcoin-core/secp256k1#610 (comment)

maflcko · 2021-07-13T14:41:52Z

Could edit OP to remove the section that no longer applies to make sure it doesn't end up in the final merge commit?

Since we now have multiple chainstate objects, only one of them is active at any given time. An active chainstate has a mempool, but there's no point to others having one. This change will simplify proposed assumeutxo semantics. See the discussion here: bitcoin#15606 (review) Co-authored-by: Russell Yanofsky <[email protected]>

Allows fewer arguments and simplification of call sites. Co-authored-by: John Newbery <[email protected]>

jamesob · 2021-07-13T15:14:52Z

Pushed a rebase:

TryUpdateMempool... -> MaybeUpdateMempool...
Removed unnecessary consts
Removed outdated content from OP

Unnecessary argument since we can make use of this->m_mempool Co-authored-by: John Newbery <[email protected]>

Makes sense and saves on arguments. Co-authored-by: John Newbery <[email protected]>

jamesob · 2021-07-13T15:28:17Z

unnecessary tx_pool removed

jnewbery · 2021-07-13T15:32:16Z

ACK ceb7b35

ryanofsky

Code review ACK ceb7b35 (just minor style and test tweaks since last review)

lsilva01

Code review ACK and tested on Signet ACK ceb7b35

naumenkogs · 2021-07-15T08:51:30Z

ACK ceb7b35

maflcko

review ACK ceb7b35 😌

Show signature and timestamp

Signature:

Timestamp of file with hash 74406e84f80bcb1056122ddab2c038117cd73fd1e1a79cd5df1958eacc8b9b20 -

jamesob force-pushed the 2021-07-mempool-ptr branch 2 times, most recently from 518531e to 349ab28 Compare July 7, 2021 20:32

DrahtBot added the Validation label Jul 7, 2021

maflcko reviewed Jul 7, 2021

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

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 8, 2021

Move pblocktree global to BlockManager #22371

Merged

jamesob force-pushed the 2021-07-mempool-ptr branch from 349ab28 to b17bb4c Compare July 8, 2021 02:02

DrahtBot mentioned this pull request Jul 8, 2021

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

Merged

naumenkogs reviewed Jul 8, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

maflcko reviewed Jul 8, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

ryanofsky approved these changes Jul 8, 2021

View reviewed changes

jamesob force-pushed the 2021-07-mempool-ptr branch from b17bb4c to 8511429 Compare July 8, 2021 16:12

ryanofsky approved these changes Jul 8, 2021

View reviewed changes

jonatack reviewed Jul 8, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Jul 9, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

jnewbery reviewed Jul 9, 2021

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

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

jamesob force-pushed the 2021-07-mempool-ptr branch from 8511429 to 29acd85 Compare July 9, 2021 17:14

ryanofsky reviewed Jul 9, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jamesob force-pushed the 2021-07-mempool-ptr branch from 29acd85 to e34a992 Compare July 9, 2021 18:48

DrahtBot mentioned this pull request Jul 10, 2021

Mempool Update Cut-Through Optimization #21464

Merged

naumenkogs reviewed Jul 12, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Jul 12, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Jul 12, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

jamesob force-pushed the 2021-07-mempool-ptr branch from e34a992 to c13e832 Compare July 12, 2021 18:38

ryanofsky approved these changes Jul 12, 2021

View reviewed changes

jnewbery reviewed Jul 13, 2021

View reviewed changes

jamesob and others added 2 commits July 13, 2021 11:11

refactor: move UpdateMempoolForReorg into CChainState

46e3efd

Allows fewer arguments and simplification of call sites. Co-authored-by: John Newbery <[email protected]>

jamesob force-pushed the 2021-07-mempool-ptr branch from c13e832 to 8a38f35 Compare July 13, 2021 15:13

jamesob and others added 2 commits July 13, 2021 11:16

refactor: no mempool arg to GetCoinsCacheSizeState

4abf077

Unnecessary argument since we can make use of this->m_mempool Co-authored-by: John Newbery <[email protected]>

refactor: move UpdateTip into CChainState

ceb7b35

Makes sense and saves on arguments. Co-authored-by: John Newbery <[email protected]>

jamesob force-pushed the 2021-07-mempool-ptr branch from 8a38f35 to ceb7b35 Compare July 13, 2021 15:16

ryanofsky approved these changes Jul 13, 2021

View reviewed changes

lsilva01 approved these changes Jul 14, 2021

View reviewed changes

maflcko approved these changes Jul 15, 2021

View reviewed changes

maflcko merged commit c0224bc into bitcoin:master Jul 15, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021

erge bitcoin#22415: Make m_mempool optional in CChainState

0f92a60

JaredTate mentioned this pull request Oct 30, 2021

WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code DigiByte-Core/digibyte#77

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge c0224bc into merged_master (Bitcoin PR bitcoin/bitcoin#22415)

40d1f3f

jamesob mentioned this pull request Jun 28, 2022

assumeutxo #15606

Closed

18 tasks

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

jamesob commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Jul 7, 2021

Uh oh!

DrahtBot commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jamesob commented Jul 8, 2021

Uh oh!

Uh oh!

naumenkogs commented Jul 8, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Jul 8, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

jamesob Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

jamesob commented Jul 8, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob commented Jul 9, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Jul 7, 2021 •

edited

Loading

DrahtBot commented Jul 8, 2021 •

edited

Loading

ryanofsky commented Jul 13, 2021 •

edited

Loading