refactor: deglobalization of bls_legacy_scheme 2/N #5443

knst · 2023-06-19T09:00:23Z

Issue being fixed or feature implemented

Many usages of CBLS{Signature,PrivateKey,PublicKey} assume using global variable, even if can be specified explicitly.
Some of these usages have been deglobalized in this PR.

Some prior improvements and fixes are here: #5403

What was done?

Refactored the uses of global variable of bls_legacy_scheme from SetHex, SetByteVector, some rpc calls.
Removed flag checkMalleable to simplify code because it's always true.
Removed dependency from txmempool.h on bls.h to speed up compilation.

How Has This Been Tested?

Run unit/functional tests.

Breaking Changes

No breaking changes assumed. But in theory behaviour of some RPC can be more explicit and predictable.

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
I have assigned this pull request to a milestone

UdjinM6

Reindexed on testnet with no issues 👍

Pls see some suggestions in rpc code.

src/rpc/quorums.cpp

UdjinM6 · 2023-06-20T12:00:41Z

Looks good ~~but Check Merge Fast-Forward Only complains... pls rebase on the recent develop~~ nvm, rebased it via GH

ogabrielides

LGTM. Let's wait for CI

UdjinM6 · 2023-06-20T13:13:19Z

hmm... so one of my suggestions was wrong https://gitlab.com/dashpay/dash/-/jobs/4505250574#L2394 and we need to partially revert 668d00d now (pls see 140c1cb).

EDIT: On the bright side, I reindex both on mainnet and testnet with no issues :)

knst · 2023-06-20T14:59:19Z

EDIT: On the bright side, I reindex both on mainnet and testnet with no issues :)

So far as I understand, the RPC verifychainlock can success (or partially success) even if we don't have this block in chain yet. So, behavior in this case indeed changed (and CI failed), but it is not a regular situation, that can not be reproduced easily without an intention. The test rpc_verifychainlock.py indeed checks this corner case.

UdjinM6

👍

lightly tested ACK (reindexed with no issues)

ogabrielides

LGTM utACK

PastaPastaPasta · 2023-06-22T04:12:54Z

src/evo/providertx.h

@@ -167,7 +167,7 @@ class CProUpServTx
        }
        if (!(s.GetType() & SER_GETHASH)) {
            READWRITE(
-                    CBLSSignatureVersionWrapper(const_cast<CBLSSignature&>(obj.sig), (obj.nVersion == LEGACY_BLS_VERSION), true)
+                    CBLSSignatureVersionWrapper(const_cast<CBLSSignature&>(obj.sig), (obj.nVersion == LEGACY_BLS_VERSION))


I'd request dropping b2d41f6 (maybe modulo this line's change) as it's not related to the deglobalization and it may be something we SHOULD use when reindexing... I'll have to check perf at some point.

There's big dangerous of how it is implemented right now.

Currently, there are 2 methods Unserialize, that has both default arguments:

template
inline void Unserialize(Stream& s, const bool specificLegacyScheme, bool checkMalleable = true);
template
inline void Unserialize(Stream& s, bool checkMalleable = true);

Let's assume that I am calling Unserialize(s, true) - it's very non-obvious which one will be called and not error prune at all.

It should be re-implemented, and there should not be default argument. Let's create an issue "check performance of Unserialize with/without checkMalleable flag" - but I really want to remove this confusing method in this PR

Reverting this change and adding new interface in future won't be difficult task so far as changes are quite trivial.

PastaPastaPasta · 2023-06-22T04:13:51Z

src/evo/providertx.h

+        if (obj.nVersion != BASIC_BLS_VERSION && obj.nType == MnType::HighPerformance) {
+            // for HPMN should be only BASIC bls, bail out early
+            return;
+        }


5989de8 should probably be it's own PR... maybe even a backport?

why do backporting only this commit but not other commits in this PR? What's a difference?

src/rpc/quorums.cpp

PastaPastaPasta · 2023-06-22T04:20:46Z

src/llmq/dkgsession.cpp

@@ -989,14 +989,15 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages)
    qc.sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(commitmentHash));
    qc.quorumSig = skShare.Sign(commitmentHash);

