Make m_mempool optional in CChainState #22415

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 b17bb4c

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 b17bb4c

src/init.cpp

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

src/validation.cpp

@@ -2254,7 +2257,7 @@ static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const C
 bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool)
 {
    AssertLockHeld(cs_main);
-    AssertLockHeld(m_mempool.cs);
+    if (m_mempool) AssertLockHeld(m_mempool->cs);


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

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

@@ -4763,7 +4772,7 @@ bool ChainstateManager::ActivateSnapshot(
    }

    auto snapshot_chainstate = WITH_LOCK(::cs_main, return std::make_unique<CChainState>(
-            this->ActiveChainstate().m_mempool, m_blockman, base_blockhash));
+            nullptr, m_blockman, base_blockhash));


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

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 8511429. Just some minor tweaks since last review: whitespace, unique_ptr assert, null mempool args

jonatack

ACK 8511429 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 29acd85. 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

@@ -4763,7 +4772,7 @@ bool ChainstateManager::ActivateSnapshot(
    }

    auto snapshot_chainstate = WITH_LOCK(::cs_main, return std::make_unique<CChainState>(
-            this->ActiveChainstate().m_mempool, m_blockman, base_blockhash));
+            nullptr, m_blockman, base_blockhash));


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 c13e832

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make m_mempool optional in CChainState #22415

Make m_mempool optional in CChainState #22415

jamesob commented Jul 7, 2021 •

edited

Loading

maflcko left a comment

ryanofsky commented Jul 7, 2021

DrahtBot commented Jul 8, 2021 •

edited

Loading

jamesob commented Jul 8, 2021

naumenkogs commented Jul 8, 2021

maflcko left a comment

jonatack commented Jul 8, 2021

ryanofsky left a comment

ryanofsky Jul 8, 2021

ryanofsky Jul 8, 2021

jnewbery Jul 12, 2021

jamesob Jul 12, 2021

jamesob commented Jul 8, 2021

ryanofsky left a comment

jonatack left a comment

jnewbery left a comment

jamesob commented Jul 9, 2021

ryanofsky left a comment

jamesob commented Jul 9, 2021

jnewbery Jul 12, 2021

jnewbery left a comment

ryanofsky left a comment

jamesob commented Jul 13, 2021

ryanofsky commented Jul 13, 2021 •

edited

Loading

maflcko commented Jul 13, 2021

maflcko commented Jul 13, 2021

jamesob commented Jul 13, 2021

jamesob commented Jul 13, 2021

jnewbery commented Jul 13, 2021

ryanofsky left a comment

lsilva01 left a comment

naumenkogs commented Jul 15, 2021

maflcko left a comment

Make m_mempool optional in CChainState #22415

Make m_mempool optional in CChainState #22415

Conversation

jamesob commented Jul 7, 2021 • edited Loading

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky commented Jul 7, 2021

DrahtBot commented Jul 8, 2021 • edited Loading

Conflicts

jamesob commented Jul 8, 2021

naumenkogs commented Jul 8, 2021

maflcko left a comment

Choose a reason for hiding this comment

jonatack commented Jul 8, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jul 8, 2021

Choose a reason for hiding this comment

ryanofsky Jul 8, 2021

Choose a reason for hiding this comment

jnewbery Jul 12, 2021

Choose a reason for hiding this comment

jamesob Jul 12, 2021

Choose a reason for hiding this comment

jamesob commented Jul 8, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

jamesob commented Jul 9, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

jamesob commented Jul 9, 2021

jnewbery Jul 12, 2021

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

jamesob commented Jul 13, 2021

ryanofsky commented Jul 13, 2021 • edited Loading

maflcko commented Jul 13, 2021

maflcko commented Jul 13, 2021

jamesob commented Jul 13, 2021

jamesob commented Jul 13, 2021

jnewbery commented Jul 13, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

lsilva01 left a comment

Choose a reason for hiding this comment

naumenkogs commented Jul 15, 2021

maflcko left a comment

Choose a reason for hiding this comment

jamesob commented Jul 7, 2021 •

edited

Loading

DrahtBot commented Jul 8, 2021 •

edited

Loading

ryanofsky commented Jul 13, 2021 •

edited

Loading