feat!: calculate quorum members using v20 cbtx clsig #5366
Merged
Conversation
This file contains 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
ogabrielides
force-pushed
the
cbtx_clsig_quorum_members
branch
from
1d9116c to
c11c6a9
Compare
UdjinM6
requested changes
May 11, 2023
ogabrielides
force-pushed
the
cbtx_clsig_quorum_members
branch
from
c0c7f48 to
02a98b5
Compare
UdjinM6
requested changes
May 15, 2023
PastaPastaPasta
approved these changes
May 17, 2023
5 tasks
5 tasks
UdjinM6
added a commit
that referenced
this pull request
May 20, 2023
## Issue being fixed or feature implemented We use `pQuorumBaseBlockIndex` name when we shouldn't and we don't check that quorum types and block indexes provided as input params in llmq utils satisfy our requirements. This is kind of ok-ish as long as we use these functions appropriately but it's better to make things clearer and to have actual checks imo. noticed this while reviewing #5366 ## What was done? Rename `pQuorumBaseBlockIndex` to `pCycleQuorumBaseBlockIndex`/`pindex` in a few places. Check that quorum types and block indexes have expected values. ## How Has This Been Tested? run tests locally ## Breaking Changes n/a ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 10, 2023
## Issue being fixed or feature implemented Implementation of Randomness Beacon Part 3. Starting from v20 activation fork, members for quorums are sorted using (if available) the best CL signature found in Coinbase. If no CL signature is present yet, then the usual way is used (By using Blockhash instead) The actual new way to shuffle is already implemented in #5366. SPV clients also need to calculate members, but they only know block headers. Since Coinbase is in the actual block, then they lack the required information to correctly calculate quorum members. ## What was done? - Message `MNLISTIDFF` is enriched with a new field `quorumsCLSigs`. This field holds the Chainlock Signature required for each set of indexes corresponding to quorums in field `newQuorums`. - Protocol version has been bumped to `70230`. - Clients with protocol version greater or equal to `70230` will receive the new field `quorumsCLSigs`. - The same field is returned in `protx diff` RPC. Note: - Field `quorumsCLSigs` will populated only after v20 activation - If for one or more quorums, no non-null CL sig was found in CbTx then a null signature is returned in `quorumsCLSigs`. ## How Has This Been Tested? - Functional test mininode's protocol version was bumped to `70230`. - `feature_llmq_rotation.py` checks that `quorumsCLSigs` match in both P2P and RPC messages. ## Breaking Changes No ## 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 - [x] 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)_ --------- Co-authored-by: thephez <[email protected]> Co-authored-by: UdjinM6 <[email protected]> Co-authored-by: pasta <[email protected]>
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.
Issue being fixed or feature implemented
Implementation of Randomness Beacon Part 2.
This PR is the next step of #5262.
Starting from v20 activation fork, members for quorums are sorted using (if available) the best CL signature found in Coinbase.
If no CL signature is present yet, then the usual way is used (By using Blockhash instead)
What was done?
How Has This Been Tested?
Test
feature_llmq_rotation.pywas updated to cover both rotated and non-rotated quorums.2 quorums are mined first to ensure Chainlock are working earlier.
Then dip_24 activation is replaced by v20 activation.
The only direct way to test this change is to make sure that all expected quorums after v20 activation are properly formed.
Note: A
wait_for_chainlocked_block_all_nodesis called between every rotation cycle to ensure that Coinbase will use a different Chainlock signature.Breaking Changes
Yes, quorum members will be calculated differently.
Checklist: