refactor(ergo-node): split snapshot_emit.rs, snapshot.rs, scala_compat.rs, messaging.rs - #206
Conversation
…odules
node/snapshot_emit.rs -> node/snapshot_emit/{mod,recent_blocks,bootstrap_panel,
mempool_projection,events_projection}.rs: mod.rs keeps publish_snapshot as the
per-tick orchestrator; each field group it collects moves to its own submodule
(tip-cached recent-blocks tail + first-deliverer merge, Mode 2 bootstrap-panel
projection, mempool-transaction DTO list, operator event-feed projection).
snapshot.rs -> snapshot/{mod,build,publisher}.rs: mod.rs keeps the NodeSnapshot
and SnapshotParts DTOs plus their small support types (not split further --
splitting a single wide DTO struct across files hurts more than it helps);
build.rs assembles a NodeSnapshot from a SnapshotParts; publisher.rs owns
SnapshotPublisher's per-tick publish + stall-clock bookkeeping.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
…ol_fee_stats.rs Light-touch split per plan: the trait impl block (NodeChainQuery for ScalaCompatBridge) stays whole -- it's one large block of small, independent read-only methods, not worth forcing apart. Only the free-function fee-per-byte pool ranking cluster (PoolFeeEntry, rank_pool_by_fee_per_byte, bin_for_wait_ms, estimate_wait_ms_from_rank, their consts, and their tests) moves out to its own submodule; the three call sites in pool_fee_histogram/pool_recommended_fee/ pool_expected_wait_time_ms are updated to the pool_fee_stats:: path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
…submodules Extract the three sizable per-batch match arms out of handle_message into named helpers -- handle_inv (CODE_INV), handle_modifier_batch (CODE_MODIFIER, ~190 lines), handle_peers_response (CODE_PEERS) -- all following the same &mut NodeState -> Vec<Action> convention as handle_message itself and the dispatch pattern already used by run_wallet_writer. handle_message plus the three new helpers live in messaging/dispatch.rs; the three Mode-2/NiPoPoW consume-side handlers (handle_inbound_manifest, handle_inbound_utxo_chunk, handle_inbound_popow_proof) each get their own submodule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
|
Warning Review limit reached
Next review available in: 36 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 (1)
📝 WalkthroughWalkthroughThis PR extracts pool fee statistics, adds opcode-based inbound peer-message handlers, and rebuilds operator snapshot publication around modular DTO projections, snapshot assembly, and atomic publishing. ChangesPool fee statistics
Inbound peer messaging
Operator snapshot publication
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
|
@coderabbitai review |
Action performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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-node/src/api_bridge/scala_compat/pool_fee_stats.rs`:
- Around line 57-87: The rank_pool_by_fee_per_byte function must use the active
network’s fee proposition bytes instead of MAINNET_FEE_PROPOSITION_BYTES. Thread
or obtain the existing network configuration used by the bridge, select its
mainnet/testnet fee proposition bytes, and use that value when filtering
transaction outputs while preserving the current ranking behavior.
In `@ergo-node/src/node/messaging/dispatch.rs`:
- Around line 390-394: Update the NiPoPoW proof-serving flow around
get_cached_popow_proof_bytes to handle as_utxo() returning None without
panicking: silently decline the request instead of calling expect. Preserve the
existing proof retrieval behavior for UTXO-backed state.
- Around line 552-563: Update the transaction branch in the dispatch flow to
derive the transaction ID from bytes and compare it with mod_id before calling
state.coordinator.on_transaction_received. Only invoke on_transaction_received
and admit_transaction when the IDs match; reject or ignore mismatches without
marking the claimed request as delivered.
In `@ergo-node/src/node/messaging/popow.rs`:
- Around line 46-66: Move the ERGO_CAPTURE_NIPOPOW_PROOF write logic out of the
pre-parsing section and into the verified BetterChain/NoBetterChain handling
paths in the surrounding message-processing function. Capture only after the
proof has passed parsing and genesis/chain validation, preserving the existing
one-shot path check and logging behavior so malformed or wrong-genesis proofs
are never written.
In `@ergo-node/src/node/snapshot_emit/bootstrap_panel.rs`:
- Around line 121-126: Update the in_catchup branch that builds the snapshot
metadata DTO to read the originally selected snapshot height and manifest ID
preserved in node state during installation, rather than using the advancing
best_full value and None. Ensure the install flow stores those selected values
and the post-install catch-up path reuses them unchanged.
In `@ergo-node/src/node/snapshot_emit/mod.rs`:
- Around line 78-117: Update the snapshot metadata assembly around
get_header_meta and genesis_block_id so empty-chain state is handled explicitly,
while live-chain lookup failures are treated as publication failures. Do not
replace missing or errored records with zero IDs, timestamps, scores, or genesis
IDs when live tip heights exist; log the storage fault and retain the previous
snapshot or skip publication. Preserve the existing read_tip_n_bits behavior and
valid metadata for successful lookups.
In `@ergo-node/src/snapshot/publisher.rs`:
- Around line 64-82: Update the difficulty carry-forward logic in the snapshot
publishing method to compare each current tip ID with its corresponding prior
snapshot tip ID before reusing prior_header_n_bits or prior_full_block_n_bits.
If the ID changed, return without publishing until the new difficulty is
readable; preserve carry-forward only when the corresponding tip ID is
unchanged. Update the related test to use distinct prior and current tip IDs and
cover this behavior.
- Around line 83-107: Update the progress tracking around last_header_height and
last_full_block_height so backward chain-height movement resets the
corresponding stall baselines before recovery advances are evaluated. Ensure
rollback from a previously observed height allows subsequent block application
through that height to refresh last_block_progress_at and prevents an actively
recovering node from being reported as stalled, while preserving normal
forward-progress handling.
🪄 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: 97148a6b-f7e8-4fd2-bcb1-f58d1b9414b2
📒 Files selected for processing (18)
ergo-node/src/api_bridge/scala_compat/mod.rsergo-node/src/api_bridge/scala_compat/pool_fee_stats.rsergo-node/src/node/messaging.rsergo-node/src/node/messaging/dispatch.rsergo-node/src/node/messaging/manifest.rsergo-node/src/node/messaging/mod.rsergo-node/src/node/messaging/popow.rsergo-node/src/node/messaging/utxo_chunk.rsergo-node/src/node/snapshot_emit.rsergo-node/src/node/snapshot_emit/bootstrap_panel.rsergo-node/src/node/snapshot_emit/events_projection.rsergo-node/src/node/snapshot_emit/mempool_projection.rsergo-node/src/node/snapshot_emit/mod.rsergo-node/src/node/snapshot_emit/recent_blocks.rsergo-node/src/snapshot.rsergo-node/src/snapshot/build.rsergo-node/src/snapshot/mod.rsergo-node/src/snapshot/publisher.rs
💤 Files with no reviewable changes (3)
- ergo-node/src/node/messaging.rs
- ergo-node/src/node/snapshot_emit.rs
- ergo-node/src/snapshot.rs
| pub(super) fn rank_pool_by_fee_per_byte( | ||
| pool: &[(ergo_primitives::digest::Digest32, std::sync::Arc<[u8]>)], | ||
| ) -> Vec<PoolFeeEntry> { | ||
| let mut entries: Vec<PoolFeeEntry> = pool | ||
| .iter() | ||
| .filter_map(|(_, bytes)| { | ||
| let tx = parse_pool_tx(bytes)?; | ||
| let fee: u64 = tx | ||
| .output_candidates | ||
| .iter() | ||
| .filter(|c| { | ||
| c.ergo_tree_bytes() == ergo_mempool::validator::MAINNET_FEE_PROPOSITION_BYTES | ||
| }) | ||
| .map(|c| c.value) | ||
| .sum(); | ||
| if fee == 0 { | ||
| return None; | ||
| } | ||
| let size = bytes.len() as u64; | ||
| if size == 0 { | ||
| return None; | ||
| } | ||
| Some(PoolFeeEntry { | ||
| fee, | ||
| fee_per_byte: fee / size, | ||
| }) | ||
| }) | ||
| .collect(); | ||
| entries.sort_by_key(|e| std::cmp::Reverse(e.fee_per_byte)); | ||
| entries | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ergo_mempool::validator exposes a testnet-specific fee proposition constant,
# and whether the network is threaded into fee-related pool ranking anywhere.
rg -n 'FEE_PROPOSITION_BYTES' --type=rust -C2
rg -n 'MAINNET_FEE_PROPOSITION_BYTES|TESTNET_FEE_PROPOSITION_BYTES' -g '!target' -C3Repository: arkadianet/ergo
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pool_fee_stats.rs ---'
sed -n '1,220p' ergo-node/src/api_bridge/scala_compat/pool_fee_stats.rs
echo
echo '--- search fee proposition constants ---'
rg -n 'FEE_PROPOSITION|fee proposition|miners fee|MAINNET_FEE' ergo-node -g '*.rs' -C 2
echo
echo '--- search network branching in scala_compat ---'
rg -n 'static_cfg\.network|testnet|mainnet' ergo-node/src/api_bridge/scala_compat -g '*.rs' -C 2
echo
echo '--- bridge construction / call sites ---'
rg -n 'ScalaCompatBridge|pool_fee_histogram|pool_recommended_fee|pool_expected_wait_time_ms' ergo-node -g '*.rs' -C 2Repository: arkadianet/ergo
Length of output: 19875
🌐 Web query:
Ergo testnet fee proposition bytes MAINNET_FEE_PROPOSITION_BYTES TESTNET_FEE_PROPOSITION_BYTES sigma-rust MINERS_FEE_TESTNET_ADDRESS
💡 Result:
In the Ergo platform, transaction fees are collected in specialized output boxes that are spendable only by miners [1][2]. These boxes are defined by a specific script, often referred to as the fee proposition [1]. Regarding your specific search terms: 1. MAINNET_FEE_PROPOSITION_BYTES / TESTNET_FEE_PROPOSITION_BYTES: These are not standard, globally defined constants exposed in the public API or primary documentation [1][3]. Instead, the fee proposition is derived dynamically from the node's chain settings [1]. The node implementation calculates the fee by filtering transaction outputs that match the ErgoTree defined in settings.chainSettings.monetary.feeProposition [1]. This contract typically allows the miner to spend the box provided they satisfy a specific condition (e.g., proving ownership of the miner's public key) [1]. 2. sigma-rust: This is the official Rust library for Ergo, which includes essential tools for transaction building, fee estimation, and wallet interactions [4]. It provides functionality such as TxBuilder::estimate_tx_size_bytes() and constants like SUGGESTED_TX_FEE to help developers estimate and construct transactions appropriately [5]. 3. MINERS_FEE_TESTNET_ADDRESS: There is no fixed "miner's fee address" in Ergo [1]. Because Ergo uses an eUTXO model, fees are not sent to a static wallet address but are instead included as output boxes within the transaction itself [6][2]. These output boxes are "protected" by the miner's fee script [1]. If you are interacting with the testnet, the miner reward is simply handled by the protocol based on the coinbase transaction of a block; for custom fee payments within your own transactions, you must ensure the fee output box uses the script that the network expects to be spent by miners [1][6]. For developers, the standard practice is to use the Ergo Node API or the sigma-rust SDK to handle fee output creation, rather than hardcoding proposition bytes, as these can change based on network upgrades (soft forks) or protocol configurations [1][7]. Developers can query their local node to understand current fee requirements or use SDK methods to suggest appropriate fees based on transaction size or validation cost [5][2].
Citations:
- 1: https://docs.ergoplatform.com/dev/protocol/tx/min-fee/
- 2: https://learn.ergoplatform.com/dev/Integration/guide/
- 3: https://docs.ergoplatform.com/node/testnet/testnetconf/
- 4: https://github.com/ergoplatform/sigma-rust/
- 5: Tx fee estimation ergoplatform/sigma-rust#128
- 6: https://docs.ergoplatform.com/dev/scs/ergoscript/reusable-functions/
- 7: Add version-aware SigmaProp.propBytes for v7.0 soft-fork ergoplatform/sigmastate-interpreter#1148
🌐 Web query:
site:github.com MINERS_FEE_TESTNET_ADDRESS ergo
💡 Result:
In the Ergo blockchain ecosystem, MINERS_FEE_TESTNET_ADDRESS is typically a configuration constant or reference point used by developers and automated tools (such as bots or wallet libraries) to facilitate transaction fee payments on the testnet [1][2]. There is no single "universal" static testnet miner fee address because mining rewards and transaction fees are distributed to the address configured by the miner running the node or mining pool [3][4]. However, developers often use specific addresses for testing purposes in their implementations: 1. Software Configuration: In projects like ergo-appkit or dex-backends, developers often configure their own wallet address (derived from a mnemonic seed) to receive fees and pay transaction costs [1][4]. 2. Code Constants: Historical references to specific fee addresses sometimes appear in repository code (such as in ergo-appkit or specific protocol prototypes) for testing [5][2]. These are often subject to change, as evidenced by release notes indicating that testnet fee addresses have been updated or fixed in library versions [2]. If you are a developer looking for the current address to use in your software, you should: - Avoid hardcoding a specific address if possible; instead, derive the address from your test wallet mnemonic as described in your project's configuration files [1][4]. - Check the current documentation or configuration schema of the specific tool or SDK you are using (e.g., ergo-appkit), as these addresses are managed at the implementation level rather than being a global blockchain constant [4][2]. - Use the Ergo Explorer (testnet) to verify addresses and monitor testnet activity [5][3].
Citations:
- 1: https://github.com/ergoplatform/ergo-dex-backend
- 2: https://github.com/ergoplatform/ergo-appkit/releases
- 3: https://github.com/Moria-Finance/multi-sig-test
- 4: https://github.com/ergoplatform/ergo-appkit/blob/d466a99bc3ef9d0ca29ebfbe7613e1ae6824a821/ergo-testnet.conf
- 5: https://github.com/glasgowm148/ergodocs/blob/main/docs/eco/miners-rights.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fee proposition config / chain settings ---'
rg -n 'feeProposition|fee proposition|monetary\.fee|miner fee|fee_tree_bytes|fee_ergo_tree' ergo-node -g '*.rs' -C 3
echo
echo '--- ScalaCompatBridge methods around pool fee routes ---'
sed -n '620,720p' ergo-node/src/api_bridge/scala_compat/mod.rs
echo
echo '--- ScalaCompatBridge constructor / network field ---'
sed -n '1,120p' ergo-node/src/api_bridge/scala_compat/mod.rs
echo
echo '--- any testnet fee constant in repo ---'
rg -n 'TESTNET_FEE_PROPOSITION_BYTES|fee proposition bytes.*testnet|testnet.*fee proposition' -g '*.rs' ergo-node ergo-*/src -C 2Repository: arkadianet/ergo
Length of output: 16774
Use the active network’s fee proposition bytes here. rank_pool_by_fee_per_byte hardcodes MAINNET_FEE_PROPOSITION_BYTES (ergo-node/src/api_bridge/scala_compat/pool_fee_stats.rs:57-87), but the bridge already handles mainnet/testnet elsewhere. On testnet this will treat every pool tx as zero-fee and break the fee histogram/recommendation endpoints.
🤖 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-node/src/api_bridge/scala_compat/pool_fee_stats.rs` around lines 57 -
87, The rank_pool_by_fee_per_byte function must use the active network’s fee
proposition bytes instead of MAINNET_FEE_PROPOSITION_BYTES. Thread or obtain the
existing network configuration used by the bridge, select its mainnet/testnet
fee proposition bytes, and use that value when filtering transaction outputs
while preserving the current ranking behavior.
| let (best_header_parent, best_header_ts, best_header_score) = state | ||
| .store | ||
| .get_header_meta(&cs_best_header_id) | ||
| .ok() | ||
| .flatten() | ||
| .map(|m| (m.parent_id, m.timestamp, m.cumulative_score)) | ||
| .unwrap_or(([0u8; 32], 0, Vec::new())); | ||
| let (best_full_parent, best_full_ts, best_full_block_score) = state | ||
| .store | ||
| .get_header_meta(&cs_best_full_block_id) | ||
| .ok() | ||
| .flatten() | ||
| .map(|m| (m.parent_id, m.timestamp, m.cumulative_score)) | ||
| .unwrap_or(([0u8; 32], 0, Vec::new())); | ||
| // Difficulty surface: HeaderMeta doesn't carry `n_bits`, so deserialize | ||
| // each tip header for it. Height 0 = no tip yet (legit 0); for a real | ||
| // tip a failure is a store/serializer fault, logged with its cause, and | ||
| // `None` tells the publisher to carry the last-known value forward | ||
| // (never a synthetic 0). | ||
| let best_header_n_bits = read_tip_n_bits( | ||
| state, | ||
| &cs_best_header_id, | ||
| cs_best_header_height, | ||
| "best_header", | ||
| ); | ||
| let best_full_block_n_bits = read_tip_n_bits( | ||
| state, | ||
| &cs_best_full_block_id, | ||
| cs_best_full_block_height, | ||
| "best_full_block", | ||
| ); | ||
| // Ergo genesis is height 1 in HEADER_CHAIN_INDEX (h=0 is never written — | ||
| // see `rewrite_best_chain_into_index` walk terminating at cur_height == 1). | ||
| // Matches Scala's chain numbering. | ||
| let genesis_block_id = state | ||
| .store | ||
| .get_header_id_at_height(1) | ||
| .ok() | ||
| .flatten() | ||
| .unwrap_or([0u8; 32]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid publishing synthetic chain metadata on read failures.
Lines 78-117 collapse both storage errors and absent records into zero parent IDs, timestamps, scores, and genesis IDs while retaining live tip heights and IDs. This creates an internally inconsistent snapshot, and downstream /info consumers use these values without fallback.
Handle the empty-chain case separately; for a live chain, log the fault and retain the previous snapshot or skip this publication.
🤖 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-node/src/node/snapshot_emit/mod.rs` around lines 78 - 117, Update the
snapshot metadata assembly around get_header_meta and genesis_block_id so
empty-chain state is handled explicitly, while live-chain lookup failures are
treated as publication failures. Do not replace missing or errored records with
zero IDs, timestamps, scores, or genesis IDs when live tip heights exist; log
the storage fault and retain the previous snapshot or skip publication. Preserve
the existing read_tip_n_bits behavior and valid metadata for successful lookups.
Flagged in PR review, both pre-existing (unmodified by the messaging.rs split) and both narrowly scoped to this file: - GetNipopowProof serving called `state.store.as_utxo().expect(...)` after only ruling out PoPowSparse header availability, which doesn't by itself prove the backend is UTXO. Decline gracefully (empty response) instead of asserting the combination can never reach here from peer input. - The tx-typed CODE_MODIFIER branch called `coordinator.on_transaction_received(peer, &mod_id)` trusting the peer's claimed `mod_id` without verifying it against the delivered bytes -- unlike the section-type branch just below it, which already has an analogous `verify_section_modifier_id` check. A peer substituting different bytes under a requested id could poison the delivery tracker's bookkeeping (clearing `late_acceptable` for the REAL awaited tx) even though `admit_transaction` itself derives its own tx_id from the bytes and stays correct either way. Added `claimed_tx_id_matches` (parse + `transaction_id` recompute + compare) and penalize-and-skip on mismatch, mirroring the existing section-type pattern. Covered with 3 focused unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
…nly after verification Flagged in PR review, pre-existing behavior unmodified by the messaging.rs split: the ERGO_CAPTURE_NIPOPOW_PROOF one-shot capture hook fired right after the outer wire-frame parse, before the proof structure was even parsed and before the reducer's genesis/validation check ran -- a malformed or wrong-genesis proof could get written to disk and mistaken for a clean Scala-served fixture. Moved the capture into the BetterChain/NoBetterChain result arms, i.e. after wire-frame parse, structure parse, AND the reducer/verifier's genesis + validation checks all succeed. Factored into maybe_capture_verified_proof to avoid duplicating the write logic across both arms. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
…post-install catch-up panel Flagged in PR review, pre-existing behavior unmodified by the snapshot split: the Mode 2 post-install catch-up branch of the bootstrap dashboard reported snapshot_height as the LIVE best_full_block_height and manifest_id as None -- but best_full_block_height keeps climbing every tick as catch-up applies blocks, so the dashboard's "snapshot height" field misreported a live, advancing value as if it were the fixed height the snapshot was actually installed at. manifest_id was always None because the snapshot-bootstrap reducer clears its own copy post-install. Added NodeState.installed_snapshot: Option<(u32, [u8; 32])>, latched by install_reconstructed_snapshot at the moment install succeeds -- before the reducer clears its copy. build_bootstrap_status's in_catchup branch now reads this fixed value instead of the advancing best_full, falling back to the prior best-effort proxy only in the (should-be-unreachable) case where post_install_catchup is true but the field was never set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
…okups Flagged in PR review, pre-existing behavior unmodified by the snapshot split: the tip header_meta lookups (best_header/best_full_block parent_id/timestamp/cumulative_score) and the genesis_block_id lookup all silently fell back to the same zeroed shape on ANY failure -- both the legitimate "height 0, nothing committed yet" case and a genuine store fault on a live, non-empty chain. The latter would be indistinguishable from the former to an operator watching the dashboard: a real storage problem reads identically to "chain hasn't started yet". Added header_meta_or_default (mirroring read_tip_n_bits's existing height-0-vs-real-fault distinction, right above it in this same file) and an explicit height-gated match for genesis_block_id, both now logging at error level when height > 0 but the lookup misses or errors. The fallback VALUE is unchanged (still zeroed) -- extending these fields to the full carry-forward-or-skip-publish treatment n_bits already has (see the companion publisher.rs fix) would mean threading Option through SnapshotParts/NodeSnapshot/build_snapshot for seven more fields, a separate, larger piece of work better done as its own dedicated pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
… rollback Flagged in PR review, pre-existing behavior unmodified by the snapshot split, two related correctness bugs in the same publish() method: 1. n_bits carry-forward on read failure only checked `height > 0 && prior_n_bits == 0` before reusing the prior snapshot's difficulty -- it never compared the CURRENT tip id against the PRIOR snapshot's tip id. A read failure on a tip that had already advanced to a genuinely different block would silently relabel the OLD tip's difficulty as belonging to the NEW one. Now compares hex::encode(parts.best_header_id)/best_full_block_id against the prior snapshot's stored tip id; carry-forward only fires when the tip is unchanged, otherwise the publish is skipped (retries next tick) exactly as the existing "no real prior to carry" case already does. 2. The stall-clock baselines (last_header_height/last_full_block_height) only ever moved forward. A reorg dropping the height below the last-observed value left the baseline pinned at the pre-reorg height, so re-applying blocks back through the rolled-back range never registered as "advanced" relative to the stale baseline -- last_block_progress_at would sit frozen for the whole recovery replay, and an actively, successfully recovering node could misreport itself as stalled if that replay took longer than STALL_THRESHOLD_SECS. Now resets the baseline DOWN when a rollback is detected, before the advanced checks run, so recovery progress is recognized again. Updated the existing carry-forward test to use explicit matching tip ids (documenting the same-tip case it actually exercises), and added two new tests: one proving a different-tip read failure skips publish rather than mislabeling stale difficulty, one proving rollback-recovery progress refreshes the stall clock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
|
Went through the review findings against current code. Verified all 8 by diffing against the pre-refactor originals — every one describes pre-existing behavior, byte-identical before and after this PR's mechanical moves. None were introduced by the split itself. Fixed (7, pushed as new commits
Partially addressed, not fully — flagged for scope reasons:
Skipped (1) — not valid:
Full gate re-verified after all fixes: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Resolves the modify/delete conflict on ergo-node/src/node/boot.rs: PR #205 (merged to main) split boot.rs into boot/{mod,peers,sync_setup,mining, api_wiring}.rs; this branch's earlier review-response commit had added NodeState::installed_snapshot's initializer to the old monolithic boot.rs. Ported that one-line addition (installed_snapshot: None,) to the corresponding NodeState-construction site in the new boot/mod.rs. Everything else merged cleanly (zero file overlap between the two split PRs by design).
|
Resolved the merge conflict with `main` (`bfa3e66`): #205 merged first and its split of `boot.rs` → `boot/{mod,peers,sync_setup,mining,api_wiring}.rs` collided with this branch's earlier `NodeState::installed_snapshot` initializer, which had been added to the old monolithic `boot.rs`. Ported that one-line addition to the corresponding spot in the new `boot/mod.rs`; everything else merged cleanly (the two split PRs touch disjoint files by design). Re-ran the full gate post-merge: `cargo test -p ergo-node` (506 passed incl. #205's own review-fix test + full integration suite), `cargo clippy -p ergo-node --all-targets --all-features -- -D warnings` clean, `cargo fmt --all -- --check` clean. PR now shows clean/mergeable. Re-verified all 8 inline findings against current code — these are the same findings from the previous review round, and all the fixable ones were already addressed in commits `4a0abf3`/`56d589f`/`430975a`/`81ad88f`/`ebfce45` (still intact post-merge, confirmed above):
No new changes needed this round beyond the merge conflict resolution. |
1 participant
Summary
Second of two PRs splitting the largest/most tangled files in
ergo-node(split from #204 to stay under coderabbitai's per-PR file-count limit — see #205 for the first half: wallet_bridge/boot). Same methodology used for theergo-compilersplits (#200, #202): incremental extraction with test verification after each step, red-team review of every commit. Independent of #205 -- zero file overlap between the two PRs, so either can merge first.node/snapshot_emit.rs+snapshot.rs: split intonode/snapshot_emit/{mod,recent_blocks,bootstrap_panel,mempool_projection,events_projection}.rs(mod.rs keepspublish_snapshotas orchestrator) andsnapshot/{mod,build,publisher}.rs(theNodeSnapshot/SnapshotPartsDTOs stay whole inmod.rs— splitting a single wide DTO struct across files hurts more than it helps).api_bridge/scala_compat.rs: light touch only — the trait impl (NodeChainQuery for ScalaCompatBridge) is one large block of small independent read-only methods, not worth forcing apart. Only the ~300-line fee-per-byte pool-ranking cluster moved toscala_compat/pool_fee_stats.rs.node/messaging.rs: extracted the three sizable per-batch match arms (CODE_INV,CODE_MODIFIER~190 lines,CODE_PEERS) into named helpers (handle_inv,handle_modifier_batch,handle_peers_response) following the same&mut NodeState -> Vec<Action>convention ashandle_message/run_wallet_writer. Split intonode/messaging/{dispatch,manifest,utxo_chunk,popow}.rs.Every commit was independently verified (
cargo test -p ergo-nodeincluding all integration tests,cargo clippy -p ergo-node --all-targets --all-features -- -D warnings,cargo fmt --all -- --check) and red-team reviewed by an independent Opus pass diffing against the pre-refactor original — no behavior-changing findings survived across any of the 3 splits. The whole-workspace gate (cargo clippy --workspace --all-targets --all-features -- -D warningsandcargo test --workspace) is also green.Purely structural -- no functional changes intended.
Test plan
cargo test -p ergo-node(500 lib tests + full integration suite, all green) after each splitcargo clippy -p ergo-node --all-targets --all-features -- -D warningscleancargo fmt --all -- --checkcleancargo clippy --workspace --all-targets --all-features -- -D warningsclean (whole-workspace gate)cargo test --workspaceclean (whole-workspace gate, every crate)🤖 Generated with Claude Code
https://claude.ai/code/session_01Kw7uCwqmiGna8fpg6TvxGi
Summary by CodeRabbit