diff --git a/src/chainlock/handler.cpp b/src/chainlock/handler.cpp index 7dacdd7b0fa5..0c71f072e674 100644 --- a/src/chainlock/handler.cpp +++ b/src/chainlock/handler.cpp @@ -110,7 +110,7 @@ MessageProcessingResult ChainlockHandler::ProcessCoinbaseChainLock(const CBlock& } const auto cbtx = GetTxPayload(*block.vtx.front(), /*assert_type=*/false); - if (!cbtx || cbtx->nVersion < CCbTx::Version::CLSIG_AND_BALANCE || !cbtx->bestCLSignature.IsValid()) { + if (!cbtx || cbtx->nVersion < CCbTx::Version::CLSIG_AND_BALANCE || !cbtx->bestCLSignature.Get().IsValid()) { return {}; } @@ -126,7 +126,7 @@ MessageProcessingResult ChainlockHandler::ProcessCoinbaseChainLock(const CBlock& if (ancestor == nullptr) { return {}; } - const ChainLockSig clsig{height, ancestor->GetBlockHash(), cbtx->bestCLSignature}; + const ChainLockSig clsig{height, ancestor->GetBlockHash(), cbtx->bestCLSignature.Get()}; return ProcessNewChainLock(/*from=*/-1, clsig, qman, ::SerializeHash(clsig)); } diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 10f23bf6deb1..e67afdddc4c5 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -51,6 +51,23 @@ bool CheckCbTx(const CCbTx& cbTx, const CBlockIndex* pindexPrev, bool is_v24_act } } + // The signature is deserialized lazily. Get() decodes it in the active scheme only and + // resets the wrapper on failure, which is what the eager parser did with bytes encoded + // in the opposite scheme; bytes that decode in neither scheme made it throw, so they + // are rejected here. + if (cbTx.nVersion >= CCbTx::Version::CLSIG_AND_BALANCE) { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << cbTx.bestCLSignature; + if (!cbTx.bestCLSignature.Get().IsValid()) { + try { + CBLSSignature sig; + ss >> sig; + } catch (const std::ios_base::failure&) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-payload"); + } + } + } + return true; } @@ -203,8 +220,8 @@ std::optional> GetNonNullCoinbaseChainlock(co } const auto payload = GetTxPayload(*block.vtx[0]); if (!payload || payload->nVersion < CCbTx::Version::CLSIG_AND_BALANCE || payload->nHeight != height || - !payload->bestCLSignature.IsValid() || payload->bestCLHeightDiff >= uint32_t(height)) { + !payload->bestCLSignature.Get().IsValid() || payload->bestCLHeightDiff >= uint32_t(height)) { return std::nullopt; } - return std::make_pair(payload->bestCLSignature, payload->bestCLHeightDiff); + return std::make_pair(payload->bestCLSignature.Get(), payload->bestCLHeightDiff); } diff --git a/src/evo/cbtx.h b/src/evo/cbtx.h index 2a4eca48658e..45ddee56b310 100644 --- a/src/evo/cbtx.h +++ b/src/evo/cbtx.h @@ -44,7 +44,7 @@ class CCbTx uint256 merkleRootMNList; uint256 merkleRootQuorums; uint32_t bestCLHeightDiff{0}; - CBLSSignature bestCLSignature; + CBLSLazySignature bestCLSignature; CAmount creditPoolBalance{0}; /** Merkle root over the instance hashes of the block's version 2+ asset unlock transactions * (block order; null when there are none). Their txids exclude the quorum signing info, so diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index 4085b40d454e..078de7ee8da2 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -163,24 +163,32 @@ bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const if (cbTx.nVersion < CCbTx::Version::CLSIG_AND_BALANCE) { return true; } - + const CBLSSignature& cbtx_sig{cbTx.bestCLSignature.Get()}; + + struct CachedChainlock { + const CBlockIndex* pindex{nullptr}; + CBLSSignature sig; + uint32_t height_diff{0}; + //! Passed VerifyChainLock in this process (or was carried over from such a block) rather than + //! merely matched the chainlock handler's best lock, whose verification scheme is unknown here + bool verified_here{false}; + }; static Mutex cached_mutex; - static const CBlockIndex* cached_pindex GUARDED_BY(cached_mutex){nullptr}; - static std::optional> cached_chainlock GUARDED_BY(cached_mutex){std::nullopt}; + static std::optional cached GUARDED_BY(cached_mutex); auto best_clsig = chainlocks.GetBestChainLock(); if (best_clsig.getHeight() == pindex->nHeight - 1 && cbTx.bestCLHeightDiff == 0 && - cbTx.bestCLSignature == best_clsig.getSig()) { + cbtx_sig == best_clsig.getSig()) { // matches our best clsig which still hold values for the previous block - LOCK(cached_mutex); - cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff); - cached_pindex = pindex; + WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/false)); return true; } std::optional> prevBlockCoinbaseChainlock{std::nullopt}; - if (LOCK(cached_mutex); cached_pindex == pindex->pprev) { - prevBlockCoinbaseChainlock = cached_chainlock; + bool prev_verified_here{false}; + if (LOCK(cached_mutex); cached && cached->pindex == pindex->pprev) { + prevBlockCoinbaseChainlock = std::make_pair(cached->sig, cached->height_diff); + prev_verified_here = cached->verified_here; } if (!prevBlockCoinbaseChainlock.has_value()) { prevBlockCoinbaseChainlock = GetNonNullCoinbaseChainlock(pindex->pprev); @@ -188,7 +196,7 @@ bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const // If std::optional prevBlockCoinbaseChainlock is empty, then up to the previous block, coinbase Chainlock is null. if (prevBlockCoinbaseChainlock.has_value()) { // Previous block Coinbase has a non-null Chainlock: current block's Chainlock must be non-null and at least as new as the previous one - if (!cbTx.bestCLSignature.IsValid()) { + if (!cbtx_sig.IsValid()) { // IsNull() doesn't exist for CBLSSignature: we assume that a non valid BLS sig is null return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-null-clsig"); } @@ -198,17 +206,23 @@ bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const } // IsNull() doesn't exist for CBLSSignature: we assume that a valid BLS sig is non-null - if (cbTx.bestCLSignature.IsValid()) { + if (cbtx_sig.IsValid()) { // Reject out-of-range bestCLHeightDiff that would yield a pre-genesis ancestor height. if (cbTx.bestCLHeightDiff >= static_cast(pindex->nHeight)) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff"); } int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast(cbTx.bestCLHeightDiff) - 1; - if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == cbTx.bestCLSignature) { + if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == cbtx_sig) { // matches our best (but outdated) clsig, no need to verify it again - LOCK(cached_mutex); - cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff); - cached_pindex = pindex; + WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/false)); + return true; + } + // The previous block's coinbase chainlock was verified by this process; carried into this + // block with the offset grown by one it names the same height, block and signature. + if (prev_verified_here && prevBlockCoinbaseChainlock.has_value() && + cbTx.bestCLHeightDiff == prevBlockCoinbaseChainlock->second + 1 && + cbtx_sig == prevBlockCoinbaseChainlock->first) { + WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/true)); return true; } const CBlockIndex* pAncestor = pindex->GetAncestor(curBlockCoinbaseCLHeight); @@ -218,14 +232,12 @@ bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff-ancestor"); } uint256 curBlockCoinbaseCLBlockHash = pAncestor->GetBlockHash(); - chainlock::ChainLockSig clsig{curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbTx.bestCLSignature}; + chainlock::ChainLockSig clsig{curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbtx_sig}; llmq::VerifyRecSigStatus ret = chainlock::VerifyChainLock(consensus_params, chain, qman, clsig); if (ret != llmq::VerifyRecSigStatus::Valid) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-invalid-clsig"); } - LOCK(cached_mutex); - cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff); - cached_pindex = pindex; + WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/true)); } else if (cbTx.bestCLHeightDiff != 0) { // Null bestCLSignature is allowed only with bestCLHeightDiff = 0 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff"); diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index 225268291879..ba1721cd8489 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -1031,6 +1031,9 @@ class CRegTestParams : public CChainParams { assert(consensus.V20Height >= consensus.DIP0003Height); // MN_RR reallocate part of reward to CreditPool which exits since V20 assert(consensus.MN_RRHeight >= consensus.V20Height); + // Coinbase chainlocks (V20) are verified once and reused by the next block, so the BLS + // scheme must not change after V20 activation + assert(consensus.V20Height >= consensus.V19Height); } private: diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 4e06d4d84bb4..93b93c28811a 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -354,7 +354,10 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc throw std::runtime_error(strprintf("%s: CalcCbTxMerkleRootQuorums failed: %s", __func__, state.ToString())); } if (fV20Active_context) { - if (CalcCbTxBestChainlock(m_chainlocks, pindexPrev, cbTx.bestCLHeightDiff, cbTx.bestCLSignature)) { + CBLSSignature best_cl_sig; + const bool has_best_cl{CalcCbTxBestChainlock(m_chainlocks, pindexPrev, cbTx.bestCLHeightDiff, best_cl_sig)}; + cbTx.bestCLSignature.Set(best_cl_sig, bls::bls_legacy_scheme.load()); + if (has_best_cl) { LogPrintf("CreateNewBlock() h[%d] CbTx bestCLHeightDiff[%d] CLSig[%s]\n", nHeight, cbTx.bestCLHeightDiff, cbTx.bestCLSignature.ToString()); } else { // not an error diff --git a/src/test/evo_cbtx_tests.cpp b/src/test/evo_cbtx_tests.cpp index ff9f42cc40c2..090ac9de96b2 100644 --- a/src/test/evo_cbtx_tests.cpp +++ b/src/test/evo_cbtx_tests.cpp @@ -22,9 +22,11 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -66,7 +68,7 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, CCbTx cbTx; cbTx.nVersion = CCbTx::Version::CLSIG_AND_BALANCE; - cbTx.bestCLSignature = valid_sig; + cbTx.bestCLSignature.Set(valid_sig, legacy_scheme); // bestCLHeightDiff == nHeight: lower boundary of the rejected range. cbTx.bestCLHeightDiff = static_cast(pindex.nHeight); @@ -79,6 +81,20 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, BlockValidationState state_big; BOOST_CHECK(!CheckCbTxBestChainlock(cbTx, &pindex, consensus_params, chain, qman, chainlocks, state_big)); BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff"); + + // The signature is deserialized lazily, so the payload parser accepts any bytes in its + // place and CheckCbTx rejects an encoding that is not a signature; the valid signature + // above and the null encoding pass. + TxValidationState tx_state; + BOOST_CHECK(CheckCbTx(cbTx, /*pindexPrev=*/nullptr, /*is_v24_active=*/false, tx_state)); + std::vector sig_bytes(CBLSSignature::SerSize, 0xff); + CDataStream(sig_bytes, SER_NETWORK, PROTOCOL_VERSION) >> cbTx.bestCLSignature; + BOOST_CHECK(!CheckCbTx(cbTx, /*pindexPrev=*/nullptr, /*is_v24_active=*/false, tx_state)); + BOOST_CHECK_EQUAL(tx_state.GetRejectReason(), "bad-cbtx-payload"); + std::fill(sig_bytes.begin(), sig_bytes.end(), 0); + CDataStream(sig_bytes, SER_NETWORK, PROTOCOL_VERSION) >> cbTx.bestCLSignature; + TxValidationState null_state; + BOOST_CHECK(CheckCbTx(cbTx, /*pindexPrev=*/nullptr, /*is_v24_active=*/false, null_state)); } namespace { diff --git a/src/test/llmq_chainlock_tests.cpp b/src/test/llmq_chainlock_tests.cpp index 8fcc541d82a8..e3aed1100e00 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -71,7 +71,7 @@ BOOST_FIXTURE_TEST_CASE(historical_coinbase_lookup_from_disk, RegTestingSetup) } else { certified = activation + 16999; } - payload.bestCLSignature = signature; + payload.bestCLSignature.Set(signature, bls::bls_legacy_scheme.load()); payload.bestCLHeightDiff = height - certified - 1; } CMutableTransaction coinbase; @@ -415,7 +415,7 @@ BOOST_FIXTURE_TEST_CASE(coinbase_chainlock_processing, RegTestingSetup) CCbTx cbtx; cbtx.nVersion = CCbTx::Version::CLSIG_AND_BALANCE; - cbtx.bestCLSignature = CreateRandomBLSSignature(); + cbtx.bestCLSignature.Set(CreateRandomBLSSignature(), bls::bls_legacy_scheme.load()); CMutableTransaction tx; tx.nVersion = CTransaction::SPECIAL_VERSION; tx.nType = TRANSACTION_COINBASE; @@ -439,7 +439,7 @@ BOOST_FIXTURE_TEST_CASE(coinbase_chainlock_processing, RegTestingSetup) set_payload(); process(&indexes.back()); const int32_t height = 499 - offset; - const ChainLockSig expected{height, hashes[height], cbtx.bestCLSignature}; + const ChainLockSig expected{height, hashes[height], cbtx.bestCLSignature.Get()}; BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, ::SerializeHash(expected)})); } const auto seen = m_node.clhandler->SeenChainLockCacheSizeForTesting(); @@ -464,7 +464,7 @@ BOOST_FIXTURE_TEST_CASE(coinbase_chainlock_processing, RegTestingSetup) process(&indexes.back()); BOOST_REQUIRE(m_node.sporkman->UpdateSpork(SPORK_19_CHAINLOCKS_ENABLED, 0).has_value()); - cbtx.bestCLSignature = {}; + cbtx.bestCLSignature = CBLSLazySignature{}; set_payload(); process(&indexes.back()); cbtx.nVersion = CCbTx::Version::MERKLE_ROOT_QUORUMS; diff --git a/src/test/quorum_proofs_tests.cpp b/src/test/quorum_proofs_tests.cpp index f79e4a463992..582f884fde54 100644 --- a/src/test/quorum_proofs_tests.cpp +++ b/src/test/quorum_proofs_tests.cpp @@ -163,11 +163,12 @@ struct QuorumProofGenerationSetup : TestingSetup { }); BOOST_REQUIRE(match != commitments.end()); const size_t key = size_t(match - commitments.begin()); - payload.bestCLSignature = keys[key].Sign(llmq::SignHash{params.type, signer->quorumHash, - chainlock::GenSigRequestId(height - 1), - previous->GetBlockHash()} - .Get(), - false); + payload.bestCLSignature.Set(keys[key].Sign(llmq::SignHash{params.type, signer->quorumHash, + chainlock::GenSigRequestId(height - 1), + previous->GetBlockHash()} + .Get(), + false), + bls::bls_legacy_scheme.load()); } const bool mining = height == checkpoint_height - params.dkgInterval || height == checkpoint_height || (mine_nonstandard_handoff && height == handoff_height); diff --git a/test/functional/feature_protx_version.py b/test/functional/feature_protx_version.py index 5af72f1719d7..f6d7adaf15ef 100755 --- a/test/functional/feature_protx_version.py +++ b/test/functional/feature_protx_version.py @@ -38,6 +38,8 @@ def set_test_params(self): f'-vbparams=v24:{self.mocktime}:999999999999:350:10:8:6:5:0', ]] * 2 self.set_dash_test_params(2, 1, evo_count=2, extra_args=self.extra_args) + # regtest requires v20 not earlier than v19 + self.delay_v20_and_mn_rr(200) def get_peer_ids(self, node_idx): return {peer['id'] for peer in self.nodes[node_idx].getpeerinfo()}