Skip to content

refactor: deglobalization of bls_legacy_scheme 2/N #5443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 5 commits into from
Jul 1, 2023
Merged

refactor: deglobalization of bls_legacy_scheme 2/N #5443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e8b426c
refactor: remove global usage of bls's version from SetHex
knst Jun 18, 2023
51c152b
refactor: remove SetByteVector's without "is legacy" flag
knst Jun 18, 2023
b5904d9
refactor: pass flag isLegacy to `ToByteVector` whenever possible inst…
knst Jun 18, 2023
c5b55d3
refactor: removed using bls.h header from txmempool.h
knst Jun 19, 2023
308e564
refactor: simplification of rpc/quorums.cpp bcs pindex can't be nullptr
knst Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions src/bls/bls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class CBLSWrapper
explicit CBLSWrapper() = default;
explicit CBLSWrapper(const std::vector<unsigned char>& vecBytes) : CBLSWrapper<ImplType, _SerSize, C>()
{
SetByteVector(vecBytes);
SetByteVector(vecBytes, bls::bls_legacy_scheme.load());
}

CBLSWrapper(const CBLSWrapper& ref) = default;
Expand Down Expand Up @@ -119,11 +119,6 @@ class CBLSWrapper
cachedHash.SetNull();
}

void SetByteVector(const std::vector<uint8_t>& vecBytes)
{
SetByteVector(vecBytes, bls::bls_legacy_scheme.load());
}

std::vector<uint8_t> ToByteVector(const bool specificLegacyScheme) const
{
if (!fValid) {
Expand Down Expand Up @@ -160,11 +155,6 @@ class CBLSWrapper
return IsValid();
}

bool SetHexStr(const std::string& str)
{
return SetHexStr(str, bls::bls_legacy_scheme.load());
}

inline void Serialize(CSizeComputer& s) const
{
s.seek(SerSize);
Expand Down
4 changes: 3 additions & 1 deletion src/governance/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,9 @@ bool CGovernanceObject::Sign(const CBLSSecretKey& key)
if (!sig.IsValid()) {
return false;
}
vchSig = sig.ToByteVector();
const auto pindex = llmq::utils::V19ActivationIndex(::ChainActive().Tip());
bool is_bls_legacy_scheme = pindex == nullptr || nTime < pindex->pprev->nTime;
Comment on lines +311 to +312
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would need cs_main locked?

Copy link
Copy Markdown
Collaborator Author

@knst knst Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
     }

Copy link
Copy Markdown

@UdjinM6 UdjinM6 Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

::ChainActive() locks it inside

vchSig = sig.ToByteVector(is_bls_legacy_scheme);
return true;
}

Expand Down
4 changes: 3 additions & 1 deletion src/governance/vote.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,9 @@ bool CGovernanceVote::Sign(const CBLSSecretKey& key)
if (!sig.IsValid()) {
return false;
}
vchSig = sig.ToByteVector();
const auto pindex = llmq::utils::V19ActivationIndex(::ChainActive().Tip());
bool is_bls_legacy_scheme = pindex == nullptr || nTime < pindex->pprev->nTime;
vchSig = sig.ToByteVector(is_bls_legacy_scheme);
return true;
}

Expand Down
10 changes: 6 additions & 4 deletions src/llmq/dkgsession.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,13 +990,15 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages)
qc.quorumSig = skShare.Sign(commitmentHash);

if (lieType == 3) {
std::vector<uint8_t> buf = qc.sig.ToByteVector();
const bool is_bls_legacy = bls::bls_legacy_scheme.load();
std::vector<uint8_t> buf = qc.sig.ToByteVector(is_bls_legacy);
buf[5]++;
qc.sig.SetByteVector(buf);
qc.sig.SetByteVector(buf, is_bls_legacy);
} else if (lieType == 4) {
std::vector<uint8_t> buf = qc.quorumSig.ToByteVector();
const bool is_bls_legacy = bls::bls_legacy_scheme.load();
std::vector<uint8_t> buf = qc.quorumSig.ToByteVector(is_bls_legacy);
buf[5]++;
qc.quorumSig.SetByteVector(buf);
qc.quorumSig.SetByteVector(buf, is_bls_legacy);
}

t3.stop();
Expand Down
1 change: 1 addition & 0 deletions src/llmq/dkgsession.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <batchedlogger.h>

#include <bls/bls.h>
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Copy Markdown
Collaborator Author

@knst knst Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#include <bls/bls_ies.h>
#include <bls/bls_worker.h>

Expand Down
20 changes: 12 additions & 8 deletions src/rpc/evo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ static CKeyID ParsePubKeyIDFromAddress(const std::string& strAddress, const std:
return ToKeyID(*pkhash);
}

