Make m_mempool optional in CChainState

[jamesob]

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

[ryanofsky]

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]

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]

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.

[naumenkogs]

Light code review ACK b17bb4c37d58ca49e877983a632bfcaa493be993

[maflcko]

Nice solution with the LOCK_RETURNED

[jonatack]

Concept ACK, will review on next push.

[ryanofsky]

Code review ACK b17bb4c37d58ca49e877983a632bfcaa493be993

[ryanofsky]

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]

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.

[jnewbery]

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?

[jamesob]

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]

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

[jnewbery]

Concept ACK. A few non-blocking suggestions inline.

[jamesob]

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.

[jamesob]

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

[jnewbery]

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.

[ryanofsky]

Code review ACK c13e832af4a044a5c4beb4694022df53fa2eaf6d

[jamesob]

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

This doesn't look like a spurious failure.

[ryanofsky]

@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]

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

[maflcko]

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

[jamesob]

Pushed a rebase:

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

[jamesob]

• unnecessary tx_pool removed

[jnewbery]

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]

ACK ceb7b35

[maflcko]

review ACK ceb7b35 😌

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a 😌
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhhlwwAtTlbDcsq2EyaUfR0ZvA00PHL6azSeUF1jZ0G0i3eTSVzYlATm+ePtd0W
wsJW4atryMHoqAGZeXTpjt8SZ9XqVdlylswiYefDVTmc9qq9T7ejW4uVtzY/sM45
D6jWRRSUg9xX8c7zrjRaxbPfdGonDTKxkh4BK3v1B9mm2+YVb5EjMDqszzsJs1ef
3VfWNwhRWmfLI9voCAIfOUzFQQC1dWI7NqS7V6MoTnEIpJouK34hvaA51un7iu38
ER5rUhmVUTT/VxkwbzlOlu4C654IVgAq4S2YQ0NWSIDgsuqxEe/C2nfUlm/bPl/v
Khu3jCJz9u26BCLOfkBEpftGUhhFK+djxgitDz/ktzD8taU6Fy2sDPp8vjL8cL3B
//Y87WELvtdUwDxBtqfxkkh+Yy7029uNm3n4Rh6Gex3dsAAZhWAaCKM/1uO1g2+9
KLFtb8sjMb57JQwG3Cdqs889JKjkUzeHdM52w9eu2WBQyWUhSAApuYsf1CUasjwS
I/SNB2Cz
=pjF5
-----END PGP SIGNATURE-----

Timestamp of file with hash 74406e84f80bcb1056122ddab2c038117cd73fd1e1a79cd5df1958eacc8b9b20 -