fix(cardano): clear delegators on retiring drep - #707
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe diff replaces raw DRep identifiers with typed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Loader as Sweep::load
participant Domain as DomainState
participant BW as BoundaryWork
participant SnapE as Snapshot(ending)
participant SnapA as Snapshot(active)
Loader->>Domain: fetch Accounts, Pools, DReps
Domain-->>Loader: AccountState{latest_pool, latest_drep}, DRepState...
note right of Loader: map latest_drep -> DRepId via drep_to_entity_key
Loader->>SnapE: track_account(acct, pool_id?, drep_id?, live_stake)
Loader->>SnapA: track_account(acct, pool_id?, drep_id?, active_stake)
Loader->>BW: insert dreps map (DRepId -> DRepState)
sequenceDiagram
autonumber
participant Compute as Sweep::compute
participant PParams as ProtocolParams
participant DReps as DRepState Map
participant BW as BoundaryWork
Compute->>PParams: active_drep_inactivity_period()
PParams-->>Compute: inactivity_period
Compute->>DReps: scan last_activity timestamps
alt inactive > period
Compute->>BW: mark retired_dreps += drep_id
Compute->>BW: add delegators -> dropped_drep_delegators
else active
note right of Compute: no retirement
end
sequenceDiagram
autonumber
participant Commit as Sweep::commit
participant BW as BoundaryWork
participant Accounts as Account store
participant DReps as DRepState store
Commit->>BW: iterate dropped_pool_delegators
BW-->>Commit: list
Commit->>Accounts: clear latest_pool for each
Commit->>BW: iterate dropped_drep_delegators
BW-->>Commit: list
Commit->>Accounts: clear latest_drep for each
Commit->>DReps: mark retired states in store for retired_dreps
sequenceDiagram
autonumber
participant Tx as Roll::apply
participant AS as AccountState
Tx->>AS: prev = AS.latest_drep
alt delegation
Tx->>AS: AS.latest_drep = Some(new_drep)
else deregistration
Tx->>AS: AS.latest_drep = None
end
note right of Tx: undo restores prev into latest_drep
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/roll/dreps.rs (1)
60-71: Setdrep_idon DRepState to keep invariants.
DRepState::has_script()usesdrep_id.first().unwrap(). Without settingdrep_id, this can panic on default entities created viaget_or_insert_default().Apply:
fn apply(&mut self, entity: &mut Option<DRepState>) { let entity = entity.get_or_insert_default(); // save undo info self.was_retired = entity.retired; // apply changes + entity.drep_id = drep_to_entity_key(self.drep.clone()).as_ref().to_vec(); entity.initial_slot = Some(self.slot); entity.voting_power = self.deposit; entity.retired = false; }
🧹 Nitpick comments (9)
src/bin/dolos/data/dump_state.rs (1)
60-61: Column shows only presence; consider making header explicit or rendering the actual DRep.Right now it prints a boolean for latest_drep. Either rename the column to reflect that it’s a presence flag, or render the DRep id (bech32) for better operator utility.
Example header tweak (outside the changed lines):
- "drep", + "has drep",crates/minibf/src/routes/accounts.rs (1)
100-106: Avoid 500s for non-bech32 DRep variants (Abstain/NoConfidence).If bech32_drep doesn’t handle special Conway variants, this will bubble up as an error and 500 the endpoint. Consider falling back to None (or a stable literal) instead of erroring.
Proposed change:
- let drep_id = self - .account_state - .latest_drep - .as_ref() - .map(bech32_drep) - .transpose()?; + let drep_id = match self.account_state.latest_drep.as_ref() { + Some(d) => bech32_drep(d).ok(), // treat unsupported variants as None + None => None, + };Please confirm whether mapping::bech32_drep already encodes Abstain/NoConfidence; if so, ignore this.
crates/cardano/src/roll/accounts.rs (1)
195-207: Do we also need to track active_drep?AccountState now has active_drep (per PR). Unlike pools (active_pool <- latest_pool at boundary), active_drep isn’t updated anywhere in these changes. If clients rely on active_drep, this may yield stale/empty data.
If intended to mirror pool behavior at epoch boundaries, update it in the sweep rotation (see commit.rs suggestion).
Also applies to: 241-259
crates/cardano/src/pallas_extras.rs (1)
272-280: Document DRep prefixes and add a quick test.Add a brief note referencing the governing CIP for these prefixes; include a unit test asserting stake_cred_to_drep round-trips with your drep_to_entity_key.
I can draft doc comments and a tiny test if you confirm the CIP reference you’re following.
crates/cardano/src/sweep/commit.rs (1)
61-78: DRep retirement marking is fine; consider skipping no-op writes.You’re rewriting every DRepState, even unchanged. Micro-optimization: only write when retired flips to true.
- if self.retired_dreps.contains(&key) { - state.retired = true; - } - - writer.write_entity_typed::<crate::DRepState>(&key, &state)?; + let retire_now = self.retired_dreps.contains(&key); + if retire_now && !state.retired { + state.retired = true; + writer.write_entity_typed::<crate::DRepState>(&key, &state)?; + }crates/cardano/src/sweep/mod.rs (1)
68-71: Avoid reusing DelegatorMap keyed by PoolId for DRep delegations.
DelegatorMapis hard-typed toPoolId(alias toEntityKey). Using it foraccounts_by_drepcompiles but obscures intent and risks accidental mix-ups.Consider making it generic:
-#[derive(Debug, Default, Clone)] -pub struct DelegatorMap(HashMap<PoolId, HashMap<AccountId, u64>>); +#[derive(Debug, Default, Clone)] +pub struct DelegatorMapBy<K>(HashMap<K, HashMap<AccountId, u64>>); + +pub type PoolDelegatorMap = DelegatorMapBy<PoolId>; +pub type DRepDelegatorMap = DelegatorMapBy<DRepId>;…and update
Snapshotto usePoolDelegatorMap/DRepDelegatorMap.crates/cardano/src/sweep/compute.rs (2)
7-8: Remove unused import.
PoolStateis unused (CI warning). Drop it from the import list.Apply:
- DRepState, EpochState, EraProtocol, Nonces, PParamsSet, PoolState, + DRepState, EpochState, EraProtocol, Nonces, PParamsSet,
394-405: Saturate epoch arithmetic to avoid theoretical overflow.Use
saturating_addfor defensive arithmetic on epoch numbers.Apply:
- let retiring_epoch = last_activity_epoch as u64 + self.active_drep_inactivity_period()?; + let retiring_epoch = + (last_activity_epoch as u64).saturating_add(self.active_drep_inactivity_period()?);crates/cardano/src/model.rs (1)
780-791: Consistency between encoded DRep IDs andhas_script().
has_script()checks the first byte’s low bits; ensureDREP_*_PREFIXvalues match that convention. Alternatively, consider deriving script-ness by decoding the prefix explicitly to avoid bit-coupling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/cardano/src/model.rs(4 hunks)crates/cardano/src/pallas_extras.rs(1 hunks)crates/cardano/src/roll/accounts.rs(2 hunks)crates/cardano/src/roll/dreps.rs(9 hunks)crates/cardano/src/sweep/commit.rs(3 hunks)crates/cardano/src/sweep/compute.rs(5 hunks)crates/cardano/src/sweep/loading.rs(5 hunks)crates/cardano/src/sweep/mod.rs(5 hunks)crates/minibf/src/routes/accounts.rs(1 hunks)src/bin/dolos/data/dump_state.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
crates/cardano/src/pallas_extras.rs (1)
crates/cardano/src/roll/dreps.rs (3)
key(56-58)key(102-104)key(144-146)
crates/cardano/src/model.rs (2)
crates/cardano/src/roll/dreps.rs (3)
key(56-58)key(102-104)key(144-146)crates/core/src/state.rs (7)
key(124-124)key(438-440)from(16-21)from(25-27)from(31-33)from(49-54)from(68-70)
crates/cardano/src/sweep/compute.rs (2)
crates/cardano/src/lib.rs (2)
default(46-55)new(71-73)crates/cardano/src/roll/dreps.rs (3)
new(42-50)new(90-96)new(132-138)
crates/cardano/src/sweep/commit.rs (2)
crates/cardano/src/roll/dreps.rs (3)
key(56-58)key(102-104)key(144-146)crates/cardano/src/sweep/loading.rs (3)
domain(53-55)domain(83-85)domain(105-107)
crates/cardano/src/roll/dreps.rs (2)
crates/cardano/src/model.rs (5)
drep_to_entity_key(780-790)from(836-838)from(842-844)from(848-850)from(854-857)crates/cardano/src/pallas_extras.rs (1)
stake_cred_to_drep(275-280)
crates/cardano/src/roll/accounts.rs (1)
crates/cardano/src/model.rs (1)
undo(1099-1119)
crates/cardano/src/sweep/loading.rs (3)
crates/cardano/src/model.rs (5)
drep_to_entity_key(780-790)from(836-838)from(842-844)from(848-850)from(854-857)crates/cardano/src/eras.rs (2)
load_active_era(182-197)domain(183-185)crates/cardano/src/utils.rs (1)
mutable_slots(12-15)
🪛 GitHub Check: Check Build
crates/cardano/src/sweep/compute.rs
[warning] 7-7:
unused import: PoolState
🔇 Additional comments (20)
crates/cardano/src/roll/accounts.rs (2)
195-203: Switch to latest_drep looks correct.Prev value is captured and restored properly; cloning DRep avoids aliasing issues.
241-259: Clearing/restoring latest_drep on deregistration is correct.Matches expected semantics: stake deregistration should drop both pool and DRep delegations.
crates/cardano/src/sweep/commit.rs (3)
39-41: Good: pool delegator drop uses the specific set.Using dropped_pool_delegators avoids false positives; order ensures active_pool reflects the cleared latest_pool.
163-172: Order of operations: update DReps before epoch promotions is acceptable.This aligns with pool/account rotations; just ensure any consumers that compute dropped_drep_delegators did so from the same boundary snapshot.
31-59: End-to-end check — retiring DRep must clear delegators' latest_drep
- Static findings: rotation clears
state.latest_drep = Nonein crates/cardano/src/sweep/commit.rs:52;latest_drepis set/undone in crates/cardano/src/roll/accounts.rs (≈lines 199–206, 249–259).- Action required: run a targeted scenario or unit test that retires a DRep, invokes
rotate_account_stake_data, assertsAccountState.latest_drep == Nonefor affected delegators, and verifies downstream consumers (crates/minibf/src/mapping.rs::bech32_drep and anyactive_drepreaders) handleNonecorrectly.crates/cardano/src/sweep/mod.rs (1)
33-33: Alias addition is fine.
DRepId = EntityKeykeeps parity with AccountId/PoolId and unblocks downstream usage.crates/cardano/src/sweep/compute.rs (5)
161-164: API exposure LGTM.
active_drep_inactivity_period()correctly delegates to PParams.
385-386: Pool retirement delegators tracking rename LGTM.
dropped_pool_delegatorsreplaces the old name; verify consumers updated (see earlier script).
429-429: Compute flow update LGTM.Adding
retire_dreps()in the sequence is appropriate.
525-532: Test scaffolding updates LGTM.Defaults for new fields are set and do not affect existing assertions.
406-424: Retirement commit consumes the sets — account latest_drep cleared and drep.retired set.crates/cardano/src/sweep/commit.rs — rotate_account_stake_data clears AccountState.latest_drep for keys in dropped_drep_delegators and writes the AccountState; update_drep_data sets DRepState.retired for keys in retired_dreps and writes the DRepState.
crates/cardano/src/model.rs (2)
617-618: Param getter exposure LGTM.
ensure_drep_inactivity_periodcorrectly added.
115-119: Schema change: confirm backward-compat for persisted AccountState.
- Renaming
drep→latest_drepkept#[n(10)]; addingactive_drepas#[n(11)]with typeOption<DRep>is backward-compatible because minicbor ignores unknown fields and absent optional fields decode toNone. (docs.rs)- I could not find unit tests that round‑trip mixed old/new AccountState CBOR; add/confirm tests that decode old-format bytes (no field 11) and round‑trip new instances. Location: crates/cardano/src/model.rs (AccountState).
crates/cardano/src/sweep/loading.rs (3)
12-43: Snapshot tracker looks good.Optional pool/drep handling and stake accumulators are consistent.
104-115: DRep state loading LGTM.Populating
boundary.drepsfrom state is straightforward.
133-149: Boundary assembly LGTM; loading order is sensible.Pools → accounts → dreps preserves dependencies.
crates/cardano/src/roll/dreps.rs (4)
15-28: Certificate DRep extraction LGTM.Coverage of relevant Conway cert variants looks correct.
102-104: Keying bydrep_to_entity_keyLGTM.Consistent across all DRep deltas.
132-139: Activity delta constructor LGTM.Minimal state carried; undo data handled in
apply.
175-197: Visitor wiring LGTM.Emits
DRepActivityalways, plus registration/unregistration deltas where applicable.
| pub dreps: HashMap<DRepId, DRepState>, | ||
| pub mutable_slots: u64, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify commit path clears DRep delegations and uses new field names.
New fields dreps, dropped_pool_delegators, dropped_drep_delegators, and retired_dreps look good. Please confirm commit.rs:
- clears
latest_drep/active_drepfordropped_drep_delegators, - handles
retired_dreps, - and no longer references the old
dropped_delegators.
Run:
Also applies to: 117-120
🏁 Script executed:
#!/bin/bash
fd commit.rs -a | xargs -I{} rg -n "dropped_delegators|dropped_pool_delegators|dropped_drep_delegators|retired_dreps|latest_drep|active_drep" {}Length of output: 226
Verify commit path clears DRep delegations and uses new field names.
- commit.rs: checks dropped_pool_delegators (ln 39); checks dropped_drep_delegators (ln 51) and sets state.latest_drep = None (ln 52); checks retired_dreps (ln 69).
- No occurrences of old dropped_delegators found.
- Missing: no clearing of active_drep observed — add clearing of state.active_drep when handling dropped_drep_delegators.
🤖 Prompt for AI Agents
In crates/cardano/src/sweep/mod.rs around lines 106-107, the struct field names
were updated (dreps, mutable_slots) and the commit path must fully clear DRep
delegations; update commit.rs (around the noted checks for
dropped_pool_delegators at ln 39, dropped_drep_delegators at ln 51–52, and
retired_dreps at ln 69) to (1) reference the new field names where applicable
and (2) when handling dropped_drep_delegators ensure you clear both
state.latest_drep = None and state.active_drep = None (or remove/reset the
active drep field) so active delegations are cleared as well. Ensure any other
references to old dropped_delegators are updated to the new names.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/cardano/src/sweep/commit.rs (3)
47-54: Stake rotation before rewards looks intentional; use saturating add.
- Ordering (wait <- live, then add rewards) appears to preserve the 2-epoch reward lag; please confirm against your ledger rules.
- Prevent potential u64 overflow on rewards accumulation.
Apply:
- let rewards = self.delegator_rewards.get(&key).unwrap_or(&0); - state.rewards_sum += rewards; + let rewards = self.delegator_rewards.get(&key).unwrap_or(&0); + state.rewards_sum = state.rewards_sum.saturating_add(*rewards);
69-86: Reduce unnecessary writes and clarify intent.Only mutated DReps need to be written; rename for clarity.
Apply:
- fn update_drep_data<W: StateWriter>( + fn apply_drep_retirements<W: StateWriter>( &self, writer: &W, dreps: impl Iterator<Item = Result<(EntityKey, crate::DRepState), StateError>>, ) -> Result<(), ChainError> { for record in dreps { let (key, mut state) = record?; - - if self.retired_dreps.contains(&key) { - state.retired = true; - } - - writer.write_entity_typed::<crate::DRepState>(&key, &state)?; + if self.retired_dreps.contains(&key) && !state.retired { + state.retired = true; + writer.write_entity_typed::<crate::DRepState>(&key, &state)?; + } } Ok(()) }And update the call site accordingly (see Lines 171-179).
171-179: DRep iteration + commit flow integration looks good.Reading DRepState and applying retirements within the same transaction is sound. If you adopt the refactor above, change:
- self.update_drep_data(&writer, dreps)?; + self.apply_drep_retirements(&writer, dreps)?;crates/cardano/src/sweep/loading.rs (2)
19-27: Use saturating add for pool stake.Avoid rare u64 overflow on pathological stake inputs.
Apply:
- self.pool_stake - .entry(pool_id.clone()) - .and_modify(|x| *x += stake) - .or_insert(stake); + self.pool_stake + .entry(pool_id.clone()) + .and_modify(|x| *x = x.saturating_add(stake)) + .or_insert(stake);
29-37: Use saturating add for DRep stake as well.Mirror the pool stake fix.
Apply:
- self.drep_stake - .entry(drep_id.clone()) - .and_modify(|x| *x += stake) - .or_insert(stake); + self.drep_stake + .entry(drep_id.clone()) + .and_modify(|x| *x = x.saturating_add(stake)) + .or_insert(stake);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/cardano/src/sweep/commit.rs(2 hunks)crates/cardano/src/sweep/loading.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/cardano/src/sweep/commit.rs (3)
crates/cardano/src/roll/dreps.rs (3)
key(56-58)key(102-104)key(144-146)crates/cardano/src/model.rs (1)
key(1055-1075)crates/cardano/src/sweep/loading.rs (3)
domain(53-55)domain(83-85)domain(105-107)
crates/cardano/src/sweep/loading.rs (3)
crates/cardano/src/model.rs (6)
drep_to_entity_key(780-790)sweep(706-716)from(836-838)from(842-844)from(848-850)from(854-857)crates/cardano/src/eras.rs (2)
load_active_era(182-197)domain(183-185)crates/cardano/src/sweep/commit.rs (3)
domain(163-165)domain(167-169)domain(171-173)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check Build
🔇 Additional comments (9)
crates/cardano/src/sweep/commit.rs (2)
39-46: Correct pool rotation order.Clearing latest_pool before promoting it to active_pool mirrors pool-drop semantics. LGTM.
55-62: Clearing latest_drep then rotating to active_drep is correct.This aligns with the PR goal (clear delegators on retiring DReps) and resolves the prior review asking to set active_drep after clearing latest_drep. LGTM.
crates/cardano/src/sweep/loading.rs (7)
6-9: Imports updated for DRep handling.Brings in drep_to_entity_key and DRep types; consistent with the new flow. LGTM.
12-17: Snapshot API change is sensible.Optional pool_id/drep_id makes the API clearer and avoids separate insert methods. LGTM.
60-67: Ending snapshot correctly uses latest_ fields.*latest_pool/latest_drep + live_stake is correct for end-of-epoch snapshot. LGTM.
69-76: Active snapshot now uses active_drep (fix confirmed).This addresses the earlier issue where latest_drep was used. LGTM.
104-115: Loading DRep state into boundary is fine.Simple pass-through; no issues spotted. LGTM.
133-149: BoundaryWork fields initialization looks correct.Adds dreps map and new dropped/retired sets; aligns with compute/commit changes. LGTM.
154-155: Loading order OK.Pools → accounts → dreps is acceptable given current usage. If future compute relies on dreps during account load, consider moving load_drep_data earlier.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes