diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bfc91cc165..d48ef750fe5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed +- A signature transaction decided whether to end a ledger chunk, and recorded that decision on the chunker, outside the version lock. A rollback landing in that window discarded the signature but left the chunk marker behind. The decision and the record are now made atomically, and skipped when the signature's view or rollback epoch no longer holds (#8246). - A transaction's `force_ledger_chunk` and `snapshot_at_next_signature` flags are no longer applied once a concurrent view change has discarded the transaction's writes, which previously left a chunk boundary, or an armed snapshot, for a transaction no longer present in the ledger. The forced chunk is also attached to the transaction's own version rather than whichever version the store had reached (#8245). - A rollback whose target is at or beyond the store's own version no longer moves ledger chunk metadata forward past it, which previously left a permanent offset skewing later chunk boundaries (#8244). - Ledger chunk metadata and snapshot scheduling are no longer restored by a transaction whose writes a concurrent view change has already discarded. Both are now updated under the same lock as the rollback, and skipped when the transaction's rollback epoch or view no longer holds (#8243). diff --git a/src/kv/committable_tx.h b/src/kv/committable_tx.h index 444fa796d8d..b542981edd0 100644 --- a/src/kv/committable_tx.h +++ b/src/kv/committable_tx.h @@ -557,20 +557,26 @@ namespace ccf::kv // This is a signature and, if the ledger chunking or snapshot flags are // enabled, we want the host to create a chunk when it sees this entry. - // version_lock held by Store::commit - if (pimpl->store->should_create_ledger_chunk_unsafe(version)) + // Deciding this and recording the chunk must be atomic with respect to + // rollback, so that a signature a rollback discards leaves no marker + // behind. + const auto should_create_chunk = + pimpl->store->should_create_ledger_chunk_for_reserved_tx( + version, pimpl->commit_view, rollback_count); + if (!should_create_chunk.has_value()) + { + committed = true; + return { + CommitResult::FAIL_NO_REPLICATE, {}, ccf::empty_claims(), {}, {}}; + } + + if (should_create_chunk.value()) { entry_flags |= EntryFlags::FORCE_LEDGER_CHUNK_AFTER; LOG_DEBUG_FMT( "Ending ledger chunk with signature at {}.{}", pimpl->commit_view, version); - - auto chunker = pimpl->store->get_chunker(); - if (chunker) - { - chunker->produced_chunk_at(version); - } } committed = true; diff --git a/src/kv/kv_types.h b/src/kv/kv_types.h index 93e3e4702c3..bf13afdbc6f 100644 --- a/src/kv/kv_types.h +++ b/src/kv/kv_types.h @@ -750,6 +750,8 @@ namespace ccf::kv Version expected_rollback_count, bool force_ledger_chunk, bool snapshot_at_next_signature) = 0; + virtual std::optional should_create_ledger_chunk_for_reserved_tx( + Version version, Term expected_term, Version expected_rollback_count) = 0; virtual std::unique_ptr snapshot_unsafe_maps( Version v) = 0; diff --git a/src/kv/store.h b/src/kv/store.h index 8d43f21a056..64c3b43e9c2 100644 --- a/src/kv/store.h +++ b/src/kv/store.h @@ -1150,6 +1150,28 @@ namespace ccf::kv return false; } + std::optional should_create_ledger_chunk_for_reserved_tx( + Version version, + Term expected_term, + Version expected_rollback_count) override + { + std::lock_guard vguard(version_lock); + if ( + term_of_next_version != expected_term || + rollback_count != expected_rollback_count) + { + return std::nullopt; + } + + const auto should_create_chunk = + should_create_ledger_chunk_unsafe(version); + if (should_create_chunk && chunker) + { + chunker->produced_chunk_at(version); + } + return should_create_chunk; + } + bool should_create_ledger_chunk(Version version) override { std::lock_guard vguard(version_lock); diff --git a/src/kv/test/kv_test.cpp b/src/kv/test/kv_test.cpp index 0c08b85cc41..1c429b543da 100644 --- a/src/kv/test/kv_test.cpp +++ b/src/kv/test/kv_test.cpp @@ -3504,6 +3504,12 @@ class InspectableChunker : public ccf::kv::LedgerChunker ccf::ds::MutexGuard guard(chunker_lock); return current_tx_version; } + + bool has_chunk_end_at(ccf::kv::Version v) + { + ccf::ds::MutexGuard guard(chunker_lock); + return chunk_ends.contains(v); + } }; // A PendingTx which rolls the store back while Store::commit() is midway @@ -3715,6 +3721,87 @@ TEST_CASE("Rollback-sensitive transaction flags are not restored") } } +TEST_CASE("Reserved signature side effects are not applied after a rollback") +{ + ccf::kv::Store store; + store.set_encryptor(std::make_shared()); + auto consensus = std::make_shared(); + store.set_consensus(consensus); + auto chunker = std::make_shared(); + store.set_chunker(chunker); + + constexpr ccf::kv::Term initial_term = 2; + store.initialise_term(initial_term); + MapTypes::StringString map("public:map"); + + for (const auto* value : {"first", "second"}) + { + auto tx = store.create_tx(); + tx.rw(map)->put("key", value); + REQUIRE(tx.commit() == ccf::kv::CommitResult::SUCCESS); + } + + const auto reserved = store.current_txid(); + REQUIRE(store.check_rollback_count(0)); + + INFO("A signature a rollback discarded records no chunk end"); + { + // A signature ends a chunk when a snapshot is due, which is what the + // reserved transaction records once it decides to replicate. + store.set_flag( + ccf::kv::AbstractStore::StoreFlag::SNAPSHOT_AT_NEXT_SIGNATURE); + store.rollback({initial_term, reserved.seqno - 1}, initial_term + 1); + REQUIRE(store.check_rollback_count(1)); + + CHECK_FALSE(store + .should_create_ledger_chunk_for_reserved_tx( + reserved.seqno, reserved.view, 0) + .has_value()); + CHECK_FALSE(chunker->has_chunk_end_at(reserved.seqno)); + } + + INFO("A signature whose view has moved on records no chunk end"); + { + auto tx = store.create_tx(); + tx.rw(map)->put("key", "next"); + REQUIRE(tx.commit() == ccf::kv::CommitResult::SUCCESS); + const auto superseded = store.current_txid(); + store.set_flag( + ccf::kv::AbstractStore::StoreFlag::SNAPSHOT_AT_NEXT_SIGNATURE); + + // A rollback which discards nothing locally, but moves the view on, so + // consensus will refuse to replicate this signature. The snapshot flag + // survives, so without the view check the chunk end would be recorded. + store.rollback(superseded, superseded.view + 1); + REQUIRE(store.check_rollback_count(1)); + REQUIRE(store.flag_enabled( + ccf::kv::AbstractStore::StoreFlag::SNAPSHOT_AT_NEXT_SIGNATURE)); + + CHECK_FALSE(store + .should_create_ledger_chunk_for_reserved_tx( + superseded.seqno, superseded.view, 1) + .has_value()); + CHECK_FALSE(chunker->has_chunk_end_at(superseded.seqno)); + } + + INFO("A signature still in its own epoch records its chunk end"); + { + auto tx = store.create_tx(); + tx.rw(map)->put("key", "replacement"); + REQUIRE(tx.commit() == ccf::kv::CommitResult::SUCCESS); + const auto replacement = store.current_txid(); + store.set_flag( + ccf::kv::AbstractStore::StoreFlag::SNAPSHOT_AT_NEXT_SIGNATURE); + + const auto should_create_chunk = + store.should_create_ledger_chunk_for_reserved_tx( + replacement.seqno, replacement.view, 1); + REQUIRE(should_create_chunk.has_value()); + CHECK(should_create_chunk.value()); + CHECK(chunker->has_chunk_end_at(replacement.seqno)); + } +} + TEST_CASE("Ledger entry chunk request") { ccf::kv::Store store;