Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: move CInstantSendManager::AskNodesForLockedTx into PeerManager #6425

Conversation

PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Instantsend manager currently relies on CConnMan, which is not needed. The function AskNodesForLockedTx is all networking logic anyhow, and these no reason why this logic would be contained to instantsend processing. Move it into net_processing instead.

What was done?

This does change the logic! We no longer prioritize asking MNs. This is probably fine? I don't specifically recall why we wanted to ask MNs besides potentially that they may be higher performing or better connected? We can potentially restore this logic once we bring masternode connection logic into Peer

Does also change logic, by short-circuiting once peersToAsk is full.

This commit has the added benefit of reducing contention on m_nodes_mutex due to no-longer calling connman.ForEachNode not once but twice

This may slightly increase contention on m_peer_mutex; but that should be an ok tradeoff for not only removing dependencies, but also reducing contention on a much more contested RecursiveMutex

How Has This Been Tested?

Built, local tests

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

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

src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
@UdjinM6
Copy link

UdjinM6 commented Nov 22, 2024

clang-format complains:

--- src/llmq/context.cpp	(before formatting)
+++ src/llmq/context.cpp	(after formatting)
@@ -46,9 +46,9 @@
     isman{[&]() -> llmq::CInstantSendManager* const {
         assert(llmq::quorumInstantSendManager == nullptr);
         llmq::quorumInstantSendManager = std::make_unique<llmq::CInstantSendManager>(*llmq::chainLocksHandler,
-                                                                                     chainman.ActiveChainstate(),
-                                                                                     *qman, *sigman, *shareman,
-                                                                                     sporkman, mempool, mn_sync, peerman,
+                                                                                     chainman.ActiveChainstate(), *qman,
+                                                                                     *sigman, *shareman, sporkman,
+                                                                                     mempool, mn_sync, peerman,
                                                                                      is_masternode, unit_tests, wipe);
         return llmq::quorumInstantSendManager.get();
     }()},
--- src/llmq/instantsend.h	(before formatting)
+++ src/llmq/instantsend.h	(after formatting)
@@ -253,[13](https://github.com/dashpay/dash/actions/runs/11979610767/job/33402154445?pr=6425#step:4:14) +253,21 @@
     std::unordered_set<uint256, StaticSaltedHasher> pendingRetryTxs GUARDED_BY(cs_pendingRetry);
 
 public:
-    explicit CInstantSendManager(CChainLocksHandler& _clhandler, CChainState& chainstate,
-                                 CQuorumManager& _qman, CSigningManager& _sigman, CSigSharesManager& _shareman,
-                                 CSporkManager& sporkman, CTxMemPool& _mempool, const CMasternodeSync& mn_sync,
-                                 const std::unique_ptr<PeerManager>& peerman, bool is_masternode, bool unitTests, bool fWipe) :
+    explicit CInstantSendManager(CChainLocksHandler& _clhandler, CChainState& chainstate, CQuorumManager& _qman,
+                                 CSigningManager& _sigman, CSigSharesManager& _shareman, CSporkManager& sporkman,
+                                 CTxMemPool& _mempool, const CMasternodeSync& mn_sync,
+                                 const std::unique_ptr<PeerManager>& peerman, bool is_masternode, bool unitTests,
+                                 bool fWipe) :
         db(unitTests, fWipe),
-        clhandler(_clhandler), m_chainstate(chainstate), qman(_qman), sigman(_sigman),
-        shareman(_shareman), spork_manager(sporkman), mempool(_mempool), m_mn_sync(mn_sync), m_peerman(peerman),
+        clhandler(_clhandler),
+        m_chainstate(chainstate),
+        qman(_qman),
+        sigman(_sigman),
+        shareman(_shareman),
+        spork_manager(sporkman),
+        mempool(_mempool),
+        m_mn_sync(mn_sync),
+        m_peerman(peerman),
         m_is_masternode{is_masternode}
     {
         workInterrupt.reset();

@PastaPastaPasta PastaPastaPasta force-pushed the refac-move-request-tx-net-proc branch from 880280b to 1e74ad2 Compare November 22, 2024 21:51
Copy link

This pull request has conflicts, please rebase.

Copy link

This pull request has conflicts, please rebase.

**This does change the logic!** We no longer prioritize asking MNs. This is probably fine? I don't specifically recall why we wanted to ask MNs besides potentially that they may be higher performing or better connected? We can potentially restore this logic once we bring masternode connection logic into Peer

Does also change logic, by short-circuiting once peersToAsk is full.

This commit has the added benefit of reducing contention on m_nodes_mutex due to no-longer calling connman.ForEachNode not once but twice

This may slightly increase contention on m_peer_mutex; but that should be an ok tradeoff for not only removing dependencies, but also reducing contention on a much more contested RecursiveMutex
@PastaPastaPasta PastaPastaPasta force-pushed the refac-move-request-tx-net-proc branch from 1e74ad2 to 090ae92 Compare November 22, 2024 21:55
UdjinM6
UdjinM6 previously approved these changes Nov 22, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 090ae92

@PastaPastaPasta PastaPastaPasta requested review from knst and kwvg November 24, 2024 20:32
kwvg
kwvg previously approved these changes Nov 25, 2024
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 090ae92

src/net_processing.cpp Outdated Show resolved Hide resolved
@PastaPastaPasta PastaPastaPasta dismissed stale reviews from kwvg and UdjinM6 via 30fc76c December 2, 2024 04:42
src/net_processing.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bb5d70c

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK bb5d70c

@PastaPastaPasta PastaPastaPasta merged commit 89ca1ae into dashpay:develop Dec 4, 2024
31 checks passed
PastaPastaPasta added a commit that referenced this pull request Dec 9, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants