refactor: move `CConnman` and `PeerManager` out of `CDeterministicMNManager`, `LLMQContext` member ctors, reduce use in `CJContext` by kwvg · Pull Request #6443 · dashpay/dash

kwvg · 2024-12-02T20:01:33Z

Additional Information

Depends on refactor: move CInstantSendManager::AskNodesForLockedTx into PeerManager #6425
Dependency for backport: merge bitcoin#23280 (coalesce chainstate loading sequence between {,non-}unittest codepaths) #6304
In order to reduce the logic used in chainstate initialization (that will be split out of init.cpp in bitcoin#23280), the spinning up of threads (as done in LLMQContext::Start()) needs to be moved down.
- They were moved up in 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 that's built on the code split off in bitcoin#23280) aims to be devoid of P2P logic and this is reflected in the source files used to build it (source). (Also, there's no NodeContext, source)
  - 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:

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 (note: N/A)
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-12-03T14:38:01Z

This pull request has conflicts, please rebase.

kwvg · 2024-12-04T18:48:50Z

Latest changes are whitespace only (applying clang-format-diff.py suggestions on a commit-by-commit basis)

knst

LGTM e5e3218

nits can be addressed in follow-up PR

knst · 2024-12-05T17:41:38Z

src/llmq/dkgsessionhandler.h

+    void ProcessMessage(const CNode& pfrom, PeerManager& peerman, const std::string& msg_type, CDataStream& vRecv);

-    void StartThread();
+    void StartThread(PeerManager& peerman);


that's a good solution to pass PeerManager when thread start. Wow!

src/llmq/signing.cpp

src/llmq/signing_shares.cpp

src/llmq/signing_shares.h

src/dsnotificationinterface.cpp

src/evo/deterministicmns.h

Alternate fix as proposed in dash#5752, needed because dependencies for threaded logic will be pulled out of ctor in upcoming commits and that needs `Start` to be pushed downwards so we can avoid having to pass `unique_ptr` references.

Co-authored-by: Konstantin Akimov <knstqq@gmail.com>