+    const bool is_bls_legacy = bls::bls_legacy_scheme.load();


please do this at the call site to avoid an atomic read when we basically never lie

PastaPastaPasta · 2023-06-22T04:22:13Z

src/governance/object.cpp

+    const auto pindex = llmq::utils::V19ActivationIndex(::ChainActive().Tip());
+    bool is_bls_legacy_scheme = pindex == nullptr || nTime < pindex->pprev->nTime;


this would need cs_main locked?

should it? if so, probably there's bug also in this commit:

bool CGovernanceVote::CheckSignature(const CBLSPublicKey& pubKey) const { - if (!CBLSSignature(vchSig).VerifyInsecure(pubKey, GetSignatureHash())) { + CBLSSignature sig; + const auto pindex = llmq::utils::V19ActivationIndex(::ChainActive().Tip()); + bool is_bls_legacy_scheme = pindex == nullptr || nTime < pindex->nTime; + sig.SetByteVector(vchSig, is_bls_legacy_scheme); + if (!sig.VerifyInsecure(pubKey, GetSignatureHash())) { LogPrintf("CGovernanceVote::CheckSignature -- VerifyInsecure() failed\n"); return false; }

::ChainActive() locks it inside

PastaPastaPasta · 2023-06-22T04:22:47Z

src/llmq/dkgsession.h

@@ -7,6 +7,7 @@

 #include <batchedlogger.h>

+#include <bls/bls.h>


Because CDGKContribution has members of type CBLSSignature:

CBLSSignature quorumSig; // threshold sig share of quorumHash+validMembers+pubKeyHash+vvecHash CBLSSignature sig; // single member sig of quorumHash+validMembers+pubKeyHash+vvecHash

This object is declared in bls/bls.h that is missing to be in list of includes.
Code is compilable because bls/bls_ies.h includes recursively bls/bls.h, but better to include it explicitly.

UdjinM6 · 2023-06-22T19:17:13Z

@knst In ac5af56 you did the opposite of what @PastaPastaPasta suggested I think. The idea was to move atomic read inside if/else if to avoid doing that outside of tests.

knst · 2023-06-24T12:46:32Z

@knst In ac5af56 you did the opposite of what @PastaPastaPasta suggested I think. The idea was to move atomic read inside if/else if to avoid doing that outside of tests.

as pasta says:

Atomic reads are slightly intensive so best to avoid when possible.

Updated, thanks for notice.

UdjinM6

LGTM, utACK

PastaPastaPasta · 2023-06-28T05:21:05Z

Still not super happy here... This PR is just doing too many things imo; options are to split into a few different PRs that only really do 1 thing or get the git log to be clean enough so that it'd make sense to merge via a merge commit. I think preference is multiple PRs. (I think I'm okay with all the actual code changes, just not the PR :) )

ogabrielides

re-ACK

…ead using global variable

txmempool.h is widely used header and the dependency bls.h should not be spread to all usages of mempool. It also noticeable improve compilation speed of project if bls.h has been changed.

knst · 2023-06-28T08:36:17Z

it split to #5462 and #5463

Remaining changes are rebased.

ogabrielides

re-ACK

UdjinM6

it looks like you (unintentionally?) dropped some related changes on rebase

knst · 2023-06-28T15:35:18Z

Probably lost after rebase. Let me check...

+partiall revert "refactor: simplification of rpc/quorums.cpp bcs pindex can't be nullptr" in some cases - can!

UdjinM6

utACK

ogabrielides

utACK

PastaPastaPasta

utACK for squash merge

