feat!: calculate quorum members using v20 cbtx clsig#5366
Merged
UdjinM6 merged 10 commits intodashpay:developfrom May 17, 2023
ogabrielides:cbtx_clsig_quorum_members
Merged
feat!: calculate quorum members using v20 cbtx clsig#5366UdjinM6 merged 10 commits intodashpay:developfrom ogabrielides:cbtx_clsig_quorum_members
UdjinM6 merged 10 commits intodashpay:developfrom
ogabrielides:cbtx_clsig_quorum_members
Conversation
UdjinM6
requested changes
May 11, 2023
UdjinM6
requested changes
May 15, 2023
PastaPastaPasta
approved these changes
May 17, 2023
Member
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK for squash merge
@UdjinM6 please post reack.
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]>
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
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: