feat!: Insertion of best CL signature in CbTx #5262

ogabrielides · 2023-03-21T12:55:11Z

Issue being fixed or feature implemented

What was done?

Bumped version of CbTx. Added fields bestCLHeightDiff, bestCLSignature
Miner starting from v20 fork, includes best CL signature in CbTx (if available) or null signature.
All nodes should verify included CL signature before accepting the block.

How Has This Been Tested?

Basically, activated v20 on in the beginning of feature_llmq_chainlocks.py

Breaking Changes

Yes, new version of CbTx

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

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

github-actions · 2023-04-15T18:19:26Z

This pull request has conflicts, please rebase.

src/evo/cbtx.cpp

src/evo/cbtx.h

github-actions · 2023-04-17T15:46:05Z

This pull request has conflicts, please rebase.

thephez · 2023-04-18T13:16:20Z

@ogabrielides We'll have to update DIP(s) with this change, right?

ogabrielides · 2023-04-18T14:19:17Z

@thephez Correct. Once this PR is merged I will open a new one for the DIPs.

…ignature in CbTx

ogabrielides · 2023-04-23T10:12:25Z

@PastaPastaPasta @UdjinM6 Adjusted to latest decisions. (Why CI isn't starting?)

src/evo/cbtx.cpp

PastaPastaPasta · 2023-04-24T17:06:07Z

src/evo/cbtx.cpp

+        bool isCbV20 = cbTx.nVersion == CCbTx::CB_V20_VERSION;
+        if (isV20 != isCbV20) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-version");


Suggested change

bool isCbV20 = cbTx.nVersion == CCbTx::CB_V20_VERSION;

if (isV20 != isCbV20) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-version");

if (isV20 && cbTx.nVersion != CCbTx::CB_V20_VERSION) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-version");

src/evo/cbtx.cpp

PastaPastaPasta · 2023-04-24T17:10:44Z

src/evo/cbtx.cpp

+        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-payload");
+    }
+
+    if (cbTx.nVersion >= CCbTx::CB_V20_VERSION) {


we shouldn't allow a version greater than current version

src/evo/cbtx.cpp

PastaPastaPasta · 2023-04-24T17:13:47Z

src/evo/cbtx.cpp

+                // IsNull() doesn't exist for CBLSSignature: we assume that a non valid BLS sig is null
+                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-clsig");
+            }
+            int prevBlockCoinbaseCLHeight = pindexPrev->nHeight - static_cast<int>(prevBlockCoinbaseChainlock.value().second) - 1;


give this variable a name :) maybe expand prevBlockCoinbaseChainlock.value() earlier into blsSig and blockHeightDiff or something like that

src/evo/cbtx.cpp

PastaPastaPasta · 2023-04-24T17:20:02Z

src/evo/cbtx.cpp

+    return true;
+}
+
+bool EmplaceBestChainlock(const llmq::CChainLocksHandler& chainlock_handler, const int nHeight, std::optional<std::pair<CBLSSignature, uint32_t>> prevBlockCoinbaseChainlock, uint32_t& bestCLHeightDiff, CBLSSignature& bestCLSignature)


we also should probably not use return parameters

src/evo/cbtx.cpp

PastaPastaPasta · 2023-04-24T17:22:11Z

src/evo/cbtx.cpp

