feat(cardano): log per-account epoch stake distribution - #1145
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds an ChangesAccount stake log persistence and export
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RupdWorkUnit
participant AccountStakeLog
participant PendingRewardState
participant RupdProgress
participant PoolStakeLog
RupdWorkUnit->>AccountStakeLog: Write account snapshot rows
RupdWorkUnit->>PendingRewardState: Persist pending rewards
RupdWorkUnit->>RupdProgress: Commit progress after shard writes
RupdWorkUnit->>PendingRewardState: Read persisted rewards at finalization
RupdWorkUnit->>PoolStakeLog: Write recomputed pool totals
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
🟡 Not ready to approve
The new test module has a duplicate StakeSnapshot import that should fail compilation, and the dump tool currently risks printing a valid-looking but incorrect stake address on decode failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a new Cardano data-layer log to persist per-account active stake distribution per epoch, enabling upcoming Blockfrost /epochs/{n}/stakes and /epochs/{n}/stakes/{pool} endpoints by emitting AccountStakeLog entries from the RUPD pipeline and exposing them via existing log-dump tooling.
Changes:
- Introduces
AccountStakeLog { amount, pool_id }under theaccount-stakesnamespace and registers it in the Cardano model schema/entity decoding. - Writes per-account stake distribution logs during
rupd::RupdWorkUnitshard commits (archive-first relative to progress cursor advancement) and adds fault-injection tests around the crash window. - Extends
dolos data dump-logsto support dumpingaccount-stakeslogs (including a db-sync compatible CSV format).
File summaries
| File | Description |
|---|---|
| src/bin/dolos/data/dump_logs.rs | Adds table formatting + CLI support for dumping account-stakes logs. |
| crates/cardano/src/rupd/work_unit.rs | Emits per-account stake logs during RUPD shard commit; adds tests for idempotency and crash-window ordering. |
| crates/cardano/src/model/mod.rs | Registers AccountStakeLog in CardanoEntity and schema so typed log reads/writes can decode/encode it. |
| crates/cardano/src/model/logs.rs | Defines the new AccountStakeLog log type and namespace. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| use pallas::{codec::minicbor, crypto::hash::Hash, ledger::primitives::StakeCredential}; | ||
|
|
||
| use super::*; | ||
| use crate::rupd::StakeSnapshot; |
| let credential = decode_stake_credential(&entity) | ||
| .unwrap_or_else(|_| StakeCredential::AddrKeyhash([0; 28].into())); | ||
| let stake = pallas_extras::stake_credential_to_address(ctx.network, &credential) | ||
| .to_bech32() | ||
| .unwrap_or_else(|_| "<invalid>".to_string()); |
bdc9c3f to
192d594
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The RUPD implementation now performs archive writes during commit_state, which conflicts with the documented WorkUnit phase contract and should be reconciled (and the tests should avoid hard-coded shard counts).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
crates/cardano/src/rupd/work_unit.rs:710
TOTAL_SHARDSis hard-coded to 32, which can drift from the production sharding factor (ACCOUNT_SHARDS). Using the shared constant keeps the test aligned if sharding changes.
const CURRENT_EPOCH: u64 = 5;
const EPOCH_LENGTH: u64 = 100;
const TOTAL_SHARDS: u32 = 32;
crates/cardano/src/rupd/work_unit.rs:443
commit_stateis performing archive writes (per-account logs) even though theWorkUnitlifecycle docs describe archive persistence happening incommit_archive. This cross-phase store mutation is easy to miss for future maintainers and could lead to incorrect assumptions about where archive side effects occur; consider updating the lifecycle contract (or adding a dedicated pre-cursor archive phase) so the ordering requirement is enforced by the framework rather than by this work unit.
// Per-account AccountStakeLog entries are written by commit_state, not
// here: this phase runs *after* the state commit that advances
// `rupd_progress`, and a resumed RUPD would skip the shard before
// these rows were ever written. See the archive block in
// `commit_state`.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
192d594 to
11c9dce
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The current log keying optimizes epoch-wide scans and per-account reads but makes pool-scoped epoch queries inherently full-scan unless a secondary index/additional pool-keyed structure is added or explicitly deferred.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
crates/cardano/src/model/logs.rs:45
- The log key is (epoch temporal key, stake credential), which makes epoch-wide scans efficient and per-account reads O(1), but it also means a pool-scoped epoch query (needed for
/epochs/{n}/stakes/{pool}) can’t be satisfied via a prefix range and would require scanning all accounts in the epoch and filtering bypool_idunless you add a secondary index or an additional pool-keyed log.
/// Written by RUPD under the same temporal key as the per-pool [`StakeLog`]
/// (the epoch whose active stake the snapshot describes), keyed by the
/// account's credential. The pool the account delegated to lives in the
/// value, so a per-account history lookup stays a point read per epoch while
/// an epoch-wide distribution stays a single prefix scan.
crates/cardano/src/rupd/work_unit.rs:169
- This comment says the archive rows are committed before the state transaction, but the state transaction is opened earlier; what matters for the durability guarantee is that the archive commits before the state transaction commits. Tweaking wording avoids confusion when reasoning about crash windows.
/// The archive rows are committed before the state transaction — see the
/// comment on the archive block for why the cursor must go last.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
11c9dce to
bb5178f
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The new log type is consistently wired through schema/entity layers and the RUPD durability ordering change is backed by focused tests covering the identified crash window.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cardano/src/rupd/work_unit.rs`:
- Around line 283-323: Ensure pool_log_shares survives shard progress
advancement and process restarts: persist it with the committed shard state, or
recompute it in initialize()/finalize() from persisted PendingRewardState before
StakeLog emission. Update the RupdWorkUnit lifecycle around commit_state,
RupdWorkUnit::new(), initialize(), and finalize() so already-committed shards
contribute their rewards and delegator counts exactly once.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13245738-a04e-48fc-8937-de0f7c579617
📒 Files selected for processing (5)
crates/cardano/src/model/logs.rscrates/cardano/src/model/mod.rscrates/cardano/src/rupd/work_unit.rscrates/core/src/work_unit.rssrc/bin/dolos/data/dump_logs.rs
bb5178f to
c42bc12
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The new shard commit path holds a state write transaction open while committing potentially large archive log batches (and the CLI pool-id encoding should validate length), which should be addressed to avoid operational issues and misleading output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
src/bin/dolos/data/dump_logs.rs:186
bech32::encodewill successfully encode any byte length, so a corrupted or incorrectly-sizedpool_idcould still render as a plausible-looking pool bech32 string instead of<invalid>. Since this code explicitly tries to avoid silently mislabeling data, it should validate the expected pool hash length (28 bytes) before encoding.
let pool = bech32::encode::<bech32::Bech32>(POOL_HRP, &self.pool_id)
.unwrap_or_else(|_| "<invalid>".to_string());
crates/cardano/src/rupd/work_unit.rs:257
- This comment says the archive rows are committed before the “state transaction below”, but the state write transaction is opened earlier (
let writer = state.start_writer()?;). Consider rewording to avoid implying the transaction begins later; the important guarantee is that the archive commit happens before the state commit that advancesrupd_progress.
// ---- Archive: per-account AccountStakeLog entries ----
//
// Committed *before* the state transaction below, because that
// transaction advances `rupd_progress` and a resumed RUPD skips every
crates/cardano/src/rupd/work_unit.rs:225
state.start_writer()(redb backend begins a write transaction immediately) happens before writing and committing the per-account archive logs. This keeps the state write transaction open while potentially emitting a very large number ofAccountStakeLogrows, which can increase lock contention and overall commit latency. Consider committing the archive rows first (and only then opening the state writer to persistPendingRewardState+ the progress cursor) so the state write transaction is held for the shortest time possible while preserving the durability ordering.
let writer = state.start_writer()?;
// Persist this shard's pending rewards as PendingRewardState
// entities. Writes are overwrite-by-key, so a crashed shard
// re-run is idempotent.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cardano/src/rupd/work_unit.rs (1)
504-530: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrite the per-pool
StakeLogentries before committingEpochState.
finalizewritesEpochState.incentivesand clearsrupd_progressbeforewrite_stake_logs. A crash in that window leaves RUPD complete with no per-pool log rows, andRupdWorkUnit::initializeresumes fromrupd_progressso it will not regenerate them.🛡️ Proposed fix for the commit ordering
debug!(slot = self.slot, "finalizing rupd"); + // ---- Archive: per-pool StakeLog entries ---- + // + // Written before the state commit below, for the same reason as the + // account logs in `commit_shard`: that commit clears `rupd_progress` + // and marks this RUPD complete, and nothing re-derives these rows. + self.write_stake_logs(domain.state(), domain.archive())?; + // ---- State: write incentives once and clear rupd_progress ---- @@ writer.commit()?; - // ---- Archive: per-pool StakeLog entries ---- - self.write_stake_logs(domain.state(), domain.archive())?; - debug!("rupd finalize committed"); Ok(())🤖 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/cardano/src/rupd/work_unit.rs` around lines 504 - 530, Reorder the finalize flow so write_stake_logs runs before the EpochState writer commits the incentives update and clears rupd_progress. Ensure the per-pool StakeLog archive succeeds before marking the RUPD complete, while preserving the existing final commit and completion behavior.
🧹 Nitpick comments (4)
crates/cardano/src/rupd/work_unit.rs (4)
67-79: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid a
Vecallocation per pending-reward record.
into_log_entries()allocates aVecfor everyPendingRewardState. This loop runs once over the whole namespace, which is millions of records on mainnet. Iterate the two reward vectors directly instead.♻️ Proposed refactor
- for (pool, value, as_leader) in pending.into_log_entries() { - let (total_rewards, operator_share) = out.entry(pool).or_insert((0, 0)); - - *total_rewards = total_rewards.saturating_add(value); - - if as_leader { - *operator_share = operator_share.saturating_add(value); - } - } + for (pool, value) in pending.as_leader.iter() { + let (total_rewards, operator_share) = out.entry(*pool).or_insert((0, 0)); + *total_rewards = total_rewards.saturating_add(*value); + *operator_share = operator_share.saturating_add(*value); + } + + for (pool, value) in pending.as_delegator.iter() { + let (total_rewards, _) = out.entry(*pool).or_insert((0, 0)); + *total_rewards = total_rewards.saturating_add(*value); + }🤖 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/cardano/src/rupd/work_unit.rs` around lines 67 - 79, Replace the per-record into_log_entries() call in the PendingRewardState iteration with direct iteration over its two reward vectors, processing operator and leader rewards separately while preserving the existing out totals and saturating-add behavior. Avoid constructing any intermediate Vec for each pending record.
221-281: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider opening the state writer after the archive commit.
The state write transaction stays open across
archive_writer.commit(). The stores are separate, so this is correct, but it holds the state write lock for the duration of the archive commit. Moving the archive block abovestate.start_writer()keeps the same archive-before-state ordering and shortens the state lock hold.🤖 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/cardano/src/rupd/work_unit.rs` around lines 221 - 281, The state writer is opened before the archive commit, unnecessarily holding the state write lock during archive persistence. In the shard-processing function, move the `state.start_writer()?` initialization and pending-reward write loop to after the `account_stake_logs` archive block and its `archive_writer.commit()`, preserving archive-before-state ordering and the existing write behavior.
963-975: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
progress_advances_when_there_is_no_snapshot_to_logdoes not exercise theNonebranch.The test sets
current_epoch = 1and passesStakeSnapshot::empty(). The assertionread_epoch(...).is_empty()holds whetherrelevant_epochs()returnsNoneorSome, because the snapshot has no accounts either way. Assert onrelevant_epochs()directly, or use a snapshot with accounts, so the test fails if the epoch guard is removed.💚 Proposed assertion
let (mut unit, chain) = loaded_work_unit(domain.genesis(), StakeSnapshot::empty()); unit.work.as_mut().unwrap().current_epoch = 1; + assert!( + unit.work.as_ref().unwrap().relevant_epochs().is_none(), + "test must drive the no-snapshot branch" + );🤖 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/cardano/src/rupd/work_unit.rs` around lines 963 - 975, Update progress_advances_when_there_is_no_snapshot_to_log so it directly verifies the no-snapshot path: assert that relevant_epochs() returns None (or use a populated snapshot and assert the corresponding absence). Keep the existing commit_shard progress assertion, but replace the empty read_epoch check because it cannot distinguish None from Some(empty).
353-364: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPopulate
live_pledgefrompool_live_pledges.
load_globalsalready computesStakeSnapshot::pool_live_pledgesover every account, andStakeLog::live_pledgeis user-visible. Writing it as0makes the rebuilt stake log omit the current pledge while other fields come from snapshot data.♻️ Proposed change
let log = StakeLog { blocks_minted, total_stake: pool_stake, relative_size, - live_pledge: 0, + live_pledge: snapshot + .pool_live_pledges + .get(pool_hash) + .copied() + .unwrap_or(0), declared_pledge,🤖 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/cardano/src/rupd/work_unit.rs` around lines 353 - 364, Update the StakeLog construction in the work-unit rebuild flow to populate live_pledge from the corresponding pool_live_pledges entry for pool_hash instead of hardcoding 0. Preserve the existing snapshot-derived behavior and use the appropriate fallback if no entry exists.
🤖 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.
Outside diff comments:
In `@crates/cardano/src/rupd/work_unit.rs`:
- Around line 504-530: Reorder the finalize flow so write_stake_logs runs before
the EpochState writer commits the incentives update and clears rupd_progress.
Ensure the per-pool StakeLog archive succeeds before marking the RUPD complete,
while preserving the existing final commit and completion behavior.
---
Nitpick comments:
In `@crates/cardano/src/rupd/work_unit.rs`:
- Around line 67-79: Replace the per-record into_log_entries() call in the
PendingRewardState iteration with direct iteration over its two reward vectors,
processing operator and leader rewards separately while preserving the existing
out totals and saturating-add behavior. Avoid constructing any intermediate Vec
for each pending record.
- Around line 221-281: The state writer is opened before the archive commit,
unnecessarily holding the state write lock during archive persistence. In the
shard-processing function, move the `state.start_writer()?` initialization and
pending-reward write loop to after the `account_stake_logs` archive block and
its `archive_writer.commit()`, preserving archive-before-state ordering and the
existing write behavior.
- Around line 963-975: Update progress_advances_when_there_is_no_snapshot_to_log
so it directly verifies the no-snapshot path: assert that relevant_epochs()
returns None (or use a populated snapshot and assert the corresponding absence).
Keep the existing commit_shard progress assertion, but replace the empty
read_epoch check because it cannot distinguish None from Some(empty).
- Around line 353-364: Update the StakeLog construction in the work-unit rebuild
flow to populate live_pledge from the corresponding pool_live_pledges entry for
pool_hash instead of hardcoding 0. Preserve the existing snapshot-derived
behavior and use the appropriate fallback if no entry exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3648d5f5-501f-474b-840c-47775386efbe
📒 Files selected for processing (7)
crates/cardano/src/model/logs.rscrates/cardano/src/model/mod.rscrates/cardano/src/rupd/loading.rscrates/cardano/src/rupd/mod.rscrates/cardano/src/rupd/work_unit.rscrates/core/src/work_unit.rssrc/bin/dolos/data/dump_logs.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/core/src/work_unit.rs
- crates/cardano/src/model/logs.rs
- src/bin/dolos/data/dump_logs.rs
- crates/cardano/src/model/mod.rs
c42bc12 to
4ad74ec
Compare
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/cardano/src/rupd/work_unit.rs (1)
516-536: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
finalizeclearsrupd_progressbefore it writes the per-poolStakeLogrows.Line 525 sets
rupd_progress = Noneand line 529 commits that state change.write_stake_logsruns afterwards at line 532. A crash in that window leaves the RUPD recorded as complete with noStakeLogrows for the epoch, and nothing re-runsfinalize.The rows are not recoverable later. They derive from
PendingRewardStateplus the snapshot globals, and Ewrap dequeues the pending rewards at the epoch boundary. After that, the inputs are gone.This is the same failure the PR inverts for the account logs in
commit_shard, and it contradicts the rule thecommit_archivecomment cites at lines 496-498: everything a phase implies must be durable before the cursor advances. Move the archive write ahead of the state commit.🔧 Proposed fix: write the pool logs before clearing the cursor
+ // ---- Archive: per-pool StakeLog entries ---- + // + // Committed before the state commit that clears `rupd_progress`, for + // the same reason as the account logs in `commit_shard`: once the + // cursor is cleared this RUPD never runs `finalize` again, and Ewrap + // dequeues the `PendingRewardState` rows these figures derive from. + self.write_stake_logs(domain.state(), domain.archive())?; + // ---- State: write incentives once and clear rupd_progress ---- // // Per-shard `commit_state` writes the `PendingRewardState` entities // and advances `rupd_progress`. The single `EpochState.incentives` // write happens here, after every shard has landed, so concurrent // shard commits can't race on this field. let writer = domain.state().start_writer()?; @@ writer.commit()?; - // ---- Archive: per-pool StakeLog entries ---- - self.write_stake_logs(domain.state(), domain.archive())?; - debug!("rupd finalize committed"); Ok(()) }
write_stake_logswrites are overwrite-by-key, so a crash after the archive commit and before the state commit is harmless:finalizere-runs and rewrites the same rows.🤖 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/cardano/src/rupd/work_unit.rs` around lines 516 - 536, Reorder finalize so write_stake_logs completes before the epoch state clears rupd_progress and writer.commit advances the completion state. Move the existing archive-write call ahead of the state commit while preserving the current overwrite-and-rerun behavior and final success logging.
🤖 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/cardano/src/rupd/work_unit.rs`:
- Around line 359-370: Replace the hardcoded live_pledge value in the StakeLog
construction with the snapshot’s pool live pledge. Add a get_pool_live_pledge
accessor to StakeSnapshot alongside get_pool_stake and get_pool_delegator_count,
then use it with pool_hash in the work-unit logging path so restored snapshots
emit the actual pledge.
---
Outside diff comments:
In `@crates/cardano/src/rupd/work_unit.rs`:
- Around line 516-536: Reorder finalize so write_stake_logs completes before the
epoch state clears rupd_progress and writer.commit advances the completion
state. Move the existing archive-write call ahead of the state commit while
preserving the current overwrite-and-rerun behavior and final success logging.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9139b3f0-8bfd-4497-abf9-56aea561cf1d
📒 Files selected for processing (7)
crates/cardano/src/model/logs.rscrates/cardano/src/model/mod.rscrates/cardano/src/rupd/loading.rscrates/cardano/src/rupd/mod.rscrates/cardano/src/rupd/work_unit.rscrates/core/src/work_unit.rssrc/bin/dolos/data/dump_logs.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/cardano/src/rupd/loading.rs
- crates/core/src/work_unit.rs
- crates/cardano/src/model/logs.rs
- crates/cardano/src/rupd/mod.rs
- src/bin/dolos/data/dump_logs.rs
- crates/cardano/src/model/mod.rs
| let log = StakeLog { | ||
| blocks_minted, | ||
| total_stake: pool_stake, | ||
| relative_size, | ||
| live_pledge: 0, | ||
| declared_pledge, | ||
| delegators_count: snapshot.get_pool_delegator_count(pool_hash), | ||
| total_rewards, | ||
| operator_share, | ||
| fixed_cost, | ||
| margin_cost, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
live_pledge is hardcoded to 0 although the snapshot carries the value.
StakeSnapshot populates pool_live_pledges during the globals pass in crates/cardano/src/rupd/loading.rs (the snapshot.pool_live_pledges.entry(*pool) accumulation). initialize() rebuilds that map, so it is available here and survives a restart, exactly like pool_stake and pool_delegator_counts. Emitting 0 makes the field unusable for the /epochs/{n}/stakes/{pool} consumer added in the follow-up PR.
If leaving it at 0 is intentional for this PR, add a short comment stating that, so the next reader does not treat 0 as a real pledge value.
🔧 Proposed fix
let log = StakeLog {
blocks_minted,
total_stake: pool_stake,
relative_size,
- live_pledge: 0,
+ live_pledge: snapshot.get_pool_live_pledge(pool_hash),
declared_pledge,This needs a get_pool_live_pledge accessor on StakeSnapshot, alongside the existing get_pool_stake and get_pool_delegator_count.
🤖 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/cardano/src/rupd/work_unit.rs` around lines 359 - 370, Replace the
hardcoded live_pledge value in the StakeLog construction with the snapshot’s
pool live pledge. Add a get_pool_live_pledge accessor to StakeSnapshot alongside
get_pool_stake and get_pool_delegator_count, then use it with pool_hash in the
work-unit logging path so restored snapshots emit the actual pledge.
There was a problem hiding this comment.
🟡 Not ready to approve
There is a confirmed performance issue in the new per-pool reward aggregation loop (avoidable per-record allocations) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
crates/cardano/src/rupd/work_unit.rs:79
aggregate_pending_pool_rewardsallocates a new Vec for everyPendingRewardStateviapending.into_log_entries(). On mainnet-scale reward sets this adds substantial avoidable allocation/CPU overhead; you can iterateas_leaderandas_delegatordirectly and update the per-pool totals without building an intermediate vector.
for (pool, value, as_leader) in pending.into_log_entries() {
let (total_rewards, operator_share) = out.entry(pool).or_insert((0, 0));
*total_rewards = total_rewards.saturating_add(value);
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
4ad74ec to
0e21525
Compare
There was a problem hiding this comment.
🟡 Human review recommended
It changes persistence/ordering semantics in a critical sharded ledger pipeline (RUPD) and adds new durable archive outputs whose correctness under restart scenarios warrants final human verification.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Data layer for
/epochs/{n}/stakes(#1082) and/epochs/{n}/stakes/{pool}(#1101). Endpoints follow in a separate PR.New
AccountStakeLog { amount, pool_id }, namespaceaccount-stakes, written by RUPD per shard.Summary by CodeRabbit
New Features
account-stakeslog exports in default and db-sync formats, including readable stake and pool identifiers.Bug Fixes
Documentation