refactor: reduce PeerManager usage in InstantSend worker functions
#6821
+83
−55
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 18, 2025
…geProcessingResult`, drop `PeerMsgRet` 491ae50 revert: new util class `expected` for return errors by more convenient way (Kittywhiskers Van Gogh) 2020757 trivial: remove `PeerMsgRet` handling logic (Kittywhiskers Van Gogh) 5d1222e chore: drop govobj/govobjvote counts, use `ret.m_inventory.size()` (UdjinM6) 4822b93 refactor: allow submitting multiple `CInv`s to `MessageProcessingResult` (Kittywhiskers Van Gogh) 0517aff refactor: mark `RelayInv{,Filtered}`'s `CInv` argument as const (Kittywhiskers Van Gogh) 9084530 refactor: migrate `CGovernanceManager::ProcessMessage()` and friends (Kittywhiskers Van Gogh) 8b9bf7c refactor: migrate `CCoinJoinServer::ProcessMessage()` (Kittywhiskers Van Gogh) 1d50a1a refactor: migrate `CCoinJoinClientQueueManager::ProcessMessage()` (Kittywhiskers Van Gogh) 6352baf refactor: migrate `CMNAuth::ProcessMessage()` (Kittywhiskers Van Gogh) 9f85bd0 refactor: migrate `CInstantSendManager::ProcessMessage()` (Kittywhiskers Van Gogh) ef4a0bb refactor: migrate `CDKGSessionManager::ProcessMessage()` and friends (Kittywhiskers Van Gogh) 93d68b8 refactor: migrate `CQuorumManager::ProcessMessage()` (Kittywhiskers Van Gogh) 1bbcb95 refactor: migrate `CSporkManager::ProcessMessage()` and friends (Kittywhiskers Van Gogh) e7b23a2 refactor: migrate `CSigningManager::ProcessMessage()` (Kittywhiskers Van Gogh) f3b98ce chore: add redefinition of `NodeId` to avoid repetitive redefinition (Kittywhiskers Van Gogh) f341ef5 chore: add `nodiscard` attrib to `MessageProcessingResult` ret functions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6821 * To avoid having to redeclare `NodeId` repeatedly to avoid circular dependencies from including `net.h` ([source](https://github.com/dashpay/dash/blob/fb87a5a937dd232d4f2ae60b5b8c5c29d49cb92f/src/net.h#L121)), a redeclaration is made in the relatively lightweight `net_types.h` that as of `develop` (fb87a5a), doesn't have includes outside the standard library ([source](https://github.com/dashpay/dash/blob/fb87a5a937dd232d4f2ae60b5b8c5c29d49cb92f/src/net_types.h#L8-L10)). Redeclarations were flagged during review (see [comment](#6820 (comment)), [comment](#6820 (comment))). * As `tl::expected` was introduced alongside `PeerMsgRet` in [dash#5782](#5782) and has not been used elsewhere, we can safely remove it as we've finished migrating all usage to `MessageProcessingResult`. ## 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: knst: utACK 491ae50 UdjinM6: utACK 491ae50 Tree-SHA512: c37fe6370555f1c33877fa5a335356e2febf25c2d0d954fa44623f49994647ea2a10aec5ceb9e788bb6fa9315dc00b3e54ca8e0da96c7899fb31c30b888eb732
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 19, 2025
…ons, ChainLock and InstantSend refactoring 34a90e6 chore: remove unused `IsInvInFilter` from `PeerManager` interface (Kittywhiskers Van Gogh) f3224ae refactor: consolidate `INPUTLOCK_REQUESTID_PREFIX` usage to `lock.cpp` (Kittywhiskers Van Gogh) 7b4ee6b trivial: document transaction confirmation safety threshold (Kittywhiskers Van Gogh) 25f05c1 refactor: make unknown block clsig flow easier to follow (Kittywhiskers Van Gogh) 9578146 refactor: document `pindex` assumptions in chainlocks code (Kittywhiskers Van Gogh) 4a744c7 refactor: use `std::chrono` for time variables, reduce resolution (Kittywhiskers Van Gogh) b051c22 refactor: consolidate `CLSIG_REQUESTID_PREFIX` usage to `clsig.cpp` (Kittywhiskers Van Gogh) 024b466 chore: move lock annotations in `chainlock.h` to the next line (Kittywhiskers Van Gogh) c6e99fb chore: apply most `clang-format` suggestions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6761 * Dependency for #6821 * Assumptions surrounding `pindex` usage have been documented in response to reviewer comments in [dash#6761](#6761) ([comment](#6761 (comment)), [comment](#6761 (comment))). * The internal structures of `CChainLocksHandler` (`txFirstSeenTime`, `seenChainLocks`, `lastCleanupTime`) have their time resolution reduced from milliseconds to seconds while migrating to `std::chrono`. * `CInstantSendManager::AskNodesForLockedTx()` was moved as `PeerManagerImpl::AskPeersForTransaction()` in [dash#6425](#6425) and with that, the sole external usage of `PeerManagerImpl::IsInvInFilter()` was moved internally, we can therefore safely remove it from the `PeerManager` interface. ## 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 - [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: UdjinM6: utACK 34a90e6 Tree-SHA512: 28b532cb5b8b5e6d7c1331c7c6c0ecd4d45b186922c279db6d2d3e8974d422ec8b67d75aeadce77986d409a8ed071e85359ee08609e0c2dde657e4520c546817
PeerManagerImpl already knows if we're a masternode or not, we don't need to accept it as an argument.
So far we cannot stop using `PeerManager` entirely the triggers are internal (i.e. not coming from P2P interactions) but we can extend and embrace MessageProcessingResult, to at least isolate its usage as best we can. Co-Authored-By: Konstantin Akimov <[email protected]>
With their usage being brokered by `MessageProcessingResult`, we can remove them from the interface and keep them as implementation details.
RequestObject is not used outside PeerManagerImpl and EraseObjectRequest is now brokered by `MessageProcessingResult`, so we can remove both from the interface and keep them as implementation details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additional Information
Depends on refactor: follow-up dash#6761, implement review suggestions, ChainLock and InstantSend refactoring #6815
Depends on refactor: migrate
ProcessMessagedefinitions toMessageProcessingResult, dropPeerMsgRet#6820While this pull request doesn't entirely decouple
PeerManagerfromCInstantSendManageras the worker thread isn't reactive (i.e. triggered byProcessMessage()), we can reduce its usage by folding all expected network activity into aMessageProcessingResultbatch that can be consumed byPostProcessMessage.To achieve this,
MessageProcessingResulthas been extended to allowm_request_tx)m_inv_filter)This has the additional effect of removing external uses of
RelayInvFiltered()andAskPeersForTransaction(), which can now be safely removed from thePeerManagerinterface as it is now brokered throughMessageProcessingResult.Note that while the old
RelayInvFiltered()allowed specifying any minimum protocol version,PostProcessMessage()will assume thatminProtoVersionisISDLOCK_PROTO_VERSION. This is acceptable asRelayInvFiltered()isn't used anywhere else and deterministic InstantSend locks were introduced in v0.18 (see dash#4381).If there is any foreseeable use needing to relay messages to even older versions of Dash Core,
m_inv_filtercan be modified to accommodate that.Breaking Changes
None expected.
Checklist