Merge #6508: refactor: deglobalization of bls_legacy_scheme 3/N

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

src/bench/bls.cpp

@@ -29,12 +29,12 @@ static void BuildTestVectors(size_t count, size_t invalidCount,
         secKeys[i].MakeNewKey();
         pubKeys[i] = secKeys[i].GetPublicKey();
         msgHashes[i] = GetRandHash();
-        sigs[i] = secKeys[i].Sign(msgHashes[i]);
+        sigs[i] = secKeys[i].Sign(msgHashes[i], false);
 
         if (invalid[i]) {
             CBLSSecretKey s;
             s.MakeNewKey();
-            sigs[i] = s.Sign(msgHashes[i]);
+            sigs[i] = s.Sign(msgHashes[i], false);
         }
     }
 }
@@ -71,8 +71,8 @@ static void BLS_SignatureAggregate_Normal(benchmark::Bench& bench)
     CBLSSecretKey secKey1, secKey2;
     secKey1.MakeNewKey();
     secKey2.MakeNewKey();
-    CBLSSignature sig1 = secKey1.Sign(hash);
-    CBLSSignature sig2 = secKey2.Sign(hash);
+    CBLSSignature sig1 = secKey1.Sign(hash, false);
+    CBLSSignature sig2 = secKey2.Sign(hash, false);
 
     // Benchmark.
     bench.run([&] {
@@ -89,7 +89,7 @@ static void BLS_Sign_Normal(benchmark::Bench& bench)
     // Benchmark.
     bench.minEpochIterations(100).run([&] {
         uint256 hash = GetRandHash();
-        sig = secKey.Sign(hash);
+        sig = secKey.Sign(hash, false);
     });
 }
 

src/bls/bls.cpp

@@ -117,11 +117,6 @@ CBLSPublicKey CBLSSecretKey::GetPublicKey() const
     return pubKey;
 }
 
