fix: canonicalize serialization of block-derived EvoDB payloads

Author: PastaPastaPasta

src/evo/chainhelper.cpp

@@ -85,7 +85,7 @@ bool CChainstateHelper::RemoveConflictingISLockByTx(const CTransaction& tx)
     return true;
 }
 
-std::unordered_map<uint8_t, int> CChainstateHelper::GetSignalsStage(const CBlockIndex* const pindexPrev)
+std::map<uint8_t, int> CChainstateHelper::GetSignalsStage(const CBlockIndex* const pindexPrev)
 {
     return ehf_manager->GetSignalsStage(pindexPrev);
 }

src/evo/chainhelper.h

@@ -6,9 +6,9 @@
 #define BITCOIN_EVO_CHAINHELPER_H
 
 #include <cstdint>
+#include <map>
 #include <memory>
 #include <optional>
-#include <unordered_map>
 
 class CBlockIndex;
 class CCreditPoolManager;
@@ -77,7 +77,7 @@ class CChainstateHelper
     bool IsInstantSendWaitingForTx(const uint256& hash) const;
     bool RemoveConflictingISLockByTx(const CTransaction& tx);
 
-    std::unordered_map<uint8_t, int> GetSignalsStage(const CBlockIndex* const pindexPrev);
+    std::map<uint8_t, int> GetSignalsStage(const CBlockIndex* const pindexPrev);
 };
 
 #endif // BITCOIN_EVO_CHAINHELPER_H

src/evo/deterministicmns.h

@@ -21,11 +21,13 @@
 #include <gsl/pointers.h>
 #include <immer/map.hpp>
 
+#include <algorithm>
 #include <atomic>
 #include <limits>
 #include <numeric>
 #include <unordered_map>
 #include <utility>
+#include <vector>
 
 class CBlock;
 class CBlockIndex;
@@ -593,9 +595,15 @@ class CDeterministicMNListDiff
         s << addedMNs;
 
         WriteCompactSize(s, updatedMNs.size());
-        for (const auto& [internalId, pdmnState] : updatedMNs) {
+        std::vector<uint64_t> updatedMNsInternalIds;
+        updatedMNsInternalIds.reserve(updatedMNs.size());
+        for (const auto& entry : updatedMNs) {
+            updatedMNsInternalIds.emplace_back(entry.first);
+        }
+        std::sort(updatedMNsInternalIds.begin(), updatedMNsInternalIds.end());
+        for (const auto& internalId : updatedMNsInternalIds) {
             WriteVarInt<Stream, VarIntMode::DEFAULT, uint64_t>(s, internalId);
-            s << pdmnState;
+            s << updatedMNs.at(internalId);
         }
 
         WriteCompactSize(s, removedMns.size());

src/evo/evodb.h

@@ -113,6 +113,10 @@ class CEvoDB
 
     /**
      * Write immutable block-derived data, accepting an identical existing value.
+     * V must have canonical serialization: its bytes must be a pure function of
+     * its logical content. Audited call-site types are CDeterministicMNListDiff,
+     * CDeterministicMNList, AbstractEHFManager::Signals, the mined-commitment
+     * pair, and CCreditPool.
      * TODO(assumeutxo): WriteDerived spot-checks are not the holistic base-state
      * comparison required at snapshot completion.
      */

src/node/miner.cpp

@@ -473,7 +473,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
     }
 
     // This map with signals is used only to find duplicates
-    std::unordered_map<uint8_t, int> signals = m_chain_helper.ehf_manager->GetSignalsStage(pindexPrev);
+    auto signals = m_chain_helper.ehf_manager->GetSignalsStage(pindexPrev);
 
     // mapModifiedTx will store sorted packages after they are modified
     // because some of their txs are already in the block

src/rpc/blockchain.cpp

@@ -1457,7 +1457,7 @@ static void SoftForkDescPushBack(const CBlockIndex* active_chain_tip, UniValue&
     softforks.pushKV(DeploymentName(dep), rv);
 }
 
-static void SoftForkDescPushBack(const CBlockIndex* active_chain_tip, const std::unordered_map<uint8_t, int>& signals, UniValue& softforks, const ChainstateManager& chainman, Consensus::DeploymentPos id)
+static void SoftForkDescPushBack(const CBlockIndex* active_chain_tip, const AbstractEHFManager::Signals& signals, UniValue& softforks, const ChainstateManager& chainman, Consensus::DeploymentPos id)
 {
     // For BIP9 deployments.
 

src/test/evo_deterministicmns_tests.cpp

@@ -1506,6 +1506,25 @@ static void SmlCache(TestChainSetup& setup)
 
 BOOST_AUTO_TEST_SUITE(evo_dip3_activation_tests)
 
+BOOST_AUTO_TEST_CASE(deterministic_mn_list_diff_serialization_is_canonical)
+{
+    CDeterministicMNListDiff forward;
+    forward.updatedMNs.emplace(1, CDeterministicMNStateDiff{});
+    forward.updatedMNs.emplace(2, CDeterministicMNStateDiff{});
+
+    CDeterministicMNListDiff reverse;
+    reverse.updatedMNs.emplace(2, CDeterministicMNStateDiff{});
+    reverse.updatedMNs.emplace(1, CDeterministicMNStateDiff{});
+
+    CDataStream forward_stream{SER_DISK, CLIENT_VERSION};
+    CDataStream reverse_stream{SER_DISK, CLIENT_VERSION};
+    forward_stream << forward;
+    reverse_stream << reverse;
+
+    BOOST_CHECK_EQUAL_COLLECTIONS(forward_stream.begin(), forward_stream.end(),
+                                  reverse_stream.begin(), reverse_stream.end());
+}
+
 struct TestChainDIP3BeforeActivationSetup : public TestChainSetup {
     TestChainDIP3BeforeActivationSetup() :
         TestChainSetup(430)

src/versionbits.h

@@ -105,7 +105,7 @@ class VersionBitsCache
 class AbstractEHFManager
 {
 public:
-    using Signals = std::unordered_map<uint8_t, int>;
+    using Signals = std::map<uint8_t, int>;
 
 public:
     AbstractEHFManager() = default;