refactor: remove circular dependencies through net_processing (1/N) #5782

knst · 2023-12-21T20:46:18Z

Issue being fixed or feature implemented

The architecture of bitcoin assumes that there's no any external class that processes network messages and knows anything about PeerManager from net_processing; no any external call for PeerManager::Misbehaving in bitcoin. All logic related to processing messages are located in net_processing.
Dash has many many extra types of network messages and many of them processed by external components such as llmq/signing or coinjoin/client. Current architecture creates multiple circular dependency.

What was done?

This PR introduces several key changes:

new helper tl::expected that can be widely used in dash code and it has very similar implementation with std::expected and can be replaced to std::expected since C++23 is used by default. Please notice that this PR will superseed refactor: convert maybe_error into generic Result template class #5109
new helper PeerMsgRet that is defined as tl::expected<void, MisbehavingError> inside net_types. That lets external modules that process network messages to don't depend directly on net_processing.
removed dependency of many classes on net_processing thanks to new PeerMsgRet type: coinjoin/client, coinjoin/server, evo/mnauth, governance/governance, llmq/blockprocessor, llmq/chainlocks, llmq/quorums, spork

What else to do?

Some network messages are processed asynchronously in external components such as llmq/signing, llmq/instantsend, llmq/dkgsessionhandler. It doesn't let to refactor them easily, because they can't just simple return status of processing; status of processing would be available sometime later and there's need callback or other way to pass result code without spreading PeerManager over codebase.

How Has This Been Tested?

run unit functional tests
run a linter test/lint/lint-circular-dependencies.sh

Breaking Changes

N/A

Checklist:

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

PastaPastaPasta · 2023-12-22T04:52:52Z

src/net_types.h

+    int score;
+    std::string message;
+
+    MisbehavingError(int score, std::string message = "") :


I think I would propose dropping the = "" and requiring all invocations provide a message; maybe a future refac however

Most of places doesn't have a text message.
Also for case of text message it's little too verbose: need to write tl::expected(MisbehavingError>{...})

For other usages (not net_processing) I'd agree if error usually provides text description

PastaPastaPasta · 2023-12-22T04:57:51Z

src/net_types.h

+    MisbehavingError(int score, std::string message = "") :
+        score{score},
+        message{std::move(message)}


What if we made this a std::string&& to ensure that this is (probably) a compile time constant? Thoughts? Would also need to make std::move into std::forward

updated, please check

PastaPastaPasta · 2023-12-22T05:01:58Z

Otherwise LGTM

github-actions · 2023-12-22T05:07:31Z

This pull request has conflicts, please rebase.

github-actions · 2023-12-22T05:07:31Z

This pull request has conflicts, please rebase.

PastaPastaPasta

utACK

github-actions · 2024-01-10T03:33:56Z

This pull request has conflicts, please rebase.

PastaPastaPasta

re-utACK; please state preference for merge commit vs squash

knst · 2024-01-10T15:22:05Z

please state preference for merge commit vs squash

either way works, commits are each atomic and complete.

One things that may worth an own commit: tl::expected but I am good with squashed

UdjinM6 · 2024-01-10T15:25:27Z

src/util/expected.h

  expected_storage_base() : m_has_val(true) {}
-     
+


3064790: should ignore src/util/expected.h in linters instead - 7515f9a

github-actions · 2024-01-10T18:08:20Z

This pull request has conflicts, please rebase.

knst · 2024-01-10T18:56:25Z

resolved conflicts and added expections for linter.

UdjinM6

utACK

PastaPastaPasta

utACK for merging via merge commit

Source: https://github.com/TartanLlama/expected Also it adds util/expected.h to exclude list of our linters

That's a prior work for removing circular dependencies over net_processing

These functions: - EraseObjectRequest - RequestObject - GetRequestedObjectCount

It is partial de-circularisation of dependencies between that includes net_processing Classes that still depends on net_processing but should not: - llmq/dkgsessionmgr - llmq/signing - llmq/instantsend They have asynchronous processing and with current impl that's impossible to do

…5792) ## Issue being fixed or feature implemented The architecture of bitcoin assumes that there's no any external class that processes network messages and knows anything about PeerManager from net_processing; no any external call for PeerManager::Misbehaving in bitcoin. All logic related to processing messages are located in net_processing. Dash has many many extra types of network messages and many of them processed by external components such as llmq/signing or coinjoin/client. Current architecture creates multiple circular dependency. ## What was done? That's part II of refactorings. This PR removes PeerManager from several constructor and let LLMQContext to forget about PeerManager. Prior work in this PR: #5782 ## What else to do? Some network messages are processed asynchronously in external components such as llmq/signing, llmq/instantsend, llmq/dkgsessionhandler. It doesn't let to refactor them easily, because they can't just simple return status of processing; status of processing would be available sometime later and there's need callback or other way to pass result code without spreading PeerManager over codebase. ## How Has This Been Tested? - Run unit/functional tests - run a linter test/lint/lint-circular-dependencies.sh ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] 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 - [x] I have assigned this pull request to a milestone