## Issue being fixed or feature implemented Current implementation of BLS wrapper has an unclear interface related to `checkMalleable` flag. There are 2 methods Unserialize, that has both default arguments: ``` template inline void Unserialize(Stream& s, const bool specificLegacyScheme, bool checkMalleable = true); template inline void Unserialize(Stream& s, bool checkMalleable = true); ``` Let's assume that I am calling `Unserialize(s, true)` - it's very non-obvious which one will be called and not error prune at all. It should be re-implemented, and there should not be default argument. Pasta noticed that this flag can be useful from performance point of view - let's have better new method such as `UnserializeNoMalleable` or similar and use it when reindexing/etc. It should be specified explicit. Reverting this change and adding new interface in future won't be difficult task so far as changes are quite trivial. ## What was done? Removed flag checkMalleable to simplify code because it's always true. It splits from #5443 ## How Has This Been Tested? Run unit functional tests. ## Breaking Changes No breaking changes - flag is always true. ## 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

…Evo Nodes (#5463) ## Issue being fixed or feature implemented #5471 ## What was done? It splits from #5443 Adds extra unit tests for BLS basic scheme; enforces BLS basic for Evo Nodes in serialization/unserialization of CProRegTx, CProUpServTx. ## How Has This Been Tested? Run functional/unit tests + added new unit tests. ## Breaking Changes Serialization slightly changed, but it should be not breaking change ## 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

knst changed the title ~~refactor: deglobalization of bls_legacy_scheme part 2/N~~ refactor: deglobalization of bls_legacy_scheme 2/N Jun 19, 2023

knst added this to the 20 milestone Jun 19, 2023

knst requested review from ogabrielides and UdjinM6 and removed request for ogabrielides June 19, 2023 12:18

UdjinM6 requested changes Jun 20, 2023

View reviewed changes

src/rpc/quorums.cpp Show resolved Hide resolved

src/rpc/quorums.cpp Show resolved Hide resolved

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

UdjinM6 force-pushed the bls-deglobal-v2 branch from 0ddbeac to 668d00d Compare June 20, 2023 12:17

ogabrielides previously approved these changes Jun 20, 2023

View reviewed changes

knst dismissed ogabrielides’s stale review via 10450e8 June 20, 2023 15:20

knst requested review from UdjinM6 and ogabrielides June 20, 2023 18:01

UdjinM6 previously approved these changes Jun 20, 2023

View reviewed changes

ogabrielides previously approved these changes Jun 21, 2023

View reviewed changes

UdjinM6 requested a review from PastaPastaPasta June 21, 2023 09:46

PastaPastaPasta reviewed Jun 22, 2023

View reviewed changes

src/rpc/quorums.cpp Show resolved Hide resolved

PastaPastaPasta reviewed Jun 22, 2023

View reviewed changes

knst dismissed stale reviews from ogabrielides and UdjinM6 via ac5af56 June 22, 2023 19:07

knst requested a review from PastaPastaPasta June 22, 2023 19:07

UdjinM6 previously approved these changes Jun 27, 2023

View reviewed changes

ogabrielides previously approved these changes Jun 28, 2023

View reviewed changes

This was referenced Jun 28, 2023

refactor: removed flag checkMalleable from bls's related code #5462

Merged

feat: improving unit tests for Basic BLS and enforcing Basic BLS for Evo Nodes #5463

Merged

knst added 4 commits June 28, 2023 15:26

refactor: remove global usage of bls's version from SetHex

e8b426c

refactor: remove SetByteVector's without "is legacy" flag

51c152b

refactor: pass flag isLegacy to ToByteVector whenever possible inst…

b5904d9

…ead using global variable

refactor: removed using bls.h header from txmempool.h

c5b55d3

txmempool.h is widely used header and the dependency bls.h should not be spread to all usages of mempool. It also noticeable improve compilation speed of project if bls.h has been changed.

knst dismissed stale reviews from ogabrielides and UdjinM6 via c5b55d3 June 28, 2023 08:27

knst force-pushed the bls-deglobal-v2 branch from 8e6a914 to c5b55d3 Compare June 28, 2023 08:27

ogabrielides previously approved these changes Jun 28, 2023

View reviewed changes

UdjinM6 requested changes Jun 28, 2023

View reviewed changes

refactor: simplification of rpc/quorums.cpp bcs pindex can't be nullptr

308e564

+partiall revert "refactor: simplification of rpc/quorums.cpp bcs pindex can't be nullptr" in some cases - can!

knst dismissed ogabrielides’s stale review via 308e564 June 30, 2023 10:09

UdjinM6 approved these changes Jun 30, 2023

View reviewed changes

ogabrielides approved these changes Jun 30, 2023

View reviewed changes

PastaPastaPasta approved these changes Jul 1, 2023

View reviewed changes

PastaPastaPasta merged commit 07fd889 into dashpay:develop Jul 1, 2023

knst mentioned this pull request Dec 25, 2024

refactor: deglobalization of bls_legacy_scheme 3/N #6508

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: deglobalization of bls_legacy_scheme 2/N #5443

refactor: deglobalization of bls_legacy_scheme 2/N #5443

knst commented Jun 19, 2023

UdjinM6 left a comment

UdjinM6 commented Jun 20, 2023 •

edited

Loading

ogabrielides left a comment

UdjinM6 commented Jun 20, 2023 •

edited

Loading

knst commented Jun 20, 2023 •

edited

Loading

UdjinM6 left a comment

ogabrielides left a comment

PastaPastaPasta Jun 22, 2023

knst Jun 22, 2023 •

edited

Loading

PastaPastaPasta Jun 22, 2023

knst Jun 22, 2023

PastaPastaPasta Jun 22, 2023

PastaPastaPasta Jun 22, 2023

knst Jun 22, 2023

UdjinM6 Jun 22, 2023

PastaPastaPasta Jun 22, 2023

knst Jun 22, 2023

UdjinM6 commented Jun 22, 2023

knst commented Jun 24, 2023

UdjinM6 left a comment

PastaPastaPasta commented Jun 28, 2023 •

edited

Loading

ogabrielides left a comment

knst commented Jun 28, 2023

ogabrielides left a comment

UdjinM6 left a comment •

edited

Loading

knst commented Jun 28, 2023

UdjinM6 left a comment

ogabrielides left a comment

PastaPastaPasta left a comment

		const auto pindex = llmq::utils::V19ActivationIndex(::ChainActive().Tip());
		bool is_bls_legacy_scheme = pindex == nullptr \|\| nTime < pindex->pprev->nTime;

		@@ -7,6 +7,7 @@

		#include <batchedlogger.h>

		#include <bls/bls.h>

refactor: deglobalization of bls_legacy_scheme 2/N #5443

refactor: deglobalization of bls_legacy_scheme 2/N #5443

Conversation

knst commented Jun 19, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jun 20, 2023 • edited Loading

ogabrielides left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jun 20, 2023 • edited Loading

knst commented Jun 20, 2023 • edited Loading

UdjinM6 left a comment

Choose a reason for hiding this comment

ogabrielides left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knst Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UdjinM6 commented Jun 22, 2023

knst commented Jun 24, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Jun 28, 2023 • edited Loading

ogabrielides left a comment

Choose a reason for hiding this comment

knst commented Jun 28, 2023

ogabrielides left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment • edited Loading

Choose a reason for hiding this comment

knst commented Jun 28, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

ogabrielides left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jun 20, 2023 •

edited

Loading

UdjinM6 commented Jun 20, 2023 •

edited

Loading

knst commented Jun 20, 2023 •

edited

Loading

knst Jun 22, 2023 •

edited

Loading

PastaPastaPasta commented Jun 28, 2023 •

edited

Loading

UdjinM6 left a comment •

edited

Loading