refactor(ergo-validation): split block.rs, header.rs, popow/algos.rs into submodule directories (1/2) - #207
Merged
Merged
Conversation
Mirrors the existing tx/voting/popow split pattern. block.rs (2623 lines,
the biggest file in the crate) becomes:
- mod.rs: SoftForkState, BlockValidationContext, CheckedBlock, and
re-exports of the public surface.
- error.rs: BlockValidationError alone (split first since every other
submodule constructs variants of it).
- extension.rs: structural extension checks (rules 400/404/405/406).
- size.rs: block-transactions section size cap (rule 306).
- interlinks.rs: interlink validation against the parent extension
(rules 401/402).
- fork_vote.rs: soft-fork vote prohibited-window check (rule 407).
- overlay.rs: the intra-block UTXO overlay both validation paths share.
- layering.rs: topological tx layering for the parallel path.
- validate.rs: validate_full_block and validate_full_block_parallel_impl
(+ its three production wrappers) -- kept together deliberately, since
these are intentionally near-duplicate mirror implementations whose
inline comments cross-reference each other's steps.
Purely structural. Verified the external re-export surface
(ergo_validation::block::{validate_full_block, validate_full_block_parallel,
validate_full_block_parallel_with_group_elements,
validate_full_block_parallel_with_costs, BlockValidationContext,
BlockValidationError, CheckedBlock, SoftForkState,
check_fork_vote_votes_collected_present}) is unchanged -- ergo-sync,
ergo-state, and ergo-difftest all compile against it unmodified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
header.rs (1148 lines) becomes header/{mod,votes,timestamp}.rs:
- header/votes.rs: the four vote-rule checks (212/213/214/215) —
check_votes_number(_active), check_votes_no_duplicates,
check_votes_no_contradictions, check_votes_known(_active) — plus
SOFT_FORK_VOTE/PARAM_VOTES_COUNT/KNOWN_VOTE_IDS consts, kept
together per the split plan since they're deliberately similar in
shape and cross-tested together. Carries the bulk of the original
test module, including the cross-cutting integration test
selected_candidate_votes_always_pass_header_validators.
- header/timestamp.rs: check_parent_id, check_timestamp,
check_future_timestamp (rule 211) + FUTURE_TIMESTAMP_DRIFT_MS,
with its own small test module.
- header/mod.rs: CheckedHeader, HeaderValidationError,
PowCheckedHeader, validate_header_after_pow, validate_header stay
here as the module's public surface; re-exports votes::*/timestamp::*
so every external call site (ergo_validation::header::{check_parent_id,
check_timestamp, check_future_timestamp, check_votes_number_active,
check_votes_known_active, validate_header, CheckedHeader,
HeaderValidationError, PowCheckedHeader, validate_header_after_pow})
resolves unchanged.
Verified: 346 lib tests pass (baseline unchanged), 36 header:: tests
present and passing, header_validation.rs integration test (16
passed/2 ignored, unchanged) exercises the re-export surface directly,
clippy -D warnings clean, fmt clean, and ergo-sync/ergo-state/ergo-difftest
all compile unmodified against the new layout.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
…modules
algos.rs (1135 lines) becomes algos/{mod,interlinks,scoring,prove,lca}.rs,
a toolbox of independent KMZ17 primitives with no "do not split" call-out:
- algos/interlinks.rs: INTERLINKS_VECTOR_PREFIX, pack_interlinks,
unpack_interlinks, kv_to_leaf, build_popow_header, update_interlinks —
everything about packing/building/updating the interlinks vector.
- algos/scoring.rs: GENESIS_LEVEL, max_level_of (KMZ17 mu-level),
best_arg / best_arg_from_levels (Algorithm 4), plus the private
pow_hit/biguint_to_f64 helpers only max_level_of needs.
- algos/prove.rs: PoPowParams, prove, prove_prefix_loop — NiPoPoW
proof construction. Named prove.rs (not proof.rs) to stay distinct
from the existing sibling popow/proof.rs (NipopowProofExt).
- algos/lca.rs: lowest_common_ancestor.
- algos/mod.rs: shared internal helpers is_genesis (pub(crate), used
externally at ergo_validation::popow::algos::is_genesis from
block/interlinks.rs) and header_id (private, used by interlinks.rs
and lca.rs via ordinary parent-module visibility); re-exports every
item so ergo_validation::popow::algos::{pack_interlinks,
unpack_interlinks, kv_to_leaf, build_popow_header, update_interlinks,
PoPowParams, max_level_of, best_arg, best_arg_from_levels,
lowest_common_ancestor, GENESIS_LEVEL, INTERLINKS_VECTOR_PREFIX}
resolve unchanged (the popow/mod.rs `pub mod algos;` + its own
re-exports needed no changes).
Verified: 346 lib tests pass (baseline unchanged), 27 popow::algos::
tests present and passing, all 5 integration test targets that
reference popow::algos:: directly pass unchanged, clippy -D warnings
clean, fmt clean, and ergo-sync/ergo-state/ergo-difftest/ergo-mining/
ergo-node all compile unmodified against the new layout.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
Contributor
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds public header and full-block validation APIs, including sequential and parallel transaction validation, structural consensus checks, intra-block UTXO handling, fork-vote and interlink validation, and modular NiPoPoW scoring, proof, and chain utilities. ChangesValidation and NiPoPoW
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FullBlockValidator
participant BlockUtxoOverlay
participant RayonWorkers
participant TransactionValidator
FullBlockValidator->>BlockUtxoOverlay: resolve transaction inputs
FullBlockValidator->>RayonWorkers: validate dependency-layer transactions
RayonWorkers->>TransactionValidator: execute transaction validation
TransactionValidator-->>FullBlockValidator: return checked transactions and costs
FullBlockValidator->>BlockUtxoOverlay: apply successful transactions in index order
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
arkadianet
marked this pull request as ready for review
July 17, 2026 09:19
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@ergo-validation/src/block/extension.rs`:
- Line 1: Replace the duplicate-key validation associated with Extension so it
uses linear-time membership tracking rather than comparing each field against
all previous fields. Track each encountered key in a set and reject duplicates
while preserving the existing validation result and error behavior.
In `@ergo-validation/src/block/mod.rs`:
- Around line 130-154: Preserve the raw soft-fork starting-height and
votes-collected options, along with the rule-407 disabled state, in the
validation context instead of collapsing them into Option<SoftForkState>. Use
these values to call check_fork_vote_votes_collected_present so the malformed
122-without-121 state is rejected, and skip validate_fork_vote whenever rule 407
is disabled in both sequential and parallel validation paths.
In `@ergo-validation/src/block/validate.rs`:
- Around line 352-360: Bind supplied group_elements to the exact parsed
transactions before using the fast path, using an opaque alongside-produced
artifact or validated metadata; otherwise reparse each transaction’s points. Do
not treat a merely length-matched, empty, reordered, or fabricated list as
authoritative. Remove the debug_assert around group_elements so mismatches
follow the documented safe fallback without panicking in debug builds, including
the related validation paths.
- Around line 314-336: Update the parallel block-validation flow around
build_tx_layers and the per-layer validation to preserve sequential
first-failure ordering by transaction index. Do not return preparation,
missing-input, or double-spend errors before all lower-index transactions have
definitive outcomes; defer and select errors according to the lowest failing
transaction index. Ensure both validate_full_block’s parallel equivalent and the
associated validation path maintain deterministic parity with sequential
validation.
In `@ergo-validation/src/header/mod.rs`:
- Around line 313-318: The verify_pow method must not trust the caller-provided
header_id when constructing CheckedHeader. Recompute the canonical ID from
header using the existing header-ID derivation mechanism, or validate the
supplied ID against that computed value and return the appropriate
HeaderValidationError on mismatch; apply the same binding to the related
proof-construction path around the additional referenced lines, then store only
the validated canonical ID.
In `@ergo-validation/src/popow/algos/scoring.rs`:
- Around line 116-139: Guard low-m scoring inputs and stop scoring once the
maximum representable score is reached: in
ergo-validation/src/popow/algos/scoring.rs lines 116-139, define or reject m = 0
before the level loop and return immediately when best saturates at u64::MAX
instead of iterating toward u32::MAX; in
ergo-validation/src/popow/algos/prove.rs lines 34-43, validate params.m >= 1
before performing length arithmetic or constructing the sub_chain prefix.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67027d74-ed77-4f08-8caf-73b2a4d44d9f
📒 Files selected for processing (18)
ergo-validation/src/block.rsergo-validation/src/block/error.rsergo-validation/src/block/extension.rsergo-validation/src/block/fork_vote.rsergo-validation/src/block/interlinks.rsergo-validation/src/block/layering.rsergo-validation/src/block/mod.rsergo-validation/src/block/overlay.rsergo-validation/src/block/size.rsergo-validation/src/block/validate.rsergo-validation/src/header/mod.rsergo-validation/src/header/timestamp.rsergo-validation/src/header/votes.rsergo-validation/src/popow/algos/interlinks.rsergo-validation/src/popow/algos/lca.rsergo-validation/src/popow/algos/mod.rsergo-validation/src/popow/algos/prove.rsergo-validation/src/popow/algos/scoring.rs
| @@ -0,0 +1,274 @@ | |||
| use ergo_ser::extension::Extension; | |||
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid quadratic duplicate-key validation on adversarial extensions.
The 32-KiB cap still permits about 10,911 minimal fields, making this scan perform roughly 59.5 million comparisons. Cryptographic binding does not make committed block data non-adversarial.
Use linear duplicate detection
+use std::collections::HashMap;
+
use ergo_ser::extension::Extension;- for first in 0..extension.fields.len() {
- let key = extension.fields[first].key;
- for second in (first + 1)..extension.fields.len() {
- if extension.fields[second].key == key {
- return Err(BlockValidationError::ExtensionDuplicateKey {
- key: hex::encode(key),
- first,
- second,
- });
- }
+ let mut first_by_key = HashMap::with_capacity(extension.fields.len());
+ for (second, field) in extension.fields.iter().enumerate() {
+ if let Some(&first) = first_by_key.get(&field.key) {
+ return Err(BlockValidationError::ExtensionDuplicateKey {
+ key: hex::encode(field.key),
+ first,
+ second,
+ });
}
+ first_by_key.insert(field.key, second);
}Also applies to: 86-100
🤖 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 `@ergo-validation/src/block/extension.rs` at line 1, Replace the duplicate-key
validation associated with Extension so it uses linear-time membership tracking
rather than comparing each field against all previous fields. Track each
encountered key in a set and reject duplicates while preserving the existing
validation result and error behavior.
Comment on lines
+130
to
+154
| /// Whether rule 215 (`hdrVotesUnknown`) has been deactivated by an | ||
| /// activated soft-fork (`ErgoValidationSettings::is_rule_disabled(215)`). | ||
| /// Scala marks the rule `mayBeDisabled = true` and mainnet's v6.0 | ||
| /// activation disabled it (`rules_to_disable = [215, 409]`) so the | ||
| /// new `SubblocksPerBlock` param and downward proposals are votable | ||
| /// at an epoch start. When `true`, rule 215 is skipped — Scala's | ||
| /// `ValidationState` never runs a disabled rule. Defaults to `false` | ||
| /// (rule active) for callers that don't track validation settings. | ||
| pub votes_unknown_rule_disabled: bool, | ||
| /// Parent block's extension. Drives interlink validation | ||
| /// (rules 401 / 402): when `Some`, the current extension's | ||
| /// interlink fields must decode and equal `update_interlinks( | ||
| /// parent_header, parent_extension_interlinks)`. When `None` | ||
| /// (genesis or pre-NiPoPoW-aware caller), rules 401/402 don't | ||
| /// fire — matches Scala's `exIlUnableToValidate` recoverable | ||
| /// path in `ExtensionValidator.validateInterlinks`. | ||
| pub parent_extension: Option<&'a Extension>, | ||
| /// In-progress soft-fork state, if any. Drives rule 407 | ||
| /// (`exCheckForkVote`): when present, headers casting a | ||
| /// SoftFork vote are checked against the prohibited | ||
| /// post-vote / pre-activation window. `None` means there's | ||
| /// no soft-fork in progress (Scala | ||
| /// `currentParameters.softForkStartingHeight.isEmpty`), and | ||
| /// the rule trivially passes. | ||
| pub soft_fork_state: Option<SoftForkState>, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Preserve every rule-407 state in the validation context.
Option<SoftForkState> collapses “no fork” and “starting height present but votes-collected missing.” The supplied full-block path consequently calls only validate_fork_vote, accepting the malformed 122-without-121 table. It also cannot skip the prohibited-window check when rule 407 is disabled. Either case can diverge from consensus.
Carry the raw options and rule-disabled state, call check_fork_vote_votes_collected_present, and skip validate_fork_vote when disabled in both sequential and parallel paths.
Proposed context additions
pub votes_unknown_rule_disabled: bool,
+ /// Whether consensus rule 407 is disabled.
+ pub fork_vote_rule_disabled: bool,
+ /// Raw parameter 122, retained to distinguish malformed state.
+ pub soft_fork_starting_height: Option<i32>,
+ /// Raw parameter 121, retained to distinguish malformed state.
+ pub soft_fork_votes_collected: Option<i32>,
pub parent_extension: Option<&'a Extension>,
pub soft_fork_state: Option<SoftForkState>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Whether rule 215 (`hdrVotesUnknown`) has been deactivated by an | |
| /// activated soft-fork (`ErgoValidationSettings::is_rule_disabled(215)`). | |
| /// Scala marks the rule `mayBeDisabled = true` and mainnet's v6.0 | |
| /// activation disabled it (`rules_to_disable = [215, 409]`) so the | |
| /// new `SubblocksPerBlock` param and downward proposals are votable | |
| /// at an epoch start. When `true`, rule 215 is skipped — Scala's | |
| /// `ValidationState` never runs a disabled rule. Defaults to `false` | |
| /// (rule active) for callers that don't track validation settings. | |
| pub votes_unknown_rule_disabled: bool, | |
| /// Parent block's extension. Drives interlink validation | |
| /// (rules 401 / 402): when `Some`, the current extension's | |
| /// interlink fields must decode and equal `update_interlinks( | |
| /// parent_header, parent_extension_interlinks)`. When `None` | |
| /// (genesis or pre-NiPoPoW-aware caller), rules 401/402 don't | |
| /// fire — matches Scala's `exIlUnableToValidate` recoverable | |
| /// path in `ExtensionValidator.validateInterlinks`. | |
| pub parent_extension: Option<&'a Extension>, | |
| /// In-progress soft-fork state, if any. Drives rule 407 | |
| /// (`exCheckForkVote`): when present, headers casting a | |
| /// SoftFork vote are checked against the prohibited | |
| /// post-vote / pre-activation window. `None` means there's | |
| /// no soft-fork in progress (Scala | |
| /// `currentParameters.softForkStartingHeight.isEmpty`), and | |
| /// the rule trivially passes. | |
| pub soft_fork_state: Option<SoftForkState>, | |
| /// Whether rule 215 (`hdrVotesUnknown`) has been deactivated by an | |
| /// activated soft-fork (`ErgoValidationSettings::is_rule_disabled(215)`). | |
| /// Scala marks the rule `mayBeDisabled = true` and mainnet's v6.0 | |
| /// activation disabled it (`rules_to_disable = [215, 409]`) so the | |
| /// new `SubblocksPerBlock` param and downward proposals are votable | |
| /// at an epoch start. When `true`, rule 215 is skipped — Scala's | |
| /// `ValidationState` never runs a disabled rule. Defaults to `false` | |
| /// (rule active) for callers that don't track validation settings. | |
| pub votes_unknown_rule_disabled: bool, | |
| /// Whether consensus rule 407 is disabled. | |
| pub fork_vote_rule_disabled: bool, | |
| /// Raw parameter 122, retained to distinguish malformed state. | |
| pub soft_fork_starting_height: Option<i32>, | |
| /// Raw parameter 121, retained to distinguish malformed state. | |
| pub soft_fork_votes_collected: Option<i32>, | |
| /// Parent block's extension. Drives interlink validation | |
| /// (rules 401 / 402): when `Some`, the current extension's | |
| /// interlink fields must decode and equal `update_interlinks( | |
| /// parent_header, parent_extension_interlinks)`. When `None` | |
| /// (genesis or pre-NiPoPoW-aware caller), rules 401/402 don't | |
| /// fire — matches Scala's `exIlUnableToValidate` recoverable | |
| /// path in `ExtensionValidator.validateInterlinks`. | |
| pub parent_extension: Option<&'a Extension>, | |
| /// In-progress soft-fork state, if any. Drives rule 407 | |
| /// (`exCheckForkVote`): when present, headers casting a | |
| /// SoftFork vote are checked against the prohibited | |
| /// post-vote / pre-activation window. `None` means there's | |
| /// no soft-fork in progress (Scala | |
| /// `currentParameters.softForkStartingHeight.isEmpty`), and | |
| /// the rule trivially passes. | |
| pub soft_fork_state: Option<SoftForkState>, |
🤖 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 `@ergo-validation/src/block/mod.rs` around lines 130 - 154, Preserve the raw
soft-fork starting-height and votes-collected options, along with the rule-407
disabled state, in the validation context instead of collapsing them into
Option<SoftForkState>. Use these values to call
check_fork_vote_votes_collected_present so the malformed 122-without-121 state
is rejected, and skip validate_fork_vote whenever rule 407 is disabled in both
sequential and parallel validation paths.
Comment on lines
+314
to
+336
| /// Parallel equivalent of [`validate_full_block`]: topologically layers the | ||
| /// block's transactions by intra-block dependency, then validates each layer | ||
| /// via `rayon::par_iter`. Identical output to the sequential path for any | ||
| /// block the sequential path accepts (and for every rejection — first-failing | ||
| /// tx by index wins, matching Scala's error-order semantics). | ||
| /// | ||
| /// Consensus invariants held constant across both paths: | ||
| /// - Per-tx structural / monetary / script validation is untouched — same | ||
| /// `validate_transaction_parsed` call, same `CostAccumulator`, same | ||
| /// `TransactionContext`. | ||
| /// - Section-id linkage + merkle-root checks are performed identically and | ||
| /// up-front, before any per-tx work. | ||
| /// - Total block cost is summed from per-tx totals after all layers finish; | ||
| /// `max_block_cost` comparison is the exact same inequality. | ||
| /// - Returned `CheckedBlock.transactions()` is ordered by original tx index, | ||
| /// so downstream AVL application mutates the UTXO tree in consensus order. | ||
| /// - Intra-block double-spend (two txs listing the same input box_id) is | ||
| /// rejected up front via `build_tx_layers` rather than being caught | ||
| /// implicitly by the sequential overlay's spent-set. | ||
| /// | ||
| /// Only difference visible to callers: errors report the first-by-index | ||
| /// failing tx, which matches sequential behavior. If two txs in the same | ||
| /// layer fail concurrently, the lower tx index is reported (deterministic). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve sequential first-failure ordering.
Layering changes error precedence. For example, a missing input in tx 1 returns during preparation before tx 0's script validation runs; similarly, a higher-index layer-0 failure can precede a lower-index failure in a later layer. build_tx_layers can also return a double-spend before any lower-index transaction validation.
This contradicts the documented sequential-error parity and the PR's no-behavior-change objective. Defer errors until all lower transaction indices have definitive outcomes.
Also applies to: 483-624
🤖 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 `@ergo-validation/src/block/validate.rs` around lines 314 - 336, Update the
parallel block-validation flow around build_tx_layers and the per-layer
validation to preserve sequential first-failure ordering by transaction index.
Do not return preparation, missing-input, or double-spend errors before all
lower-index transactions have definitive outcomes; defer and select errors
according to the lowest failing transaction index. Ensure both
validate_full_block’s parallel equivalent and the associated validation path
maintain deterministic parity with sequential validation.
Comment on lines
+352
to
+360
| // The points (when supplied) must be 1:1 with the transactions. The | ||
| // production caller guarantees this (same parse), so a mismatch is a caller | ||
| // bug: assert it in debug, and degrade safely below via `.get(i)` (a missing | ||
| // index falls back to re-parsing that tx's points — correct, just slower — | ||
| // so a wiring error can never bypass or misapply the curve-check). | ||
| debug_assert!( | ||
| group_elements.is_none_or(|ge| ge.len() == block_transactions.transactions.len()), | ||
| "per-tx group_elements must be index-aligned with transactions", | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bind supplied group elements to the parsed transactions.
A correctly sized but empty, reordered, or fabricated group_elements list is accepted as authoritative and skips reparsing, so curve checks can target different—or no—points than those encoded in the transaction.
Use an opaque artifact produced alongside the parsed transactions, or verify the metadata binding before the fast path. Also, the debug_assert! contradicts the documented graceful fallback by panicking on mismatches in debug builds.
Also applies to: 580-602, 683-703
🤖 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 `@ergo-validation/src/block/validate.rs` around lines 352 - 360, Bind supplied
group_elements to the exact parsed transactions before using the fast path,
using an opaque alongside-produced artifact or validated metadata; otherwise
reparse each transaction’s points. Do not treat a merely length-matched, empty,
reordered, or fabricated list as authoritative. Remove the debug_assert around
group_elements so mismatches follow the documented safe fallback without
panicking in debug builds, including the related validation paths.
Comment on lines
+313
to
+318
| /// Verify the Autolykos PoW solution and return a proof. Dispatch | ||
| /// is on the solution variant (Scala parity — see `pow.rs` doc), so | ||
| /// no `DifficultyParams` is needed here. | ||
| pub fn verify_pow(header: Header, header_id: [u8; 32]) -> Result<Self, HeaderValidationError> { | ||
| pow::verify_pow_solution(&header)?; | ||
| Ok(Self { header, header_id }) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Bind the proof object to the header’s canonical ID.
verify_pow copies the caller-provided header_id without recomputing or comparing it. This permits a valid header to become a CheckedHeader with an unrelated ID, which downstream block validation trusts for checkpoint and section-linkage checks. Derive the ID internally or reject a mismatch before constructing the proof object.
Also applies to: 365-386
🤖 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 `@ergo-validation/src/header/mod.rs` around lines 313 - 318, The verify_pow
method must not trust the caller-provided header_id when constructing
CheckedHeader. Recompute the canonical ID from header using the existing
header-ID derivation mechanism, or validate the supplied ID against that
computed value and return the appropriate HeaderValidationError on mismatch;
apply the same binding to the related proof-construction path around the
additional referenced lines, then store only the validated canonical ID.
Comment on lines
+116
to
+139
| let mut level: u32 = 1; | ||
| loop { | ||
| let count = levels.iter().filter(|&&l| l >= level).count() as u64; | ||
| if count < m as u64 { | ||
| return best; | ||
| } | ||
| // 2^level * count, saturating at u64::MAX so a hypothetical | ||
| // 2^64 wrap-around can't underestimate the score. | ||
| let score = (1u64) | ||
| .checked_shl(level) | ||
| .unwrap_or(u64::MAX) | ||
| .saturating_mul(count); | ||
| if score > best { | ||
| best = score; | ||
| } | ||
| // u32 level cap: a chain whose every header has level ≥ 32 is | ||
| // already at score ~chain.len() * 2^32. Beyond that we'd need | ||
| // u64 levels, which KMZ17 does not produce in practice. Cap | ||
| // here defensively rather than overflowing the shift. | ||
| if level == u32::MAX { | ||
| return best; | ||
| } | ||
| level += 1; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Bound low-m behavior before looping or indexing.
With genesis at u32::MAX, best_arg_from_levels(..., 1) iterates through roughly four billion levels; the included m = 1 test reaches this path. m = 0 also prevents termination, while prove accepts it and can underflow or index past sub_chain.
ergo-validation/src/popow/algos/scoring.rs#L116-L139: reject/definem = 0and return once the score saturates instead of continuing towardu32::MAX.ergo-validation/src/popow/algos/prove.rs#L34-L43: requireparams.m >= 1before length arithmetic and prefix construction.
📍 Affects 2 files
ergo-validation/src/popow/algos/scoring.rs#L116-L139(this comment)ergo-validation/src/popow/algos/prove.rs#L34-L43
🤖 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 `@ergo-validation/src/popow/algos/scoring.rs` around lines 116 - 139, Guard
low-m scoring inputs and stop scoring once the maximum representable score is
reached: in ergo-validation/src/popow/algos/scoring.rs lines 116-139, define or
reject m = 0 before the level loop and return immediately when best saturates at
u64::MAX instead of iterating toward u32::MAX; in
ergo-validation/src/popow/algos/prove.rs lines 34-43, validate params.m >= 1
before performing length arithmetic or constructing the sub_chain prefix.
…207 review) Addresses a review finding on the popow split. `NipopowProof.m` is read unvalidated off the wire (`ergo-ser::read_nipopow_proof`), so a malformed `m = 0` is reachable: - `best_arg_from_levels`: with `m = 0` the `count < m` cutoff (`count < 0`) is unreachable, so the level loop would spin toward `u32::MAX`. Coerce `m.max(1)` (the loosest valid threshold) so the function stays total and bounded on invalid input; legitimate `m >= 1` is unaffected. Separately, add a saturation early-return: once `best == u64::MAX` no later level can change the result (best is monotonic, every score saturates), so return immediately. This bounds a chain carrying the GENESIS_LEVEL sentinel (which qualifies at every level, ~2^32 iterations) to O(64) — and provably never changes any non-saturating result. - `prove`: `prove_prefix_loop` indexes `sub_chain[sub_chain.len() - m]`, so `m == 0` computes `sub_chain[len]` and panics out of bounds. Reject it up front alongside the existing sibling `k < 1` guard. Root-cause note: the true fix for the untrusted path is rejecting `m < 1` / `k < 1` at the `ergo-ser` deserialize boundary (a separate crate/PR); these two guards are the in-crate defense-in-depth the reviewer flagged, and make both functions total on invalid input regardless. Verified: 349 lib tests pass (346 baseline + 3 new: m=0 coercion parity, genesis-sentinel saturation termination, prove m=0 rejection); clippy -D warnings clean; fmt clean; full workspace check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
This was referenced Jul 17, 2026
arkadianet
added a commit
that referenced
this pull request
Jul 17, 2026
…ary (#210) `read_nipopow_proof` read `m` and `k` straight off the wire with no lower-bound check, so a peer-supplied malformed proof with `m = 0` or `k = 0` was accepted. Both are invalid per KMZ17 (positive parameters; mainnet uses m=6, k=10) and dangerous downstream: - `m = 0` makes `best_arg`'s `count < m` cutoff (`count < 0`) unreachable, turning proof scoring in the verifier's `is_better_than` into an unbounded loop on a peer-controlled input. - `m = 0` makes `prove`'s `sub_chain[sub_chain.len() - m]` index out of bounds (panic). Reject both at the untrusted wire boundary — the root gate, so no such value ever reaches the scoring / proving code. ergo-validation's `best_arg_from_levels` and `prove` carry matching defense-in-depth guards (PR #207), but this closes the source. Verified: 415 ergo-ser lib tests pass (+2 new: m=0 and k=0 each rejected with the right message; existing round-trip and hostile-size fixtures all use m=6/k=10 and are unaffected); clippy -D warnings clean; fmt clean; downstream ergo-validation popow tests pass. Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
First half of the ergo-validation crate-organization pass (same methodology as the ergo-node splits #205/#206). Pure code motion — zero behavior change: function bodies moved verbatim, visibility widened only where cross-file access required it, and every externally-reachable path preserved via re-exports.
What's split
src/block.rs(2,623 lines) →block/— 9 files:error.rs—BlockValidationError(extracted first; every other submodule constructs its variants)extension.rs— structural extension rules 400/404/405/406 + size capssize.rs— block-transactions size cap (rule 306)interlinks.rs— interlink validation vs parent extension (rules 401/402)fork_vote.rs— soft-fork prohibited-window check (rule 407)overlay.rs— intra-block UTXO overlaylayering.rs— topological tx layering for the parallel pathvalidate.rs—validate_full_block+validate_full_block_parallel_implkept together deliberately: they are intentionally near-duplicate mirror implementations whose inline comments cross-reference each other's steps; diffing the two paths for parity is the design intentmod.rs—SoftForkState,BlockValidationContext,CheckedBlock+ re-exportssrc/header.rs(1,148 lines) →header/— 3 files:votes.rs— the four vote rules (212/213/214/215) kept together in one file (deliberately similar in shape, cross-tested viaselected_candidate_votes_always_pass_header_validators, which stays here)timestamp.rs— parent-id / monotonicity / future-timestamp (rule 211)mod.rs—CheckedHeader,HeaderValidationError,PowCheckedHeader,validate_header(_after_pow)+ re-exportssrc/popow/algos.rs(1,135 lines) →popow/algos/— 5 files:interlinks.rs— pack/unpack/kv_to_leaf/build_popow_header/update_interlinksscoring.rs—max_level_of(KMZ17 μ-level) +best_arg(_from_levels)prove.rs— NiPoPoW proof construction (namedprove.rs, notproof.rs, to stay distinct from the siblingpopow/proof.rs)lca.rs—lowest_common_ancestormod.rs— sharedis_genesis/header_idhelpers + re-exportsVerification (run after each individual split, not just at the end)
cargo test -p ergo-validation --lib: 346 passed / 0 failed at every step (identical to the pre-refactor baseline)cargo clippy -p ergo-validation --all-targets --all-features -- -D warnings: cleancargo fmt --all -- --check: cleanergo-sync,ergo-state,ergo-difftest,ergo-mining,ergo-node) compiles unmodified — the external re-export surface is byte-identicalheader_validation,popow_*,interlinks_*,nipopow_*) pass unchangedSecond half (active_params, tx/script, voting/recompute, voting/extension_validation) follows in the stacked PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
Summary by CodeRabbit
New Features
Bug Fixes