diff --git a/src/Makefile.test.include b/src/Makefile.test.include index faaee5aa1913..4334a96d9664 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -123,6 +123,7 @@ BITCOIN_TESTS =\ test/governance_inv_tests.cpp \ test/governance_superblock_tests.cpp \ test/governance_validators_tests.cpp \ + test/governance_vote_sync_tests.cpp \ test/governance_vote_wire_tests.cpp \ test/coinjoin_inouts_tests.cpp \ test/coinjoin_dstxmanager_tests.cpp \ diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index d87fef40fac5..ea04abc07956 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -656,17 +656,20 @@ std::vector CGovernanceManager::GetSyncableVoteInvs(const uint256& nProp, const auto& govobj = *Assert(it->second); LOCK(govobj.cs); - const auto& fileVotes = govobj.GetVoteFile(); - for (const auto& vote : fileVotes.GetVotes()) { + // Visit the stored votes in place: CheckSignature memoises its verdict on + // the vote instance, and a GetVotes() copy would discard that memo, so every + // walk would pay a fresh ECDSA recovery or BLS pairing per vote. + invs.reserve(govobj.GetVoteFile().GetVoteCount()); + govobj.GetVoteFile().ForEachVote([&](const CGovernanceVote& vote) { uint256 nVoteHash = vote.GetHash(); bool onlyVotingKeyAllowed = govobj.GetObjectType() == GovernanceObject::PROPOSAL && vote.GetSignal() == VOTE_SIGNAL_FUNDING; if (filter.contains(nVoteHash) || !vote.IsValid(tip_mn_list, onlyVotingKeyAllowed)) { - continue; + return; } invs.emplace_back(MSG_GOVERNANCE_OBJECT_VOTE, nVoteHash); - } + }); return invs; } diff --git a/src/governance/vote.cpp b/src/governance/vote.cpp index b1ba269872be..afef2543fbae 100644 --- a/src/governance/vote.cpp +++ b/src/governance/vote.cpp @@ -19,6 +19,42 @@ static_assert(CGovernanceVote::COMPACT_SIG_SIZE == CPubKey::COMPACT_SIGNATURE_SIZE); static_assert(CGovernanceVote::BLS_SIG_SIZE == CBLSSignature::SerSize); +namespace { +//! Hash the inputs that determine the verification result. Dropping any of them +//! breaks the memo; see CGovernanceVote::SignatureMemo. +uint256 SignatureCacheKey(const CKeyID& key, const uint256& sigHash, const std::vector& vchSig) +{ + HashWriter ss{}; + ss << key << sigHash << vchSig; + return ss.GetHash(); +} + +//! The BLS verdict is always computed with VerifyInsecure(..., /*specificLegacyScheme=*/false), +//! so the key must be fingerprinted under that same scheme. Serializing via operator<< would +//! instead use the mutable bls_legacy_scheme global, and a legacy encoding of P can equal the +//! basic encoding of -P. Across an activation or a reorg that flips the global, a verdict cached +//! for one key could then be served for the other. +uint256 SignatureCacheKey(const CBLSPublicKey& key, const uint256& sigHash, + const std::vector& vchSig) +{ + HashWriter ss{}; + const auto key_bytes = key.ToBytes(/*specificLegacyScheme=*/false); + ss.write(MakeByteSpan(key_bytes)); + ss << sigHash << vchSig; + return ss.GetHash(); +} +} // namespace + +std::optional CGovernanceVote::GetMemoisedVerdict(const CKeyID& keyID) const +{ + return m_sig_memo.Lookup(SignatureCacheKey(keyID, GetSignatureHash(), vchSig)); +} + +std::optional CGovernanceVote::GetMemoisedVerdict(const CBLSPublicKey& pubKey) const +{ + return m_sig_memo.Lookup(SignatureCacheKey(pubKey, GetSignatureHash(), vchSig)); +} + std::string CGovernanceVoting::ConvertOutcomeToString(vote_outcome_enum_t nOutcome) { static const std::map mapOutcomeString = { @@ -127,33 +163,49 @@ uint256 CGovernanceVote::GetHash() const bool CGovernanceVote::CheckSignature(const CKeyID& keyID) const { + const uint256 sigHash{GetSignatureHash()}; + const uint256 fingerprint{SignatureCacheKey(keyID, sigHash, vchSig)}; + if (const auto memoised{m_sig_memo.Lookup(fingerprint)}) { + return *memoised; + } + std::string strError; + bool valid{false}; // Harden Spork6 so that it is active on testnet and no other networks if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { - if (!CHashSigner::VerifyHash(GetSignatureHash(), keyID, vchSig, strError)) { + valid = CHashSigner::VerifyHash(sigHash, keyID, vchSig, strError); + if (!valid) { LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- VerifyHash() failed, error: %s\n", strError); - return false; } } else { - if (!CMessageSigner::VerifyMessage(keyID, vchSig, GetSignatureString(), strError)) { + valid = CMessageSigner::VerifyMessage(keyID, vchSig, GetSignatureString(), strError); + if (!valid) { LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- VerifyMessage() failed, error: %s\n", strError); - return false; } } - return true; + m_sig_memo.Store(fingerprint, valid); + return valid; } bool CGovernanceVote::CheckSignature(const CBLSPublicKey& pubKey) const { + const uint256 sigHash{GetSignatureHash()}; + const uint256 fingerprint{SignatureCacheKey(pubKey, sigHash, vchSig)}; + if (const auto memoised{m_sig_memo.Lookup(fingerprint)}) { + return *memoised; + } + CBLSSignature sig; sig.SetBytes(vchSig, false); - if (!sig.VerifyInsecure(pubKey, GetSignatureHash(), false)) { + const bool valid{sig.VerifyInsecure(pubKey, sigHash, false)}; + if (!valid) { LogPrintf("CGovernanceVote::CheckSignature -- VerifyInsecure() failed\n"); - return false; } - return true; + + m_sig_memo.Store(fingerprint, valid); + return valid; } bool CGovernanceVote::IsValid(const CDeterministicMNList& tip_mn_list, bool useVotingKey) const diff --git a/src/governance/vote.h b/src/governance/vote.h index cd33d5434898..4c20c917c038 100644 --- a/src/governance/vote.h +++ b/src/governance/vote.h @@ -12,6 +12,8 @@ #include #include +#include + class CBLSPublicKey; class CDeterministicMNList; class CKeyID; @@ -81,6 +83,44 @@ class CGovernanceVote const uint256 hash{0}; void UpdateHash() const; + /** + * Result of the last CheckSignature call, so that re-verifying an unchanged + * vote costs a hash instead of an ECDSA recovery or a BLS pairing. + * + * The fingerprint covers the key, the signature hash and the signature + * bytes, so a hit means this exact verification already ran. Answering from + * a bare "already checked" flag would be wrong two ways: the same vote gets + * checked against different keys (voting vs operator, and operator keys + * rotate), and a vote can be modified after verification, which would + * otherwise keep its stale verdict. + */ + class SignatureMemo + { + std::optional m_fingerprint; + bool m_valid{false}; + + public: + std::optional Lookup(const uint256& fingerprint) const + { + if (m_fingerprint != fingerprint) return std::nullopt; + return m_valid; + } + + void Store(const uint256& fingerprint, bool valid) + { + m_fingerprint = fingerprint; + m_valid = valid; + } + }; + + /** + * Memory only, and not internally synchronised. Shared votes live in + * CGovernanceObjectVoteFile, reachable only through + * CGovernanceObject::GetVoteFile() under the object's cs; every other vote + * has a single owner. + */ + mutable SignatureMemo m_sig_memo; + public: CGovernanceVote() = default; CGovernanceVote(const COutPoint& outpointMasternodeIn, const uint256& nParentHashIn, vote_signal_enum_t eVoteSignalIn, vote_outcome_enum_t eVoteOutcomeIn); @@ -109,6 +149,11 @@ class CGovernanceVote bool CheckSignature(const CKeyID& keyID) const; bool CheckSignature(const CBLSPublicKey& pubKey) const; bool IsValid(const CDeterministicMNList& tip_mn_list, bool useVotingKey) const; + + /** The memoised verdict for this key, or nullopt if the next CheckSignature + * with it would have to run the cryptography. */ + std::optional GetMemoisedVerdict(const CKeyID& keyID) const; + std::optional GetMemoisedVerdict(const CBLSPublicKey& pubKey) const; std::string GetSignatureString() const { return masternodeOutpoint.ToStringShort() + "|" + nParentHash.ToString() + "|" + diff --git a/src/governance/votedb.h b/src/governance/votedb.h index d38d9b87ebdd..5032d6317001 100644 --- a/src/governance/votedb.h +++ b/src/governance/votedb.h @@ -66,6 +66,16 @@ class CGovernanceObjectVoteFile std::vector GetVotes() const; + /** Visit the stored votes in place, so callers warm and reuse the per-vote + * signature memo instead of discarding it with a GetVotes() copy. Takes a + * callback rather than returning the list: the caller's lock on the owning + * object's cs cannot be expressed on a returned reference. */ + template + void ForEachVote(Fn&& fn) const + { + for (const auto& vote : listVotes) fn(vote); + } + void RemoveVotesFromMasternode(const COutPoint& outpointMasternode); std::set RemoveInvalidVotes(const CDeterministicMNList& tip_mn_list, const COutPoint& outpointMasternode, bool fProposal); diff --git a/src/test/governance_vote_sync_tests.cpp b/src/test/governance_vote_sync_tests.cpp new file mode 100644 index 000000000000..b6e06e5b8a8c --- /dev/null +++ b/src/test/governance_vote_sync_tests.cpp @@ -0,0 +1,201 @@ +// Copyright (c) 2026 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +#include +#include + +namespace { +//! A vote carrying a valid BLS operator signature over its own signed content. +CGovernanceVote MakeSignedVote(const CBLSSecretKey& sk, const COutPoint& outpoint, const uint256& parent, + vote_signal_enum_t signal, int64_t time) +{ + CGovernanceVote vote{outpoint, parent, signal, VOTE_OUTCOME_YES}; + vote.SetTime(time); + vote.SetSignature(sk.Sign(vote.GetSignatureHash(), /*legacy_scheme=*/false).ToByteVector(/*legacy=*/false)); + return vote; +} +} // namespace + +BOOST_FIXTURE_TEST_SUITE(governance_vote_sync_tests, BasicTestingSetup) + +// CNetFulfilledRequestManager must stay keyed on the full CService, port included. +// +// It is tempting to strip the port: peer.addr carries the remote ephemeral source +// port for inbound peers, so the govsync quota is per-connection and an attacker +// resets it by reconnecting. But this map is shared with SyncManager's per-peer +// outbound bookkeeping ("full-sync", "governance-sync", "spork-sync", +// "mempool-sync") and the govsync quota is coupled to PeerMisbehaving(20). +// Several honest nodes legitimately share one address (NAT, colocation, CI), and +// collapsing them onto one key makes the second honest peer look like a repeat +// offender -- the ban score escalates to discouragement, and mnsync breaks +// outright (feature_governance.py, where every node is on 127.0.0.1). +// +// This test pins port-sensitivity as the intended behaviour. Bounding the cost +// of a single vote-sync request is a separate problem from this quota. +BOOST_AUTO_TEST_CASE(fulfilled_request_manager_is_port_sensitive) +{ + in_addr ipv4{}; + ipv4.s_addr = htonl(0x0a000001); // 10.0.0.1 + const CService peer_a{CNetAddr{ipv4}, /*port=*/40000}; + const CService peer_b{CNetAddr{ipv4}, /*port=*/40001}; + + CNetFulfilledRequestManager fulfilled; + BOOST_REQUIRE(fulfilled.LoadCache(/*load_cache=*/false)); + + const std::string request{"mngovernancesync-votes-deadbeef"}; + BOOST_CHECK(!fulfilled.HasFulfilledRequest(peer_a, request)); + fulfilled.AddFulfilledRequest(peer_a, request); + BOOST_CHECK(fulfilled.HasFulfilledRequest(peer_a, request)); + + // A different peer on the same host must NOT inherit peer_a's quota, or it + // would be misbehaviour-scored for its own first request. + BOOST_CHECK_MESSAGE(!fulfilled.HasFulfilledRequest(peer_b, request), + "distinct peers sharing an address must not share a fulfilled-request quota"); + + // Per-peer cleanup must likewise not disturb the other peer's entries. + fulfilled.AddFulfilledRequest(peer_b, request); + fulfilled.RemoveAllFulfilledRequests(peer_b); + BOOST_CHECK(!fulfilled.HasFulfilledRequest(peer_b, request)); + BOOST_CHECK(fulfilled.HasFulfilledRequest(peer_a, request)); +} + +// Every vote-sync request walks the stored votes and calls IsValid/CheckSignature +// on each one, so an unchanged vote checked against the same key must reuse its +// memoised verdict instead of paying for another BLS pairing or ECDSA recovery. +BOOST_AUTO_TEST_CASE(vote_signature_verification_is_cached) +{ + CBLSSecretKey sk; + sk.MakeNewKey(); + const CBLSPublicKey pk{sk.GetPublicKey()}; + + CGovernanceVote vote{MakeSignedVote(sk, COutPoint{uint256S("01"), /*n=*/0}, uint256S("02"), VOTE_SIGNAL_VALID, + 1'700'000'000)}; + + // Nothing memoised yet, so the first check has to run the pairing. + BOOST_CHECK(!vote.GetMemoisedVerdict(pk).has_value()); + + BOOST_REQUIRE(vote.CheckSignature(pk)); + BOOST_CHECK(vote.GetMemoisedVerdict(pk) == true); + + // A different operator key is a different question: it must not be answered + // from the memo, and checking it takes the single slot. + CBLSSecretKey sk_other; + sk_other.MakeNewKey(); + const CBLSPublicKey pk_other{sk_other.GetPublicKey()}; + BOOST_CHECK(!vote.GetMemoisedVerdict(pk_other).has_value()); + BOOST_CHECK(!vote.CheckSignature(pk_other)); + BOOST_CHECK(vote.GetMemoisedVerdict(pk_other) == false); + BOOST_CHECK(!vote.GetMemoisedVerdict(pk).has_value()); + + // Re-checking the original key repopulates it. + BOOST_REQUIRE(vote.CheckSignature(pk)); + BOOST_CHECK(vote.GetMemoisedVerdict(pk) == true); +} + +// The cache must live on the vote instances stored in CGovernanceObjectVoteFile, +// not on ephemeral GetVotes() copies, so repeated sync walks stay cheap. +BOOST_AUTO_TEST_CASE(stored_vote_file_reuses_signature_cache) +{ + CBLSSecretKey sk; + sk.MakeNewKey(); + const CBLSPublicKey pk{sk.GetPublicKey()}; + + CGovernanceVote vote{MakeSignedVote(sk, COutPoint{uint256S("11"), /*n=*/1}, uint256S("22"), VOTE_SIGNAL_DELETE, + 1'700'000'100)}; + + CGovernanceObjectVoteFile file; + // Pre-check so AddVote stores a vote whose cache is already warm — the same + // sequence ProcessVote uses (IsValid then AddVote). + BOOST_REQUIRE(vote.CheckSignature(pk)); + file.AddVote(vote); + BOOST_REQUIRE_EQUAL(file.GetVoteCount(), 1); + + // The stored instance must carry the warm memo, so a sync walk answers from + // it rather than re-running the pairing for every vote it visits. + file.ForEachVote([&](const CGovernanceVote& stored) { + BOOST_CHECK_MESSAGE(stored.GetMemoisedVerdict(pk) == true, + "the stored vote lost the memo warmed before AddVote"); + BOOST_REQUIRE(stored.CheckSignature(pk)); + BOOST_CHECK(stored.GetMemoisedVerdict(pk) == true); + }); +} + +// The memo must never be keyed on the verification key alone. nTime is part of +// the signed payload (GetSignatureString/SerializeHash) but is mutable via +// SetTime(), which governance.cpp and object.cpp call on reconstructed votes +// before verifying them. A key-only memo would return the stale "valid" verdict +// for content that was never signed -- a signature-check bypass. +BOOST_AUTO_TEST_CASE(signature_cache_invalidated_by_signed_field_mutation) +{ + CBLSSecretKey sk; + sk.MakeNewKey(); + const CBLSPublicKey pk{sk.GetPublicKey()}; + + CGovernanceVote vote{MakeSignedVote(sk, COutPoint{uint256S("33"), /*n=*/2}, uint256S("44"), VOTE_SIGNAL_FUNDING, + 1'700'000'200)}; + + BOOST_REQUIRE(vote.CheckSignature(pk)); + + // Mutate a signed field. The signature no longer covers this content, so the + // very next check with the *same* key must re-verify and reject. + vote.SetTime(1'700'009'999); + BOOST_CHECK_MESSAGE(!vote.GetMemoisedVerdict(pk).has_value(), + "mutating a signed field must leave no usable memo"); + BOOST_CHECK_MESSAGE(!vote.CheckSignature(pk), + "mutating a signed field must invalidate the signature memo"); + + // Restoring the original signed content makes the vote valid again. + vote.SetTime(1'700'000'200); + BOOST_CHECK(vote.CheckSignature(pk)); +} + +// The two CheckSignature overloads (CKeyID / CBLSPublicKey) share one memo slot. +// A verdict produced by the BLS overload must never be served to the ECDSA +// overload or vice versa: the key types serialise to different lengths and +// contents, so the derived cache keys must differ. If they ever collided, a +// BLS-valid vote would be reported valid for an unrelated voting key -- a +// signature-check bypass across key types. +BOOST_AUTO_TEST_CASE(signature_cache_does_not_confuse_key_types) +{ + CBLSSecretKey sk; + sk.MakeNewKey(); + const CBLSPublicKey pk{sk.GetPublicKey()}; + + CGovernanceVote vote{MakeSignedVote(sk, COutPoint{uint256S("55"), /*n=*/3}, uint256S("66"), VOTE_SIGNAL_VALID, + 1'700'000'300)}; + + // Warm the memo with a *valid* BLS verdict. + BOOST_REQUIRE(vote.CheckSignature(pk)); + + // Now ask the ECDSA overload. The BLS signature is not a valid compact ECDSA + // signature for any CKeyID, so this must re-verify and reject rather than + // inherit the cached "valid". + CKey ecdsa_key; + ecdsa_key.MakeNewKey(/*fCompressedIn=*/true); + const CKeyID keyid{ecdsa_key.GetPubKey().GetID()}; + + BOOST_CHECK_MESSAGE(!vote.GetMemoisedVerdict(keyid).has_value(), + "a BLS verdict must not be visible to the CKeyID overload"); + BOOST_CHECK_MESSAGE(!vote.CheckSignature(keyid), + "a BLS verdict must not be reused by the CKeyID overload"); + + // And the BLS verdict must still be reachable afterwards (recomputed, not corrupted). + BOOST_CHECK(vote.CheckSignature(pk)); +} + +BOOST_AUTO_TEST_SUITE_END()