PR #488 follow-up: deferred review items

[spreston8]

PR #488 follow-up: deferred review items

Tracks all items deferred from a multi-pass review of #488 (fix(merge): close merge-stale-diff bug class). Items addressed during the review are not listed here — they landed in commits 35b918e4, 837fd392, b79bd5a3, e77a70da, 19b0297f, 3a6baf15, dc217d5f, a1cd5233, and e1c1bd1d.

This issue groups items by severity. Test coverage gaps are in a follow-up comment to keep this main body focused on code-level items.

---

Medium severity

1. Consolidate KeyValueRejectedDeployBuffer and KeyValueDeployStorage

The new buffer module is ~85% byte-identical to existing KeyValueDeployStorage:

Method	KeyValueDeployStorage	KeyValueRejectedDeployBuffer	Differs?
Struct field	pub store: KeyValueTypedStoreImpl<ByteString, Signed<DeployData>>	identical	no
new(kvm)	opens "deploy_storage"	opens "rejected_deploy_buffer"	name only
add(deploys)	put, key by sig	identical	no
remove(deploys)	delete, key by sig	identical	no
remove_by_sig(sig)	exists-check + delete	identical	no
read_all()	to_map().map(...)	identical	no
non_empty()	store.non_empty()	identical	no
any(pred)	present	absent	only KeyValueDeployStorage
contains_sig(sig)	absent	present	only buffer
get_by_sig(sig)	absent	present	only buffer

The buffer's own header comment is honest: // Mirrors KeyValueDeployStorage in shape and storage backing.

Cost of leaving as-is: every future change to the deploy-sig storage API lands in two files; helpers drift between the two over time.

Recommended fix: parameterize KeyValueDeployStorage::new on store name, add the missing helpers (contains_sig, get_by_sig, any) once, delete the buffer module. ~30-line consolidation removes 91 lines. Update ~7 import sites across engine/casper_launch.rs, engine/engine.rs, engine/genesis_validator.rs, engine/genesis_ceremony_master.rs, engine/initializing.rs, node/src/rust/runtime/setup.rs, and tests.

Bundle: make pub store: field pub(crate). The whole point of the wrapper is to expose only sig-keyed methods. pub store: lets any caller bypass the wrapper and call store.put(...) / store.delete(...) directly with arbitrary keys. Verified no Rust call site outside the module accesses .store directly today, but pub invites future drift. Same smell exists on KeyValueDeployStorage.store (existing); the consolidated module should make it private.

Files: block-storage/src/rust/deploy/{key_value_deploy_storage,key_value_rejected_deploy_buffer}.rs.

2. repeat_deploy should use resolve_batch instead of per-sig resolve

validate.rs::repeat_deploy (line 353-415) calls resolve_finalization_status(...) once per sig that's in rejected_in_scope. Each call runs a fresh BFS over deploy_lifespan blocks.

let deploy_key_set: HashSet<Vec<u8>> = block.body.deploys
    .iter()
    .filter(|pd| {
        if !s.rejected_in_scope.contains(&pd.deploy.sig) {
            return true;
        }
        match resolve_finalization_status(  // ← per-sig BFS over deploy_lifespan
            &s.dag, block_store, expiration_threshold as i64, &pd.deploy.sig,
        ) { ... }
    })
    ...

For a block with M deploys whose sigs are in rejected_in_scope:

• Each resolve_finalization_status runs a fresh BFS bounded by deploy_lifespan blocks
• Each block visited triggers an LMDB read + protobuf deserialization
• Total cost per block validation: M × deploy_lifespan LMDB reads

Concrete numbers:

• deploy_lifespan = 50 (test default), M = 10 → 500 reads per block validation
• deploy_lifespan = 200 (typical production), M = 20 → 4 000 reads per block validation

The resolve_batch API exists in deploy_finalization_status.rs:497 (added in this PR) and amortizes the BFS across all sigs in one walk. Cost goes from O(M × lifespan) to O(M + lifespan) — for M = 20, ~220 reads instead of 4 000, an ~18× reduction for the validation hot path.

Recommended fix: swap the per-sig resolve for one resolve_batch call upstream of the filter, then look up per-sig results from the resulting HashMap:

let rejected_in_scope_sigs: HashSet<Bytes> = block.body.deploys
    .iter()
    .filter_map(|pd| s.rejected_in_scope.contains(&pd.deploy.sig).then(|| pd.deploy.sig.clone()))
    .collect();
let statuses = if rejected_in_scope_sigs.is_empty() {
    HashMap::new()
} else {
    resolve_batch(&s.dag, block_store, expiration_threshold as i64, &rejected_in_scope_sigs)?
};
// per-deploy filter then looks up statuses[sig] instead of calling resolve()

File: casper/src/rust/validate.rs:353-415.

---

Low / Medium — non-deterministic block bytes

These are pre-existing patterns the PR inherits but doesn't introduce. Two-line fixes each. They don't break cross-validator consensus (each validator signs their own block) but they break crash-recovery idempotency, test reproducibility, and any byte-keyed cache.

