feat!: BLS scheme upgrade #5021

ogabrielides · 2022-09-20T08:37:41Z

Tracking issue is: (https://github.com/dashpay/dash/issues/5001)

github-actions · 2022-09-22T11:17:53Z

This pull request has conflicts, please rebase.

github-actions · 2022-09-26T09:14:37Z

This pull request has conflicts, please rebase.

src/chainparams.cpp

thephez · 2022-09-29T14:25:48Z

src/chainparams.cpp


+        // Deployment of basic BLS scheme
+        consensus.vDeployments[Consensus::DEPLOYMENT_BASIC_BLS_SCHEME].bit = 8;
+        consensus.vDeployments[Consensus::DEPLOYMENT_BASIC_BLS_SCHEME].nStartTime = 1625097600; // July 1st, 2021


Probably doesn't matter if the same date is re-used from a previous fork, but just in case?

Suggested change

consensus.vDeployments[Consensus::DEPLOYMENT_BASIC_BLS_SCHEME].nStartTime = 1625097600; // July 1st, 2021

consensus.vDeployments[Consensus::DEPLOYMENT_BASIC_BLS_SCHEME].nStartTime = 1661990400; // Sep 1st, 2022

OK I will include this change in the next version

src/evo/providertx.h

src/llmq/blockprocessor.cpp

src/rpc/rpcevo.cpp

knst

please, check these 2 comments like blockers and potentially bugs:

ToString() for CBLSSignature
checkMalleable usage

Other my comments are hints and more about code style rather that potentially issues.

knst · 2022-10-20T15:34:55Z

src/bls/bls.h

+        obj.Serialize(s, legacy);
+    }
+    template <typename Stream>
+    inline void Unserialize(Stream& s, bool checkMalleable = true) {


looks like a bug for me due to shadowing class's member checkMalleable by this argument.
Either it should be removed from constructor, nor this argument to be removed.

src/evo/providertx.h

knst · 2022-10-20T16:13:09Z

src/llmq/commitment.h

+        obj.pushKV("quorumPublicKey", quorumPublicKey.ToString(nVersion == LEGACY_BLS_NON_INDEXED_QUORUM_VERSION || nVersion == LEGACY_BLS_INDEXED_QUORUM_VERSION));
        obj.pushKV("quorumVvecHash", quorumVvecHash.ToString());
        obj.pushKV("quorumSig", quorumSig.ToString());
        obj.pushKV("membersSig", membersSig.ToString());


looks like missing nVersion == ....

I think better to remove whole function ToString() without arguments from CBLSWrapper to prevent misunderstandings like that:

diff --git a/src/bls/bls.h b/src/bls/bls.h index 2cbe356d38..493d953b72 100644 --- a/src/bls/bls.h +++ b/src/bls/bls.h @@ -228,11 +228,6 @@ public: std::vector<uint8_t> buf = ToByteVector(specificLegacyScheme); return HexStr(buf); } - - inline std::string ToString() const - { - return ToString(bls::bls_legacy_scheme.load()); - } };

(in assumption that it is misusage)

knst · 2022-10-20T16:19:17Z

src/bls/bls.h


    //TODO: Add version with argument (bool specificLegacyScheme)
-    bool SetHexStr(const std::string& str)
+    bool SetHexStr(const std::string& str, const bool specificLegacyScheme)


I think it is better to use enum class instead bool for better code reability.
Otherwise it is more difficult to read code such as "SetHexStr(str, true)" without using IDE's hints.

Also this bool can be extra pain in case if it would be once 3rd version of BLS

Right, there are two other schemes in the BLS library.

src/test/evo_deterministicmns_tests.cpp

github-actions · 2022-10-21T18:10:33Z

This pull request has conflicts, please rebase.

src/bls/bls.h

HashEngineering

I see the changes to CFinalCommitment that will allow an SPV node to determine which BLS scheme to use for the public key and signatures. Additionally the various Provider Special transactions are using version 2.

