refactor: remove dependency of llmq/chainlocks, llmq/quorum_block_processor, ehf_signals on PeerManager #6292

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

+        auto inv_opt = quorumBlockProcessor.AddMineableCommitment(fqc);
+        if (inv_opt.has_value()) {


nit: could combine

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

src/llmq/ehf_signals.cpp

        // skip deployments that do not use dip0023 or that have already been mined
        if (!deployment.useEHF || ehfSignals.find(deployment.bit) != ehfSignals.end()) continue;

+        MNHFTxPayload mnhfPayload;


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

PastaPastaPasta

generally LGTM, don't love 5e489bc 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

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
16 of 35 checks passed

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

		auto inv_opt = quorumBlockProcessor.AddMineableCommitment(fqc);
		if (inv_opt.has_value()) {

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

refactor: remove dependency of llmq/chainlocks, llmq/quorum_block_processor, ehf_signals on PeerManager #6292

refactor: remove dependency of llmq/chainlocks, llmq/quorum_block_processor, ehf_signals on PeerManager #6292

Uh oh!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

knst commented Sep 27, 2024 •

edited

Loading