…Manager` ctor

kwvg · 2024-12-05T22:45:55Z

First push rebased on develop (1e55310) to avoid linter errors from not having header guard formatting as required by #6429, incorporated review comments and commit suggestions. Second push applied clang-format-diff.py suggestions.

knst

utACK 7d26061

range-diff looks good

UdjinM6

utACK 7d26061

…ng sequence between {,non-}unittest codepaths) 1974651 refactor: move remaining `LogPrintf` usage outside (Kittywhiskers Van Gogh) 04dbaa8 style-only: Remove redundant scope in *Chainstate (Kittywhiskers Van Gogh) 872158d Remove all #include // for * comments (Kittywhiskers Van Gogh) 09ab629 test/setup: Use LoadChainstate (Kittywhiskers Van Gogh) 459f339 node/chainstate: extract Dash post-`InitializeChainstate` logic (Kittywhiskers Van Gogh) c06e074 node/chainstate: Add options for in-memory DBs (Kittywhiskers Van Gogh) 52bb35d node/caches: Remove intermediate variables (Kittywhiskers Van Gogh) 4ab1827 node/caches: Extract cache calculation logic (Kittywhiskers Van Gogh) d7f1e23 validation: VerifyDB only needs Consensus::Params (Kittywhiskers Van Gogh) c405492 node/chainstate: Decouple from ShutdownRequested (Kittywhiskers Van Gogh) fdf803d node/chainstate: Decouple from GetTime (Kittywhiskers Van Gogh) f7aef8d init: Delay RPC block notif until warmup finished (Kittywhiskers Van Gogh) 94c0ceb Move -checkblocks LogPrintf to AppInitMain (Kittywhiskers Van Gogh) d3345ee node/chainstate: Reduce coupling of LogPrintf (Kittywhiskers Van Gogh) a141f5d node/chainstate: Decouple from concept of uiInterface (Kittywhiskers Van Gogh) 913411e Split off VerifyLoadedChainstate (Kittywhiskers Van Gogh) 53231ca node/chainstate: Remove do/while loop (Kittywhiskers Van Gogh) 2ea1bbc Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (Kittywhiskers Van Gogh) 29c7362 Move mempool nullptr Assert out of LoadChainstate (Kittywhiskers Van Gogh) 7071282 node/chainstate: Decouple from concept of NodeContext (Kittywhiskers Van Gogh) ee9d3dd node/chainstate: Decouple from ArgsManager (Kittywhiskers Van Gogh) d7419e4 node/chainstate: Decouple from stringy errors (Kittywhiskers Van Gogh) 9ab08c4 node/chainstate: Decouple from GetTimeMillis (Kittywhiskers Van Gogh) 2455c06 node: Extract chainstate loading sequence (Kittywhiskers Van Gogh) 620146b chore: sync chainstate loading logic with upstream (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6296 * Dependent on #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](#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](https://github.com/bitcoin/bitcoin/pull/24304/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R795-R798)), 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 - [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 - [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: UdjinM6: utACK 1974651 PastaPastaPasta: utACK 1974651 Tree-SHA512: 8ebfce48dccc6a867339aff9374a4cb8dc7b02b0c17432db2b97c982523d0d9589a87e6527a103f314bf32be176486311ef6dcde1c4d5cdccc1eeb1f80bbb040

kwvg modified the milestones: 22.1, 22 Dec 2, 2024

github-actions bot added the needs rebase label Dec 3, 2024

kwvg force-pushed the conn_split branch from bcc310d to 68a7f92 Compare December 4, 2024 18:07

github-actions bot removed the needs rebase label Dec 4, 2024

kwvg force-pushed the conn_split branch from 68a7f92 to e5e3218 Compare December 4, 2024 18:38

kwvg marked this pull request as ready for review December 4, 2024 18:47

kwvg requested review from PastaPastaPasta, UdjinM6 and knst December 4, 2024 19:10

knst previously approved these changes Dec 5, 2024

View reviewed changes

kwvg added 5 commits December 5, 2024 22:37

refactor: move PeerManager out of CDKGSession{,Handler,Manager} ctor

01876c7

refactor: move CConnman out of CDKGSession{,Handler,Manager} ctor

c07b522

refactor: move CConnman out of CQuorumManager ctor

7ebc61e

refactor: move PeerManager out of CSigningManager ctor

7498a38

kwvg dismissed knst’s stale review via 39df678 December 5, 2024 22:41

kwvg force-pushed the conn_split branch from e5e3218 to 39df678 Compare December 5, 2024 22:41

kwvg and others added 6 commits December 5, 2024 22:42

refactor: move CConnman, PeerManager out of CSigSharesManager ctor

82d1aed

Co-authored-by: Konstantin Akimov <knstqq@gmail.com>

refactor: move PeerManager out of CInstantSendManager ctor

d9e5cc7

refactor: remove CConnman, PeerManager from LLMQContext ctor

a14e604

refactor: remove unused CConnman from CDeterministicMNManager ctor

ac930a8

refactor: move CConnman out of CoinJoinWalletManager ctor

953ba96

refactor: move CConnman, PeerManager out of `CCoinJoinClientQueue…

7d26061

…Manager` ctor

kwvg force-pushed the conn_split branch from 39df678 to 7d26061 Compare December 5, 2024 22:42

kwvg requested a review from knst December 6, 2024 05:37

knst approved these changes Dec 6, 2024

View reviewed changes

UdjinM6 approved these changes Dec 7, 2024

View reviewed changes

PastaPastaPasta merged commit ff1ddc9 into dashpay:develop Dec 9, 2024

kwvg mentioned this pull request Dec 15, 2024

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

refactor: move `CConnman` and `PeerManager` out of `CDeterministicMNManager`, `LLMQContext` member ctors, reduce use in `CJContext`#6443
PastaPastaPasta merged 11 commits intodashpay:developfrom
kwvg:conn_split

kwvg commented Dec 2, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Dec 3, 2024

Uh oh!

kwvg commented Dec 4, 2024 •

edited

Loading

Uh oh!

knst left a comment

Uh oh!

knst Dec 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwvg commented Dec 5, 2024 •

edited

Loading

Uh oh!

knst left a comment

Uh oh!

UdjinM6 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kwvg commented Dec 2, 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 Dec 3, 2024

Uh oh!

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

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwvg commented Dec 2, 2024 •

edited

Loading

kwvg commented Dec 4, 2024 •

edited

Loading

kwvg commented Dec 5, 2024 •

edited

Loading