3. Sort justifications Vec before signing

justifications is collected as DashSet<Justification> in multi_parent_casper_impl.rs:331-342:

let justifications = {
    let bonded_validators = &on_chain_state.bonds_map;
    latest_msgs_hashes
        .iter()
        .filter(|(validator, _)| bonded_validators.contains_key(*validator))
        .map(|(validator, block_hash)| Justification {
            validator: validator.clone(),
            latest_block_hash: block_hash.clone(),
        })
        .collect::<dashmap::DashSet<_>>()
};

Then iterated into a Vec in block_creator.rs:723 and serialized into the signed block:

justifications.iter().map(|j| j.clone()).collect()

Two layers of non-determinism: imbl::HashMap source iteration and DashSet collection. Same proposer, same DAG state, two creation attempts → different signed bytes. PR #488 widens this (Section 3 puts equivocators in the set, growing the variance from non-deterministic ordering).

Fix: sort the Vec by validator pk before passing to package_block:

let mut justifications_vec: Vec<Justification> = justifications.iter().map(|j| j.clone()).collect();
justifications_vec.sort_by(|a, b| a.validator.cmp(&b.validator));

The sort key is unique per justification by construction (one entry per validator).

4. Sort all_deploys Vec before passing to runtime

all_deploys: HashSet<Signed<DeployData>> is collected via into_iter().collect() in block_creator.rs:540-545, 669 and passed to the runtime, which processes deploys in input order and emits processed_deploys in the same order — into the signed block body.

The deploy-cap path (lines 247-298) DOES sort the deploys deterministically before selection, but only when valid_unique.len() > max_user_deploys. Under the cap (the common case), the HashSet is returned as-is.

PR #488 amplifies this: the recovery buffer adds entries to the merged HashSet, growing the set and making cap-hit more common.

Fix: apply the existing cap-path sort unconditionally, not just when over the cap:

let mut deploys_vec: Vec<Signed<DeployData>> = all_deploys.into_iter().collect();
deploys_vec.sort_by(|a, b| a.sig.cmp(&b.sig));

---

Low / Verify

5. Convert active_validators: Vec<Validator> to a HashSet

filter_slashable_invalid_messages (block_creator.rs:359-366) calls active_validators.contains(validator) — O(N) slice scan in a filter loop:

fn filter_slashable_invalid_messages(
    invalid_latest_messages: HashMap<Validator, BlockHash>,
    bonds_map: &HashMap<Validator, i64>,
    active_validators: &[Validator],   // slice
) -> Vec<(Validator, BlockHash)> {
    invalid_latest_messages
        .into_iter()
        .filter(|(validator, _)| {
            bonds_map.get(validator).copied().unwrap_or(0) > 0
                && active_validators.contains(validator)   // O(N)
        })
        .collect()
}

bonds_map is already a HashMap right next to it. Two different data structures for the same "is this validator in this set?" check. Negligible at typical sizes but inconsistent.

Fix: either build a HashSet<Validator> at the call site, or change OnChainCasperState::active_validators: Vec<Validator> (casper.rs:272) to a HashSet upstream.

6. Recovered-slash log: original_issuer is misleading after dedup

block_creator.rs:637-641:

tracing::info!(
    "Recovering merge-rejected slash: invalid_block={}, original_issuer={}",
    pretty_printer::PrettyPrinter::build_string_bytes(&rs.invalid_block_hash),
    hex::encode(&rs.issuer_public_key.bytes)
);

After b79bd5a3's dedup-by-invalid_block_hash collapses multiple rejected slashes for the same equivocator to one survivor, the survivor's issuer_public_key is whichever happened to be first in sorted order — not necessarily informative.

Fix: drop the field from the log, rename to one_of_original_issuers=, or extend RejectedSlash with Vec<PublicKey> to log all of them.

7. Verify two compute_parents_post_state calls hit the cache

block_creator::create calls compute_parents_post_state once at line 593 (to discover rejected slashes), then compute_deploys_checkpoint calls it again internally at line 666. The PR claims caching prevents recomputation. The cache key (per slash_recovery_spec.rs:331-338):

ParentsPostStateCacheKey {
    sorted_parent_hashes,
    snapshot_lfb_hash,
    disable_late_block_filtering,
}

None of these mutate during a single create() invocation, so the cache should hit. But it's load-bearing for perf and unverified.

Fix: add a tracing-target counter that increments on cache miss vs hit during compute_deploys_checkpoint calls inside create(). CI assertion that miss-count = 0 in steady state.

8. Sort rejected_slashes Vec at construction site

compute_parents_post_state (interpreter_util.rs:1230-1281) builds rejected_slashes: Vec<RejectedSlash> by iterating a HashMap<BlockHash, Vec<Bytes>>:

let mut by_block: HashMap<BlockHash, Vec<Bytes>> = HashMap::new();
for (sig, src_block) in &rejected_slash_pairs {
    by_block.entry(src_block.clone()).or_default().push(sig.clone());
}
let mut out = Vec::new();
for (src_block, _sigs) in by_block {       // ← non-deterministic iteration
    ...
    out.push(RejectedSlash { ... });
}
out

HashMap iteration is non-deterministic (per-process random hasher seed). The output Vec therefore has non-deterministic order. Currently saved by downstream filter_recoverable which sorts on invalid_block_hash — but the cache stores the unsorted Vec, so any future consumer of the cached value that doesn't re-sort inherits the non-determinism. Fragile coupling.

Fix: sort out before returning (or sort the unique source blocks before iteration).

9. Resolver bfs_finalized_window is DFS, not BFS (in two places)

Both bfs_finalized_window in deploy_finalization_status.rs:198-217 and descendants_within_scope in dag_merger.rs:39-68 use Vec::pop() which is LIFO/DFS:

let mut frontier: Vec<BlockHash> = vec![lfb_hash.clone()];
while let Some(candidate_hash) = frontier.pop() {  // pop() = LIFO = DFS
    ...
    for parent in &candidate_block.header.parents_hash_list {
        if !visited.contains(parent) {
            frontier.push(parent.clone());
        }
    }
}

Function names and comments say BFS. Two validators with the same DAG state visit blocks in the same DFS order, so today's behavior is consistent across validators.

The risk: a future reader who notices the doc/code mismatch and "fixes" it by switching to frontier.remove(0) or VecDeque::pop_front would silently change which block wins the latest_block_hash tiebreak in the resolver, and the traversal order in the merge — both observable.

Fix: rename to dfs_finalized_window / descendants_within_scope_dfs and document the determinism guarantee, OR convert to true BFS via VecDeque::pop_front. Pick one and lock it in.

10. Drop deploy_storage lock earlier in prepare_user_deploys

block_creator.rs:67-231:

async fn prepare_user_deploys(...) -> Result<...> {
    let mut deploy_storage_guard = deploy_storage.lock()    // ① held entire function (~lines 67-305)
        .map_err(|e| CasperError::LockError(e.to_string()))?;

    // ... read deploy_storage ...

    let recovered: HashSet<Signed<DeployData>> = {            // ② brief buffer acquisition
        let buffer_guard = rejected_deploy_buffer.lock()
            .map_err(|e| CasperError::LockError(e.to_string()))?;
        buffer_guard.read_all()?
    };  // buffer guard dropped here

    // ... ~130 lines of categorize/filter/sort work, deploy_storage_guard still held ...

    if !all_expired.is_empty() {
        let expired_list = ...;
        deploy_storage_guard.remove(expired_list.clone())?;

        let mut buffer_guard = rejected_deploy_buffer.lock() // ③ buffer re-acquisition
            .map_err(|e| CasperError::LockError(e.to_string()))?;
        buffer_guard.remove(expired_list)?;
    }

    // ... rest of function, deploy_storage_guard still held ...
}

Issues:

1. deploy_storage_guard is held for the entire function (~50-200 ms).
2. Buffer mutex acquired twice in one function (read in ②, write in ③). Between them, a validator-thread replay can mutate the buffer. Today benign because ③ removes only expired sigs which the validator-thread replay won't add — undocumented invariant.

Under contention: validator thread blocks on buffer; any other deploy-storage-touching code blocks on deploy_storage_guard for the full ~200 ms.

Fix: drop deploy_storage_guard after the read-and-classify phase, before deploy-cap selection. Combine buffer read+write into a single critical section, or document the "buffer can grow but not shrink for these sigs" invariant.

11. Buffer has no admission size cap

KeyValueRejectedDeployBuffer::add has no upper bound on entries. The expiry-purge fix (837fd392) closes the realistic unbounded-growth path (sigs that age out without finalization). With expiry purge in place, the remaining unbounded-growth scenario is pathological (many deploys rejected that never expire and never re-finalize).

Fix: defensive cap with oldest-first eviction or simple max-entries reject. Belt-and-suspenders, not a known bug.

---

Open questions to investigate

12. Buffer recovery when LFB advances past the source block

The catchup gate (Section 5) resolves to Finalized | Failed | Expired to skip re-proposing. But it doesn't explicitly handle the case where the source block of a buffered deploy has been pruned from history (pre-LFB). If this fires, the resolver might return pending_unknown and the deploy stays in the buffer indefinitely.

Action: trace the lookup_by_deploy_id path for the pruned-source case, document the behavior, add a test.

13. Buffer behavior on reorg

If sig X is rejected in branch B1 (orphaned) and finalized in branch B2 (canonical), record_directly_finalized purges based on B2's body.deploys and body.rejected_deploys. But if the rejection only happened on B1, the validator's local buffer may have admitted X on B1's merge and never see X cleaned up via B2's finalization.

Action: trace this case, verify the catchup gate handles it on the next propose attempt.

14. LMM-advance non-determinism for same-seq equivocations (Section 3)