…ction c7c7ee9 test: Check max transaction weight in CoinSelection (Aurèle Oulès) 6b563ca wallet: Check max tx weight in coin selector (Aurèle Oulès) Pull request description: This PR is an attempt to fix dashpay#5782. I have added 4 test scenarios, 3 of them provided here bitcoin#5782 (comment) (slightly modified to use a segwit wallet). Here are my benchmarks : ## PR | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 1,466,341.00 | 681.97 | 0.6% | 11,176,762.00 | 3,358,752.00 | 3.328 | 1,897,839.00 | 0.3% | 0.02 | `CoinSelection` ## Master | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 1,526,029.00 | 655.30 | 0.5% | 11,142,188.00 | 3,499,200.00 | 3.184 | 1,994,156.00 | 0.2% | 0.02 | `CoinSelection` ACKs for top commit: achow101: reACK c7c7ee9 w0xlt: ACK bitcoin@c7c7ee9 furszy: diff ACK c7c7ee9 Tree-SHA512: ef0b28576ff845174651ba494aa9adee234c96e6f886d0e032eceb7050296e45b099dda0039d1dfb9944469f067627b2101f3ff855c70353cf39d1fc7ee81828

…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

knst added this to the 20.1 milestone Dec 21, 2023

knst requested review from ogabrielides, UdjinM6 and PastaPastaPasta December 21, 2023 20:46

knst force-pushed the fix-circular-net-processing branch 2 times, most recently from 92c7b34 to 90aa991 Compare December 21, 2023 21:39

PastaPastaPasta reviewed Dec 22, 2023

View reviewed changes

github-actions bot added the needs rebase label Dec 22, 2023

knst force-pushed the fix-circular-net-processing branch from 90aa991 to 78e3eb4 Compare December 22, 2023 09:00

github-actions bot removed the needs rebase label Dec 22, 2023

knst force-pushed the fix-circular-net-processing branch from 78e3eb4 to e10199d Compare December 22, 2023 11:15

knst requested a review from PastaPastaPasta December 26, 2023 13:21

This comment was marked as duplicate.

Sign in to view

PastaPastaPasta previously approved these changes Jan 6, 2024

View reviewed changes

github-actions bot added the needs rebase label Jan 10, 2024

knst dismissed PastaPastaPasta’s stale review via 7673bc6 January 10, 2024 09:20

knst force-pushed the fix-circular-net-processing branch from e10199d to 7673bc6 Compare January 10, 2024 09:20

github-actions bot removed the needs rebase label Jan 10, 2024

PastaPastaPasta previously approved these changes Jan 10, 2024

View reviewed changes

UdjinM6 requested changes Jan 10, 2024

View reviewed changes

github-actions bot added the needs rebase label Jan 10, 2024

knst dismissed PastaPastaPasta’s stale review via bbb342e January 10, 2024 18:55

knst force-pushed the fix-circular-net-processing branch from 7673bc6 to bbb342e Compare January 10, 2024 18:55

UdjinM6 approved these changes Jan 10, 2024

View reviewed changes

knst requested a review from PastaPastaPasta January 10, 2024 19:09

github-actions bot removed the needs rebase label Jan 10, 2024

PastaPastaPasta approved these changes Jan 10, 2024

View reviewed changes

knst added 13 commits January 10, 2024 15:12

refactor: removed unused PeerMan from several classes

d3da62f

feat: new util class expected for return errors by more convenient way

bb1d46c

Source: https://github.com/TartanLlama/expected Also it adds util/expected.h to exclude list of our linters

refactor: new helpers in net_processing for external handlers

051cdb3

That's a prior work for removing circular dependencies over net_processing

refactor: coinjoin/client no more depends on net_processing

1681eb8

refactor: coinjoin/server no more depends on net_processing

91eca51

refactor: spork no more depends on net_processing

08d1b35

refactor: moved net Object's helpers from net_processing to net.h

fdcc1b7

These functions: - EraseObjectRequest - RequestObject - GetRequestedObjectCount

refactor: governance/governance no more depends on net_processing

3e31f29

refactor: evo/mnauth no more depends on net_processing

6ffc745

refactor: llmq/chainlocks no more depends on net_processing

f613f34

refactor: llmq/blockprocessor no more depends on net_processing

c6a21bb

refactor: llmq/quorums no more depends on net_processing

f4fd143

PastaPastaPasta force-pushed the fix-circular-net-processing branch from bbb342e to a9401cc Compare January 10, 2024 21:12

PastaPastaPasta merged commit 95fad52 into dashpay:develop Jan 10, 2024

knst mentioned this pull request Jan 13, 2024

refactor: convert maybe_error into generic Result template class #5109

Closed

5 tasks

kwvg mentioned this pull request Aug 15, 2025

refactor: migrate ProcessMessage definitions to MessageProcessingResult, drop PeerMsgRet #6820

Merged

5 tasks

refactor: remove circular dependencies through net_processing (1/N) #5782

refactor: remove circular dependencies through net_processing (1/N) #5782

Uh oh!

Conversation

knst commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

What else to do?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

PastaPastaPasta Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

knst Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

knst Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Dec 22, 2023

Uh oh!

github-actions bot commented Dec 22, 2023

Uh oh!

github-actions bot commented Dec 22, 2023

Uh oh!

This comment was marked as duplicate.

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 10, 2024

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

knst commented Jan 10, 2024

Uh oh!

UdjinM6 Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 10, 2024

Uh oh!

knst commented Jan 10, 2024

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!

Uh oh!

knst commented Dec 21, 2023 •

edited

Loading

knst Dec 22, 2023 •

edited

Loading