You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The auction strategy is rejected by the bundled GUI’s unchanged schema, and the configuration documentation incorrectly claims votes bypass auction batching.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The tests do not exercise bulk promotion with multiple entries, including a penalty treap, leaving transaction preservation and account-conflict enforcement unverified for the new traversal.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Test bulk promotion from treaps with shared-account conflicts
src/disco/pack/test_pack.c:1755
This only promotes one held transaction; the later three-transaction case deletes or expires every entry before ending the auction. Consequently, the new traversal that clears the source treap and reinserts its nodes is never exercised with multiple entries. Please add a bulk-promotion case, including enough shared-account transactions to enter a penalty treap, then verify the pack and drain it to check that no entries are lost and account conflicts remain enforced.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The auction-flush tests leave multi-entry traversal and conflict tracking untested; coverage should verify priority order and blocking of transactions sharing writable accounts until the preceding microblock completes.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Test multi-entry auction flush traversal and conflict tracking
src/disco/pack/fd_pack.c:3025
test_next_auction flushes only one transaction; its multi-entry case deletes or expires every entry before ending the auction. This leaves multi-entry traversal and conflict tracking during the flush untested. Add a case with multiple held transactions sharing writable accounts, then verify priority order and that conflicting work stays blocked until the preceding microblock completes.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
An unpublished mismatch between the metric’s documented cutoff and its actual trigger requires human assessment of its impact on exported documentation and schema.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Timing-sensitive interactions among auction deadlines, CU pacing, bundle arrivals, and leader-slot transitions require representative runtime/load validation beyond the queue-focused unit test.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because auction-end tests leave multi-transaction transfers, shared-account allocation, and penalty handling untested, so transaction preservation and conflict enforcement remain unverified.
No open issues 🎉
Review effort: Balanced
This branch has not been deployed
No deployments
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
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.
Credit to Puffin Validator (https://puffin-validator.xyz/pebble)