static CBLSPublicKey ParseBLSPubKey(const std::string& hexKey, const std::string& paramName, bool specific_legacy_bls_scheme = false)
static CBLSPublicKey ParseBLSPubKey(const std::string& hexKey, const std::string& paramName, bool specific_legacy_bls_scheme)
{
CBLSPublicKey pubKey;
if (!pubKey.SetHexStr(hexKey, specific_legacy_bls_scheme)) {
Expand All @@ -193,10 +193,11 @@ static CBLSPublicKey ParseBLSPubKey(const std::string& hexKey, const std::string
return pubKey;
}

static CBLSSecretKey ParseBLSSecretKey(const std::string& hexKey, const std::string& paramName)
static CBLSSecretKey ParseBLSSecretKey(const std::string& hexKey, const std::string& paramName, bool specific_legacy_bls_scheme)
{
CBLSSecretKey secKey;
if (!secKey.SetHexStr(hexKey)) {

if (!secKey.SetHexStr(hexKey, specific_legacy_bls_scheme)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be a valid BLS secret key", paramName));
}
return secKey;
Expand Down Expand Up @@ -899,7 +900,8 @@ static UniValue protx_update_service_common_wrapper(const JSONRPCRequest& reques

EnsureWalletIsUnlocked(wallet.get());

bool isV19active = llmq::utils::IsV19Active(WITH_LOCK(cs_main, return ::ChainActive().Tip();));
const bool isV19active = llmq::utils::IsV19Active(WITH_LOCK(cs_main, return ::ChainActive().Tip();));
const bool is_bls_legacy = !isV19active;
if (isHPMNrequested && !isV19active) {
throw JSONRPCError(RPC_INVALID_REQUEST, "HPMN aren't allowed yet");
}
Expand All @@ -912,7 +914,7 @@ static UniValue protx_update_service_common_wrapper(const JSONRPCRequest& reques
throw std::runtime_error(strprintf("invalid network address %s", request.params[1].get_str()));
}

CBLSSecretKey keyOperator = ParseBLSSecretKey(request.params[2].get_str(), "operatorKey");
CBLSSecretKey keyOperator = ParseBLSSecretKey(request.params[2].get_str(), "operatorKey", is_bls_legacy);

size_t paramIdx = 3;
if (isHPMNrequested) {
Expand Down Expand Up @@ -1139,11 +1141,13 @@ static UniValue protx_revoke(const JSONRPCRequest& request)

EnsureWalletIsUnlocked(wallet.get());

const bool isV19active = llmq::utils::IsV19Active(WITH_LOCK(cs_main, return ::ChainActive().Tip();));
const bool is_bls_legacy = !isV19active;
CProUpRevTx ptx;
ptx.nVersion = CProUpRevTx::GetVersion(llmq::utils::IsV19Active(::ChainActive().Tip()));
ptx.nVersion = CProUpRevTx::GetVersion(isV19active);
ptx.proTxHash = ParseHashV(request.params[0], "proTxHash");

CBLSSecretKey keyOperator = ParseBLSSecretKey(request.params[1].get_str(), "operatorKey");
CBLSSecretKey keyOperator = ParseBLSSecretKey(request.params[1].get_str(), "operatorKey", is_bls_legacy);

if (!request.params[2].isNull()) {
int32_t nReason = ParseInt32V(request.params[2], "reason");
Expand Down Expand Up @@ -1701,11 +1705,11 @@ static UniValue bls_fromsecret(const JSONRPCRequest& request)
{
bls_fromsecret_help(request);

CBLSSecretKey sk = ParseBLSSecretKey(request.params[0].get_str(), "secretKey");
bool bls_legacy_scheme = !llmq::utils::IsV19Active(::ChainActive().Tip());
if (!request.params[1].isNull()) {
bls_legacy_scheme = ParseBoolV(request.params[1], "bls_legacy_scheme");
}
CBLSSecretKey sk = ParseBLSSecretKey(request.params[0].get_str(), "secretKey", bls_legacy_scheme);
UniValue ret(UniValue::VOBJ);
ret.pushKV("secret", sk.ToString());
ret.pushKV("public", sk.GetPublicKey().ToString(bls_legacy_scheme));
Expand Down
47 changes: 34 additions & 13 deletions src/rpc/quorums.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,8 +576,9 @@ static UniValue quorum_sigs_cmd(const JSONRPCRequest& request)
return obj;
}
} else if (cmd == "quorumverify") {
const bool use_bls_legacy = bls::bls_legacy_scheme.load();
CBLSSignature sig;
if (!sig.SetHexStr(request.params[3].get_str())) {
if (!sig.SetHexStr(request.params[3].get_str(), use_bls_legacy)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
}

Expand Down Expand Up @@ -874,20 +875,37 @@ static UniValue verifychainlock(const JSONRPCRequest& request)

const uint256 nBlockHash(ParseHashV(request.params[0], "blockHash"));

CBLSSignature sig;
if (!sig.SetHexStr(request.params[1].get_str())) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
}

int nBlockHeight;
CBlockIndex* pIndex{nullptr};
if (request.params[2].isNull()) {
const CBlockIndex* pIndex = WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(nBlockHash));
pIndex = WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(nBlockHash));
if (pIndex == nullptr) {
throw JSONRPCError(RPC_INTERNAL_ERROR, "blockHash not found");
}
nBlockHeight = pIndex->nHeight;
} else {
nBlockHeight = ParseInt32V(request.params[2], "blockHeight");
LOCK(cs_main);
if (nBlockHeight < 0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range");
}
if (nBlockHeight <= ::ChainActive().Height()) {
pIndex = ::ChainActive()[nBlockHeight];
}
Comment thread
UdjinM6 marked this conversation as resolved.
Outdated
}

CBLSSignature sig;
if (pIndex) {
Comment thread
PastaPastaPasta marked this conversation as resolved.
bool use_legacy_signature = !llmq::utils::IsV19Active(pIndex);
if (!sig.SetHexStr(request.params[1].get_str(), use_legacy_signature)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
}
} else {
if (!sig.SetHexStr(request.params[1].get_str(), false) &&
!sig.SetHexStr(request.params[1].get_str(), true)
) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
}
Comment thread
UdjinM6 marked this conversation as resolved.
Outdated
}

LLMQContext& llmq_ctx = EnsureLLMQContext(request.context);
Expand Down Expand Up @@ -917,11 +935,6 @@ static UniValue verifyislock(const JSONRPCRequest& request)
uint256 id(ParseHashV(request.params[0], "id"));
uint256 txid(ParseHashV(request.params[1], "txid"));

CBLSSignature sig;
if (!sig.SetHexStr(request.params[2].get_str())) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
}

if (g_txindex) {
g_txindex->BlockUntilSyncedToCurrentChain();
}
Expand All @@ -948,7 +961,7 @@ static UniValue verifyislock(const JSONRPCRequest& request)
signHeight = pindexMined->nHeight;
}

CBlockIndex* pBlockIndex;
CBlockIndex* pBlockIndex{nullptr};
{
LOCK(cs_main);
if (signHeight == -1) {
Expand All @@ -958,6 +971,14 @@ static UniValue verifyislock(const JSONRPCRequest& request)
}
}

CHECK_NONFATAL(pBlockIndex != nullptr);

CBLSSignature sig;
const bool use_bls_legacy = !llmq::utils::IsV19Active(pBlockIndex);
if (!sig.SetHexStr(request.params[2].get_str(), use_bls_legacy)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
}

LLMQContext& llmq_ctx = EnsureLLMQContext(request.context);

auto llmqType = llmq::utils::GetInstantSendLLMQType(*llmq_ctx.qman, pBlockIndex);
Expand Down
12 changes: 6 additions & 6 deletions src/test/bls_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,16 @@ void FuncSetHexStr(const bool legacy_scheme)
CBLSSecretKey sk;
std::string strValidSecret = "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";
// Note: invalid string passed to SetHexStr() should cause it to fail and reset key internal data
BOOST_CHECK(sk.SetHexStr(strValidSecret));
BOOST_CHECK(!sk.SetHexStr("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1g")); // non-hex
BOOST_CHECK(sk.SetHexStr(strValidSecret, legacy_scheme));
BOOST_CHECK(!sk.SetHexStr("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1g", legacy_scheme)); // non-hex
BOOST_CHECK(!sk.IsValid());
BOOST_CHECK(sk == CBLSSecretKey());
// Try few more invalid strings
BOOST_CHECK(sk.SetHexStr(strValidSecret));
BOOST_CHECK(!sk.SetHexStr("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e")); // hex but too short
BOOST_CHECK(sk.SetHexStr(strValidSecret, legacy_scheme));
BOOST_CHECK(!sk.SetHexStr("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e", legacy_scheme)); // hex but too short
BOOST_CHECK(!sk.IsValid());
BOOST_CHECK(sk.SetHexStr(strValidSecret));
BOOST_CHECK(!sk.SetHexStr("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20")); // hex but too long
BOOST_CHECK(sk.SetHexStr(strValidSecret, legacy_scheme));
BOOST_CHECK(!sk.SetHexStr("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20", legacy_scheme)); // hex but too long
BOOST_CHECK(!sk.IsValid());

return;
Expand Down
6 changes: 5 additions & 1 deletion src/txmempool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include <addressindex.h>
#include <spentindex.h>
#include <amount.h>
#include <bls/bls.h>
#include <coins.h>
#include <crypto/siphash.h>
#include <indirectmap.h>
Expand All @@ -38,6 +37,11 @@ class CBlockIndex;
class CChainState;
extern RecursiveMutex cs_main;

// Forward declation for CBLSLazyPublicKey:
template<typename T> class CBLSLazyWrapper;
class CBLSPublicKey;
using CBLSLazyPublicKey = CBLSLazyWrapper<CBLSPublicKey>;

/** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */
static const uint32_t MEMPOOL_HEIGHT = 0x7FFFFFFF;

Expand Down