+
+std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(const CBlockIndex* pindex)
+{
+    auto opt_cbtx = GetCoinbaseTx(pindex);


we should probably storecbtx.bestCLSignature, cbtx.bestCLHeightDiff in state somewhere so we don't have to load blocks from disk

Do you mean to cache {cbtx.bestCLSignature, cbtx.bestCLHeightDiff} per block ?

Implemented in #5347 by the way

UdjinM6 · 2023-04-24T19:09:25Z

pls see https://github.com/UdjinM6/dash/commits/pr5262 for some suggestions

ogabrielides · 2023-04-25T12:31:19Z

@PastaPastaPasta @UdjinM6 Applied (most of) the suggestions

…b_v20 in CheckCbTxBestChainlock

UdjinM6

LGTM, utACK

PastaPastaPasta

utACK for squash merge, let's do it :)

## 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.py` was 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_nodes` is 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: - [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)_ --------- Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

Relates to dashpay/dash#5262

* docs: deprecate MSG_LEGACY_TXLOCK_REQUEST Aligns with dashpay/dash#5483 * chore: update link to prev version of docs * docs: update mnlistdiff nversion location Relates to dashpay/dash#5450 * docs(p2p): update mnlistdiff Relates to dashpay/dash#5377 * docs: update cbtx for v3 Relates to dashpay/dash#5262 * docs: update mnhf with details of final implementation Relates to dashpay/dash#5469 and dashpay/dash#5505 * docs: note removal of NODE_GETUTXO Relates to dashpay/dash#5500 * chore: revert "docs: update mnhf with details of final implementation" This reverts commit 8e4bf6c since there may still be additional changes to the implementation (it's not merged)

## Issue being fixed or feature implemented ## What was done? Added release notes for #5262. ## How Has This Been Tested? ## Breaking Changes ## 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 - [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: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: thephez <thephez@users.noreply.github.com> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

* docs: deprecate MSG_LEGACY_TXLOCK_REQUEST Aligns with dashpay/dash#5483 * chore: update link to prev version of docs * docs: update mnlistdiff nversion location Relates to dashpay/dash#5450 * docs(p2p): update mnlistdiff Relates to dashpay/dash#5377 * docs: update cbtx for v3 Relates to dashpay/dash#5262 * docs: update mnhf with details of final implementation Relates to dashpay/dash#5469 and dashpay/dash#5505 * docs: note removal of NODE_GETUTXO Relates to dashpay/dash#5500 * chore: revert "docs: update mnhf with details of final implementation" This reverts commit 8e4bf6c since there may still be additional changes to the implementation (it's not merged)

ogabrielides added this to the 20 milestone Mar 21, 2023

ogabrielides requested review from UdjinM6 and PastaPastaPasta March 21, 2023 12:58

ogabrielides force-pushed the cb_clsig branch 2 times, most recently from a0e595d to 6e24f7e Compare April 15, 2023 08:19

github-actions bot added the needs rebase label Apr 15, 2023

ogabrielides force-pushed the cb_clsig branch from 6e24f7e to 59ec1c9 Compare April 17, 2023 08:56

github-actions bot removed the needs rebase label Apr 17, 2023

knst reviewed Apr 17, 2023

View reviewed changes

src/evo/cbtx.cpp Outdated Show resolved Hide resolved

knst reviewed Apr 17, 2023

View reviewed changes

src/evo/cbtx.h Outdated Show resolved Hide resolved

github-actions bot added the needs rebase label Apr 17, 2023

ogabrielides force-pushed the cb_clsig branch from 1300fae to 297c822 Compare April 17, 2023 20:48

github-actions bot removed the needs rebase label Apr 17, 2023

knst pushed a commit to knst/dash that referenced this pull request Apr 19, 2023

cummulative update from PR dashpay#5262 feat!: Insertion of best CL s…

b05c044

…ignature in CbTx

PastaPastaPasta pushed a commit to knst/dash that referenced this pull request Apr 19, 2023

cummulative update from PR dashpay#5262 feat!: Insertion of best CL s…

825e501

…ignature in CbTx

ogabrielides marked this pull request as draft April 20, 2023 19:37

knst pushed a commit to knst/dash that referenced this pull request Apr 21, 2023

cummulative update from PR dashpay#5262 feat!: Insertion of best CL s…

eade838

…ignature in CbTx

ogabrielides marked this pull request as ready for review April 23, 2023 10:09

ogabrielides force-pushed the cb_clsig branch from cec8591 to 0d2189a Compare April 24, 2023 15:27

PastaPastaPasta reviewed Apr 24, 2023

View reviewed changes

src/evo/cbtx.cpp Show resolved Hide resolved

PastaPastaPasta reviewed Apr 24, 2023

View reviewed changes

PastaPastaPasta requested changes Apr 24, 2023

View reviewed changes

ogabrielides requested a review from PastaPastaPasta April 25, 2023 12:30

ogabrielides and others added 15 commits May 5, 2023 09:55

fix

7e002b6

Rebase adjustements

cce1184

tests and refactoring

9a5648c

Added expected_null_cl

2a50226

adjustements

60994a1

refactor

3fffb2f

use best known cl as a shortcut in few places

b2cbaab

fix bench

3fb85ee

feat: skip CheckCbTxBestChainlock for assumevalid blocks

032cfb1

refactor VerifyChainLock and use it in more places

5d094a9

adjustement

380e9e0

suggestions

04d1986

fix: no cb_v20 prior to v20 activation, only cb_v20 after v20 activation

3999739

fix: post-cb_v20 should use cb_v20 rules too, bail out early on pre-c…

29b0c30

…b_v20 in CheckCbTxBestChainlock

fix: tweak test

c5ee6ed

ogabrielides force-pushed the cb_clsig branch from 2f54f40 to c5ee6ed Compare May 5, 2023 08:06

ogabrielides requested a review from UdjinM6 May 5, 2023 11:04

UdjinM6 approved these changes May 8, 2023

View reviewed changes

PastaPastaPasta approved these changes May 9, 2023

View reviewed changes

PastaPastaPasta added the Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. label May 9, 2023

PastaPastaPasta merged commit b1626f9 into dashpay:develop May 9, 2023
5 of 8 checks passed

ogabrielides deleted the cb_clsig branch May 9, 2023 07:54

ogabrielides mentioned this pull request May 10, 2023

feat!: calculate quorum members using v20 cbtx clsig #5366

Merged

5 tasks

thephez added a commit to thephez/docs-core that referenced this pull request Aug 16, 2023

docs: update cbtx for v3

3adf359

Relates to dashpay/dash#5262

ogabrielides mentioned this pull request Aug 31, 2023

docs: Added release notes for #5262 #5558

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Insertion of best CL signature in CbTx #5262

feat!: Insertion of best CL signature in CbTx #5262

ogabrielides commented Mar 21, 2023 •

edited

github-actions bot commented Apr 15, 2023

github-actions bot commented Apr 17, 2023

thephez commented Apr 18, 2023

ogabrielides commented Apr 18, 2023

ogabrielides commented Apr 23, 2023

PastaPastaPasta Apr 24, 2023

PastaPastaPasta Apr 24, 2023

PastaPastaPasta Apr 24, 2023

PastaPastaPasta Apr 24, 2023

PastaPastaPasta Apr 24, 2023

ogabrielides Apr 25, 2023

ogabrielides Apr 25, 2023

UdjinM6 commented Apr 24, 2023

ogabrielides commented Apr 25, 2023

UdjinM6 left a comment

PastaPastaPasta left a comment

		bool isCbV20 = cbTx.nVersion == CCbTx::CB_V20_VERSION;
		if (isV20 != isCbV20) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-version");

	bool isCbV20 = cbTx.nVersion == CCbTx::CB_V20_VERSION;
	if (isV20 != isCbV20) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-version");
	if (isV20 && cbTx.nVersion != CCbTx::CB_V20_VERSION) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-version");

feat!: Insertion of best CL signature in CbTx #5262

feat!: Insertion of best CL signature in CbTx #5262

Conversation

ogabrielides commented Mar 21, 2023 • edited

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

github-actions bot commented Apr 15, 2023

github-actions bot commented Apr 17, 2023

thephez commented Apr 18, 2023

ogabrielides commented Apr 18, 2023

ogabrielides commented Apr 23, 2023

PastaPastaPasta Apr 24, 2023

Choose a reason for hiding this comment

PastaPastaPasta Apr 24, 2023

Choose a reason for hiding this comment

PastaPastaPasta Apr 24, 2023

Choose a reason for hiding this comment

PastaPastaPasta Apr 24, 2023

Choose a reason for hiding this comment

PastaPastaPasta Apr 24, 2023

Choose a reason for hiding this comment

ogabrielides Apr 25, 2023

Choose a reason for hiding this comment

ogabrielides Apr 25, 2023

Choose a reason for hiding this comment

UdjinM6 commented Apr 24, 2023

ogabrielides commented Apr 25, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

ogabrielides commented Mar 21, 2023 •

edited