I would suggest something similar for SimplifiedMNListEntry - The reason being that an SPV node will not be able to tell what scheme is being used since it doesn't know about BIP9 or other hard fork mechanisms. Of course, there may be other ways to deal with this problem.

ogabrielides · 2022-10-31T14:43:39Z

@HashEngineering I agree with you. The problem is tha CSimplifiedMNListEntry doesn't include a nVersion field.
(Nor CSimplifiedMNListDiff which holds CSimplifiedMNListEntry entries).
By taking into consideration the fact that SPV clients are not able to see status of BIP9 forks, I can not think of a backward compatible way to support this.
cc @QuantumExplorer @PastaPastaPasta @UdjinM6 any ideas?

HashEngineering · 2022-11-01T04:32:09Z

@HashEngineering I agree with you. The problem is tha CSimplifiedMNListEntry doesn't include a nVersion field. (Nor CSimplifiedMNListDiff which holds CSimplifiedMNListEntry entries). By taking into consideration the fact that SPV clients are not able to see status of BIP9 forks, I can not think of a backward compatible way to support this. cc @QuantumExplorer @PastaPastaPasta @UdjinM6 any ideas?

What about a new protocol version that supports a version field in the necessary structures?

This is what is happening with 70224, changing the format of a message that is backward compatible with previous releases.

src/evo/providertx.h

thephez · 2022-11-02T18:16:00Z

@ogabrielides title should be in conventional commit format feat!: ...

HashEngineering

I have found a bug that breaks SPV clients that rely on "mnlistdiff" or "qrinfo" to know the BLS scheme that should be used.

src/evo/deterministicmns.cpp

src/consensus/params.h

src/test/evo_deterministicmns_tests.cpp

src/test/evo_trivialvalidation.cpp

src/bls/bls.h

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6

LGTM now, let's see if it works on devnets/testnet :)

utACK

PastaPastaPasta

Time to test on devnet/testnet

utACK for squash merge

PastaPastaPasta · 2022-12-30T05:46:25Z

@ogabrielides can you write release notes for this in the near future?

@ogabrielides

