Fix stale entry being published to ledger history after rollback

[Amaury Chamayou (achamayou)]

Stacked on #8209 - this PR targets the life_raft branch, so the diff shows only the two files changed here.

The problem

While reviewing #8209 I found a window in which a rolled-back transaction can still publish its digest to the ledger history.

Store::commit() checks the transaction's view under version_lock, then releases the lock before serialising the entry and appending it to the history:

CCF/src/kv/store.h

Lines 978 to 1079 in ec26daf

	{
	std::lock_guard<ccf::pal::Mutex> vguard(version_lock);
	if (txid.view != term_of_next_version)
	{
	LOG_DEBUG_FMT(
	"Discarding transaction {} after Store moved to view {}",
	txid.to_str(),
	term_of_next_version);
	return CommitResult::FAIL_NO_REPLICATE;
	}
	if (globally_committable && txid.seqno > last_committable)
	{
	last_committable = txid.seqno;
	}
	auto [it, inserted] = pending_txs.try_emplace(
	txid.seqno, txid, std::move(pending_tx), globally_committable);
	if (!inserted)
	{
	// Extremely unexpected case: Something went very wrong with TxID
	// assignment, but still fail report here rather than persisting the
	// confusion
	const auto& existing_txid = std::get<0>(it->second);
	LOG_FAIL_FMT(
	"Conflicting pending transactions at seqno {}: {} and {}",
	txid.seqno,
	existing_txid.to_str(),
	txid.to_str());
	return CommitResult::FAIL_NO_REPLICATE;
	}
	LOG_TRACE_FMT("Inserting pending tx at {}", txid.seqno);
	for (Version offset = 1; true; ++offset)
	{
	auto search = pending_txs.find(last_replicated + offset);
	if (search == pending_txs.end())
	{
	LOG_TRACE_FMT(
	"Couldn't find {} = {} + {}, giving up on batch while committing "
	"{}",
	last_replicated + offset,
	last_replicated,
	offset,
	txid.to_str());
	break;
	}
	contiguous_pending_txs.emplace_back(std::move(search->second));
	pending_txs.erase(search);
	}
	previous_rollback_count = rollback_count;
	previous_last_replicated = last_replicated;
	}
	// Release version lock
	if (contiguous_pending_txs.empty())
	{
	return CommitResult::SUCCESS;
	}
	size_t offset = 1;
	for (auto& [pending_txid_, pending_tx_, committable_] :
	contiguous_pending_txs)
	{
	auto
	[success_, data_, claims_digest_, commit_evidence_digest_, hooks_] =
	pending_tx_->call();
	auto data_shared =
	std::make_shared<std::vector<uint8_t>>(std::move(data_));
	auto hooks_shared =
	std::make_shared<ccf::kv::ConsensusHookPtrs>(std::move(hooks_));
	// A pending tx may fail here if rollback invalidated a reserved
	// signature tx after it was dequeued from pending_txs.
	if (success_ == CommitResult::FAIL_NO_REPLICATE)
	{
	LOG_DEBUG_FMT(
	"Failed Tx commit {}", previous_last_replicated + offset);
	return success_;
	}
	// We should never fail from here, as normal txs have already succeeded
	// and reserved txs only fail with FAIL_NO_REPLICATE
	if (success_ != CommitResult::SUCCESS)
	{
	LOG_FAIL_FMT(
	"Unexpected failure reason {} during commit of {}",
	static_cast<int>(success_),
	txid.to_str());
	}
	if (h)
	{
	h->append_entry(ccf::entry_leaf(
	*data_shared, commit_evidence_digest_, claims_digest_));
	}

A rollback can land in that window. Store::rollback() takes maps_lock and version_lock, but not commit_lock, so it is free to run while a commit is in flight.

Thread A: transaction commit	Thread B: election
Applies writes, is assigned TxID 2.2
Enters Store::commit(), takes commit_lock
Checks 2 == term_of_next_version, releases version_lock
Serialises the entry	Wins election in view 4, become_leader() rolls back to committed index 1
	Store::rollback() retracts the history to seqno 1
Appends the stale 2.2 digest to the retracted history
Calls Raft::replicate(), blocks on state->lock
	A concurrent reader observes a root that is in neither the KV nor the ledger
Consensus rejects the entry (view 2 != view 4) and rolls back again

The history's own state_lock does not help: it only serialises rollback() against append_entry(), so "retract to 1, then append leaf 2" is an accepted sequence.

The stale root has a real consumer. set_root_on_proposals() reads it without holding any Store lock, and the proposal id is a digest of that root:

CCF/src/node/rpc/frontend.h

Lines 1119 to 1132 in ec26daf

	if (endpoints.request_needs_root(ctx))
	{
	if (current_history != nullptr)
	{
	// Warning: Retrieving the current TxID and root from the history
	// should only ever be used for the proposal creation endpoint and
	// nothing else. Many bad things could happen otherwise (e.g. breaking
	// session consistency).
	const auto& [txid, root] =
	current_history->get_replicated_state_txid_and_root();
	tx.set_read_txid(txid);
	tx.set_root_at_read_version(root);
	}
	}

Before #8209 this was prevented: Store::commit() captured replication_view under version_lock and passed it to TxHistory::append_entry(), which compared it against term_of_next_version while holding the history lock and dropped the append if it no longer matched. #8209 removes both the argument and the check.

The test

src/consensus/aft/test/view_straddling_transactions.cpp gains a test that drives the interleaving deterministically:

• PausingMovePendingTx behaves exactly like ccf::kv::MovePendingTx (the PendingTx of every normal transaction - it hands over an already-serialised entry and cannot fail), but pauses first. That models the committing thread being descheduled after the view check and before the history append. The signature path is not affected, because commit_reserved() already re-checks the rollback count.
• PausableHistory signals and pauses on the next entry appended, after the base class has released its lock, so the resulting tree is observable by other threads.

The core assertion is that the rolled-back entry is never appended at all. Reverting the store.h hunk reproduces the failure:

view_straddling_transactions.cpp:672: ERROR: CHECK_FALSE( published ) is NOT correct!
view_straddling_transactions.cpp:688: ERROR: CHECK( observed_txid.seqno == 1 ) is NOT correct!
  values: CHECK( 2 == 1 )
view_straddling_transactions.cpp:689: ERROR: CHECK( observed_root == baseline_root ) is NOT correct!
  values: CHECK( a9f80187... == 0b31bab1... )

The history is at seqno 2 while store->current_txid(), raft->get_last_idx() and the ledger are all at seqno 1.

The fix

The store.h change is deliberately minimal, and is a suggestion rather than the only option: re-check the entry's view atomically with the append, under version_lock, which rollback also takes. Lock ordering is unchanged (version_lock then history state_lock, as in Store::rollback()).

An alternative that fits #8209's direction more closely would be to restore a view/generation guard inside TxHistory::append_entry() itself, keyed on each entry's TxID. Happy to switch if you prefer that.

Testing

• raft_test, kv_test, history_test pass.
• 18 KV-dependent unit tests pass: map_test, state_machine_test, kv_test, ledger_test, raft_test, raft_enclave_test, history_test, encryptor_test, historical_queries_test, indexing_test, snapshot_test, snapshotter_test, frontend_test, endpoint_registry_test, tx_status_test, node_frontend_test, internal_tables_access_test, merkle_test.
• cpp-format-checks.sh, copyright-checks.sh, ascii-checks.sh clean.

No CHANGELOG.md entry: this fixes a regression that is not in any release, and there is no user-visible API change.