From 93d83c920207c710dd6f82b9c9bf2e1712438d01 Mon Sep 17 00:00:00 2001 From: Amaury Chamayou Date: Mon, 7 Sep 2026 14:59:53 +0100 Subject: [PATCH 1/2] Remove unused KV read serialization Preserve legacy read headers and decoding while removing the unused read-export API. Cover writes-only bytes, sizes, and legacy replay for public and private maps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/kv/committable_tx.h | 15 ++-- src/kv/generic_serialise_wrapper.h | 6 -- src/kv/kv_types.h | 6 +- src/kv/test/kv_serialisation.cpp | 124 +++++++++++++++++++++++++++++ src/kv/untyped_map.h | 22 +---- 5 files changed, 135 insertions(+), 38 deletions(-) diff --git a/src/kv/committable_tx.h b/src/kv/committable_tx.h index b542981edd0..bc46238c71a 100644 --- a/src/kv/committable_tx.h +++ b/src/kv/committable_tx.h @@ -65,8 +65,7 @@ namespace ccf::kv TxFlags flags = 0; SerialisedEntryFlags entry_flags = 0; - void serialise_all_changes( - KvStoreSerialiser& serialiser, bool include_reads) + void serialise_all_changes(KvStoreSerialiser& serialiser) { // Process in security domain order for (auto domain : {SecurityDomain::PUBLIC, SecurityDomain::PRIVATE}) @@ -77,7 +76,7 @@ namespace ccf::kv const auto& changeset = it.second.changeset; if (map->get_security_domain() == domain && changeset->has_writes()) { - map->serialise_changes(changeset.get(), serialiser, include_reads); + map->serialise_changes(changeset.get(), serialiser); } } } @@ -91,8 +90,7 @@ namespace ccf::kv }); } - size_t projected_serialised_size( - const ccf::ClaimsDigest& claims_digest_, bool include_reads = false) + size_t projected_serialised_size(const ccf::ClaimsDigest& claims_digest_) { if (claims_digest_.empty()) { @@ -115,7 +113,7 @@ namespace ccf::kv ccf::crypto::Sha256Hash{}, claims_digest_); - serialise_all_changes(size_serialiser, include_reads); + serialise_all_changes(size_serialiser); return size_serialiser.get_serialised_size(); } @@ -124,8 +122,7 @@ namespace ccf::kv ccf::crypto::Sha256Hash& commit_evidence_digest, std::string& commit_evidence, const ccf::ClaimsDigest& claims_digest_, - size_t max_transaction_size, - bool include_reads = false) + size_t max_transaction_size) { if (!committed) { @@ -173,7 +170,7 @@ namespace ccf::kv false /* historical_hint */, max_transaction_size); - serialise_all_changes(serialiser, include_reads); + serialise_all_changes(serialiser); return serialiser.get_raw_data(); } diff --git a/src/kv/generic_serialise_wrapper.h b/src/kv/generic_serialise_wrapper.h index d0e28218561..d9972dd33c2 100644 --- a/src/kv/generic_serialise_wrapper.h +++ b/src/kv/generic_serialise_wrapper.h @@ -130,12 +130,6 @@ namespace ccf::kv serialise_internal(ctr); } - void serialise_read(const SerialisedKey& k, const Version& version) override - { - serialise_internal(k); - serialise_internal(version); - } - void serialise_write( const SerialisedKey& k, const SerialisedValue& v) override { diff --git a/src/kv/kv_types.h b/src/kv/kv_types.h index bf13afdbc6f..a7a85b15426 100644 --- a/src/kv/kv_types.h +++ b/src/kv/kv_types.h @@ -319,8 +319,6 @@ namespace ccf::kv const std::vector& view_history) = 0; virtual void serialise_entry_version(const Version& version) = 0; virtual void serialise_count_header(uint64_t ctr) = 0; - virtual void serialise_read( - const SerialisedKey& k, const Version& version) = 0; virtual void serialise_write( const SerialisedKey& k, const SerialisedValue& v) = 0; virtual void serialise_remove(const SerialisedKey& k) = 0; @@ -641,9 +639,7 @@ namespace ccf::kv virtual AbstractStore* get_store() = 0; virtual void serialise_changes( - const AbstractChangeSet* changes, - KvStoreSerialiser& s, - bool include_reads) = 0; + const AbstractChangeSet* changes, KvStoreSerialiser& s) = 0; virtual void compact(Version v) = 0; virtual std::unique_ptr snapshot(Version v) = 0; virtual void post_compact() = 0; diff --git a/src/kv/test/kv_serialisation.cpp b/src/kv/test/kv_serialisation.cpp index 553d82b3258..0f64bbf6a9f 100644 --- a/src/kv/test/kv_serialisation.cpp +++ b/src/kv/test/kv_serialisation.cpp @@ -32,6 +32,130 @@ static std::vector make_size_prefixed_bytes( return bytes; } +TEST_CASE( + "Whole-map dependency ledger encoding compatibility" * + doctest::test_suite("serialisation")) +{ + using namespace ccf::kv; + auto encryptor = std::make_shared(); + for (const auto domain : {SecurityDomain::PUBLIC, SecurityDomain::PRIVATE}) + { + Store store; + const std::string name = domain == SecurityDomain::PUBLIC ? + "public:compatibility" : + "compatibility"; + INFO("Map: ", name); + untyped::Map map(&store, name, domain); + auto changes = map.create_change_set(0, false); + REQUIRE(changes != nullptr); + const SerialisedKey key = {'k'}; + const SerialisedKey removed_key = {'r'}; + const SerialisedKey missing_key = {'m'}; + const SerialisedValue value = {'v'}; + changes->writes[key] = value; + changes->writes[removed_key] = std::nullopt; + + const auto legacy_entry = [&]( + Version write_version, + Version read_version, + const untyped::Read& reads) { + RawWriter public_writer; + RawWriter private_writer; + public_writer.append(EntryType::WriteSet); + public_writer.append(write_version); + public_writer.append(NoVersion); + auto& writer = + domain == SecurityDomain::PUBLIC ? public_writer : private_writer; + writer.append(map.get_name()); + writer.append(read_version); + writer.append(static_cast(reads.size())); + for (const auto& [read_key, versions] : reads) + { + writer.append(read_key); + writer.append(std::get<0>(versions)); + } + writer.append(uint64_t{1}); + writer.append(key); + writer.append(value); + writer.append(uint64_t{1}); + writer.append(removed_key); + RawKvStoreSerialiser serialiser( + encryptor, {0, write_version}, EntryType::WriteSet, 0); + return serialiser.serialise_domains( + public_writer.get_raw_data(), private_writer.get_raw_data()); + }; + const auto serialise = [&]() { + RawKvStoreSerialiser serialiser( + encryptor, {0, 1}, EntryType::WriteSet, 0); + map.serialise_changes(changes.get(), serialiser); + SizeKvStoreSerialiser size_serialiser( + encryptor, {0, 1}, EntryType::WriteSet, 0); + map.serialise_changes(changes.get(), size_serialiser); + auto bytes = serialiser.get_raw_data(); + REQUIRE(size_serialiser.get_serialised_size() == bytes.size()); + return bytes; + }; + + REQUIRE(changes->read_version == NoVersion); + const auto legacy = legacy_entry(1, NoVersion, {}); + REQUIRE(serialise() == legacy); + + untyped::MapHandle handle(*changes, map.get_name()); + REQUIRE_FALSE(handle.get(missing_key).has_value()); + const auto key_reads = changes->reads; + REQUIRE(key_reads.contains(missing_key)); + REQUIRE(serialise() == legacy); + handle.foreach([](const auto&, const auto&) { return true; }); + REQUIRE(changes->read_version == 0); + REQUIRE(serialise() == legacy); + REQUIRE(changes->read_version == 0); + + changes->read_version = 7; + REQUIRE(serialise() == legacy); + REQUIRE(changes->read_version == 7); + REQUIRE(changes->reads == key_reads); + + const untyped::Read legacy_reads = { + {key, {7, NoVersion}}, {missing_key, {NoVersion, NoVersion}}}; + for (const auto read_version : {NoVersion, Version{7}}) + { + for (const auto& reads : {untyped::Read{}, legacy_reads}) + { + const auto bytes = legacy_entry(1, read_version, reads); + RawKvStoreDeserialiser deserialiser(encryptor); + Term term = 0; + EntryFlags flags = {}; + REQUIRE( + deserialiser.init(bytes.data(), bytes.size(), term, flags, false) == + 1); + REQUIRE(deserialiser.start_map() == map.get_name()); + auto decoded = map.deserialise_internal(deserialiser, 0); + REQUIRE(decoded->writes == changes->writes); + REQUIRE(decoded->reads == reads); + REQUIRE(deserialiser.end()); + REQUIRE(decoded->read_version == read_version); + } + } + + Store restored; + restored.set_encryptor(encryptor); + REQUIRE(restored.deserialize(legacy)->apply() == ApplyResult::PASS); + const untyped::Read replay_reads = { + {key, {1, NoVersion}}, {missing_key, {NoVersion, NoVersion}}}; + REQUIRE( + restored.deserialize(legacy_entry(2, 1, replay_reads))->apply() == + ApplyResult::PASS); + REQUIRE( + restored.deserialize(legacy_entry(3, NoVersion, {}))->apply() == + ApplyResult::PASS); + auto reader = restored.create_tx(); + auto* restored_map = reader.ro(map.get_name()); + REQUIRE(restored_map->get(key) == value); + REQUIRE_FALSE(restored_map->has(removed_key)); + REQUIRE(restored_map->get_version_of_previous_write(key) == 3); + } +} + TEST_CASE( "Raw reader rejects truncated entries" * doctest::test_suite("serialisation")) { diff --git a/src/kv/untyped_map.h b/src/kv/untyped_map.h index 7d930fb486f..0d4b8c80107 100644 --- a/src/kv/untyped_map.h +++ b/src/kv/untyped_map.h @@ -326,9 +326,7 @@ namespace ccf::kv::untyped } void serialise_changes( - const AbstractChangeSet* changes, - KvStoreSerialiser& s, - bool include_reads) override + const AbstractChangeSet* changes, KvStoreSerialiser& s) override { const auto* const non_abstract = dynamic_cast(changes); @@ -342,21 +340,9 @@ namespace ccf::kv::untyped s.start_map(name, security_domain); - if (include_reads) - { - s.serialise_entry_version(change_set.read_version); - - s.serialise_count_header(change_set.reads.size()); - for (const auto& [key, value] : change_set.reads) - { - s.serialise_read(key, std::get<0>(value)); - } - } - else - { - s.serialise_entry_version(NoVersion); - s.serialise_count_header(0); - } + // Retain the legacy read-set headers for ledger compatibility. + s.serialise_entry_version(NoVersion); + s.serialise_count_header(0); uint64_t write_ctr = 0; uint64_t remove_ctr = 0; From 60be5a5cbc54d720d1614a4855467eb733e578af Mon Sep 17 00:00:00 2001 From: Amaury Chamayou Date: Mon, 7 Sep 2026 17:01:07 +0100 Subject: [PATCH 2/2] Remove redundant serialization compatibility test Rely on the existing LTS compatibility test for ledger format coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/kv/test/kv_serialisation.cpp | 124 ------------------------------- 1 file changed, 124 deletions(-) diff --git a/src/kv/test/kv_serialisation.cpp b/src/kv/test/kv_serialisation.cpp index 0f64bbf6a9f..553d82b3258 100644 --- a/src/kv/test/kv_serialisation.cpp +++ b/src/kv/test/kv_serialisation.cpp @@ -32,130 +32,6 @@ static std::vector make_size_prefixed_bytes( return bytes; } -TEST_CASE( - "Whole-map dependency ledger encoding compatibility" * - doctest::test_suite("serialisation")) -{ - using namespace ccf::kv; - auto encryptor = std::make_shared(); - for (const auto domain : {SecurityDomain::PUBLIC, SecurityDomain::PRIVATE}) - { - Store store; - const std::string name = domain == SecurityDomain::PUBLIC ? - "public:compatibility" : - "compatibility"; - INFO("Map: ", name); - untyped::Map map(&store, name, domain); - auto changes = map.create_change_set(0, false); - REQUIRE(changes != nullptr); - const SerialisedKey key = {'k'}; - const SerialisedKey removed_key = {'r'}; - const SerialisedKey missing_key = {'m'}; - const SerialisedValue value = {'v'}; - changes->writes[key] = value; - changes->writes[removed_key] = std::nullopt; - - const auto legacy_entry = [&]( - Version write_version, - Version read_version, - const untyped::Read& reads) { - RawWriter public_writer; - RawWriter private_writer; - public_writer.append(EntryType::WriteSet); - public_writer.append(write_version); - public_writer.append(NoVersion); - auto& writer = - domain == SecurityDomain::PUBLIC ? public_writer : private_writer; - writer.append(map.get_name()); - writer.append(read_version); - writer.append(static_cast(reads.size())); - for (const auto& [read_key, versions] : reads) - { - writer.append(read_key); - writer.append(std::get<0>(versions)); - } - writer.append(uint64_t{1}); - writer.append(key); - writer.append(value); - writer.append(uint64_t{1}); - writer.append(removed_key); - RawKvStoreSerialiser serialiser( - encryptor, {0, write_version}, EntryType::WriteSet, 0); - return serialiser.serialise_domains( - public_writer.get_raw_data(), private_writer.get_raw_data()); - }; - const auto serialise = [&]() { - RawKvStoreSerialiser serialiser( - encryptor, {0, 1}, EntryType::WriteSet, 0); - map.serialise_changes(changes.get(), serialiser); - SizeKvStoreSerialiser size_serialiser( - encryptor, {0, 1}, EntryType::WriteSet, 0); - map.serialise_changes(changes.get(), size_serialiser); - auto bytes = serialiser.get_raw_data(); - REQUIRE(size_serialiser.get_serialised_size() == bytes.size()); - return bytes; - }; - - REQUIRE(changes->read_version == NoVersion); - const auto legacy = legacy_entry(1, NoVersion, {}); - REQUIRE(serialise() == legacy); - - untyped::MapHandle handle(*changes, map.get_name()); - REQUIRE_FALSE(handle.get(missing_key).has_value()); - const auto key_reads = changes->reads; - REQUIRE(key_reads.contains(missing_key)); - REQUIRE(serialise() == legacy); - handle.foreach([](const auto&, const auto&) { return true; }); - REQUIRE(changes->read_version == 0); - REQUIRE(serialise() == legacy); - REQUIRE(changes->read_version == 0); - - changes->read_version = 7; - REQUIRE(serialise() == legacy); - REQUIRE(changes->read_version == 7); - REQUIRE(changes->reads == key_reads); - - const untyped::Read legacy_reads = { - {key, {7, NoVersion}}, {missing_key, {NoVersion, NoVersion}}}; - for (const auto read_version : {NoVersion, Version{7}}) - { - for (const auto& reads : {untyped::Read{}, legacy_reads}) - { - const auto bytes = legacy_entry(1, read_version, reads); - RawKvStoreDeserialiser deserialiser(encryptor); - Term term = 0; - EntryFlags flags = {}; - REQUIRE( - deserialiser.init(bytes.data(), bytes.size(), term, flags, false) == - 1); - REQUIRE(deserialiser.start_map() == map.get_name()); - auto decoded = map.deserialise_internal(deserialiser, 0); - REQUIRE(decoded->writes == changes->writes); - REQUIRE(decoded->reads == reads); - REQUIRE(deserialiser.end()); - REQUIRE(decoded->read_version == read_version); - } - } - - Store restored; - restored.set_encryptor(encryptor); - REQUIRE(restored.deserialize(legacy)->apply() == ApplyResult::PASS); - const untyped::Read replay_reads = { - {key, {1, NoVersion}}, {missing_key, {NoVersion, NoVersion}}}; - REQUIRE( - restored.deserialize(legacy_entry(2, 1, replay_reads))->apply() == - ApplyResult::PASS); - REQUIRE( - restored.deserialize(legacy_entry(3, NoVersion, {}))->apply() == - ApplyResult::PASS); - auto reader = restored.create_tx(); - auto* restored_map = reader.ro(map.get_name()); - REQUIRE(restored_map->get(key) == value); - REQUIRE_FALSE(restored_map->has(removed_key)); - REQUIRE(restored_map->get_version_of_previous_write(key) == 3); - } -} - TEST_CASE( "Raw reader rejects truncated entries" * doctest::test_suite("serialisation")) {