Merge dashpay#6808: fix: fix and re-enable snapshot (de)serialization tests, fix and test CQuorumRotationInfo (de)ser, fix CSimplifiedMNListDiff deser

fb23601 fix: `CSimplifiedMNListDiff` deser (UdjinM6)
aac519f fix: use `extraShare` flag to guard extra share info in `CQuorumRotationInfo`, don't use `std::optional` (UdjinM6)
5dbe407 test: add serialization tests for `CQuorumRotationInfo` (UdjinM6)
24c6cf4 fix: re-enable serialization tests in `llmq_snapshot_tests` (UdjinM6)
f818d13 fix: use temporary data stream in `TestSerializationRoundtrip` (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Fix broken (de)serialization logic - it "works" in develop because we don't actually use it anywhere. Improve tests.

  ## What was done?
  pls see individual commits

  ## How Has This Been Tested?
  run tests

  ## Breaking Changes
  n/a

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK fb23601

Tree-SHA512: ffba65d8621341113a3e4189981bd85ef977a916356d6b79b2676b33467e923ec00b2b559ab73d600b09e1f26adf1d796c53e743dcd7963f91f218423e670506

Author: PastaPastaPasta

src/evo/core_write.cpp

@@ -197,7 +197,7 @@
     ssCbTxMerkleTree << cbTxMerkleTree;
     obj.pushKV("cbTxMerkleTree", HexStr(ssCbTxMerkleTree));
 
-    obj.pushKV("cbTx", EncodeHexTx(*cbTx));
+    obj.pushKV("cbTx", EncodeHexTx(CTransaction(cbTx)));
 
     UniValue deletedMNsArr(UniValue::VARR);
     for (const auto& h : deletedMNs) {
@@ -227,7 +227,7 @@
     obj.pushKV("newQuorums", newQuorumsArr);
 
     // Do not assert special tx type here since this can be called prior to DIP0003 activation
-    if (const auto opt_cbTxPayload = GetTxPayload<CCbTx>(*cbTx, /*assert_type=*/false)) {
+    if (const auto opt_cbTxPayload = GetTxPayload<CCbTx>(cbTx, /*assert_type=*/false)) {
         obj.pushKV("merkleRootMNList", opt_cbTxPayload->merkleRootMNList.ToString());
         if (opt_cbTxPayload->nVersion >= CCbTx::Version::MERKLE_ROOT_QUORUMS) {
             obj.pushKV("merkleRootQuorums", opt_cbTxPayload->merkleRootQuorums.ToString());

src/evo/smldiff.cpp

@@ -209,7 +209,7 @@ bool BuildSimplifiedMNListDiff(CDeterministicMNManager& dmnman, const Chainstate
         return false;
     }
 
-    mnListDiffRet.cbTx = block.vtx[0];
+    mnListDiffRet.cbTx = CMutableTransaction(*block.vtx[0]);
 
     std::vector<uint256> vHashes;
     std::vector<bool> vMatch(block.vtx.size(), false);

src/evo/smldiff.h

@@ -49,7 +49,7 @@ class CSimplifiedMNListDiff
     uint256 baseBlockHash;
     uint256 blockHash;
     CPartialMerkleTree cbTxMerkleTree;
-    CTransactionRef cbTx;
+    CMutableTransaction cbTx;
     std::vector<uint256> deletedMNs;
     std::vector<CSimplifiedMNListEntry> mnList;
     uint16_t nVersion{CURRENT_VERSION};

src/llmq/snapshot.cpp

@@ -45,8 +45,8 @@ UniValue CQuorumRotationInfo::ToJson() const
     obj.pushKV("quorumSnapshotAtHMinus2C", quorumSnapshotAtHMinus2C.ToJson());
     obj.pushKV("quorumSnapshotAtHMinus3C", quorumSnapshotAtHMinus3C.ToJson());
 
-    if (extraShare && quorumSnapshotAtHMinus4C.has_value()) {
-        obj.pushKV("quorumSnapshotAtHMinus4C", quorumSnapshotAtHMinus4C->ToJson());
+    if (extraShare) {
+        obj.pushKV("quorumSnapshotAtHMinus4C", quorumSnapshotAtHMinus4C.ToJson());
     }
 
     obj.pushKV("mnListDiffTip", mnListDiffTip.ToJson());
@@ -55,8 +55,8 @@ UniValue CQuorumRotationInfo::ToJson() const
     obj.pushKV("mnListDiffAtHMinus2C", mnListDiffAtHMinus2C.ToJson());
     obj.pushKV("mnListDiffAtHMinus3C", mnListDiffAtHMinus3C.ToJson());
 
-    if (extraShare && mnListDiffAtHMinus4C.has_value()) {
-        obj.pushKV("mnListDiffAtHMinus4C", mnListDiffAtHMinus4C->ToJson());
+    if (extraShare) {
+        obj.pushKV("mnListDiffAtHMinus4C", mnListDiffAtHMinus4C.ToJson());
     }
     UniValue hqclists(UniValue::VARR);
     for (const auto& qc : lastCommitmentPerIndex) {
@@ -267,7 +267,7 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
             errorRet = strprintf("Can not find quorum snapshot at H-4C");
             return false;
         } else {
-            response.quorumSnapshotAtHMinus4C = std::move(snapshotHMinus4C);
+            response.quorumSnapshotAtHMinus4C = std::move(snapshotHMinus4C.value());
         }
 
         CSimplifiedMNListDiff mn4c;
@@ -283,8 +283,6 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
         response.mnListDiffAtHMinus4C = std::move(mn4c);
     } else {
         response.extraShare = false;
-        response.quorumSnapshotAtHMinus4C.reset();
-        response.mnListDiffAtHMinus4C.reset();
     }
 
     std::set<int> snapshotHeightsNeeded;

src/llmq/snapshot.h

@@ -116,8 +116,8 @@ class CQuorumRotationInfo
     CSimplifiedMNListDiff mnListDiffAtHMinus3C;
 
     bool extraShare{false};
-    std::optional<CQuorumSnapshot> quorumSnapshotAtHMinus4C;
-    std::optional<CSimplifiedMNListDiff> mnListDiffAtHMinus4C;
+    CQuorumSnapshot quorumSnapshotAtHMinus4C;
+    CSimplifiedMNListDiff mnListDiffAtHMinus4C;
 
     std::vector<llmq::CFinalCommitment> lastCommitmentPerIndex;
     std::vector<CQuorumSnapshot> quorumSnapshotList;
@@ -142,12 +142,9 @@ class CQuorumRotationInfo
     {
         const_cast<CQuorumRotationInfo*>(this)->SerializationOpBase(s, CSerActionSerialize());
 
-        if (extraShare && quorumSnapshotAtHMinus4C.has_value()) {
-            ::Serialize(s, quorumSnapshotAtHMinus4C.value());
-        }
-
-        if (extraShare && mnListDiffAtHMinus4C.has_value()) {
-            ::Serialize(s, mnListDiffAtHMinus4C.value());
+        if (extraShare) {
+            ::Serialize(s, quorumSnapshotAtHMinus4C);
+            ::Serialize(s, mnListDiffAtHMinus4C);
         }
 
         WriteCompactSize(s, lastCommitmentPerIndex.size());
@@ -171,12 +168,9 @@ class CQuorumRotationInfo
     {
         SerializationOpBase(s, CSerActionUnserialize());
 
-        if (extraShare && quorumSnapshotAtHMinus4C.has_value()) {
-            ::Unserialize(s, quorumSnapshotAtHMinus4C.value());
-        }
-
-        if (extraShare && mnListDiffAtHMinus4C.has_value()) {
-            ::Unserialize(s, mnListDiffAtHMinus4C.value());
+        if (extraShare) {
+            ::Unserialize(s, quorumSnapshotAtHMinus4C);
+            ::Unserialize(s, mnListDiffAtHMinus4C);
         }
 
         size_t cnt = ReadCompactSize(s);

src/test/llmq_snapshot_tests.cpp

@@ -5,10 +5,12 @@
 #include <test/util/llmq_tests.h>
 #include <test/util/setup_common.h>
 
-#include <llmq/snapshot.h>
 #include <streams.h>
 #include <univalue.h>
 
+#include <llmq/params.h>
+#include <llmq/snapshot.h>
+
 #include <boost/test/unit_test.hpp>
 
 #include <vector>
@@ -116,23 +118,16 @@ BOOST_AUTO_TEST_CASE(quorum_snapshot_empty_data_test)
     // Test with empty data
     CQuorumSnapshot emptySnapshot({}, MODE_NO_SKIPPING, {});
 
-    // TODO: Serialization roundtrip tests are disabled because CQuorumSnapshot uses custom
-    // serialization for bit vectors that may not produce byte-identical output after roundtrip.
-    // These tests should be re-enabled once proper equality operators are implemented for CQuorumSnapshot.
-    // TODO: Enable serialization roundtrip test once CQuorumSnapshot serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(emptySnapshot));
+    // Test serialization roundtrip
+    BOOST_CHECK(TestSerializationRoundtrip(emptySnapshot));
 
     // Test with empty active members but non-empty skip list
     CQuorumSnapshot snapshot1({}, MODE_SKIPPING_ENTRIES, {1, 2, 3});
-    // TODO: See above - custom bit vector serialization prevents byte-identical roundtrip
-    // TODO: Enable serialization roundtrip test once CQuorumSnapshot serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(snapshot1));
+    BOOST_CHECK(TestSerializationRoundtrip(snapshot1));
 
     // Test with non-empty active members but empty skip list
     CQuorumSnapshot snapshot2({true, false, true}, MODE_NO_SKIPPING, {});
-    // TODO: See above - custom bit vector serialization prevents byte-identical roundtrip
-    // TODO: Enable serialization roundtrip test once CQuorumSnapshot serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(snapshot2));
+    BOOST_CHECK(TestSerializationRoundtrip(snapshot2));
 }
 
 BOOST_AUTO_TEST_CASE(quorum_snapshot_bit_serialization_test)
@@ -141,32 +136,24 @@ BOOST_AUTO_TEST_CASE(quorum_snapshot_bit_serialization_test)
 
     // Test single bit
     CQuorumSnapshot snapshot1({true}, MODE_NO_SKIPPING, {});
-    // TODO: See above - custom bit vector serialization prevents byte-identical roundtrip
-    // TODO: Enable serialization roundtrip test once CQuorumSnapshot serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(snapshot1));
+    BOOST_CHECK(TestSerializationRoundtrip(snapshot1));
 
     // Test 8 bits (full byte)
     CQuorumSnapshot snapshot8(std::vector<bool>(8, true), MODE_NO_SKIPPING, {});
-    // TODO: See above - custom bit vector serialization prevents byte-identical roundtrip
-    // TODO: Enable serialization roundtrip test once CQuorumSnapshot serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(snapshot8));
+    BOOST_CHECK(TestSerializationRoundtrip(snapshot8));
 
     // Test 9 bits (more than one byte)
     CQuorumSnapshot snapshot9(std::vector<bool>(9, false), MODE_NO_SKIPPING, {});
     snapshot9.activeQuorumMembers[8] = true; // Set last bit
-    // TODO: See above - custom bit vector serialization prevents byte-identical roundtrip
-    // TODO: Enable serialization roundtrip test once CQuorumSnapshot serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(snapshot9));
+    BOOST_CHECK(TestSerializationRoundtrip(snapshot9));
 
     // Test alternating pattern
     std::vector<bool> alternating(16);
     for (size_t i = 0; i < alternating.size(); i++) {
         alternating[i] = (i % 2 == 0);
     }
     CQuorumSnapshot snapshotAlt(alternating, MODE_NO_SKIPPING, {});
-    // TODO: See above - custom bit vector serialization prevents byte-identical roundtrip
-    // TODO: Enable serialization roundtrip test once CQuorumSnapshot serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(snapshotAlt));
+    BOOST_CHECK(TestSerializationRoundtrip(snapshotAlt));
 }
 
 BOOST_AUTO_TEST_CASE(quorum_rotation_info_construction_test)
@@ -175,8 +162,8 @@ BOOST_AUTO_TEST_CASE(quorum_rotation_info_construction_test)
 
     // Test default state
     BOOST_CHECK(!rotInfo.extraShare);
-    BOOST_CHECK(!rotInfo.quorumSnapshotAtHMinus4C.has_value());
-    BOOST_CHECK(!rotInfo.mnListDiffAtHMinus4C.has_value());
+    BOOST_CHECK_EQUAL(::SerializeHash(rotInfo.quorumSnapshotAtHMinus4C), ::SerializeHash(CQuorumSnapshot()));
+    BOOST_CHECK_EQUAL(::SerializeHash(rotInfo.mnListDiffAtHMinus4C), ::SerializeHash(CSimplifiedMNListDiff()));
     BOOST_CHECK(rotInfo.lastCommitmentPerIndex.empty());
     BOOST_CHECK(rotInfo.quorumSnapshotList.empty());
     BOOST_CHECK(rotInfo.mnListDiffList.empty());
@@ -185,7 +172,7 @@ BOOST_AUTO_TEST_CASE(quorum_rotation_info_construction_test)
 // Note: CQuorumRotationInfo serialization requires complex setup
 // This is better tested in functional tests
 
-BOOST_AUTO_TEST_CASE(get_quorum_rotation_info_test)
+BOOST_AUTO_TEST_CASE(get_quorum_rotation_info_serialization_test)
 {
     CGetQuorumRotationInfo getInfo;
 
@@ -194,19 +181,46 @@ BOOST_AUTO_TEST_CASE(get_quorum_rotation_info_test)
     getInfo.blockRequestHash = GetTestBlockHash(100);
     getInfo.extraShare = true;
 
-    // TODO: CGetQuorumRotationInfo serialization test disabled - uses standard SERIALIZE_METHODS
-    // but may have issues with empty vectors. Should investigate and re-enable.
-    // TODO: Enable serialization roundtrip test once CGetQuorumsBaseBlockInfo serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(getInfo));
+    // Test serialization
+    BOOST_CHECK(TestSerializationRoundtrip(getInfo));
 
     // Test with empty base block hashes
     CGetQuorumRotationInfo emptyInfo;
     emptyInfo.blockRequestHash = GetTestBlockHash(200);
     emptyInfo.extraShare = false;
 
-    // TODO: See above - investigate serialization issues with empty base block hashes
-    // TODO: Enable serialization roundtrip test once CGetQuorumsBaseBlockInfo serialization is fixed
-    // BOOST_CHECK(TestSerializationRoundtrip(emptyInfo));
+    BOOST_CHECK(TestSerializationRoundtrip(emptyInfo));
+}
+
+BOOST_AUTO_TEST_CASE(quorum_rotation_info_serialization_test)
+{
+    // Note: mnListDiff{smth} testing requires proper CSimplifiedMNListDiff setup
+    // which is complex and better tested in functional tests
+
+    // Test CQuorumRotationInfo serialization with various optional field combinations
+    CQuorumRotationInfo rotInfo;
+
+    // Set up basic required fields
+    rotInfo.quorumSnapshotAtHMinusC = CQuorumSnapshot({true, false, true}, MODE_SKIPPING_ENTRIES, {1, 2});
+    rotInfo.quorumSnapshotAtHMinus2C = CQuorumSnapshot({false, true, false}, MODE_NO_SKIPPING, {});
+    rotInfo.quorumSnapshotAtHMinus3C = CQuorumSnapshot({true, true, false}, MODE_ALL_SKIPPED, {3});
+
+    // Test without extraShare
+    rotInfo.extraShare = false;
+    BOOST_CHECK(TestSerializationRoundtrip(rotInfo));
+
+    // Test with extraShare but uninitialized optional fields
+    rotInfo.extraShare = true;
+    BOOST_CHECK(TestSerializationRoundtrip(rotInfo));
+
+    // Test with extraShare and initialized snapshot
+    rotInfo.quorumSnapshotAtHMinus4C = CQuorumSnapshot({false, false, true}, MODE_SKIPPING_ENTRIES, {4, 5, 6});
+    BOOST_CHECK(TestSerializationRoundtrip(rotInfo));
+
+    CFinalCommitment commitment{GetLLMQParams(Consensus::LLMQType::LLMQ_TEST), uint256::ONE};
+    rotInfo.lastCommitmentPerIndex.push_back(commitment);
+    rotInfo.quorumSnapshotList.push_back(CQuorumSnapshot({false, false, true}, MODE_SKIPPING_ENTRIES, {7, 8}));
+    BOOST_CHECK(TestSerializationRoundtrip(rotInfo));
 }
 
 BOOST_AUTO_TEST_CASE(quorum_snapshot_json_test)

src/test/util/llmq_tests.h

@@ -88,17 +88,19 @@ inline std::vector<bool> CreateBitVector(size_t size, const std::vector<size_t>&
 template <typename T>
 inline bool TestSerializationRoundtrip(const T& obj)
 {
-    CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
-    ss << obj;
-
-    T deserialized;
-    ss >> deserialized;
-
-    // Re-serialize and compare
-    CDataStream ss2(SER_NETWORK, PROTOCOL_VERSION);
-    ss2 << deserialized;
-
-    return ss.str() == ss2.str();
+    // Datastreams we will compare at the end
+    CDataStream ss_obj(SER_NETWORK, PROTOCOL_VERSION);
+    CDataStream ss_obj_rt(SER_NETWORK, PROTOCOL_VERSION);
+    // A temporary datastream to perform serialization round-trip
+    CDataStream ss_tmp(SER_NETWORK, PROTOCOL_VERSION);
+    T obj_rt;
+
+    ss_obj << obj;
+    ss_tmp << obj;
+    ss_tmp >> obj_rt;
+    ss_obj_rt << obj_rt;
+
+    return ss_obj.str() == ss_obj_rt.str();
 }
 
 // Helper to create deterministic test data