Skip to content

fix(resharding): use the boundary block to resolve the child shard layout#16068

Open
Wiezzel wants to merge 3 commits into
masterfrom
wiezzel/resharding/child-layout-reference-block
Open

fix(resharding): use the boundary block to resolve the child shard layout#16068
Wiezzel wants to merge 3 commits into
masterfrom
wiezzel/resharding/child-layout-reference-block

Conversation

@Wiezzel

@Wiezzel Wiezzel commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

At an E → E+1 resharding split, the block producer and stateless validators resolved the child shard layout from different reference blocks. The producer uses the boundary block (last block of epoch E), giving the new post-split layout; the validator's resharding-transition arm used the witness's main-transition block, i.e. the split shard's last new chunk. These agree only while that last new chunk lies in E. If the shard being split produced no chunk anywhere in E (a full-epoch, normally recoverable chunk outage), its last new chunk falls in an earlier epoch, so the validator resolved to the old layout, the child's brand-new shard id was absent from it, and get_child_congestion_infofinalize_allowed_shard failed with InvalidShardId. Every validator then deterministically rejected the child shard's first chunk, permanently halting the resharded shards with no self-recovery.

The fix resolves both epochs (and thus the child shard layout) from the resharding transition's own block (transition.block_hash, the boundary block) instead of the main-transition block, matching exactly what the producer does. This is validator-side only; it changes no committed protocol state and needs no protocol-version gate.

Also adds a test-loop regression test that drives a resharding split while the shard being split misses all of its chunks in the last old-layout epoch and asserts the children still produce endorsed chunks afterward (fails before this fix, passes after), plus a supporting DropCondition::ChunksForShardsInEpochs that silences a shard for whole specific epochs.

@Wiezzel
Wiezzel requested a review from wacban July 20, 2026 14:36
@Wiezzel
Wiezzel marked this pull request as ready for review July 20, 2026 14:37
@Wiezzel
Wiezzel requested a review from a team as a code owner July 20, 2026 14:37
@github-actions

Copy link
Copy Markdown

Pull request overview

Fixes a validator-side layout-resolution bug during resharding chunk validation. In the ImplicitTransitionParams::Resharding arm of validate_chunk_state_witness_impl, the code resolved parent/child epoch IDs (and hence shard layouts) from block_hash (the main transition's last new chunk of the parent). If the parent shard produced no new chunk in the last old-layout epoch E, that block lies in an earlier epoch, so both get_epoch_id and get_next_epoch_id returned the pre-split layout, the child shard id was absent, and get_child_congestion_infofinalize_allowed_shard failed with InvalidShardId. The fix switches to transition.block_hash (the resharding transition's own block = the boundary block, last block of E), matching what ReshardingManager::process_memtrie_resharding_storage_update uses on the producer side. Also adds a DropCondition::ChunksForShardsInEpochs variant and a regression test that reproduces the halt on unfixed code.

Changes:

  • chain/chain/src/stateless_validation/chunk_validation.rs: resolve epoch_id / child_epoch_id from transition.block_hash instead of the main-transition block_hash.
  • test-loop-tests/src/setup/drop_condition.rs: new DropCondition::ChunksForShardsInEpochs { shards, epoch_heights } variant keyed on absolute epoch height (unlike ChunksProducedByHeight).
  • test-loop-tests/src/tests/resharding_missing_chunks.rs: new regression test slow_test_resharding_parent_missing_full_epoch_before_split that silences the parent through the split and asserts children still produce endorsed chunks; includes a precondition check that fails loudly if the trigger isn't reproduced.
  • test-loop-tests/src/tests/mod.rs: register the new test module.

Reviewed changes

Per-file summary
File Description
chain/chain/src/stateless_validation/chunk_validation.rs Use transition.block_hash (boundary block) instead of the main-transition block_hash to resolve parent/child epochs during resharding-transition validation.
test-loop-tests/src/setup/drop_condition.rs Add ChunksForShardsInEpochs drop condition — drops every chunk for the listed shards during listed absolute epoch heights.
test-loop-tests/src/tests/resharding_missing_chunks.rs New regression test covering the "parent silent for an entire epoch before split" trigger.
test-loop-tests/src/tests/mod.rs Register new test module.

I verified the fix aligns with the producer path: in ReshardingManager::process_memtrie_resharding_storage_update, parent_epoch_id = block.header().epoch_id() and child_epoch_id = get_next_epoch_id(&block_hash) are computed against the boundary block. In create_state_witness / collect_state_transition_data, the ChunkStateTransition created when the shard id changes stores the boundary block hash, so transition.block_hash on the validator side is the same block that the producer used. The two paths now agree regardless of when the parent last produced.

Findings

No blocking issues found. Two very minor nits (non-blocking):

  • chain/chain/src/stateless_validation/chunk_validation.rs:717 — the comment references ReshardingManager::process_...; consider spelling out process_memtrie_resharding_storage_update so future greps land on it. Not required.
  • test-loop-tests/src/setup/drop_condition.rs:165get_epoch_height_from_prev_block(...).unwrap() inside the closure will panic (rather than drop the chunk) if the epoch manager can't resolve prev_block_hash for some racy testing scenario. Matches the style of nearby closures (is_chunk_validated_by also unwraps), so acceptable for a test-only path.

The long block comment at chunk_validation.rs:711 is appropriate here — the WHY is genuinely non-obvious and the failure mode (permanent halt of a resharded shard) warrants an in-code explanation. Similarly the test-file header comment is justified as it documents the specific race/edge case the test targets.

✅ Approved

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.34%. Comparing base (55a0234) to head (62d0261).

Files with missing lines Patch % Lines
...chain/src/stateless_validation/chunk_validation.rs 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16068      +/-   ##
==========================================
+ Coverage   73.31%   73.34%   +0.03%     
==========================================
  Files         857      857              
  Lines      189233   189264      +31     
  Branches   189233   189264      +31     
==========================================
+ Hits       138731   138812      +81     
+ Misses      46068    46020      -48     
+ Partials     4434     4432       -2     
Flag Coverage Δ
pytests-nightly 1.22% <0.00%> (ø)
unittests 69.98% <93.93%> (+0.01%) ⬆️
unittests-nightly 69.99% <93.93%> (+0.01%) ⬆️
unittests-spice 65.58% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant