diff --git a/src/cachemultimap.h b/src/cachemultimap.h index 88f3f12c2bea..2901a2df0446 100644 --- a/src/cachemultimap.h +++ b/src/cachemultimap.h @@ -86,22 +86,17 @@ class CacheMultiMap bool Insert(const K& key, const V& value) { - map_it mit = mapIndex.find(key); - if(mit == mapIndex.end()) { - mit = mapIndex.emplace(key, it_map_t()).first; - } - it_map_t& mapIt = mit->second; - - if(mapIt.count(value) > 0) { + if (map_cit mit = mapIndex.find(key); mit != mapIndex.end() && mit->second.count(value) > 0) { // Don't insert duplicates return false; } + // Pruning may erase this key's entry in mapIndex, so look it up only afterwards if(listItems.size() == nMaxSize) { PruneLast(); } listItems.push_front(item_t(key, value)); - mapIt.emplace(value, listItems.begin()); + mapIndex[key].emplace(value, listItems.begin()); return true; } @@ -137,6 +132,18 @@ class CacheMultiMap return true; } + bool GetAll(const K& key, const V& lower, const V& upper, std::vector& vecValues) const + { + const map_cit mit = mapIndex.find(key); + if (mit == mapIndex.end() || upper < lower) return false; + const it_map_t& mapIt = mit->second; + const auto range_end = mapIt.upper_bound(upper); + for (auto it = mapIt.lower_bound(lower); it != range_end; ++it) { + vecValues.push_back(it->second->value); + } + return true; + } + void GetKeys(std::vector& vecKeys) { for(map_cit it = mapIndex.begin(); it != mapIndex.end(); ++it) { diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 22dafc4214a1..ceb99f33a42d 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -26,6 +26,7 @@ #include +#include #include const std::string GovernanceStore::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-17"; @@ -37,6 +38,39 @@ constexpr std::chrono::seconds MAX_TIME_FUTURE_DEVIATION{1h}; // Margin below MAX_TIME_FUTURE_DEVIATION so lagging-clock peers don't reject near-limit objects. constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{60}; +// The encoding chooses which key to verify: IsValidForUnknownParent accepts only compact +// signatures by the voting key and BLS signatures by the operator key. +bool UsesVotingKey(const CGovernanceVote& vote) +{ + return vote.GetSignal() == VOTE_SIGNAL_FUNDING && vote.GetSignatureSize() == CGovernanceVote::COMPACT_SIG_SIZE; +} + +bool InsertOrphanVote(CacheMultiMap& cache, const CDeterministicMNList& tip_mn_list, + const governance::OrphanVote& orphan_vote) +{ + auto lower{orphan_vote}; + auto upper{orphan_vote}; + lower.vote.SetTime(std::numeric_limits::min()); + lower.vote.SetSignature({}); + upper.vote.SetTime(std::numeric_limits::max()); + upper.vote.SetSignature(std::vector(CGovernanceVote::BLS_SIG_SIZE)); + std::vector variants; + const auto& parent = orphan_vote.vote.GetParentHash(); + cache.GetAll(parent, lower, upper, variants); + const bool use_voting_key{UsesVotingKey(orphan_vote.vote)}; + for (const auto& cached : variants) { + const bool cached_uses_voting_key{UsesVotingKey(cached.vote)}; + // Only current-key authentication may suppress a new vote. + if (cached.expiration < Now() || !cached.vote.IsValid(tip_mn_list, cached_uses_voting_key)) { + cache.Erase(parent, cached); + } else if (cached_uses_voting_key == use_voting_key) { + if (cached.vote.GetTimestamp() >= orphan_vote.vote.GetTimestamp()) return false; + cache.Erase(parent, cached); + } + } + return cache.Insert(parent, orphan_vote); +} + bool IsSyncableObject(const std::shared_ptr& govobj) { const auto& obj = *Assert(govobj); @@ -820,9 +854,8 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc AssertLockNotHeld(cs_store); hashToRequest = uint256{}; - const auto tip_mn_list{m_dmnman.GetListAtChainTip()}; - LOCK(cs_store); + const auto tip_mn_list{m_dmnman.GetListAtChainTip()}; uint256 nHashVote = vote.GetHash(); uint256 nHashGovobj = vote.GetParentHash(); @@ -846,7 +879,8 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc // No penalty: the vote is signed by a masternode, it just arrived before its parent object, // which routinely happens during governance sync. Misbehaviour scores never decay. exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_WARNING); - if (cmmapOrphanVotes.Insert(nHashGovobj, governance::OrphanVote{vote, Now() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) { + if (InsertOrphanVote(cmmapOrphanVotes, tip_mn_list, + governance::OrphanVote{vote, Now() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) { hashToRequest = nHashGovobj; // Caller should request this object } LogPrint(BCLog::GOBJECT, "%s\n", msg); diff --git a/src/governance/governance.h b/src/governance/governance.h index 04536cb5ef04..a0e212763bd2 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -56,6 +56,8 @@ struct OrphanVote { inline bool operator<(const OrphanVote& lhs, const OrphanVote& rhs) { + // Equal signed fields may arrive authenticated by different voting/operator keys. + if (lhs.vote == rhs.vote) return lhs.vote.GetSignatureSize() < rhs.vote.GetSignatureSize(); return lhs.vote < rhs.vote; } } // namespace governance diff --git a/src/governance/vote.cpp b/src/governance/vote.cpp index c0081d7bb053..f8baaf1ac04b 100644 --- a/src/governance/vote.cpp +++ b/src/governance/vote.cpp @@ -16,6 +16,8 @@ #include #include +#include + static_assert(CGovernanceVote::COMPACT_SIG_SIZE == CPubKey::COMPACT_SIGNATURE_SIZE); static_assert(CGovernanceVote::BLS_SIG_SIZE == CBLSSignature::SerSize); @@ -262,31 +264,6 @@ bool operator==(const CGovernanceVote& vote1, const CGovernanceVote& vote2) bool operator<(const CGovernanceVote& vote1, const CGovernanceVote& vote2) { - bool fResult = (vote1.masternodeOutpoint < vote2.masternodeOutpoint); - if (!fResult) { - return false; - } - fResult = (vote1.masternodeOutpoint == vote2.masternodeOutpoint); - - fResult = fResult && (vote1.nParentHash < vote2.nParentHash); - if (!fResult) { - return false; - } - fResult = (vote1.nParentHash == vote2.nParentHash); - - fResult = fResult && (vote1.nVoteOutcome < vote2.nVoteOutcome); - if (!fResult) { - return false; - } - fResult = (vote1.nVoteOutcome == vote2.nVoteOutcome); - - fResult = fResult && (vote1.nVoteSignal == vote2.nVoteSignal); - if (!fResult) { - return false; - } - fResult = (vote1.nVoteSignal == vote2.nVoteSignal); - - fResult = fResult && (vote1.nTime < vote2.nTime); - - return fResult; + return std::tie(vote1.masternodeOutpoint, vote1.nParentHash, vote1.nVoteOutcome, vote1.nVoteSignal, vote1.nTime) < + std::tie(vote2.masternodeOutpoint, vote2.nParentHash, vote2.nVoteOutcome, vote2.nVoteSignal, vote2.nTime); } diff --git a/src/governance/vote.h b/src/governance/vote.h index 36df8a199fed..4beffef728cc 100644 --- a/src/governance/vote.h +++ b/src/governance/vote.h @@ -145,6 +145,7 @@ class CGovernanceVote } void SetSignature(const std::vector& vchSigIn) { vchSig = vchSigIn; } + size_t GetSignatureSize() const { return vchSig.size(); } bool CheckSignature(const CKeyID& keyID) const; bool CheckSignature(const CBLSPublicKey& pubKey) const; diff --git a/src/test/cachemultimap_tests.cpp b/src/test/cachemultimap_tests.cpp index 46712b831b98..6d375e499e3c 100644 --- a/src/test/cachemultimap_tests.cpp +++ b/src/test/cachemultimap_tests.cpp @@ -166,4 +166,43 @@ BOOST_AUTO_TEST_CASE(cachemultimap_test) BOOST_CHECK(Compare(cmmapTest1, mapTest4)); } +BOOST_AUTO_TEST_CASE(cachemultimap_insert_evicts_sole_value_of_same_key) +{ + CacheMultiMap cmmap(2); + BOOST_CHECK(cmmap.Insert(1, 10)); + BOOST_CHECK(cmmap.Insert(2, 20)); + + // The cache is full and its oldest item is key 1's only value + BOOST_CHECK(cmmap.Insert(1, 11)); + + BOOST_CHECK_EQUAL(cmmap.GetSize(), 2U); + std::vector vecVals; + BOOST_CHECK(cmmap.GetAll(1, vecVals)); + BOOST_CHECK(vecVals == std::vector{11}); + BOOST_CHECK(cmmap.HasKey(2)); + + // The index still tracks the new item, so erasing it empties the cache + cmmap.Erase(1); + cmmap.Erase(2); + BOOST_CHECK_EQUAL(cmmap.GetSize(), 0U); + BOOST_CHECK(!cmmap.HasKey(1)); +} + +BOOST_AUTO_TEST_CASE(cachemultimap_get_all_in_value_range) +{ + CacheMultiMap cmmap(5); + for (const int value : {1, 2, 3, 4}) + BOOST_REQUIRE(cmmap.Insert(1, value)); + BOOST_REQUIRE(cmmap.Insert(2, 2)); + std::vector values; + BOOST_CHECK(cmmap.GetAll(1, 2, 3, values)); + BOOST_CHECK(values == (std::vector{2, 3})); + values.clear(); + BOOST_CHECK(cmmap.GetAll(1, 5, 6, values)); + BOOST_CHECK(values.empty()); + BOOST_CHECK(!cmmap.GetAll(3, 1, 4, values)); + BOOST_CHECK(!cmmap.GetAll(1, 3, 2, values)); + BOOST_CHECK(values.empty()); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/governance_vote_processing_tests.cpp b/src/test/governance_vote_processing_tests.cpp index 57c05c103b8f..f01e5c425c41 100644 --- a/src/test/governance_vote_processing_tests.cpp +++ b/src/test/governance_vote_processing_tests.cpp @@ -5,6 +5,8 @@ #include #include #include +#include +#include #include #include #include @@ -74,6 +76,24 @@ CGovernanceObject MakeFailedTrigger(const COutPoint& mn_collateral, const CBLSSe return govobj; } +struct GovernanceVoteNotifications final : CValidationInterface { + mutable Mutex cs; + std::vector hashes GUARDED_BY(cs); + + void NotifyGovernanceVote(const std::shared_ptr&, + const std::shared_ptr& vote) override EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + LOCK(cs); + hashes.push_back(vote->GetHash()); + } + + std::vector GetHashes() const EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + LOCK(cs); + return hashes; + } +}; + // A chain with one registered masternode whose voting (ECDSA) and operator (BLS) keys are known // to the test, so votes can be signed for real and CGovernanceVote::CheckSignature is actually // reached. Without a populated masternode list every CGovernanceVote::IsValid() call would @@ -83,6 +103,7 @@ struct GovernanceVoteSetup : public TestChainSetup { CBLSSecretKey mn_operator_key; COutPoint mn_collateral; SimpleUTXOMap utxos; + std::shared_ptr vote_notifications; //! Fixed so a proposal can be rebuilt bit-for-bit (and keep its hash) once its fee tx is known. int64_t proposal_time{0}; std::string proposal_payment_address; @@ -135,6 +156,8 @@ struct GovernanceVoteSetup : public TestChainSetup { *m_node.sbman, *m_node.dmnman, *m_node.mn_sync); BOOST_REQUIRE(m_node.govman->LoadCache(/*load_cache=*/false)); + vote_notifications = std::make_shared(); + RegisterSharedValidationInterface(vote_notifications); proposal_time = TicksSinceEpoch(GetAdjustedTime()); CKey payment_key; @@ -144,6 +167,11 @@ struct GovernanceVoteSetup : public TestChainSetup { void TearDown() { + if (vote_notifications) { + SyncWithValidationInterfaceQueue(); + UnregisterSharedValidationInterface(vote_notifications); + vote_notifications.reset(); + } // govman holds a reference to node.sbman, so it must go first (matches PrepareShutdown // ordering in init.cpp). m_node.govman.reset(); @@ -282,6 +310,167 @@ BOOST_AUTO_TEST_CASE(orphan_vote_is_cached_and_applied_when_parent_arrives) BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_FUNDING), 1); } +BOOST_AUTO_TEST_CASE(orphan_funding_votes_from_multiple_masternodes_are_applied) +{ + CKey second_voting_key; + CBLSSecretKey second_operator_key; + const auto protx{CreateProRegTx(*m_node.chainman, utxos, /*port=*/2, payout_script(), coinbaseKey, + second_voting_key, second_operator_key)}; + MineBlock({protx}); + const COutPoint second_collateral{protx.GetHash(), 0}; + BOOST_REQUIRE(tip_mn_list().GetMNByCollateral(second_collateral) != nullptr); + + auto& govman = *m_node.govman; + const uint256 parent_hash{MakeProposal(uint256{}).GetHash()}; + CGovernanceVote first_vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES)}; + SignWithVotingKey(first_vote, mn_voting_key); + CGovernanceVote second_vote{second_collateral, parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES}; + SignWithVotingKey(second_vote, second_voting_key); + + for (const auto& vote : {first_vote, second_vote, second_vote}) { + BOOST_REQUIRE(vote.IsValid(tip_mn_list(), /*useVotingKey=*/true)); + CGovernanceException exception; + uint256 hash_to_request; + BOOST_CHECK(!govman.ProcessVote(vote, exception, hash_to_request)); + BOOST_CHECK_EQUAL(exception.GetType(), GOVERNANCE_EXCEPTION_WARNING); + BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 0); + } + + const uint256 collateral_hash{ConfirmProposalCollateral(parent_hash)}; + CGovernanceObject proposal{MakeProposal(collateral_hash)}; + BOOST_REQUIRE_EQUAL(proposal.GetHash(), parent_hash); + govman.AddGovernanceObject(proposal, /*peer_str=*/""); + const auto stored{govman.FindConstGovernanceObject(parent_hash)}; + BOOST_REQUIRE(stored != nullptr); + BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_FUNDING), 2); + const auto current_votes{govman.GetCurrentVotes(parent_hash, COutPoint{})}; + BOOST_REQUIRE_EQUAL(current_votes.size(), 2U); + BOOST_CHECK(govman.GetOrphanVoteObjectHashes().empty()); +} + +BOOST_AUTO_TEST_CASE(orphan_timestamp_variants_replay_only_the_newest_vote) +{ + auto& govman = *m_node.govman; + const uint256 parent_hash{MakeProposal(uint256{}).GetHash()}; + uint256 newest_hash; + for (const int offset : {5, 1, 4, 3, 2, 5}) { + CGovernanceVote vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES, std::chrono::seconds{offset})}; + SignWithVotingKey(vote, mn_voting_key); + if (offset == 5) newest_hash = vote.GetHash(); + CGovernanceException exception; + uint256 hash_to_request; + BOOST_CHECK(!govman.ProcessVote(vote, exception, hash_to_request)); + BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 0); + } + + const uint256 collateral_hash{ConfirmProposalCollateral(parent_hash)}; + CGovernanceObject proposal{MakeProposal(collateral_hash)}; + govman.AddGovernanceObject(proposal, /*peer_str=*/""); + SyncWithValidationInterfaceQueue(); + BOOST_CHECK(vote_notifications->GetHashes() == std::vector{newest_hash}); + const auto stored{govman.FindConstGovernanceObject(parent_hash)}; + BOOST_REQUIRE(stored != nullptr); + BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_FUNDING), 1); + const auto votes{govman.GetCurrentVotes(parent_hash, mn_collateral)}; + BOOST_REQUIRE_EQUAL(votes.size(), 1U); + BOOST_CHECK_EQUAL(votes.front().GetHash(), newest_hash); +} + +BOOST_AUTO_TEST_CASE(newer_operator_orphan_does_not_replace_voting_key_orphan) +{ + auto& govman = *m_node.govman; + const uint256 parent_hash{MakeProposal(uint256{}).GetHash()}; + CGovernanceVote operator_vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES, 30s)}; + SignWithOperatorKey(operator_vote, mn_operator_key); + CGovernanceVote voting_vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES, 1s)}; + SignWithVotingKey(voting_vote, mn_voting_key); + for (const auto& vote : {operator_vote, voting_vote, voting_vote}) { + CGovernanceException exception; + uint256 hash_to_request; + BOOST_CHECK(!govman.ProcessVote(vote, exception, hash_to_request)); + BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 0); + } + const uint256 collateral_hash{ConfirmProposalCollateral(parent_hash)}; + CGovernanceObject proposal{MakeProposal(collateral_hash)}; + govman.AddGovernanceObject(proposal, /*peer_str=*/""); + SyncWithValidationInterfaceQueue(); + BOOST_CHECK(vote_notifications->GetHashes() == std::vector{voting_vote.GetHash()}); + const auto votes{govman.GetCurrentVotes(parent_hash, mn_collateral)}; + BOOST_REQUIRE_EQUAL(votes.size(), 1U); + BOOST_CHECK_EQUAL(votes.front().GetHash(), voting_vote.GetHash()); +} + +BOOST_AUTO_TEST_CASE(equal_time_operator_orphan_does_not_hide_voting_key_orphan) +{ + auto& govman = *m_node.govman; + const uint256 parent_hash{MakeProposal(uint256{}).GetHash()}; + CGovernanceVote operator_vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES)}; + SignWithOperatorKey(operator_vote, mn_operator_key); + CGovernanceVote voting_vote{operator_vote}; + SignWithVotingKey(voting_vote, mn_voting_key); + for (const auto& vote : {operator_vote, voting_vote}) { + CGovernanceException exception; + uint256 hash_to_request; + BOOST_CHECK(!govman.ProcessVote(vote, exception, hash_to_request)); + BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 0); + } + const uint256 collateral_hash{ConfirmProposalCollateral(parent_hash)}; + CGovernanceObject proposal{MakeProposal(collateral_hash)}; + govman.AddGovernanceObject(proposal, /*peer_str=*/""); + SyncWithValidationInterfaceQueue(); + BOOST_CHECK(vote_notifications->GetHashes() == std::vector{voting_vote.GetHash()}); + const auto votes{govman.GetCurrentVotes(parent_hash, mn_collateral)}; + BOOST_REQUIRE_EQUAL(votes.size(), 1U); + BOOST_CHECK_EQUAL(votes.front().GetHash(), voting_vote.GetHash()); +} + +BOOST_AUTO_TEST_CASE(old_key_orphan_does_not_suppress_a_current_key_vote) +{ + // Before DIP3 enforcement, registrar updates must keep the owner and voting keys equal. + MineBlocks(500 - tip_mn_list().GetHeight()); + auto& govman = *m_node.govman; + const uint256 parent_hash{MakeProposal(uint256{}).GetHash()}; + CGovernanceVote old_vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES, 30s)}; + SignWithVotingKey(old_vote, mn_voting_key); + CGovernanceException exception; + uint256 hash_to_request; + BOOST_CHECK(!govman.ProcessVote(old_vote, exception, hash_to_request)); + BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 0); + + CKey new_voting_key; + new_voting_key.MakeNewKey(true); + CProUpRegTx update; + update.nVersion = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false); + update.proTxHash = mn_collateral.hash; + update.pubKeyOperator.Set(mn_operator_key.GetPublicKey(), update.nVersion == ProTxVersion::LegacyBLS); + update.keyIDVoting = new_voting_key.GetPubKey().GetID(); + update.scriptPayout = payout_script(); + CMutableTransaction tx; + tx.nVersion = 3; + tx.nType = TRANSACTION_PROVIDER_UPDATE_REGISTRAR; + const auto spent{FundTransaction(*m_node.chainman, tx, utxos, payout_script(), 1 * COIN)}; + update.inputsHash = CalcTxInputsHash(CTransaction{tx}); + BOOST_REQUIRE(CHashSigner::SignHash(SerializeHash(update), mn_voting_key, update.vchSig)); + SetTxPayload(tx, update); + SignTransaction(tx, spent, coinbaseKey); + MineBlock({tx}); + BOOST_REQUIRE(!old_vote.IsValid(tip_mn_list(), /*useVotingKey=*/true)); + + CGovernanceVote current_vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES, 1s)}; + SignWithVotingKey(current_vote, new_voting_key); + BOOST_REQUIRE(current_vote.IsValid(tip_mn_list(), /*useVotingKey=*/true)); + BOOST_CHECK(!govman.ProcessVote(current_vote, exception, hash_to_request)); + BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 0); + const uint256 collateral_hash{ConfirmProposalCollateral(parent_hash)}; + CGovernanceObject proposal{MakeProposal(collateral_hash)}; + govman.AddGovernanceObject(proposal, /*peer_str=*/""); + SyncWithValidationInterfaceQueue(); + BOOST_CHECK(vote_notifications->GetHashes() == std::vector{current_vote.GetHash()}); + const auto votes{govman.GetCurrentVotes(parent_hash, mn_collateral)}; + BOOST_REQUIRE_EQUAL(votes.size(), 1U); + BOOST_CHECK_EQUAL(votes.front().GetHash(), current_vote.GetHash()); +} + // Votes are only counted if they carry a signature from a registered masternode. BOOST_AUTO_TEST_CASE(unsigned_and_unknown_masternode_votes_are_rejected) {