fix: avoid block_discovery panic after block validation rejection by roberts-pumpurs · Pull Request #1235 · Irys-xyz/irys

roberts-pumpurs · 2026-03-20T14:27:39Z

Bug: heavy_test_block_with_incorrect_pd_base_fee_gets_rejected (on the feat/pd branch) exposed a race where a bad parent block was correctly rejected for a PdBaseFeeUpdate shadow transaction mismatch, but a descendant was still processed afterward.

Error: block_discovery panicked with "Parent block ... should be in the block tree!" after the rejected parent had already been removed, which cascaded into service shutdown and SendError failures in the test harness.`

Fix: return PreviousBlockNotFound instead of panicking when the parent disappears during block discovery, and make block_tree invalid-result cleanup idempotent when a descendant was already removed as part of ancestor cleanup.

Checklist

Tests have been added/updated for the changes.
Documentation has been updated for the changes (if applicable).
The code follows Rust's style guidelines.

Summary by CodeRabbit

Bug Fixes
- Graceful handling when a block's parent is missing to avoid crashes; now returns a recoverable error and logs a warning.
- Improved validation cleanup to only remove blocks that exist and log ignored validations when blocks are already gone.
Tests
- Updated mempool test to wait for ingress proof and confirm a transaction in the raw mempool before continuing mining, for more realistic behavior.

Bug: heavy_test_block_with_incorrect_pd_base_fee_gets_rejected exposed a race where a bad parent block was correctly rejected for a PdBaseFeeUpdate shadow transaction mismatch, but a descendant was still processed afterward. Error: block_discovery panicked with "Parent block ... should be in the block tree!" after the rejected parent had already been removed, which cascaded into service shutdown and SendError failures in the test harness.` Fix: return PreviousBlockNotFound instead of panicking when the parent disappears during block discovery, and make block_tree invalid-result cleanup idempotent when a descendant was already removed as part of ancestor cleanup.

coderabbitai · 2026-03-20T14:27:46Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7f0c4121-b2ce-4a43-8af8-8b88b6ab621e

📥 Commits

Reviewing files that changed from the base of the PR and between ba1c2c7 and a81f7fc.

📒 Files selected for processing (1)

crates/actors/src/block_discovery.rs

📝 Walkthrough

Walkthrough

Replaces panics with Result-based errors in block discovery, tightens cache-removal logic after validation, and stages a mempool test's mining sequence to wait for ingress proofs and raw-mempool confirmation before full migration mining. (47 words)

Changes

Cohort / File(s)	Summary
Block Discovery Error Handling `crates/actors/src/block_discovery.rs`	Replaced `unwrap_or_else`/`panic!` parent lookups with `let Some(...) else` guards. Logs `warn!` and returns `BlockDiscoveryError::PreviousBlockNotFound` when parent block is missing. Simplified parent snapshot binding by removing `parent_meta` tuple usage.
Block Validation Cache Management `crates/actors/src/block_tree_service.rs`	In `on_block_validation_finished`, store optional height as `maybe_height`, set event `height` from it, and only call `cache.remove_block()` when block exists; emits debug log if block already removed instead of attempting removal.
Mempool Test Flow `crates/chain-tests/src/multi_node/mempool_tests.rs`	`heavy_pending_chunks_test` now waits for ingress proofs, mines one block, waits for raw-mempool confirmation at that height, then mines remaining blocks to trigger migration, replacing the previous unconditional multi-block mining step.

Sequence Diagram(s)

(silently omitted — changes are refactoring/bugfixes and a test sequencing change that do not introduce a new multi-component control flow requiring diagrams)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

feat: better sync crash heuristics #1132: Overlaps with modifications to on_block_validation_finished in crates/actors/src/block_tree_service.rs.
fix: exclude confirmed txs from submit selection #1224: Adds raw-mempool helpers (e.g., wait_for_tx_confirmed_in_raw_mempool) referenced by the updated mempool test.

Suggested reviewers

antouhou
DanMacDonald

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title directly captures the main fix: avoiding a block_discovery panic when parent block validation is rejected, which aligns with the core issue and changes in block_discovery.rs.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch rob/flake-fix

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `ba1c2c7`	Previous: `9725fe9`	Ratio
`parallel_verification/testing`	`3.659624` ms/iter (`± 4.064656`)	`2.54714` ms/iter (`± 0.211408`)	`1.44`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions · 2026-03-20T14:46:46Z

Benchmark results: https://irys-xyz.github.io/irys/dev/bench/1235%2Fmerge/index.html

JesseTheRobot · 2026-03-23T09:59:54Z

@coderabbitai full review

JesseTheRobot

LGTM, some nonblocking nits

roberts-pumpurs changed the title ~~fix: avoid block_discovery panic after pd base fee shadow tx rejection~~ fix: avoid block_discovery panic after block validation rejection Mar 20, 2026

roberts-pumpurs self-assigned this Mar 20, 2026

github-actions Bot reviewed Mar 20, 2026

View reviewed changes

roberts-pumpurs and others added 2 commits March 23, 2026 10:11

Merge branch 'master' into rob/flake-fix

9300e47

flake fix

ba1c2c7

roberts-pumpurs marked this pull request as ready for review March 23, 2026 09:53

roberts-pumpurs requested review from JesseTheRobot and glottologist March 23, 2026 09:53

JesseTheRobot approved these changes Mar 23, 2026

View reviewed changes

Comment thread crates/actors/src/block_discovery.rs

Comment thread crates/actors/src/block_discovery.rs

comments

a81f7fc

roberts-pumpurs merged commit 36469cb into master Mar 23, 2026
18 checks passed

roberts-pumpurs deleted the rob/flake-fix branch March 23, 2026 10:29

This was referenced May 13, 2026

feat: preval perf #1414

Merged

fix: validation error disambiguation #1416

Merged

fix: cached data roots block set #1421

Merged

coderabbitai Bot mentioned this pull request Jun 2, 2026

Block producer re-includes already-confirmed txs under fast mining (caught by prevalidation DuplicateTransaction) #1439

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid block_discovery panic after block validation rejection#1235

fix: avoid block_discovery panic after block validation rejection#1235
roberts-pumpurs merged 4 commits into
masterfrom
rob/flake-fix

roberts-pumpurs commented Mar 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 20, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

github-actions Bot left a comment •

edited

Loading

Uh oh!

github-actions Bot commented Mar 20, 2026 •

edited

Loading

Uh oh!

JesseTheRobot commented Mar 23, 2026

Uh oh!

JesseTheRobot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

roberts-pumpurs commented Mar 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

github-actions Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

github-actions Bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JesseTheRobot commented Mar 23, 2026

Uh oh!

JesseTheRobot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

roberts-pumpurs commented Mar 20, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 20, 2026 •

edited

Loading

github-actions Bot left a comment •

edited

Loading

github-actions Bot commented Mar 20, 2026 •

edited

Loading