When two equivocation blocks B1 and B2 (same sender, same seq num, different content) arrive in different orders on different nodes, the LMM advance rule (block.seq_num >= metadata.sequence_number) means whichever arrives second wins. Two nodes can have different invalid-latest entries for the same equivocator.

Verified safe in the review: all downstream uses are either set-based (justification_follows) or filter-out-invalid (fork choice, Estimator, slashing only checks "latest is invalid", not which specific block).

Action: add a comment to Section 3's safety argument explicitly noting the same-seq case so future readers don't assume LMM-content equality across validators is required.

15. compute_parents_post_state cache coherence vs. concurrent finalization

If finalization advances LFB between block_creator's first compute_parents_post_state call and the second one inside compute_deploys_checkpoint, the cache key includes snapshot_lfb_hash which is captured at snapshot time — so the lookup uses the snapshot's LFB, not a freshly-fetched one. Need to verify this end-to-end.

Action: trace the cache lookup site to confirm snapshot_lfb_hash is used (not dag.last_finalized_block()).

---

Style / cleanup

16. Stale references in committed comments

Four PR-introduced comments anchor on transient patch context. Mechanical cleanup.

16a. block-storage/src/rust/dag/block_dag_key_value_storage.rs:645-679 — Scala source-of-truth + pre-fix narration in safety-argument comment:

"This matches the Scala source-of-truth (BlockDagKeyValueStorage.scala, where newLatestMessages and shouldAddAsLatest never reference invalid)."
"The pre-fix if invalid { return empty } guard had no Scala counterpart and silently disabled the slashing pipeline (no slashes ever issued, equivocators never punished)."

The substantive safety argument (fork choice / slashing / justification_follows downstream consumers) should stay. Drop the Scala anchors and the pre-fix narration.

16b. casper/tests/batch2/validate_test.rs:974-981 — "Pre-fix / Post-fix" doc block:

"Pre-fix: the rejected_in_scope filter in repeat_deploy exempts the sig unconditionally → returns Valid → double-execution slips through. This test fails."
"Post-fix: the filter is gated on status != Finalized. ... This test passes."

Once merged, "pre-fix" describes a state that no longer exists, and "this test fails" becomes literally untrue. The four lines above this block already describe the test's intent. Delete the pre/post block.

16c. casper/tests/api/deploy_finalization_status_test.rs:493-500 — "With the buggy implementation / with the fix" framing:

"Tests the canonical-descendant invalidation gap: with the buggy implementation, is_in_main_chain(A, S) = true causes invalidation of the clean inclusion at A even though S itself is non-canonical. With the fix that additionally requires the rejection block to be on LFB's main-parent chain, this rejection is correctly ignored and the sig stays Finalized."

Rewrite to describe the rule (both is_in_main_chain(A, S) AND is_in_main_chain(S, C) must hold) rather than the patch history.

16d. Scala-style camelCase identifiers in comments. The Rust code uses snake_case; readers grep for deploy_lifespan and find nothing because the comment used deployLifespan.

File:line	Stale form	Rust form
casper/src/rust/api/deploy_finalization_status.rs:36	deployLifespan	deploy_lifespan
casper/src/rust/api/deploy_finalization_status.rs:395	deployLifespan	deploy_lifespan
casper/src/rust/storage/rnode_key_value_store_manager.rs:121	deployLifespan, currentBlock, maxBlockNum	snake_case
casper/src/rust/storage/rnode_key_value_store_manager.rs:122	earliestBlockNumber	earliest_block_number
casper/tests/blocks/block_creator_spec.rs:184	maxBlockNum	max_block_num
casper/tests/blocks/block_creator_spec.rs:296	maxBlockNum	max_block_num
casper/tests/blocks/block_creator_spec.rs:341	maxBlockNum, deployLifespan, earliestBlockNumber	snake_case

Note: pre-existing // See casper/src/main/scala/coop/rchain/... file headers at the top of files like block_creator.rs are pre-existing and out of scope here — that's a wider codebase pattern. The four items above are PR-#488-introduced.

17. Test setup boilerplate duplication across two new specs

dedup_orphan_recovery_spec.rs:62-117 and multi_validator_recovery_spec.rs:60-111 share ~50 lines of setup boilerplate (genesis, kvm, runtime_manager, block_store, dag_storage, buffer init, init_logger, now_millis closure, mk_snapshot closure). The differences between the two are minor (number of validators captured, one extra explanatory comment).

Fix: extract into casper/tests/util/recovery_test_setup.rs returning a builder:

pub struct RecoveryTestSetup {
    pub kvm: Box<dyn KeyValueStoreManager>,
    pub block_store: KeyValueBlockStore,
    pub dag_storage: BlockDagKeyValueStorage,
    pub runtime_manager: RuntimeManager,
    pub rejected_deploy_buffer: Arc<Mutex<KeyValueRejectedDeployBuffer>>,
    pub genesis_context: GenesisContext,
    pub now_millis: fn() -> i64,
}
pub async fn setup_recovery_test() -> RecoveryTestSetup { ... }

