refactor: split llmq/utils to Quorum Calculation and llmq/options#5790
Merged
Conversation
knst
changed the title
5 tasks
UdjinM6
requested changes
Dec 29, 2023
knst
force-pushed
the
refactor-llmq_utils
branch
from
6229ce8 to
5d265ac
Compare
Member
|
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-llmq_utils
branch
from
5d265ac to
87c5163
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-llmq_utils
branch
from
87c5163 to
9002f39
Compare
Collaborator
Author
|
need guix image to test a theory with crash: run as |
Member
|
Guix Automation has began to build this PR tagged as v20.1.0-devpr5790.9002f39c. A new comment will be made when the image is pushed. |
Member
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v20.1.0-devpr5790.9002f39c. The image should be on dockerhub soon. |
Member
|
Guix Automation has began to build this PR tagged as v20.1.0-devpr5790.98457039. A new comment will be made when the image is pushed. |
Member
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v20.1.0-devpr5790.98457039. The image should be on dockerhub soon. |
|
This pull request has conflicts, please rebase. |
Also this commit add llmq/utils in modules where it is included indirectly but it requires to be used such as rpc/quorums
That's an internal helper!
In the past implementation has been non-trivial:
```cpp
return Params().GetConsensus().llmqs.at(llmqType);
```
But over time it was simplified and now does nothing but hide usage of
global variable Params():
```cpp
return Params().GetLLMQ(llmqType);
```
Also it adds missing [[nodiscard]] for Params().GetLLMQ()
Eventually it caused some circular dependencies to disapear.
newly introduced circular dependencies are just longer pathes through
same modules that has been in past short-circuited
It seems as it is an artefact from non-deterministic IS. It used only for LLMQ_TEST_INSTANTSEND quorum and doesn't look useful. Removing this dependency helps to: - simplify implementation of many classes significantly so far as they don't need to pass quorumManager to low-level helpers - achieved significant progress of de-globalization quorumManager. Only one step left (evo/assertlocks, evo/creditpool, evo/mnhftx) and we can drop global variable ::quorumManager - disapeared 2 circular dependencies through llmq/quorum
knst
force-pushed
the
refactor-llmq_utils
branch
from
9845703 to
45957d5
Compare
Collaborator
Author
rebase to top of develop + resolved conflicts |
PastaPastaPasta
approved these changes
Jan 18, 2024
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK for squash merge
5 tasks
PastaPastaPasta
pushed a commit
that referenced
this pull request
Jan 28, 2024
## Issue being fixed or feature implemented `develop` can't sync from genesis on mainnet, b8a086d broke it. #5790 follow-up ## What was done? Revive the old logic but using hardcoded block heights instead of scanning via quorum manager. ## How Has This Been Tested? Synced on mainnet/testnet, CI is happy https://gitlab.com/UdjinM6/dash/-/pipelines/1148980046. ## 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 - [x] 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 _(for repository code-owners and collaborators only)_
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Issue being fixed or feature implemented
llmq/utilshas simple util code that used all over code base and also have too heavy code for calculation quorums such as:GetAllQuorumMembers,EnsureQuorumConnectionsand other.These helpers for calculation quorums are used only by evo/deterministicmns, evo/simplifiedmns and llmq/* modules, but llmq/utils is included in many other modules for various trivial helpers.
What was done?
Prior work:
See also refactor(llmq): separate llmq/utils into llmq/utils and llmq/complex_… #4798
This PR remove all non-quorum calculation code from llmq/utils. Eventually it happens that easier to take everything out rather than move Quorum Calculation to new place atm:
BuildCommitmentHashgoes to llmq/commitmentBuildSignHashgoes to llmq/signingGetLLMQParaminlined since it's trivial (it has not been trivial when introduced ages ago)IsQuorumEnabledon CQuorumManager which meansquorumManagerdeglobalization is done for 90%How Has This Been Tested?
test/lint/lint-circular-dependencies.shBreaking Changes
N/A
Checklist: