fix: make sure that accounts don't automatically redelegate to unregistered and re-registered pools#1030
Open
etorreborre wants to merge 2 commits into
Open
fix: make sure that accounts don't automatically redelegate to unregistered and re-registered pools#1030etorreborre wants to merge 2 commits into
etorreborre wants to merge 2 commits into
Conversation
Contributor
WalkthroughThe change introduces explicit account create/update values, unbinds accounts delegated to retiring pools during epoch transitions, records the unbound count in tracing schemas, and adds store and RocksDB integration coverage. ChangesRetired-pool account unbinding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EpochTransition
participant AccountsStore
participant TraceSpan
EpochTransition->>AccountsStore: inspect accounts during pool retirement
AccountsStore-->>EpochTransition: unset delegations to retired pools
EpochTransition->>TraceSpan: record accounts_unbound
EpochTransition->>AccountsStore: update retired and remaining pool rows
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
etorreborre
force-pushed
the
etorreborre/feat/unregistered-pools
branch
from
1398a3a to
e77a4c9
Compare
etorreborre
changed the title
5 tasks
Signed-off-by: Eric Torreborre <etorreborre@yahoo.com>
Signed-off-by: Eric Torreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/feat/unregistered-pools
branch
from
e77a4c9 to
13edd8e
Compare
![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: 1
🧹 Nitpick comments (1)
crates/amaru-ledger/src/state/volatile/fragment.rs (1)
410-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDitch the inline commentary, mate — let the match speak for itself.
The logic's grand (Create-vs-Update split reads true to the new
AccountsValuecontract), but that freshly-bolted-on two-liner explaining the deposit semantics runs against the repo's own house rules.As per coding guidelines: "Never include comments in code unless explicitly asked; use descriptive names instead."
♻️ Suggested tidy-up
iterator.map(|(credential, Bind { left: pool, right: drep, value: deposit })| { - // A bound deposit denotes a (re-)registration within the window (see DiffBind::register); - // without one, only delegations changed and the account is known to exist already. - let value = match deposit { + let value = match deposit { Some(deposit) => AccountsValue::Create { pool, drep, deposit, rewards: 0 }, None => AccountsValue::Update { pool, drep }, }; (credential, 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/amaru-ledger/src/state/volatile/fragment.rs` around lines 410 - 424, Remove the inline two-line comment inside add_accounts while leaving the existing deposit match and AccountsValue::Create/Update behavior unchanged.Source: Coding guidelines
🤖 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/amaru-ledger/src/store/epoch_transition.rs`:
- Around line 179-197: The pool-retirement handling currently scans every
account through with_accounts; replace this full-table traversal with a
pool-to-delegator lookup or equivalent delegation index. Update the retirement
branch to fetch only accounts delegated to pools in retirements, clear their
pool assignments, and preserve the accounts_unbound metric and existing error
propagation.
---
Nitpick comments:
In `@crates/amaru-ledger/src/state/volatile/fragment.rs`:
- Around line 410-424: Remove the inline two-line comment inside add_accounts
while leaving the existing deposit match and AccountsValue::Create/Update
behavior unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbf6379f-71ea-4639-8dd2-d908aef21c4d
📒 Files selected for processing (12)
CHANGELOG.mdcrates/amaru-ledger/src/bootstrap.rscrates/amaru-ledger/src/epoch_transition.rscrates/amaru-ledger/src/state/volatile/fragment.rscrates/amaru-ledger/src/store/columns/accounts.rscrates/amaru-ledger/src/store/epoch_transition.rscrates/amaru-observability/src/schemas.rscrates/amaru-stores/src/lib.rscrates/amaru-stores/src/rocksdb/ledger/columns/accounts.rscrates/amaru-stores/src/rocksdb/mod.rsdocs/TRACES.mddocs/traces-schema.json
💤 Files with no reviewable changes (1)
- crates/amaru-ledger/src/epoch_transition.rs
Comment on lines
+179
to
+197
| if !retirements.is_empty() { | ||
| db.with_accounts(|iterator| { | ||
| let mut accounts_unbound: u64 = 0; | ||
| for (_, mut row) in iterator { | ||
| let delegated_to_retired = row | ||
| .borrow() | ||
| .as_ref() | ||
| .and_then(|account| account.pool.as_ref()) | ||
| .is_some_and(|(pool, _)| retirements.contains(pool)); | ||
|
|
||
| if delegated_to_retired && let Some(account) = row.borrow_mut() { | ||
| account.pool = None; | ||
| accounts_unbound += 1; | ||
| } | ||
| } | ||
|
|
||
| Span::current().record("accounts_unbound", accounts_unbound); | ||
| })?; | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Gauge how large the accounts column can get vs pools, and whether any pool->accounts index exists
rg -nP -C2 'with_accounts|iter_accounts' crates/amaru-ledger/src/store
rg -nP -C2 'fn account\b|by_pool|delegators' crates/amaru-stores crates/amaru-ledgerRepository: pragma-org/amaru
Length of output: 8936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant store APIs and the epoch transition context.
ast-grep outline crates/amaru-ledger/src/store.rs --view expanded
ast-grep outline crates/amaru-ledger/src/store/epoch_transition.rs --view expanded
printf '\n--- account-related store methods ---\n'
rg -n -C 2 '\bwith_accounts\b|\baccounts\b|\baccount\(' crates/amaru-ledger/src/store.rs crates/amaru-stores/src/lib.rs crates/amaru-stores/src/rocksdb/mod.rs
printf '\n--- retirement / epoch transition context ---\n'
sed -n '1,260p' crates/amaru-ledger/src/store/epoch_transition.rs
printf '\n--- store trait around account iteration ---\n'
sed -n '200,280p' crates/amaru-ledger/src/store.rsRepository: pragma-org/amaru
Length of output: 39147
Full account scan on pool retirement is a scalability hotspot
with_accounts is the only lookup path here, so every retirement walks the whole accounts table just to clear a handful of delegations. Fine for correctness, but at mainnet scale it turns epoch boundary work into a chunky boss fight. A pool→delegator index, or some equivalent delegation lookup, would let this touch only affected accounts.
🤖 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/amaru-ledger/src/store/epoch_transition.rs` around lines 179 - 197,
The pool-retirement handling currently scans every account through
with_accounts; replace this full-table traversal with a pool-to-delegator lookup
or equivalent delegation index. Update the retirement branch to fetch only
accounts delegated to pools in retirements, clear their pool assignments, and
preserve the accounts_unbound metric and existing error propagation.
Contributor
Author
There was a problem hiding this comment.
@KtorZ that would require a larger change, only effective when there are retirements. Should we do it? If so, should we do it in the same PR?
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.
This PR implements the
FIXMEsection described in #878Summary by CodeRabbit
Bug Fixes
Observability
accounts_unboundmetric to epoch transition traces, showing how many accounts were unbound.