Each spec then opens with two lines instead of fifty.

18. MutexExt trait or From<PoisonError> for lock-error idiom

.map_err(|e| CasperError::LockError(e.to_string()))? repeats 4× in PR-added lines of block_creator.rs (more in surrounding existing code).

Fix: add a tiny helper or From<PoisonError<...>> for CasperError so ? works directly:

trait MutexExt<T> {
    fn lock_for_casper(&self) -> Result<MutexGuard<'_, T>, CasperError>;
}
impl<T> MutexExt<T> for Mutex<T> { ... }

19. as_ref().map(...).unwrap_or(true) → map_or(true, ...)

4× in resolver body-scan loop, e.g.:

state.clean_finalized_event
    .as_ref()
    .map(|(h, _)| height > *h)
    .unwrap_or(true)

Option::map_or covers exactly this case:

state.clean_finalized_event
    .map_or(true, |(h, _)| height > h)

File: casper/src/rust/api/deploy_finalization_status.rs.

20. Replace active_sigs HashSet with direct per_sig.get_mut

deploy_finalization_status.rs:206-208:

let active_sigs: HashSet<Bytes> = per_sig.keys().cloned().collect();

per_sig is a HashMap<Bytes, ResolverState> — per_sig.contains_key(sig) is already O(1) without the duplicate set. The whole active_sigs allocation can go away — replace if active_sigs.contains(&pd.deploy.sig) with a direct if let Some(state) = per_sig.get_mut(&pd.deploy.sig) { ... }, which combines the membership check and the mutable lookup the next line is doing anyway.

This also removes 3 of the 4 .expect("active_sigs and per_sig must agree on key set") calls.

21. Convert dag_merger.rs cache .unwrap() to .expect("just-inserted")

Three cache.get(&addr).unwrap() calls in dag_merger.rs:361, 398, 399 on or_insert_with lookups. Safe today (entry exists by construction), but a bare .unwrap() panics with a generic "called Option::unwrap on a None value".

Fix: convert to .expect("just-inserted in get_chain_derived") to make the implicit invariant visible if a future refactor breaks it.

22. Add descendants_within_scope size metric

dag_merger.rs:39-68 walks DAG descendants restricted to scope. Worth surfacing scope size via tracing::debug!(scope_size=...) to make catchup or attack scenarios visible.

23. Buffer LMDB env shares 1 GB sizing with deploy_storage

rnode_key_value_store_manager.rs:121-127 registers the new buffer LMDB env using deploy_storage_env_config() (1 GB reservation). Practical disk usage will be much smaller (the buffer holds at most O(rejected_per_merge × deploy_lifespan) entries), but total LMDB reservation grows by 1 GB on disk on first launch.

Action: worth a note in operator-facing release notes if disk planning is tight. No code fix needed.

24. PR description vs. reality drift

PR description still says "Two deferred code-level tests (concurrent_registry_inserts_both_eventually_included, phase_c_dedup_orphans_loser_block_registry_writes) — rewritten but pending cleanup before commit." Neither test name appears in the codebase. Strategy actually changed: commit 88595e74 removed the existing ignored concurrent_registry_inserts_should_not_conflict test, and 3 commits (cf5d013d, bedaa83f, 88595e74) cover the review gaps under different names.

Action: update PR description to remove the deferred-tests line and describe the actual coverage delta.

25. thread_rng() non-determinism in RSpace::shuffle_with_index

rspace++/src/rspace/rspace.rs:1124-1133. Called from fetch_channel_to_index_data:654, extract_produce_candidate:727, fetch_matching_data:743. The PR description flagged this as the suspected cause of test_contract_lifecycle.py non-determinism on exploratory queries against recovery-merged state. Local fix exists (remove thread_rng()) but parity with Scala's SpaceMatcher.fetchChannelToIndexedData not yet verified.

Why it may be invisible in normal block flow: block replay rigging (Blake2b512Random + produce_counter + event-log replay) constrains matches by event log, masking shuffle non-determinism. Exploratory deploy is read-only and bypasses replay rigging.

Action: verify Scala parity before deciding fix direction. Tracked separately from this issue if it's part of a wider rspace investigation; included here for completeness.

26. Commit-message PR references

Two commits in this PR's history reference review item numbers in their commit messages (282f31fa: "Address PR #488 review request to document why..."; 55cbbb06: "Address remaining concern from PR #488 review #5..."). Code comments were cleaned up via 35b918e4 and 19b0297f; commit-message references don't survive squash-merge so they're harmless if the PR is squashed. Worth a one-line note if the PR is not squashed.

---

Minor recommendations from per-section review

27. Comment clarity on dag_merger.rs scope: None vs scope: Some(...)

dag_merger.rs lines 92-107 and 112-123 distinguish "legacy no-scope path (non-deterministic)" from "production multi-parent merge path (deterministic, scope-bounded)". The comment "non_finalized_blocks (non-deterministic, but no scope means this is not a multi-parent merge validation)" is alarming on first read — should clarify who actually calls without scope (proposer-only? legacy compat?).

