backport: merge bitcoin#23280 (coalesce chainstate loading sequence between {,non-}unittest codepaths) by kwvg · Pull Request #6304 · dashpay/dash

kwvg · 2024-10-03T22:11:49Z

Additional Information

Dependent on backport: merge bitcoin#22772, #22987, #23185, #23157, #23173, #23249, #23211, #22677, #23649, #23683, #23636, #22626 (auxiliary backports: part 17) #6296
Dependent on refactor: move CConnman and PeerManager out of CDeterministicMNManager, LLMQContext member ctors, reduce use in CJContext #6443
As one of the backport's intentions were to unify code between init.cpp and setup_common.cpp, Dash-specific initialization code (to the extent that it can be moved out non-disruptively) has been spun out into DashChainstateSetup{,Close}() so it can also be used in setup_common.cpp and validation_chainstatemanager_tests.cpp.

This is also why DashTestSetup{,Close}() (now DashPostChainstateSetup{,Close}()) was introduced in dash#5531.
- DashChainstateSetup{,Close}() (as defined in node/chainstate.cpp) cannot take NodeContext in because node/chainstate.cpp is used in bitcoin-chainstate, which doesn't include NodeContext (source), this is reflected by neither LoadChainstate nor VerifyLoadedChainstate taking in NodeContext.
- To make it less onerous to use in unit tests, DashChainstateSetup{,Close}() has been overloaded with a variant that accepts NodeContext.
To remove LogPrintf usage in node/chainstate.cpp, index enablement reporting has been pulled out of chainstate loading and BLS scheme reporting has been abstracted out using notify_bls_state.

Breaking Changes

None expected.

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas (note: N/A)
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation (note: N/A)
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

github-actions · 2024-10-04T19:23:23Z

This pull request has conflicts, please rebase.

github-actions · 2024-10-08T14:47:03Z

This pull request has conflicts, please rebase.

UdjinM6

LGTM, light ACK 1a0d1256968d79af39b57de98e9a9612c2ee78cb

nit: clang format complains a bit

kwvg · 2024-10-15T09:38:09Z

Current approach of allowing NodeContext seems to cause problems down the line, marking as draft until resolved

github-actions · 2024-10-21T16:34:50Z

This pull request has conflicts, please rebase.

