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

        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

    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


 #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

f76f943 docs: comments about invariant for CBLSSecretKey for bls scheme (Konstantin Akimov) 5c36bb2 docs: add TODO about bls global variable (Konstantin Akimov) 6fa033e feat: p2p message mnauth to always use Basic BLS scheme (Konstantin Akimov) ce18dcd refactor: enforce passing bls scheme for each call of sign by BLS (Konstantin Akimov) 954e5ef fix: undefined behaviour in evo RPC calls of RPCHelpMan (Konstantin Akimov) 5ea9ff9 feat: enforce using _legacy suffix for protx commands; no more guessing based if v19 is activated (Konstantin Akimov) 99e9c65 feat: use Basic scheme only for public key serialization in bls_ies (Konstantin Akimov) 91f10b1 refactor: removed unused functions from bls_ies (Konstantin Akimov) 26c058a refactor: optimize usages bls_ies.h and bls.h (Konstantin Akimov) 7cbb26b style: clang suggestion for bls rpcs (Konstantin Akimov) 394d649 feat: use basic scheme for RPC `bls generate` by default (Konstantin Akimov) caedaba refactor: remove CheckMalleable without bls scheme argument (Konstantin Akimov) f728fc9 refactor: drop legacy flag from CActiveMasternodeInfo (Konstantin Akimov) 4acdd79 refactor: remove general constructor of BLS objects from bytes, add only to objects that uses it (Konstantin Akimov) f1a7e95 fix: condition of bls activation to make work even from block 1 in specialtxman (Konstantin Akimov) b5cd09e fix: rename argument name for IsTriviallyValid for protx (Konstantin Akimov) 544031d fix: do not pass param 'modOrder' for bls::PrivateKey (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Many usages of CBLS{Signature,PrivateKey,PublicKey} assume using global variable, even if it can be specified explicitly in some situations. These over-usages of `bls::bls_legacy_scheme` are blocker for changing buried height of activation of fork `V19` on Regtest. This PR helps unblock changing v19 height on RegTest, see: #6511 Prior improvements for BLS: #5443 ## What was done? This PR does: - fixes Undefined Behavior in rpc `protx register_legacy`, `protx register_fund_legacy`, `protx resiter_prepare_legacy`, `protx update_registrar_legacy` - drop flag "is legacy" from secret key initialization and serialization (as it has no difference) - enforce specifying legacy flag to bls functions `Sign`, `ToByteVector` and in a helper `SignSpecialTxPayloadByHash` - removed unused methods from bls_ies.h and optimized headers usages - enforce p2p message `mnauth` to use Basic BLS scheme - enforce using flag `legacy` in RPC `bls generate` and `bls fromsecret` instead guessing which one user want based on v19 activation status. ## How Has This Been Tested? Run unit and functional tests; update testing framework `test_framework.py` to use `_legacy` RPCs which has been ignored before. Reindex testnet `src/qt/dash-qt -reindex -testnet -assumevalid=0` - SUCCEED Reindex mainnet `src/qt/dash-qt -reindex -assumevalid=0` - SUCCEED ## Breaking Changes On RegTest RPC: `bls generate`, `bls fromsecret` should pass flag `is_legacy` for pre-v19 blocks On RegTest RPC: `protx register_legacy`, `protx register_fund_legacy`, `protx register_prepare_legacy`, `protx update_registrar_legacy` should be used in pre-v19 blocks. ## 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 ACKs for top commit: UdjinM6: reindexed with no issues, light ACK f76f943 PastaPastaPasta: utACK f76f943 Tree-SHA512: 69878d2e3037c27d504103a4f5f38ee8cc5ca56676cea0ff92a309762c90ef6399ddb1359193669353af13fee98f3902aa5f3926c9f9b3c7ad7aa3b66ca9f23b

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 UdjinM6 and ogabrielides 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 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

Merged

5 tasks

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

Uh oh!

refactor: deglobalization of bls_legacy_scheme 2/N #5443

refactor: deglobalization of bls_legacy_scheme 2/N #5443

Uh oh!

Conversation

knst commented Jun 19, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UdjinM6 commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogabrielides left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knst commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

ogabrielides left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knst Jun 22, 2023 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Jun 22, 2023

Uh oh!

knst commented Jun 24, 2023

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogabrielides left a comment

Choose a reason for hiding this comment

Uh oh!

knst commented Jun 28, 2023

Uh oh!

ogabrielides left a comment

Choose a reason for hiding this comment

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

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