28. disable_late_block_filtering flag interaction in dag_merger.rs

When scope.is_some(), late blocks are forced empty regardless of the flag. The flag's role only applies to the legacy scope: None path. Add a code comment noting this asymmetry.

29. Variable name first_seen_block_hash in resolver is misleading

lookup_by_deploy_id returns the last-inserted block from the deploy_index, not the first-seen. Doesn't affect correctness because valid_after_block_number is identical across blocks containing the same sig (it's part of the signed payload). Rename to last_indexed_block_hash or similar.

30. latest_block_hash resolver doc says "canonical block" but BFS walks all parents

The doc comment claims "highest-block-number canonical block" but bfs_finalized_window enqueues every parent in parents_hash_list, so the returned hash can be on a non-main-parent ancestor. Either tighten the BFS to main-parent-only or update the doc to say "in the finalized-ancestor closure of LFB."

31. Inconsistent definitions of "canonical" within the resolver

Canonical-descendant invalidation uses main-parent-chain checks (is_in_main_chain). latest_block_hash uses any-parent BFS. Two definitions, depending on what's being checked. Worth a comment explaining why each definition is the right one for its context.

32. Failed sigs are exempted from repeat_deploy (design choice — confirm)

The state machine in repeat_deploy treats Failed the same as Pending/Expired for exemption. Failed sigs are unlikely to succeed on re-execution (failures are usually deterministic — out of phlo, contract error). Allowing re-proposal is wasted work but not unsafe. Confirm this is the intended behavior; could be tightened to "exempt only Pending/Expired" if Failed re-execution is undesired.

33. Block-creator vs. validator asymmetry on Finalized gate (defense-in-depth — comment)

Block creator's collect_self_chain_deploy_sigs filter exempts rejected_in_scope unconditionally; validator's repeat_deploy exempts only non-Finalized. The asymmetry is defense-in-depth (proposer permissive, validator strict), but a buggy buffer admission could produce a block the validator will reject. Worth a code comment at the block creator filter explaining the deliberate asymmetry.

34. runtime_manager_test.rs size

File grew to ~2700+ lines and now contains a mix of original test scaffolding, general-purpose regression tests, and PR-#488-specific bug-demo tests. The author moved recovery-cycle / dedup-orphan / multi-validator / slash tests into casper/tests/batch2/*_spec.rs (good pattern) but left stale_diff_application_corrupts_merged_state and bridge_query_survives_multi_parent_merge in runtime_manager_test.rs.

Action: extract these into casper/tests/batch2/merge_corruption_spec.rs or similar.

35. compute_state_then_compute_bonds_should_be_replayable_after_all is #[ignore]