…erministicMNManager`, `LLMQContext` member ctors, reduce use in `CJContext` 7d26061 refactor: move `CConnman`, `PeerManager` out of `CCoinJoinClientQueueManager` ctor (Kittywhiskers Van Gogh) 953ba96 refactor: move `CConnman` out of `CoinJoinWalletManager` ctor (Kittywhiskers Van Gogh) ac930a8 refactor: remove unused `CConnman` from `CDeterministicMNManager` ctor (Kittywhiskers Van Gogh) a14e604 refactor: remove `CConnman`, `PeerManager` from `LLMQContext` ctor (Kittywhiskers Van Gogh) d9e5cc7 refactor: move `PeerManager` out of `CInstantSendManager` ctor (Kittywhiskers Van Gogh) 82d1aed refactor: move `CConnman`, `PeerManager` out of `CSigSharesManager` ctor (Kittywhiskers Van Gogh) 7498a38 refactor: move `PeerManager` out of `CSigningManager` ctor (Kittywhiskers Van Gogh) 7ebc61e refactor: move `CConnman` out of `CQuorumManager` ctor (Kittywhiskers Van Gogh) c07b522 refactor: move `CConnman` out of `CDKGSession{,Handler,Manager}` ctor (Kittywhiskers Van Gogh) 01876c7 refactor: move `PeerManager` out of `CDKGSession{,Handler,Manager}` ctor (Kittywhiskers Van Gogh) cc0e771 refactor: start BLS thread in `LLMQContext` ctor, move `Start` downwards (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6425 * Dependency for #6304 * In order to reduce the logic used in chainstate initialization (that will be split out of `init.cpp` in [bitcoin#23280](bitcoin#23280)), the spinning up of threads (as done in `LLMQContext::Start()`) needs to be moved down. * They were moved up in [dash#5752](#5752) as `CBLSWorker` is a part of `LLMQContext` and `CBLSWorker` is needed during chainstate verification. As suggested in dash#5752, an alternate fix to the one already merged in was to move `CBLSWorker` `Start()`/`Stop()` to the constructor, which is done here. * Another alternate fix is that we move it out of `LLMQContext` entirely and let it remain in `NodeContext` though this approach has not been taken. * The reason we cannot retain the status quo is because `bitcoin-chainstate` (the binary introduced in [bitcoin#24304](bitcoin#24304) that's built on the code split off in [bitcoin#23280](bitcoin#23280)) aims to be devoid of P2P logic and this is reflected in the source files used to build it ([source](https://github.com/bitcoin/bitcoin/pull/24304/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R794)). (Also, there's no `NodeContext`, [source](https://github.com/bitcoin/bitcoin/pull/24304/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R795-R798)) * This means need to separate P2P and validation components from Dash-specific logic in order for the split to work as expected. This PR is a step in that direction by moving P2P elements (`CConnman` and `PeerManager`) out of constructors. * As it stands, there are two sources for Dash-specific components to have access to P2P components, initialization (e.g. through `LLMQContext::Start()` or `PeerManagerImpl::ProcessMessage()`). ## Breaking Changes None expected. While changes are present in initialization order, consensus behaviour should remain unchanged. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 7d26061 UdjinM6: utACK 7d26061 Tree-SHA512: 4f12cbda935cad3a186acb31fed513cea489b0d3a55aa80be8e1336d10cbe1579d6d3db862a78a167134650c9e97816732acaf0c85ab759f6555b1b6be99ec02

Match formatting with what upcoming commits expect to limit conflicts

UdjinM6

utACK 04523fc

one tine issue, see below

UdjinM6

utACK 1974651

PastaPastaPasta

utACK 1974651

…erministicMNManager`, `LLMQContext` member ctors, reduce use in `CJContext` f27f564 refactor: move `CConnman`, `PeerManager` out of `CCoinJoinClientQueueManager` ctor (Kittywhiskers Van Gogh) 1d2f62c refactor: move `CConnman` out of `CoinJoinWalletManager` ctor (Kittywhiskers Van Gogh) 465b0c0 refactor: remove unused `CConnman` from `CDeterministicMNManager` ctor (Kittywhiskers Van Gogh) c9e56e4 refactor: remove `CConnman`, `PeerManager` from `LLMQContext` ctor (Kittywhiskers Van Gogh) 8555ae5 refactor: move `PeerManager` out of `CInstantSendManager` ctor (Kittywhiskers Van Gogh) 7a4b4e4 refactor: move `CConnman`, `PeerManager` out of `CSigSharesManager` ctor (Kittywhiskers Van Gogh) bd6f2a4 refactor: move `PeerManager` out of `CSigningManager` ctor (Kittywhiskers Van Gogh) e2b81dd refactor: move `CConnman` out of `CQuorumManager` ctor (Kittywhiskers Van Gogh) 6c7da9b refactor: move `CConnman` out of `CDKGSession{,Handler,Manager}` ctor (Kittywhiskers Van Gogh) 6a4de36 refactor: move `PeerManager` out of `CDKGSession{,Handler,Manager}` ctor (Kittywhiskers Van Gogh) 0836130 refactor: start BLS thread in `LLMQContext` ctor, move `Start` downwards (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on dashpay/dash#6425 * Dependency for dashpay/dash#6304 * In order to reduce the logic used in chainstate initialization (that will be split out of `init.cpp` in [bitcoin#23280](bitcoin/bitcoin#23280)), the spinning up of threads (as done in `LLMQContext::Start()`) needs to be moved down. * They were moved up in [dash#5752](dashpay/dash#5752) as `CBLSWorker` is a part of `LLMQContext` and `CBLSWorker` is needed during chainstate verification. As suggested in dash#5752, an alternate fix to the one already merged in was to move `CBLSWorker` `Start()`/`Stop()` to the constructor, which is done here. * Another alternate fix is that we move it out of `LLMQContext` entirely and let it remain in `NodeContext` though this approach has not been taken. * The reason we cannot retain the status quo is because `bitcoin-chainstate` (the binary introduced in [bitcoin#24304](bitcoin/bitcoin#24304) that's built on the code split off in [bitcoin#23280](bitcoin/bitcoin#23280)) aims to be devoid of P2P logic and this is reflected in the source files used to build it ([source](https://github.com/bitcoin/bitcoin/pull/24304/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R794)). (Also, there's no `NodeContext`, [source](https://github.com/bitcoin/bitcoin/pull/24304/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R795-R798)) * This means need to separate P2P and validation components from Dash-specific logic in order for the split to work as expected. This PR is a step in that direction by moving P2P elements (`CConnman` and `PeerManager`) out of constructors. * As it stands, there are two sources for Dash-specific components to have access to P2P components, initialization (e.g. through `LLMQContext::Start()` or `PeerManagerImpl::ProcessMessage()`). ## Breaking Changes None expected. While changes are present in initialization order, consensus behaviour should remain unchanged. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK f27f564 UdjinM6: utACK f27f564 Tree-SHA512: 4f12cbda935cad3a186acb31fed513cea489b0d3a55aa80be8e1336d10cbe1579d6d3db862a78a167134650c9e97816732acaf0c85ab759f6555b1b6be99ec02

github-actions Bot added the needs rebase label Oct 4, 2024

kwvg added this to the 21.2 milestone Oct 5, 2024

kwvg force-pushed the bp_23280 branch from 5adc1a4 to 33355e4 Compare October 8, 2024 14:31

github-actions Bot removed the needs rebase label Oct 8, 2024

github-actions Bot added the needs rebase label Oct 8, 2024

kwvg force-pushed the bp_23280 branch from 33355e4 to 832ab1d Compare October 8, 2024 17:30

github-actions Bot removed the needs rebase label Oct 8, 2024

kwvg force-pushed the bp_23280 branch 2 times, most recently from da80bdd to 1a0d125 Compare October 9, 2024 16:13

kwvg modified the milestones: 21.2, 22 Oct 9, 2024

kwvg marked this pull request as ready for review October 10, 2024 06:17

kwvg requested review from PastaPastaPasta, UdjinM6 and knst October 10, 2024 06:17

UdjinM6 previously approved these changes Oct 10, 2024

View reviewed changes

kwvg marked this pull request as draft October 15, 2024 09:38

github-actions Bot added the needs rebase label Oct 21, 2024

PastaPastaPasta modified the milestones: 22, 22.1 Nov 14, 2024

kwvg mentioned this pull request Dec 2, 2024

refactor: move CConnman and PeerManager out of CDeterministicMNManager, LLMQContext member ctors, reduce use in CJContext #6443

Merged

5 tasks

kwvg added 4 commits December 15, 2024 09:10

chore: sync chainstate loading logic with upstream

620146b

Match formatting with what upcoming commits expect to limit conflicts

node: Extract chainstate loading sequence

2455c06

node/chainstate: Decouple from GetTimeMillis

9ab08c4

node/chainstate: Decouple from stringy errors

d7419e4

kwvg requested review from PastaPastaPasta and UdjinM6 December 15, 2024 12:49

UdjinM6 previously approved these changes Dec 16, 2024

View reviewed changes

Comment thread src/node/chainstate.cpp Outdated

kwvg added 20 commits December 16, 2024 12:19

node/chainstate: Decouple from concept of NodeContext

7071282

Move mempool nullptr Assert out of LoadChainstate

29c7362

Move init logistics message for BAD_GENESIS_BLOCK to init.cpp

2ea1bbc

node/chainstate: Remove do/while loop

53231ca

Split off VerifyLoadedChainstate

913411e

node/chainstate: Decouple from concept of uiInterface

a141f5d

node/chainstate: Reduce coupling of LogPrintf

d3345ee

Move -checkblocks LogPrintf to AppInitMain

94c0ceb

init: Delay RPC block notif until warmup finished

f7aef8d

node/chainstate: Decouple from GetTime

fdf803d

node/chainstate: Decouple from ShutdownRequested

c405492

validation: VerifyDB only needs Consensus::Params

d7f1e23

node/caches: Extract cache calculation logic

4ab1827

node/caches: Remove intermediate variables

52bb35d

node/chainstate: Add options for in-memory DBs

c06e074

node/chainstate: extract Dash post-InitializeChainstate logic

459f339

test/setup: Use LoadChainstate

09ab629

Remove all #include // for * comments

872158d

style-only: Remove redundant scope in *Chainstate

04dbaa8

refactor: move remaining LogPrintf usage outside

1974651

kwvg dismissed UdjinM6’s stale review via 1974651 December 16, 2024 12:41

kwvg force-pushed the bp_23280 branch from 04523fc to 1974651 Compare December 16, 2024 12:41

kwvg requested a review from UdjinM6 December 16, 2024 12:41

UdjinM6 approved these changes Dec 16, 2024

View reviewed changes

PastaPastaPasta approved these changes Dec 17, 2024

View reviewed changes

PastaPastaPasta merged commit a5d5400 into dashpay:develop Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

backport: merge bitcoin#23280 (coalesce chainstate loading sequence between {,non-}unittest codepaths)#6304

backport: merge bitcoin#23280 (coalesce chainstate loading sequence between {,non-}unittest codepaths)#6304
PastaPastaPasta merged 25 commits into
dashpay:developfrom
kwvg:bp_23280

kwvg commented Oct 3, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 4, 2024

Uh oh!

github-actions Bot commented Oct 8, 2024

Uh oh!

UdjinM6 left a comment

Uh oh!

kwvg commented Oct 15, 2024

Uh oh!

github-actions Bot commented Oct 21, 2024

Uh oh!

UdjinM6 left a comment

Uh oh!

Uh oh!

UdjinM6 left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kwvg commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Information

Breaking Changes

Checklist

Uh oh!

github-actions Bot commented Oct 4, 2024

Uh oh!

github-actions Bot commented Oct 8, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg commented Oct 15, 2024

Uh oh!

github-actions Bot commented Oct 21, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kwvg commented Oct 3, 2024 •

edited

Loading