docs(block_producer): correct test-budget mechanism narrative#1438
Merged
Conversation
Follow-up to #1436. The fix landed correct, but its comments, test names, and description attributed the duplicate-height production to "two valid solutions built on the same parent before the first was validated — a sibling." Tracing the CI logs shows the real mechanism: the first height-3 block was *published* (consuming a budget slot), then failed prevalidation with `DuplicateTransaction` because it re-included a transaction already confirmed in a recent block (the producer's mempool view hadn't caught up under fast mining), and was rebuilt at the same height. The parent-selection tracker is not at fault — it correctly reverts to the prior tip when a published block is rejected. Comments and test names only; no behaviour change.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates documentation comments and test descriptions in the block producer to clarify that the test-mode block production budget counts canonical height increments and does not consume slots for same-height block replacements (rebuilds after prevalidation rejection). All changes are to documentation; no runtime logic or public API signatures were modified. ChangesTest block production budget documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/actors/src/block_producer.rs (1)
1962-1993: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor terminology note in assertion message.
Line 1989's assertion message uses "despite the sibling," while the test description and other documentation refer to this as a "replacement" or "rejected-then-replaced block." While the intent is clear in context, "despite the same-height replacement" would align better with the updated narrative throughout this PR.
Suggested terminology alignment
assert_eq!( max_height_reached_while_budget_positive, 7, - "must reach the target height (1 + 6) despite the sibling" + "must reach the target height (1 + 6) despite the same-height replacement" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/actors/src/block_producer.rs` around lines 1962 - 1993, Update the assertion message in the test rejected_and_replaced_block_still_reaches_target_height to use the preferred terminology; replace "despite the sibling" with "despite the same-height replacement" (the assertion that checks max_height_reached_while_budget_positive equals 7) so the message aligns with the test description and PR narrative.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/actors/src/block_producer.rs`:
- Around line 410-418: Update the inline comment in block_producer.rs that
currently says "sibling that was published... but then rejected at
prevalidation" to use the consistent "replacement" or "same-height replacement"
terminology used elsewhere (e.g., lines referenced as 157, 164, 857, 947, 1958)
so the narrative matches other comments; edit the phrase in the comment block
beginning "The test budget counts canonical height increments..." to replace
"sibling" with "replacement" and ensure the sentence still reads clearly about a
published block failing its own prevalidation.
---
Outside diff comments:
In `@crates/actors/src/block_producer.rs`:
- Around line 1962-1993: Update the assertion message in the test
rejected_and_replaced_block_still_reaches_target_height to use the preferred
terminology; replace "despite the sibling" with "despite the same-height
replacement" (the assertion that checks max_height_reached_while_budget_positive
equals 7) so the message aligns with the test description and PR narrative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f7a2e56-2dcc-45f6-a711-9829738cbcdc
📒 Files selected for processing (1)
crates/actors/src/block_producer.rs
Contributor
…get comment Address review on #1438: the inline comment in the SolutionFound handler still called the rejected block a 'sibling', inconsistent with the 'same-height replacement' wording used in the surrounding doc comments and tests. Reword it (and the lone remaining 'sibling' in a regression-test assertion) so the narrative is consistent; still describes a published block failing its own prevalidation. Comments/test-message only; no behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1436 (comments + test names only — no behaviour change).
Why
#1436 fixed a real CI flake (
heavy3_pending_chunks_testhanging because a duplicate-height production consumed two test-budget slots), and the fix is correct. But its explanation was wrong: it attributed the duplicate-height block to "two valid solutions built on the same parent before the first was validated — a sibling."Tracing the actual CI logs to the production events tells a different story:
The first height-3 block was published (consuming a budget slot), then failed its own prevalidation with
DuplicateTransaction— it re-included a transaction already confirmed in a recent block, because the producer's mempool view hadn't caught up under fast mining. It was rebuilt at the same height, consuming a second slot. The parent-selection tracker (BlockValidationTracker) is not at fault: it correctly reverts to the prior tip when a published block is rejected.The height-counting fix from #1436 handles this correctly regardless of cause, so this PR only corrects:
blocks_remaining_for_test/highest_test_block_height_produced,apply_test_budget_after_productiondoc,replacement_at_same_height_does_not_consume_a_slot,rejected_and_replaced_block_still_reaches_target_height).Related
The underlying producer behaviour — building a block that re-includes an already-confirmed tx under fast mining, caught by the prevalidation
DuplicateTransactioncheck — is tracked separately in #1439.Testing
cargo nextest run -p irys-actors test_budget_tests(5 pass); fmt + clippy clean.Summary by CodeRabbit