Pre-existing flakiness (added 2025-05-20, before PR #488). But this PR's changes affect deploy execution and replay, so flakiness may be related — author should run the ignored test with --include-ignored to see if PR #488 fixes or worsens it.

36. Cache invalidation question for slash recovery

block_creator::create calls compute_parents_post_state once before system-deploy construction, then compute_deploys_checkpoint later. If the cache is invalidated between those calls (DAG generation advances during block creation), the merge runs twice and the rejected_slashes from the first call may not match what the second call sees. Cache key includes (sorted_parent_hashes, snapshot_lfb_hash, disable_late_block_filtering) — none of which include DAG generation, so concurrent finalization shouldn't invalidate. Worth confirming end-to-end.

---

Summary

Code-level items: 36 total.

Category	Count
Medium severity	2
Low/Medium (block-byte determinism)	2
Low / Verify	7
Open questions	4
Style / cleanup	11
Minor recommendations	10

None merge-blocking on their own. Items 1 and 2 are the highest-value follow-ups (real maintenance / perf cost). Items 3, 4 close a coherent determinism story. Items 12-15 are open questions that warrant a trace-level investigation before they bite.

Test coverage gaps are tracked in a follow-up comment.

---

Co-Authored-By: Claude noreply@anthropic.com

[linear]

F1R-150

[spreston8]

Test coverage gaps

Compiled from the per-section test-scenario lists in the multi-pass review. Grouped by which PR section they cover. Several of these were flagged in the PR's own header comments as deferred — they remain real and worth tracking explicitly.

---

Critical / high-value gaps

Block-creation byte-determinism regression test

There is no test that creates a block twice from the same setup and asserts byte-equality. Such a test would fail today (per items 3 and 4 in the main issue) and would document the determinism contract going forward.

Sketch:

#[tokio::test(...)]
async fn block_creation_is_byte_deterministic() {
    let setup = setup_recovery_test().await;
    let casper_snapshot = make_snapshot(...);

    let block_a = block_creator::create(&casper_snapshot, ...).await.unwrap();
    let block_b = block_creator::create(&casper_snapshot, ...).await.unwrap();

    assert_eq!(block_a.justifications, block_b.justifications,
        "justifications order non-deterministic");
    assert_eq!(
        block_a.body.deploys.iter().map(|d| d.deploy.sig.clone()).collect::<Vec<_>>(),
        block_b.body.deploys.iter().map(|d| d.deploy.sig.clone()).collect::<Vec<_>>(),
        "processed_deploys order non-deterministic");
    assert_eq!(block_a.block_hash, block_b.block_hash,
        "signed block bytes non-deterministic");
}

Multi-equivocator slashing in same block (slash seed coverage extension)

The slash-seed fix 3a6baf15 includes a unit test asserting distinct seeds for distinct invalid_block_hash values. An end-to-end integration test that actually puts N=2 distinct equivocators in one block and asserts replay determinism would extend that coverage one layer up.

Specifically:

• 4-validator network, V1 and V2 both equivocate, V3 proposes a block that slashes both.
• Assert PoS state shows both slashed.
• Assert replay produces matching state hash.

Companion scenarios:

• One own + one recovered for distinct equivocators (V3 has E1 in own invalid_latest_messages; merge surfaces a RejectedSlash for E2 — both must land with distinct seeds).
• Recovered-only multi-slash (V3 has zero own slashes; merge surfaces RejectedSlashes for E1, E2, E3 — all three should land and replay deterministically).

Cost-starvation simulation against slashing

The attack vector Section 4 was designed to close. Attacker submits cheap conflicts to keep slash chains merge-rejected. Verify slash still lands within bounded delay even under sustained conflict pressure.

The empty-block-skip fix a1cd5233 partially addresses the liveness side (recovery now fires on otherwise-empty blocks). End-to-end attack simulation would close the loop.

Buffer recovery after crash with in-flight rejections

Validator crashes after admitting a sig to the buffer but before propagating the rejection to peers. On restart, buffer has the sig but the local DAG may not have the rejection event. The catchup gate should handle this via state != Finalized, but the path wasn't traced end-to-end.

Variants:

• Buffer survives restart with finalized sig: rejection finalizes via a later block, restart, verify the sig isn't re-admitted from historical replay (catchup gate works).
• Buffer survives restart with pending sig: rejection isn't yet finalized, verify the buffer entry persists across restart and gets re-proposed.

Pre-LFB source-block pruning behavior

When LFB advances past the source block of a buffered deploy, the catchup gate may return pending_unknown and the deploy stays in the buffer indefinitely. Need an explicit test for this case.

---

Section 1 — DAG-descendant rejection expansion

• Stale dependency through system contract: regression test that BC depends on BA only via a registry write/read pair (no direct event-log channel overlap), to confirm the conservative rejection catches what event-log analysis cannot. The existing stale_diff_application_corrupts_merged_state uses bridge.rho — verify the bug actually goes through the system contract path, not a direct channel overlap.
• Three-deep descendant chain: BA rejected, BC is descendant of BA, BD is descendant of BC. Confirm BD is also rejected (BFS reaches it).
• Diamond merge: BA rejected, BC is descendant of both BA and BB (where BB is not rejected). Confirm BC is rejected because any parent rejection should taint it.
• Rejected chain whose source_block_hash is the LFB itself: should never happen by construction (LFB is not in actual_blocks), but worth an assertion or test.

Section 2 — Per-node rejected-deploy buffer

• Buffer doesn't grow with expired sigs: submit deploy with short expiration, get it rejected, let it expire without finalization. Verify buffer is purged. (Should pass after 837fd392.)
• Buffer + merge convergence: two-node setup where node A buffers sig X but node B doesn't. Both propose at the same height. Verify merge converges (one node's proposal wins, the other's gets rejected and re-buffered, eventual consistency).
• Buffer interaction with reorg: sig X rejected in branch B1, finalized in branch B2 (different fork). When the validator follows B2 and B1 becomes orphaned, verify buffer state is correct.
• Replay-time buffer population: validate that a non-proposing validator's buffer also fills when it validates blocks containing rejections.
• Catchup gate skips already-finalized sig: construct a DAG where sig X was rejected in block B (height N) and finalized in block C (height N+5). Restart a node, replay historical merges. Verify the catchup gate prevents re-admission of X on the replay of B's merge.

Section 3 — Invalid-block DAG state

• Equivocation-order nondeterminism: two-node setup where equivocator submits B1 and B2 (same sender, same seq num, different content). Node A receives B1 then B2; node B receives B2 then B1. Verify both nodes (a) have an invalid-latest entry for the equivocator, (b) issue a slash, (c) produce blocks whose justifications pass justification_follows on the other node, (d) reach finalization on the same canonical block.
• Equivocation block with lower seq num doesn't replace LMM: insert valid block with seq=5, then invalid block from same sender with seq=4. Verify LMM stays at the seq=5 valid block.
• Multiple equivocators: three validators all equivocate at the same height. Verify all three appear in invalid_latest_messages and all three trigger slashes.
• Equivocator's invalid block as justification target: block creator includes the equivocator's invalid block hash in justifications. Verify validation passes (set membership) and the slash deploy is generated in a downstream block.

Section 4 — Slash recovery via block-creator dedup

• Two-honest-validator slash convergence (high bond floor): 3-validator network with bond_floor > 0. V1 and V2 both slash equivocator E. Merge proposer V3 also has E in invalid_latest_messages. Verify the merge block contains at most one SlashDeploy for E. (Should pass after b79bd5a3 + e77a70da.)
• Same proposer, slash chain rejected: V1 proposes a block with slash for E. V1's chain is merge-rejected. V1 is also the merge proposer. Verify V1 doesn't double-slash (own-detection + recovered).
• Bond-floor-aware slash recovery: prepare_slashing_deploys should arguably check parent-state bond status, not just bonds_map > 0. Test that an equivocator already at bond floor in parent state doesn't trigger redundant own-detection slashes. (Mostly addressed by e77a70da's active_validators check; a regression test would lock it in.)
• Cache invalidation during block creation: simulate finalization advancing between compute_parents_post_state (rejected_slashes lookup) and compute_deploys_checkpoint (cache hit). Verify behavior — does the cache return stale rejected_slashes? Does the block fail validation?
• Cross-merge slash consistency: two validators independently building merge blocks at the same height, both observing the same merge-rejected slash. Verify both produce blocks where E is at bond floor in the post-state, regardless of which one wins fork choice.