-CBLSSignature CBLSSecretKey::Sign(const uint256& hash) const
-{
-    return Sign(hash, bls::bls_legacy_scheme.load());
-}
-
 CBLSSignature CBLSSecretKey::Sign(const uint256& hash, const bool specificLegacyScheme) const
 {
     if (!IsValid()) {

src/bls/bls.h

@@ -57,10 +57,6 @@ class CBLSWrapper
     static constexpr size_t SerSize = _SerSize;
 
     explicit CBLSWrapper() = default;
-    explicit CBLSWrapper(Span<const unsigned char> vecBytes) : CBLSWrapper<ImplType, _SerSize, C>()
-    {
-        SetByteVector(vecBytes, bls::bls_legacy_scheme.load());
-    }
 
     CBLSWrapper(const CBLSWrapper& ref) = default;
     CBLSWrapper& operator=(const CBLSWrapper& ref) = default;
@@ -131,11 +127,6 @@ class CBLSWrapper
         return impl.Serialize(specificLegacyScheme);
     }
 
-    std::vector<uint8_t> ToByteVector() const
-    {
-        return ToByteVector(bls::bls_legacy_scheme.load());
-    }
-
     const uint256& GetHash() const
     {
         if (cachedHash.IsNull()) {
@@ -215,11 +206,6 @@ class CBLSWrapper
         return true;
     }
 
-    inline bool CheckMalleable(Span<uint8_t> vecBytes) const
-    {
-        return CheckMalleable(vecBytes, bls::bls_legacy_scheme.load());
-    }
-
     inline std::string ToString(const bool specificLegacyScheme) const
     {
         std::vector<uint8_t> buf = ToByteVector(specificLegacyScheme);
@@ -263,6 +249,7 @@ class CBLSId : public CBLSWrapper<CBLSIdImplicit, BLS_CURVE_ID_SIZE, CBLSId>
     explicit CBLSId(const uint256& nHash);
 };
 
+//! CBLSSecretKey is invariant to BLS scheme for Creation / Serialization / Deserialization
 class CBLSSecretKey : public CBLSWrapper<bls::PrivateKey, BLS_CURVE_SECKEY_SIZE, CBLSSecretKey>
 {
 public:
@@ -272,19 +259,26 @@ class CBLSSecretKey : public CBLSWrapper<bls::PrivateKey, BLS_CURVE_SECKEY_SIZE,
     using CBLSWrapper::CBLSWrapper;
 
     CBLSSecretKey() = default;
+    explicit CBLSSecretKey(Span<const unsigned char> vecBytes)
+    {
+        // The second param here is not 'is_legacy', but `modOrder`
+        SetByteVector(vecBytes, false);
+    }
     CBLSSecretKey(const CBLSSecretKey&) = default;
     CBLSSecretKey& operator=(const CBLSSecretKey&) = default;
 
     void AggregateInsecure(const CBLSSecretKey& o);
     static CBLSSecretKey AggregateInsecure(Span<CBLSSecretKey> sks);
 
 #ifndef BUILD_BITCOIN_INTERNAL
+    //! MakeNewKey() is invariant to BLS scheme
     void MakeNewKey();
 #endif
+    //! SecretKeyShare() is invariant to BLS scheme
     bool SecretKeyShare(Span<CBLSSecretKey> msk, const CBLSId& id);
 
+    //! GetPublicKey() is invariant to BLS scheme
     [[nodiscard]] CBLSPublicKey GetPublicKey() const;
-    [[nodiscard]] CBLSSignature Sign(const uint256& hash) const;
     [[nodiscard]] CBLSSignature Sign(const uint256& hash, const bool specificLegacyScheme) const;
 };
 
@@ -338,6 +332,10 @@ class CBLSSignature : public CBLSWrapper<bls::G2Element, BLS_CURVE_SIG_SIZE, CBL
     using CBLSWrapper::CBLSWrapper;
 
     CBLSSignature() = default;
+    explicit CBLSSignature(Span<const unsigned char> bytes, bool is_serialized_legacy)
+    {
+        SetByteVector(bytes, is_serialized_legacy);
+    }
     CBLSSignature(const CBLSSignature&) = default;
     CBLSSignature& operator=(const CBLSSignature&) = default;
 

src/bls/bls_ies.cpp

@@ -9,8 +9,7 @@
 
 #include <crypto/aes.h>
 
-template <typename Out>
-static bool EncryptBlob(const void* in, size_t inSize, Out& out, const void* symKey, const void* iv)
+static bool EncryptBlob(const void* in, size_t inSize, std::vector<unsigned char>& out, const void* symKey, const void* iv)
 {
     out.resize(inSize);
 
@@ -38,32 +37,14 @@ uint256 CBLSIESEncryptedBlob::GetIV(size_t idx) const
     return iv;
 }
 
-bool CBLSIESEncryptedBlob::Encrypt(size_t idx, const CBLSPublicKey& peerPubKey, const void* plainTextData, size_t dataSize)
-{
-    CBLSSecretKey ephemeralSecretKey;
-    ephemeralSecretKey.MakeNewKey();
-    ephemeralPubKey = ephemeralSecretKey.GetPublicKey();
-
-    CBLSPublicKey pk;
-    if (!pk.DHKeyExchange(ephemeralSecretKey, peerPubKey)) {
-        return false;
-    }
-
-    std::vector<unsigned char> symKey = pk.ToByteVector();
-    symKey.resize(32);
-
-    uint256 iv = GetIV(idx);
-    return EncryptBlob(plainTextData, dataSize, data, symKey.data(), iv.begin());
-}
-
 bool CBLSIESEncryptedBlob::Decrypt(size_t idx, const CBLSSecretKey& secretKey, CDataStream& decryptedDataRet) const
 {
     CBLSPublicKey pk;
     if (!pk.DHKeyExchange(secretKey, ephemeralPubKey)) {
         return false;
     }
 
-    std::vector<unsigned char> symKey = pk.ToByteVector();
+    std::vector<unsigned char> symKey = pk.ToByteVector(false);
     symKey.resize(32);
 
     uint256 iv = GetIV(idx);
@@ -75,24 +56,6 @@ bool CBLSIESEncryptedBlob::IsValid() const
     return ephemeralPubKey.IsValid() && !data.empty() && !ivSeed.IsNull();
 }
 
-
-bool CBLSIESMultiRecipientBlobs::Encrypt(const std::vector<CBLSPublicKey>& recipients, const BlobVector& _blobs)
-{
-    if (recipients.size() != _blobs.size()) {
-        return false;
-    }
-
-    InitEncrypt(_blobs.size());
-
-    for (size_t i = 0; i < _blobs.size(); i++) {
-        if (!Encrypt(i, recipients[i], _blobs[i])) {
-            return false;
-        }
-    }
-
-    return true;
-}
-
 void CBLSIESMultiRecipientBlobs::InitEncrypt(size_t count)
 {
     ephemeralSecretKey.MakeNewKey();
@@ -117,7 +80,7 @@ bool CBLSIESMultiRecipientBlobs::Encrypt(size_t idx, const CBLSPublicKey& recipi
         return false;
     }
 
-    std::vector<uint8_t> symKey = pk.ToByteVector();
+    std::vector<uint8_t> symKey = pk.ToByteVector(false);
     symKey.resize(32);
 
     return EncryptBlob(blob.data(), blob.size(), blobs[idx], symKey.data(), ivVector[idx].begin());
@@ -134,7 +97,7 @@ bool CBLSIESMultiRecipientBlobs::Decrypt(size_t idx, const CBLSSecretKey& sk, Bl
         return false;
     }
 
-    std::vector<uint8_t> symKey = pk.ToByteVector();
+    std::vector<uint8_t> symKey = pk.ToByteVector(false);
     symKey.resize(32);
 
     uint256 iv = ivSeed;

src/bls/bls_ies.h

@@ -8,6 +8,11 @@
 #include <bls/bls.h>
 #include <streams.h>
 
+/**
+ * All objects in this module working from assumption that basic scheme is
+ * available on all masternodes. Serialization of public key for Encrypt and
+ * Decrypt by bls_ies.h done using Basic Scheme.
+ */
 class CBLSIESEncryptedBlob
 {
 public:
@@ -22,7 +27,6 @@ class CBLSIESEncryptedBlob
         READWRITE(obj.ephemeralPubKey, obj.ivSeed, obj.data);
     }
 
-    bool Encrypt(size_t idx, const CBLSPublicKey& peerPubKey, const void* data, size_t dataSize);
     bool Decrypt(size_t idx, const CBLSSecretKey& secretKey, CDataStream& decryptedDataRet) const;
     bool IsValid() const;
 };
@@ -40,17 +44,6 @@ class CBLSIESEncryptedObject : public CBLSIESEncryptedBlob
         data = dataIn;
     }
 
-    bool Encrypt(size_t idx, const CBLSPublicKey& peerPubKey, const Object& obj, int nVersion)
-    {
-        try {
-            CDataStream ds(SER_NETWORK, nVersion);
-            ds << obj;
-            return CBLSIESEncryptedBlob::Encrypt(idx, peerPubKey, ds.data(), ds.size());
-        } catch (const std::exception&) {
-            return false;
-        }
-    }
-
     bool Decrypt(size_t idx, const CBLSSecretKey& secretKey, Object& objRet, int nVersion) const
     {
         CDataStream ds(SER_NETWORK, nVersion);
@@ -80,8 +73,6 @@ class CBLSIESMultiRecipientBlobs
     CBLSSecretKey ephemeralSecretKey;
     std::vector<uint256> ivVector;
 
-    bool Encrypt(const std::vector<CBLSPublicKey>& recipients, const BlobVector& _blobs);
-
     void InitEncrypt(size_t count);
     bool Encrypt(size_t idx, const CBLSPublicKey& recipient, const Blob& blob);
     bool Decrypt(size_t idx, const CBLSSecretKey& sk, Blob& blobRet) const;
@@ -96,28 +87,6 @@ template <typename Object>
 class CBLSIESMultiRecipientObjects : public CBLSIESMultiRecipientBlobs
 {
 public:
-    using ObjectVector = std::vector<Object>;
-
-    bool Encrypt(const std::vector<CBLSPublicKey>& recipients, const ObjectVector& _objects, int nVersion)
-    {
-        BlobVector blobs;
-        blobs.resize(_objects.size());
-
-        try {
-            CDataStream ds(SER_NETWORK, nVersion);
-            for (size_t i = 0; i < _objects.size(); i++) {
-                ds.clear();
-
-                ds << _objects[i];
-                blobs[i].assign(UCharCast(ds.data()), UCharCast(ds.data() + ds.size()));
-            }
-        } catch (const std::exception&) {
-            return false;
-        }
-
-        return CBLSIESMultiRecipientBlobs::Encrypt(recipients, blobs);
-    }
-
     bool Encrypt(size_t idx, const CBLSPublicKey& recipient, const Object& obj, int nVersion)
     {
         CDataStream ds(SER_NETWORK, nVersion);

src/bls/bls_worker.cpp

@@ -762,7 +762,7 @@ bool CBLSWorker::VerifyVerificationVectors(Span<BLSVerificationVectorPtr> vvecs)
 void CBLSWorker::AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, const CBLSWorker::SignDoneCallback& doneCallback)
 {
     workerPool.push([secKey, msgHash, doneCallback](int threadId) {
-        doneCallback(secKey.Sign(msgHash));
+        doneCallback(secKey.Sign(msgHash, bls::bls_legacy_scheme.load()));
     });
 }
 

src/coinjoin/coinjoin.cpp

@@ -62,7 +62,7 @@ bool CCoinJoinQueue::Sign(const CActiveMasternodeManager& mn_activeman)
 
 bool CCoinJoinQueue::CheckSignature(const CBLSPublicKey& blsPubKey) const
 {
-    if (!CBLSSignature(Span{vchSig}).VerifyInsecure(blsPubKey, GetSignatureHash(), false)) {
+    if (!CBLSSignature(Span{vchSig}, false).VerifyInsecure(blsPubKey, GetSignatureHash(), false)) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinQueue::CheckSignature -- VerifyInsecure() failed\n");
         return false;
     }
@@ -101,7 +101,7 @@ bool CCoinJoinBroadcastTx::Sign(const CActiveMasternodeManager& mn_activeman)
 
 bool CCoinJoinBroadcastTx::CheckSignature(const CBLSPublicKey& blsPubKey) const
 {
-    if (!CBLSSignature(Span{vchSig}).VerifyInsecure(blsPubKey, GetSignatureHash(), false)) {
+    if (!CBLSSignature(Span{vchSig}, false).VerifyInsecure(blsPubKey, GetSignatureHash(), false)) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinBroadcastTx::CheckSignature -- VerifyInsecure() failed\n");
         return false;
     }

src/evo/assetlocktx.h

@@ -5,7 +5,7 @@
 #ifndef BITCOIN_EVO_ASSETLOCKTX_H
 #define BITCOIN_EVO_ASSETLOCKTX_H
 
-#include <bls/bls_ies.h>
+#include <bls/bls.h>
 #include <consensus/amount.h>
 #include <gsl/pointers.h>
 #include <primitives/transaction.h>

src/evo/mnauth.cpp

@@ -51,7 +51,8 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CActiveMasternode
 
     mnauth.proRegTxHash = mn_activeman.GetProTxHash();
 
-    mnauth.sig = mn_activeman.Sign(signHash);
+    // all clients uses basic BLS
+    mnauth.sig = mn_activeman.Sign(signHash, false);
 
     LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- Sending MNAUTH, peer=%d\n", __func__, peer.GetId());
     connman.PushMessage(&peer, CNetMsgMaker(peer.GetCommonVersion()).Make(NetMsgType::MNAUTH, mnauth));
@@ -86,7 +87,8 @@ PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_services, CCon
     }
 
     if (!mnauth.sig.IsValid()) {
-        LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- invalid mnauth for protx=%s with sig=%s\n", mnauth.proRegTxHash.ToString(), mnauth.sig.ToString());
+        LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- invalid mnauth for protx=%s with sig=%s\n",
+                 mnauth.proRegTxHash.ToString(), mnauth.sig.ToString(false));
         return tl::unexpected{MisbehavingError{100, "invalid mnauth signature"}};
     }
 
@@ -112,7 +114,7 @@ PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_services, CCon
     }
     LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, peer.nVersion, peer.GetId());
 
-    if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash)) {
+    if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash, false)) {
         // Same as above, MN seems to not know its fate yet, so give it a chance to update. If this is a
         // malicious node (DoSing us), it'll get banned soon.
         return tl::unexpected{MisbehavingError{10, "mnauth signature verification failed"}};

src/evo/providertx.h

@@ -112,7 +112,7 @@ class CProRegTx
         return obj;
     }
 
-    bool IsTriviallyValid(bool is_bls_legacy_scheme, TxValidationState& state) const;
+    bool IsTriviallyValid(bool is_basic_scheme_active, TxValidationState& state) const;
 };
 
 class CProUpServTx
@@ -192,7 +192,7 @@ class CProUpServTx
         return obj;
     }
 
-    bool IsTriviallyValid(bool is_bls_legacy_scheme, TxValidationState& state) const;
+    bool IsTriviallyValid(bool is_basic_scheme_active, TxValidationState& state) const;
 };
 
 class CProUpRegTx
@@ -257,7 +257,7 @@ class CProUpRegTx
         return obj;
     }
 
-    bool IsTriviallyValid(bool is_bls_legacy_scheme, TxValidationState& state) const;
+    bool IsTriviallyValid(bool is_basic_scheme_active, TxValidationState& state) const;
 };
 
 class CProUpRevTx
@@ -321,7 +321,7 @@ class CProUpRevTx
         return obj;
     }
 
-    bool IsTriviallyValid(bool is_bls_legacy_scheme, TxValidationState& state) const;
+    bool IsTriviallyValid(bool is_basic_scheme_active, TxValidationState& state) const;
 };
 
 template <typename ProTx>