…ff (#5139) ## Issue being fixed or feature implemented We shouldn't have changed it in the first place if we want to avoid additional migration/reindexing. I did not think about it when I proposed this patch in #5021, sorry 🙈 Thanks @ogabrielides for noticing 👍 #5021 follow-up ## What was done?  ## 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 - [ ] I have made corresponding changes to the documentation **For repository code-owners and collaborators only** - [x] I have assigned this pull request to a milestone

## Issue being fixed or feature implemented ``` POTENTIAL DEADLOCK DETECTED Previous lock order was: (2) 'cs_main' in governance/governance.cpp:1096 (in thread 'init') (1) 'cs' in governance/governance.cpp:1096 (in thread 'init') Current lock order is: (1) 'cs' in governance/governance.cpp:778 (in thread 'msghand') 'cs' in governance/object.cpp:104 (in thread 'msghand') (2) '::cs_main' in validation.cpp:117 (in thread 'msghand') ``` #5021 follow-up ## What was done? Lock `cs_main` earlier ## How Has This Been Tested? run dashd on testnet ## Breaking Changes none ## Checklist:  - [x] I have performed a self-review of my own code - [x] 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** - [x] I have assigned this pull request to a milestone

thephez · 2023-01-31T18:26:14Z

Release notes are in #5137

ogabrielides added this to the 19 milestone Sep 20, 2022

ogabrielides marked this pull request as draft September 20, 2022 08:37

github-actions bot added the needs rebase label Sep 22, 2022

github-actions bot added needs rebase and removed needs rebase labels Sep 23, 2022

github-actions bot removed the needs rebase label Sep 26, 2022

ogabrielides marked this pull request as ready for review September 27, 2022 12:18

ogabrielides requested review from PastaPastaPasta and UdjinM6 September 27, 2022 12:19

thephez reviewed Sep 29, 2022

View reviewed changes

ogabrielides requested a review from knst October 10, 2022 08:40

knst requested changes Oct 10, 2022

View reviewed changes

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

src/llmq/blockprocessor.cpp Outdated Show resolved Hide resolved

src/rpc/rpcevo.cpp Outdated Show resolved Hide resolved

ogabrielides marked this pull request as draft October 10, 2022 12:10

ogabrielides marked this pull request as ready for review October 12, 2022 15:48

knst requested changes Oct 20, 2022

View reviewed changes

github-actions bot added the needs rebase label Oct 21, 2022

knst mentioned this pull request Oct 25, 2022

fix: build on ubuntu 22.10 due to new gcc 12 #5064

Merged

2 tasks

github-actions bot removed the needs rebase label Oct 25, 2022

HashEngineering reviewed Oct 31, 2022

View reviewed changes

src/bls/bls.h Outdated Show resolved Hide resolved

HashEngineering suggested changes Oct 31, 2022

View reviewed changes

PastaPastaPasta reviewed Nov 2, 2022

View reviewed changes

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

ogabrielides changed the title ~~Bls scheme upgrade~~ feat!: BLS scheme upgrade Nov 2, 2022

HashEngineering suggested changes Nov 5, 2022

View reviewed changes

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

UdjinM6 requested changes Nov 5, 2022

View reviewed changes

src/consensus/params.h Outdated Show resolved Hide resolved

src/test/evo_deterministicmns_tests.cpp Outdated Show resolved Hide resolved

src/test/evo_trivialvalidation.cpp Outdated Show resolved Hide resolved

PastaPastaPasta and others added 17 commits December 27, 2022 11:41

some WIP changes

ba1cd61

some fixes to WIP changes

21f51fa

Applied suggestions

afe2553

fix dmstate

1502892

Fix for LazyBLS + corrections

b97986b

fix

8577b9c

Migration lazy BLS case

a675be6

Rebase fix

e1d99c6

Linter fixes

ebb4063

experimental linter, linking fix

156fcb7

cleanup

68e2a3d

chore: wrap bitwise AND in ()

d573ecb

legacy/basic for governance

37b66a8

do not allow BASIC_BLS_VERSION protx version before the v19 fork

eabbe24

Add legacy/basic bls versioning to MNHFTx

7517277

add bls sig serialization test

7dd3bec

don't throw exception, serialise all-zero buffer

e65f93a

UdjinM6 reviewed Dec 29, 2022

View reviewed changes

src/bls/bls.h Outdated Show resolved Hide resolved

Update src/bls/bls.h

16b6bdd

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6 approved these changes Dec 29, 2022

View reviewed changes

PastaPastaPasta approved these changes Dec 30, 2022

View reviewed changes

PastaPastaPasta merged commit 78d057d into dashpay:develop Dec 30, 2022

PastaPastaPasta added the Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. label Dec 30, 2022

ogabrielides deleted the bls_scheme_upgrade branch December 30, 2022 09:16

This was referenced Jan 5, 2023

fix: resolve potential deadlock #5138

Merged

fix: preserve the original order of fields in CDeterministicMNStateDiff #5139

Merged

	consensus.vDeployments[Consensus::DEPLOYMENT_BASIC_BLS_SCHEME].nStartTime = 1625097600; // July 1st, 2021
	consensus.vDeployments[Consensus::DEPLOYMENT_BASIC_BLS_SCHEME].nStartTime = 1661990400; // Sep 1st, 2022

feat!: BLS scheme upgrade #5021

feat!: BLS scheme upgrade #5021

Uh oh!

Conversation

ogabrielides commented Sep 20, 2022

Uh oh!

github-actions bot commented Sep 22, 2022

Uh oh!

github-actions bot commented Sep 26, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 21, 2022

Uh oh!

Uh oh!

HashEngineering left a comment

Choose a reason for hiding this comment

Uh oh!

ogabrielides commented Oct 31, 2022

Uh oh!

HashEngineering commented Nov 1, 2022

Uh oh!

Uh oh!

thephez commented Nov 2, 2022

Uh oh!

HashEngineering left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UdjinM6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Dec 30, 2022

Uh oh!

thephez commented Jan 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

UdjinM6 left a comment •

edited

Loading