Section 5 — deployFinalizationStatus API

• Recovery cycle: clean canonical inclusion, rejection in non-canonical sibling: deploy is in canonical block A, then re-proposed in sibling fork B' (non-canonical). B' has the deploy in body.rejected_deploys. Verify resolver returns Finalized, not Pending.
• Recovery cycle: clean inclusion, then canonical rejection: deploy in canonical block A, canonical descendant B has the deploy in body.rejected_deploys. Verify resolver returns Pending.
• Same-block clean + reject: construct a block where the same sig appears in both body.deploys (cleanly) and body.rejected_deploys. Verify resolver returns Finalized (the clean_block != reject_block guard fires).
• Sig in two finalized blocks: included in finalized A, also in finalized B (B is canonical descendant of A). Verify latest_block_hash = B and state = Finalized.
• Sig only in non-finalized block: outside the LFB-deploy_lifespan window. Verify latest_block_hash falls back to last-indexed and state = Pending.
• Sig past lifespan, never finalized: valid_after_block_number=10, deploy_lifespan=100, tip=200. Verify Expired.
• Sig past lifespan, finalized: same setup but a finalized block contains the sig. Verify Finalized (expiry suppressed correctly).
• resolve_batch correctness for mixed-corruption batch: 3 sigs where one is unknown, one is healthy, one is corrupted. Verify the unknown returns pending_unknown, the healthy returns its actual state, the corrupted causes the entire batch to return Err.

Section 6 — repeat_deploy validation exemption

• Failed-sig re-proposal: deploy fails (insufficient phlo), gets included as is_failed=true, then ends up in rejected_in_scope via sibling-fork rejection. Verify the validator allows re-proposal AND verify the re-execution behaves sensibly (presumably fails again).
• Resolver error during validation: simulate resolve returning Err (LMDB transient failure). Verify the sig is kept in the check set and the block either passes (no real duplicate) or fails with InvalidRepeatDeploy (real duplicate). Confirms conservative-fail is implemented. (Now extra-relevant after dc217d5f's error bubbling.)
• Concurrent buffer admission and validation race: a block proposer admits a sig at time T; another node validates a recovery-path block at time T+ε. DAG state advances between calls. Verify no false positives or false negatives in the resolver's verdict.
• Performance benchmark: per-sig vs resolve_batch in repeat_deploy: 100 deploys with 50 in rejected_in_scope, deploy_lifespan = 100. Compare wall time. (Will inform main-issue item 2.)

Section 7 — Test infrastructure

• System-level integration test spanning Sections 1-6: rejection expansion → buffer admit → invalid LMM advance → slash recovery → finalization status query → repeat-deploy exemption all in one test. Each section has unit/spec coverage; a combined integration test would prove the pieces work together.
• Random multi-parent DAG construction with random conflict patterns: assert state-hash determinism across validators. proptest-based fuzzing.
• Buffer + reorg fuzz: random reorgs with buffer entries; verify buffer state remains consistent.
• Re-run compute_state_then_compute_bonds_should_be_replayable_after_all with --include-ignored to see if this PR affects pre-existing flakiness (linked to main-issue item 35).

---

Summary

~30 test scenarios across critical/high-value gaps, per-section coverage, and stress/fuzz. The 5 critical/high-value tests at the top close the most impactful gaps. The per-section lists exist for completeness — many are nice-to-have rather than must-have, but they document what coverage the team should aim for as the recovery surface matures.

---

Co-Authored-By: Claude noreply@anthropic.com

[spreston8]

This issue has been migrated to F1R3FLY-io/f1r3node-rust#44 as part of the deprecation of this repository. Please continue tracking the work there.