diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a668a8dd1bc..82552ea9c273 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,14 +5,6 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). -## [3.0.3] - -[3.0.3]: https://github.com/microsoft/CCF/releases/tag/ccf-3.0.3 - -### Fixed - -- Node-to-node channels no longer check certificate expiry times. This previously caused "Peer certificate verification failed" error messages when node or service certs expired. (#4733) - ## [3.0.2] [3.0.2]: https://github.com/microsoft/CCF/releases/tag/ccf-3.0.2 @@ -26,6 +18,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Access to restricted KV tables (eg - private or non-governance reads during governance, or governance writes during application execution) produce more descriptive errors. The documentation has been extended to describe these restrictions. (#4686) - `TCP_NODELAY` is now set for all incoming and outgoing TCP connections (#4717). +### Fixed + +- Node-to-node channels no longer check certificate expiry times. This previously caused "Peer certificate verification failed" error messages when node or service certs expired. (#4733) +- Fixed issue where invalid snapshots could be generated depending on the pattern of additions/removals of keys in a given key-value map (#4730). + ## [3.0.1] [3.0.1]: https://github.com/microsoft/CCF/releases/tag/ccf-3.0.1 diff --git a/include/ccf/ds/logger.h b/include/ccf/ds/logger.h index 643a93ddff62..e68166ea2f59 100644 --- a/include/ccf/ds/logger.h +++ b/include/ccf/ds/logger.h @@ -271,6 +271,7 @@ namespace logger static inline void default_init() { + get_loggers().clear(); add_text_console_logger(); } #endif diff --git a/src/ds/champ_map.h b/src/ds/champ_map.h index ae086aba09b3..0334b8b4f450 100644 --- a/src/ds/champ_map.h +++ b/src/ds/champ_map.h @@ -126,8 +126,10 @@ namespace champ const auto& entry = bin[i]; if (k == entry->key) { + const auto diff = map::get_serialized_size_with_padding(entry->key) + + map::get_serialized_size_with_padding(entry->value); bin[i] = std::make_shared>(k, v); - return map::get_size(k) + map::get_size(v); + return diff; } } bin.push_back(std::make_shared>(k, v)); @@ -143,8 +145,8 @@ namespace champ const auto& entry = bin[i]; if (k == entry->key) { - const auto diff = - map::get_size(entry->key) + map::get_size(entry->value); + const auto diff = map::get_serialized_size_with_padding(entry->key) + + map::get_serialized_size_with_padding(entry->value); bin.erase(bin.begin() + i); return diff; } @@ -211,6 +213,7 @@ namespace champ return node_as>(c_idx)->getp(depth + 1, hash, k); } + // Returns serialised size of overwritten (k,v) if k exists, 0 otherwise size_t put_mut(SmallIndex depth, Hash hash, const K& k, const V& v) { const auto idx = mask(hash, depth); @@ -246,8 +249,8 @@ namespace champ const auto& entry0 = node_as>(c_idx); if (k == entry0->key) { - auto current_size = map::get_size_with_padding(entry0->key) + - map::get_size_with_padding(entry0->value); + auto current_size = map::get_serialized_size_with_padding(entry0->key) + + map::get_serialized_size_with_padding(entry0->value); nodes[c_idx] = std::make_shared>(k, v); return current_size; } @@ -297,6 +300,7 @@ namespace champ std::make_shared>(std::move(node)), r); } + // Returns serialised size of removed (k,v) if k exists, 0 otherwise size_t remove_mut(SmallIndex depth, Hash hash, const K& k) { const auto idx = mask(hash, depth); @@ -311,8 +315,8 @@ namespace champ if (entry->key != k) return 0; - const auto diff = map::get_size_with_padding(entry->key) + - map::get_size_with_padding(entry->value); + const auto diff = map::get_serialized_size_with_padding(entry->key) + + map::get_serialized_size_with_padding(entry->value); nodes.erase(nodes.begin() + c_idx); data_map = data_map.clear(idx); return diff; @@ -437,9 +441,10 @@ namespace champ if (r.second == 0) size_++; - const auto size_change = - (map::get_size_with_padding(key) + map::get_size_with_padding(value)) - + const auto size_change = (map::get_serialized_size_with_padding(key) + + map::get_serialized_size_with_padding(value)) - r.second; + return Map(std::move(r.first), size_, size_change + serialized_size); } @@ -492,17 +497,14 @@ namespace champ { std::vector ordered_state; ordered_state.reserve(map.size()); - size_t size = 0; + size_t serialized_size = 0; - map.foreach([&](auto& key, auto& value) { + map.foreach([&ordered_state, &serialized_size](auto& key, auto& value) { K* k = &key; V* v = &value; - uint32_t ks = map::get_size(key); - uint32_t vs = map::get_size(value); - uint32_t key_size = ks + map::get_padding(ks); - uint32_t value_size = vs + map::get_padding(vs); - - size += (key_size + value_size); + uint32_t key_size = map::get_serialized_size_with_padding(key); + uint32_t value_size = map::get_serialized_size_with_padding(value); + serialized_size += (key_size + value_size); ordered_state.emplace_back(k, static_cast(H()(key)), v); @@ -517,9 +519,10 @@ namespace champ }); CCF_ASSERT_FMT( - size == map.get_serialized_size(), - "size:{}, map->size:{} ==> count:{}, vect:{}", - size, + serialized_size == map.get_serialized_size(), + "Serialized size:{}, map.get_serialized_size():{} (map count:{}, " + "ordered state count:{})", + serialized_size, map.get_serialized_size(), map.size(), ordered_state.size()); @@ -527,15 +530,19 @@ namespace champ for (const auto& p : ordered_state) { // Serialize the key - uint32_t key_size = map::serialize(*p.k, data, size); - map::add_padding(key_size, data, size); + uint32_t key_size = map::serialize(*p.k, data, serialized_size); + map::add_padding(key_size, data, serialized_size); // Serialize the value - uint32_t value_size = map::serialize(*p.v, data, size); - map::add_padding(value_size, data, size); + uint32_t value_size = map::serialize(*p.v, data, serialized_size); + map::add_padding(value_size, data, serialized_size); } - CCF_ASSERT_FMT(size == 0, "buffer not filled, remaining:{}", size); + CCF_ASSERT_FMT( + serialized_size == 0, + "Serialization buffer is not complete, remaining:{}/{}", + serialized_size, + map.get_serialized_size()); } }; diff --git a/src/ds/map_serializers.h b/src/ds/map_serializers.h index b5c5a5bc6369..fbaaaea7ae37 100644 --- a/src/ds/map_serializers.h +++ b/src/ds/map_serializers.h @@ -38,7 +38,7 @@ namespace map } template - static size_t get_size_with_padding(const T& t) + static size_t get_serialized_size_with_padding(const T& t) { const uint32_t t_size = get_size(t); return t_size + get_padding(t_size); diff --git a/src/ds/rb_map.h b/src/ds/rb_map.h index a5fbaf15d23f..187a83cdeab8 100644 --- a/src/ds/rb_map.h +++ b/src/ds/rb_map.h @@ -38,8 +38,8 @@ namespace rb _rgt(rgt) { total_size = 1; - total_serialized_size = - map::get_size_with_padding(key) + map::get_size_with_padding(val); + total_serialized_size = map::get_serialized_size_with_padding(key) + + map::get_serialized_size_with_padding(val); if (lft) { total_size += lft->size(); diff --git a/src/ds/test/map_test.cpp b/src/ds/test/map_test.cpp index 61d9dde95fa0..58f33f85821b 100644 --- a/src/ds/test/map_test.cpp +++ b/src/ds/test/map_test.cpp @@ -2,8 +2,12 @@ // Licensed under the Apache 2.0 License. #define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN #include "ccf/byte_vector.h" +#include "ccf/ds/logger.h" +#include "ccf/kv/serialisers/serialised_entry.h" #include "ds/champ_map.h" #include "ds/rb_map.h" +#include "ds/std_formatters.h" +#include "kv/untyped_change_set.h" #include #include @@ -18,8 +22,9 @@ struct CollisionHash } }; -using K = uint64_t; -using V = uint64_t; +using K = kv::serialisers::SerialisedEntry; +using V = kv::serialisers::SerialisedEntry; +constexpr static size_t max_key_value_size = 128; namespace map { @@ -103,7 +108,7 @@ struct Put : public Op std::string str() { auto ss = std::stringstream(); - ss << "Put(" << H()(k) << ", " << v << ")"; + ss << "Put(" << H()(k) << ", value of size " << v.size() << ")"; return ss.str(); } }; @@ -157,8 +162,9 @@ std::vector>> gen_ops(size_t n) std::vector>> ops; std::vector keys; - for (V v = 0; v < n; ++v) + for (size_t i = 0; i < n; ++i) { + V v(gen() % max_key_value_size, 'v'); std::unique_ptr> op; auto op_i = keys.empty() ? 0 : gen_op(gen); switch (op_i) @@ -166,7 +172,7 @@ std::vector>> gen_ops(size_t n) case 0: case 1: // insert { - auto k = gen(); + K k(gen() % max_key_value_size, 'k'); keys.push_back(k); op = std::make_unique>(k, v); @@ -254,130 +260,77 @@ template static const M gen_map(size_t size) { M map; - for (size_t i = 0; i < size; ++i) + Model model; + + auto ops = gen_ops(size); + for (auto& op : ops) { - map = map.put(i, i); + auto r = op->apply(model, map); + map = r.second; } + return map; } -TEST_CASE_TEMPLATE("Snapshot map", M, RBMap, ChampMap) +TEST_CASE_TEMPLATE("Snapshot map", M, ChampMap, RBMap) { - std::vector results; - uint32_t num_elements = 100; - auto map = gen_map(num_elements); + size_t ops_count = 2048; + auto map = gen_map(ops_count); + std::map contents; // Ordered content as Champ is unordered + std::vector serialised_snapshot; + + M new_map; - INFO("Check initial content of map"); + INFO("Record content of source map"); { - map.foreach([&results](const auto& key, const auto& value) { - results.push_back({key, value}); + map.foreach([&contents](const auto& key, const auto& value) { + contents[key] = value; return true; }); - REQUIRE_EQ(num_elements, results.size()); - REQUIRE_EQ(map.size(), num_elements); - } - - INFO("Populate second map and compare"); - { - std::set keys; - M new_map; - for (const auto& p : results) - { - REQUIRE_LT(p.k, num_elements); - keys.insert(p.k); - new_map = new_map.put(p.k, p.v); - } - REQUIRE_EQ(num_elements, new_map.size()); - REQUIRE_EQ(num_elements, keys.size()); + REQUIRE_EQ(map.size(), contents.size()); } - INFO("Serialize map to array"); + INFO("Generate snapshot"); { auto snapshot = map.make_snapshot(); - std::vector s(map.get_serialized_size()); - snapshot->serialize(s.data()); - - auto new_map = map::deserialize_map(s); - - std::set keys; - new_map.foreach([&keys](const auto& key, const auto& value) { - keys.insert(key); - REQUIRE_EQ(key, value); - return true; - }); - REQUIRE_EQ(map.size(), new_map.size()); - REQUIRE_EQ(map.size(), keys.size()); + serialised_snapshot.resize(map.get_serialized_size()); + snapshot->serialize(serialised_snapshot.data()); - // Check that new entries can be added to deserialised map - uint32_t offset = 1000; - for (uint32_t i = offset; i < offset + num_elements; ++i) - { - new_map = new_map.put(i, i); - } - REQUIRE_EQ(new_map.size(), map.size() + num_elements); - for (uint32_t i = offset; i < offset + num_elements; ++i) + INFO("Ensure serialised state is byte identical"); { - auto p = new_map.get(i); - REQUIRE(p.has_value()); - REQUIRE(p.value() == i); + auto snapshot_2 = map.make_snapshot(); + std::vector serialised_snapshot_2(map.get_serialized_size()); + snapshot_2->serialize(serialised_snapshot_2.data()); + REQUIRE_EQ(serialised_snapshot, serialised_snapshot_2); } } - INFO("Ensure serialized state is byte identical"); + INFO("Apply snapshot to target map"); { - auto snapshot_1 = map.make_snapshot(); - std::vector s_1(map.get_serialized_size()); - snapshot_1->serialize(s_1.data()); - - auto snapshot_2 = map.make_snapshot(); - std::vector s_2(map.get_serialized_size()); - snapshot_2->serialize(s_2.data()); + new_map = map::deserialize_map(serialised_snapshot); + REQUIRE_EQ(map.size(), new_map.size()); - REQUIRE_EQ(s_1, s_2); + std::map new_contents; + new_map.foreach([&new_contents](const auto& key, const auto& value) { + new_contents[key] = value; + return true; + }); + REQUIRE_EQ(contents.size(), new_contents.size()); + REQUIRE_EQ(contents, new_contents); } - INFO("Snapshot is immutable"); + INFO("Check that new entries can be added to target map"); { - size_t current_size = map.size(); - auto snapshot = map.make_snapshot(); - std::vector s_1(map.get_serialized_size()); - snapshot->serialize(s_1.data()); - - // Add entry in map - auto key = current_size + 1; - REQUIRE(map.get(key) == std::nullopt); - map = map.put(key, key); - - // Even though map has been updated, snapshot is not modified - std::vector s_2(s_1.size()); - snapshot->serialize(s_2.data()); - REQUIRE_EQ(s_1, s_2); + Model new_model; + auto new_ops = gen_ops(ops_count); + for (auto& op : new_ops) + { + auto r = op->apply(new_model, new_map); + new_map = r.second; + } } } -using SerialisedKey = ccf::ByteVector; -using SerialisedValue = ccf::ByteVector; - -TEST_CASE_TEMPLATE( - "Serialize map with different key sizes", - M, - UntypedChampMap, - UntypedRBMap) -{ - M map; - SerialisedKey key(16); - SerialisedValue long_key(128); - SerialisedValue value(8); - SerialisedValue long_value(256); - - map = map.put(key, value); - map = map.put(long_key, long_value); - - auto snapshot = map.make_snapshot(); - std::vector s(map.get_serialized_size()); - snapshot->serialize(s.data()); -} - template std::map get_all_entries(const M& map) { @@ -390,6 +343,55 @@ std::map get_all_entries(const M& map) return entries; } +TEST_CASE_TEMPLATE("Snapshot is immutable", M, ChampMap, RBMap) +{ + size_t ops_count = 2048; + auto map = gen_map(ops_count); + + // Take snapshot at original state + auto snapshot = map.make_snapshot(); + size_t serialised_snapshot_before_size = snapshot->get_serialized_size(); + std::vector serialised_snapshot_before( + serialised_snapshot_before_size); + snapshot->serialize(serialised_snapshot_before.data()); + + INFO("Meanwhile, modify map"); + { + // Remove operation is not yet implemented for RBMap + if constexpr (std::is_same_v) + { + auto all_entries = get_all_entries(map); + auto& key_to_remove = all_entries.begin()->first; + map = map.remove(key_to_remove); + } + + // Modify existing key with value that must be different from what `gen_map` + // populated the map with + auto all_entries = get_all_entries(map); + auto& key_to_add = all_entries.begin()->first; + map = map.put(key_to_add, V(max_key_value_size * 2, 'x')); + } + + INFO("Even though map has been updated, original snapshot is not modified"); + { + REQUIRE_EQ( + snapshot->get_serialized_size(), serialised_snapshot_before_size); + std::vector serialised_snapshot_after( + serialised_snapshot_before_size); + snapshot->serialize(serialised_snapshot_after.data()); + REQUIRE_EQ(serialised_snapshot_before, serialised_snapshot_after); + } + + INFO("But new snapshot is different"); + { + auto new_snapshot = map.make_snapshot(); + size_t serialised_snapshot_new_size = new_snapshot->get_serialized_size(); + std::vector serialised_snapshot_new(serialised_snapshot_new_size); + new_snapshot->serialize(serialised_snapshot_new.data()); + REQUIRE_NE(serialised_snapshot_before, serialised_snapshot_new); + } +} + template void verify_snapshot_compatibility(const S& source_map, T& target_map) { @@ -432,11 +434,11 @@ void forall_threshold(const M& map, size_t threshold) { size_t iterations_count = 0; map.foreach([&iterations_count, threshold](const K& k, const V& v) { - iterations_count++; if (iterations_count >= threshold) { return false; } + iterations_count++; return true; }); REQUIRE(iterations_count == threshold); @@ -445,8 +447,8 @@ void forall_threshold(const M& map, size_t threshold) TEST_CASE_TEMPLATE("Foreach", M, RBMap, ChampMap) { size_t size = 100; - size_t threshold = size / 2; - auto map = gen_map(size); + + size_t threshold = map.size() / 2; forall_threshold(map, threshold); } diff --git a/src/kv/test/kv_snapshot.cpp b/src/kv/test/kv_snapshot.cpp index 66b2604637d8..3b9e2b2dfdb4 100644 --- a/src/kv/test/kv_snapshot.cpp +++ b/src/kv/test/kv_snapshot.cpp @@ -170,7 +170,7 @@ TEST_CASE("Old snapshots" * doctest::test_suite("snapshot")) // Test that this code can still parse snapshots produced by old versions of // the code // NB: These raw strings are base64 encodings from - // `sencond_serialised_snapshot` in the "Simple snapshot" test + // `second_serialised_snapshot` in the "Simple snapshot" test std::string raw_snapshot_b64; SUBCASE("Tombstone deletions") { diff --git a/src/kv/untyped_map.h b/src/kv/untyped_map.h index 690b8ca6f541..dfe20b4bcb5f 100644 --- a/src/kv/untyped_map.h +++ b/src/kv/untyped_map.h @@ -303,6 +303,7 @@ namespace kv::untyped void serialise(KvStoreSerialiser& s) override { + LOG_TRACE_FMT("Serialising snapshot for map: {}", name); s.start_map(name, security_domain); s.serialise_entry_version(version); diff --git a/tests/partitions_test.py b/tests/partitions_test.py index b9c4669a2ca7..13330fdce712 100644 --- a/tests/partitions_test.py +++ b/tests/partitions_test.py @@ -339,9 +339,7 @@ def set_certs(from_days_diff, validity_period_days, nodes): stack.enter_context(network.partitioner.partition([primary])) # Restore connectivity between backups and wait for election - network.wait_for_primary_unanimity( - nodes=[backup_a, backup_b], min_view=r.view - ) + network.wait_for_primary_unanimity(nodes=[backup_a, backup_b], min_view=r.view) # Should now be able to make progress check_can_progress(backup_a)