test(safety-rules): repro for #706 — on_disk_storage no-fsync enables double-vote

[keanji-x]

What

Reproduction / demonstration for gravity-audit #706 (F4 in galxe/RESTART_RECOVERY_FINDINGS.md): the safety-rules last_voted_round — the only guard against a validator voting twice in a round — is persisted via the on_disk_storage backend (the production default, cluster/templates/validator.yaml.tpl:8-11, type: on_disk_storage), whose write path performs no fsync, so a power-loss crash can revert it and enable a double-vote (BFT safety violation).

The buggy write (gravity-aptos@e9544c8 secure/storage/src/on_disk.rs, the rev compiled per Cargo.lock):

fn write(&self, data: &HashMap<String, Value>) -> Result<(), Error> {
    let contents = serde_json::to_vec(data)?;
    let mut file = File::create(self.temp_path.path())?;
    file.write_all(&contents)?;
    fs::rename(&self.temp_path, &self.file_path)?;   // NO sync_all/fsync on file OR dir
    Ok(())
}

There is no sync_all/fsync anywhere in the crate. By contrast the consensus DB is durable: schemadb's default_write_options() calls opts.set_sync(true) (storage/schemadb/src/lib.rs:331-333). So on power-loss the durable consensusdb last_vote + the wire-sent vote survive while secure_storage.json reverts to a lower last_voted_round. enable_cached_safety_data defaults to true, so the gap is invisible in-process.

Tests added

All three live in the compiled aptos-safety-rules crate (the production code, a workspace path dep — Cargo.toml:88), not a throwaway harness.

1. on_disk_write_is_not_durable_unlike_consensusdb — exercises the buggy OnDiskStorage::write and shows that a write which returned Ok is byte-revertible (no fsync), contrasted with consensusdb's set_sync(true).
2. power_loss_revert_of_safety_data_enables_double_vote — end-to-end through the real SafetyRules vote path: vote in round R (persisting last_voted_round=R, last_vote=Some(..)), then revert the persisted SafetyData to its pre-vote value (models the lost, never-fsync'd page), reload, and successfully sign a second, conflicting vote in round R. Includes a control proving the guard works when the state is intact (the conflicting proposal is refused, the prior vote is returned).
3. cache_off_safety_data_read_is_broken — bonus finding surfaced while building the repro: with enable_cached_safety_data=false, PersistentSafetyStorage::safety_data() always fails with unknown variant \epoch`, expected `Ok` or `Err`because a misplaced?makes the genericgetinfer its value type asResult<SafetyData, Error>. A direct internal_store().get::(SAFETY_DATA)` reads the same bytes fine. This is relevant to fix(cluster): default genesisTimestampSecs so e2e survives genesis-tool strict mode #706's recovery fix, which would re-read SafetyData on the (currently broken) cache-off path.

Evidence

running 3 tests
test tests::fsync_durability_706::cache_off_safety_data_read_is_broken ... ok
test tests::fsync_durability_706::on_disk_write_is_not_durable_unlike_consensusdb ... ok
test tests::fsync_durability_706::power_loss_revert_of_safety_data_enables_double_vote ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 11 filtered out

Run with:

export CARGO_TARGET_DIR=.../target
RUSTFLAGS="--cfg tokio_unstable" cargo test -p aptos-safety-rules --lib fsync_durability_706

Honest scope (proven vs. requires real power-loss)

• Proven by these tests: the no-fsync write path is exercised; a "succeeded" on-disk write is byte-revertible; and given a SafetyData revert, the real safety-rules vote path deterministically signs a second conflicting vote in the same round.
• Not provable by a unit test (modeled, not reproduced): that a real power-loss loses exactly the page holding the vote update. We restore the pre-vote bytes/state to model that loss. The bug is that nothing fsyncs to prevent it.

Proposed fix (NOT applied here — kept test-only)

• In OnDiskStorage::write, sync_all() the temp file and fsync the containing directory before/after fs::rename (or back SAFETY_DATA with a sync-durable store).
• On recovery, re-tighten last_voted_round to >= the durable consensusdb last_vote round, so a reverted secure-storage cannot regress below what was already committed/sent.
• Separately, fix the cache-off safety_data() read (Ok(self.internal_store.get(SAFETY_DATA)?.value)), which the recovery cross-check would depend on.

No production code was modified.

Refs Galxe/gravity-audit#706

🤖 Generated with Claude Code

[Lchangliang]

I think there are two separate points here:

1. The durability gap in on_disk_storage looks real. PersistentSafetyStorage documents that set is expected to be synced before returning, but the OnDiskStorage backend is only doing temp-file write + rename without fsync. For safety-critical state, adding fsync looks like a reasonable hardening fix and should not have semantic side effects. The only expected cost is write latency, which should be acceptable for this path.

A proper fix should sync the temp file before rename and fsync the parent directory after rename, so both the file contents and the rename are durable.

2. I am less convinced that this test proves the full production double-vote consequence. The test directly reverts SafetyData and calls SafetyRules again, which does show that SafetyRules by itself can forget the previous vote if its persisted SafetyData rolls back.

However, the real validator path has another layer: after SafetyRules signs, RoundManager::vote_block persists the vote into consensusdb via save_vote, and only then sends/broadcasts the vote. On restart, recovery reads that consensusdb last_vote and RoundManager::init(last_vote) records it into round_state. Then vote_block checks round_state.vote_sent().is_none() before voting again. So if the original vote was actually sent, consensusdb should normally have the last vote and RoundManager may still prevent a second vote for the same round even if secure storage rolled back.

Because of that, I think the fsync fix is still reasonable and low-risk, but the current unit test is more a modeled SafetyRules-only consequence than a full production repro. To prove the double-vote consequence, the test would need to drive the restart/RoundManager path with consensusdb last_vote preserved and still show that a conflicting vote for the same round can be emitted.

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.