refactor: remove dependency of llmq/chainlocks, llmq/quorum_block_processor, ehf_signals on PeerManager by knst · Pull Request #6292 · dashpay/dash

knst · 2024-09-27T05:21:33Z

Issue being fixed or feature implemented

It reduces circular dependencies and simplify code.

What was done?

Removed circular dependency over PeerManager for llmq::QuorumBlockProcessor, llmq::CEhfSignals, llmq::ChainLocks, and several extra useful refactorings: see commits.

How Has This Been Tested?

Run 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 · 2024-10-01T15:02:39Z

src/llmq/dkgsessionhandler.cpp

nit: could combine

PastaPastaPasta · 2024-10-01T15:03:49Z

src/llmq/ehf_signals.cpp

595c4cd88b0935e17ffcdcd096a180d444b48cc0 this commit shouldn't be included here? If it's a fix? I'm confused by it

PastaPastaPasta

generally LGTM, don't love 5e489bc9c6ba33c8caf42649773f68e4a82b1651 feels like it makes struct MessageProcessingResult too complicated

github-actions · 2024-10-01T19:00:26Z

This pull request has conflicts, please rebase.

…atic function

…r dependency over PeerManager

knst · 2024-10-03T09:38:54Z

@knst knst force-pushed the fix-circular-net-processing-p3 branch from 5e489bc to 5739e77

1st force-push to resolve conflict

@knst knst force-pushed the fix-circular-net-processing-p3 branch from 5739e77 to 32683b3

2nd force-push to address comments

UdjinM6 · 2024-10-03T11:01:47Z

build fails

UdjinM6

utACK d0f1778

PastaPastaPasta · 2024-10-04T18:07:27Z

src/llmq/chainlocks.cpp

 }

-void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)
+MessageProcessingResult CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)


I'm confused, this method doesn't doesn't return any error values?

HandleNewRecoveredSig is override of interface method

[[nodiscard]] virtual MessageProcessingResult HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) = 0;

which belongs to CRecoveredSigsListener.
Some classes which implements CRecoveredSigsListener has errors to return, some - doesn't. See other commits, for example: https://github.com/dashpay/dash/pull/6292/files/#diff-f7c0d20797337d77a9cfbf40c4f627ec68ed5a12e2f1193fa91c32cf285d9e34R526 - returns MisbehavingError when ProcessNewChainlock or https://github.com/dashpay/dash/pull/6292/files/#diff-93616b1f609ab7dc32d07bcdd3de3611d59bb285cbd6dba5a3bc5b4c70e6c867R131 - return TX

PastaPastaPasta · 2024-10-04T18:08:15Z

src/llmq/ehf_signals.cpp

 }

-void CEHFSignalsHandler::HandleNewRecoveredSig(const CRecoveredSig& recoveredSig)
+MessageProcessingResult CEHFSignalsHandler::HandleNewRecoveredSig(const CRecoveredSig& recoveredSig)


src/llmq/chainlocks.cpp

PastaPastaPasta · 2024-10-04T18:12:11Z

src/protocol.h

    std::optional<CInv> m_inventory;
+    std::optional<CInv> m_to_erase;


confused what the difference is between these? maybe document this API a bit?

UdjinM6

utACK c77216e

PastaPastaPasta

utACK c77216e

knst added this to the 22 milestone Sep 27, 2024

knst marked this pull request as draft September 27, 2024 07:00

knst force-pushed the fix-circular-net-processing-p3 branch 2 times, most recently from b3fc5f6 to 5e489bc Compare September 30, 2024 07:16

knst marked this pull request as ready for review September 30, 2024 14:44

knst requested review from PastaPastaPasta and UdjinM6 September 30, 2024 14:44

PastaPastaPasta reviewed Oct 1, 2024

View reviewed changes

src/llmq/dkgsessionhandler.cpp Outdated

Comment on lines 615 to 616

Copy link

Member

PastaPastaPasta Oct 1, 2024

Choose a reason for hiding this comment

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

nit: could combine

knst reacted with thumbs up emoji

PastaPastaPasta reviewed Oct 1, 2024

View reviewed changes

github-actions bot added the needs rebase label Oct 1, 2024

knst added 5 commits October 3, 2024 16:30

refactor: remove unused include spork.h from validation.cpp

cab700a

refactor: move ChainLocksSigningEnabled from header to cpp file as st…

d54b3ee

…atic function

refactor: new type of message processing result for resolving circula…

09565fe

…r dependency over PeerManager

refactor: HandleNewRecoveredSig return PostProcessingMessage

5383421

refactor: remove dependency of chainlocks on PeerManager

f1c6d17

knst force-pushed the fix-circular-net-processing-p3 branch from 5e489bc to 5739e77 Compare October 3, 2024 09:32

github-actions bot removed the needs rebase label Oct 3, 2024

knst force-pushed the fix-circular-net-processing-p3 branch from 5739e77 to 32683b3 Compare October 3, 2024 09:38

refactor: remove dependency of QuorumBlockProcessor on PeerManager

1d13f01

knst force-pushed the fix-circular-net-processing-p3 branch from 32683b3 to ebff21f Compare October 3, 2024 11:33

refactor: drop dependency of EhfSignals on PeerManager

d0f1778

knst force-pushed the fix-circular-net-processing-p3 branch from ebff21f to d0f1778 Compare October 3, 2024 12:34

UdjinM6 previously approved these changes Oct 3, 2024

View reviewed changes

knst requested a review from PastaPastaPasta October 3, 2024 18:03

PastaPastaPasta reviewed Oct 4, 2024

View reviewed changes

src/llmq/chainlocks.cpp Show resolved Hide resolved

PastaPastaPasta reviewed Oct 4, 2024

View reviewed changes

knst dismissed UdjinM6’s stale review via 9cf789d October 4, 2024 19:20

docs: explain meaning of MessageProcessingResult's members

c77216e

knst force-pushed the fix-circular-net-processing-p3 branch from 9cf789d to c77216e Compare October 4, 2024 19:24

knst requested a review from PastaPastaPasta October 4, 2024 19:30

UdjinM6 approved these changes Oct 4, 2024

View reviewed changes

PastaPastaPasta reviewed Oct 4, 2024

View reviewed changes

PastaPastaPasta merged commit 74e54b8 into dashpay:develop Oct 4, 2024

knst deleted the fix-circular-net-processing-p3 branch October 5, 2024 04:18

knst mentioned this pull request Oct 8, 2024

refactor: remove circular dependency governance/object over governance/classes #6318

Merged

5 tasks

knst mentioned this pull request Nov 3, 2025

refactor: drop dependency on net_processing for multiple consensus related objects #6934

Open

5 tasks

		std::optional<CInv> m_inventory;
		std::optional<CInv> m_to_erase;

Conversation

knst commented Sep 27, 2024 • 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?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

PastaPastaPasta Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 1, 2024

Uh oh!

knst commented Oct 3, 2024

Uh oh!

UdjinM6 commented Oct 3, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

knst Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PastaPastaPasta Oct 4, 2024

Choose a reason for hiding this comment

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

knst commented Sep 27, 2024 •

edited

Loading