feat(cardano,minibf): implement /epochs/{number}, /next and /previous - #1171
feat(cardano,minibf): implement /epochs/{number}, /next and /previous#1171michalrus wants to merge 9 commits into
/epochs/{number}, /next and /previous#1171Conversation
…ndpoints Map the archived per-epoch `EpochState` log to the Blockfrost `EpochContent` shape. `block_count` and `fees` come from the epoch's rolling stats; `start_time`/`end_time` from the chain summary; `active_stake` from the sum of per-pool `StakeLog` entries; and `tx_count`, `output` (gross sum of all tx outputs) and first/last block time by scanning the epoch's archived blocks. `/next` and `/previous` list the epochs after/before N in ascending order with standard pagination. Epoch numbers above `i32::MAX` are rejected with 400:(matching the reference API's out-of-range behaviour), in-range but unknown epochs with 404. The unpaginated 100-epoch `/next` and `/previous` listings are failing in the `blockfrost-tests` suite. Deriving the per-block aggregates by scanning every block of every epoch at request time exceeds the 15s test timeout. Precomputing those aggregates on `EpochState` is a follow-up that requires a data resync.
`/epochs/{n}` renders four fields that the archived `EpochState` log does
not store: `tx_count`, `output`, and the first and last block time.
`output` is the gross sum of all transaction output lovelace. The old
code derived these fields at request time. It scanned every block of the
epoch. The unpaginated `/epochs/{n}/next` and `/previous` listings return
up to 100 epochs. This scanned about 400k blocks and passed the 15s
`blockfrost-tests` timeout.
Now the roll accumulates these aggregates on `RollingStats`, like
`blocks_minted` and `gathered_fees`. The endpoints read them straight
from the epoch log. `visit_tx` sums `tx_count`. `visit_output` sums
`output` from `tx.produces()`, which matches db-sync `epoch.out_sum`.
`visit_root` records the first and last block slots. The reader converts
these slots to time with `ChainSummary`. `undo` restores the previous
min and max, because arithmetic cannot reverse them.
`output` is a `u128`, because the per-epoch total can exceed `u64`. This
minicbor has no native `u128`, so the field is a 16-byte big-endian byte
string.
The roll writes the new fields only as it processes blocks. Existing
nodes must resync to backfill the fields into historical epoch logs.
Byron epoch boundary blocks (EBBs) do not flow through the roll. So
`first_block_slot` points at the first regular block, not the EBB. For
every Byron epoch, `first_block_time` then differs from Blockfrost. This
affects epoch 0 on Preview and Preprod, and epochs 0 to 207 on mainnet.
This is the same EBB gap that `/epochs/{n}/blocks` and `/blocks/{block}`
already have. A separate EBB-ingestion fix, validated against mainnet,
must correct all three.
michalrus
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR extends epoch statistics with transaction, output, and block-slot data. It adds stake aggregation and implements epoch retrieval, latest, next, and previous navigation endpoints with validation, pagination, ordering, error handling, and tests. It also updates governance proposal outcome mappings. ChangesEpoch statistics and API
Governance proposal mappings
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EpochRoute
participant Facade
participant EpochState
participant StakeLog
Client->>EpochRoute: request epoch content
EpochRoute->>Facade: load current or archived epoch state
Facade->>EpochState: read epoch state
Facade->>StakeLog: sum active stake
EpochRoute->>Client: return EpochContent
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/minibf/src/lib.rs (1)
303-312: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the duplicate stake-log scan in the fallback path.
The fallback calls
stake_logs_sum_at_epoch(epoch + 1, chain_summary)twice: once to test.is_some()and once to return the value. Each call performs a full archive range scan over the epoch's stake logs.collect_epoch_contentsinvokessum_active_stake_for_epochonce per epoch in a paginated listing, so the duplicate scan multiplies I/O on the/nextand/previoushot path.Bind the result once.
♻️ Proposed fix
// No logs for this epoch. If the *next* epoch is the earliest one that // does have logs (i.e. this epoch has none but `epoch + 1` does), this // epoch shares that first snapshot's genesis stake. Any earlier epoch // (where neither it nor its successor has logs) has no active stake. - if self - .stake_logs_sum_at_epoch(epoch + 1, chain_summary)? - .is_some() - { - return self.stake_logs_sum_at_epoch(epoch + 1, chain_summary); - } - - Ok(None) + self.stake_logs_sum_at_epoch(epoch + 1, chain_summary)🤖 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/minibf/src/lib.rs` around lines 303 - 312, In the fallback branch of sum_active_stake_for_epoch, bind the single result of stake_logs_sum_at_epoch(epoch + 1, chain_summary) and use that value for both the is_some check and return. Preserve the existing behavior for epochs without logs while eliminating the duplicate archive scan.crates/minibf/src/routes/epochs/mapping.rs (1)
202-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the
RollingStatsderivation into one place.
build_epoch_contentincrates/minibf/src/routes/epochs/mod.rs(lines 56, 76-77) already clonesstate.rolling.live()and forwardstx_countandoutputas explicit builder fields.into_modelthen clones the sameRollingStatsagain to readblocks_mintedandgathered_fees. The same value is derived twice per epoch, and the builder carries fields that duplicate data already insidestate.Choose one owner for the derivation. Deriving everything inside
into_modelis the smaller change: drop thetx_countandoutputbuilder fields and read them from the localrolling.♻️ Proposed fix
pub struct EpochContentModelBuilder { pub state: EpochState, pub start_time: u64, pub end_time: u64, pub first_block_time: u64, pub last_block_time: u64, - pub tx_count: u64, - pub output: u128, pub active_stake: Option<u64>, }let Self { state, start_time, end_time, first_block_time, last_block_time, - tx_count, - output, active_stake, } = self; let rolling = state.rolling.live().cloned().unwrap_or_default(); @@ block_count: rolling.blocks_minted as i32, - tx_count: tx_count as i32, - output: output.to_string(), + tx_count: rolling.tx_count as i32, + output: rolling.output.to_string(), fees: rolling.gathered_fees.to_string(),Then drop the matching
tx_countandoutputinitializers inbuild_epoch_contentincrates/minibf/src/routes/epochs/mod.rs.Also applies to: 232-243
🤖 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/minibf/src/routes/epochs/mapping.rs` around lines 202 - 211, Consolidate RollingStats derivation in EpochContentModelBuilder::into_model: remove the redundant tx_count and output fields from EpochContentModelBuilder, read both values from the single local rolling value alongside blocks_minted and gathered_fees, and remove their corresponding initializers in build_epoch_content.crates/minibf/src/routes/epochs/mod.rs (1)
381-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new epoch aggregates in a test.
This test targets epoch 1, which has no blocks in the synthetic chain, so
block_count,tx_count,output, andfeesare all zero and the assertions never observe them. The test only checksepochand thestart_time < end_timeordering.The precomputed aggregates are the central change in this PR. They flow from
EpochStatsUpdate::applythroughRollingStatsintoEpochContent, and no test currently covers a non-zero value on that path. The comment states that the synthetic chain places all blocks in epoch 2, so an additional test against/epochs/2can assert non-zeroblock_countandtx_countand confirm thatfirst_block_timeandlast_block_timeare populated.💚 Proposed additional test
#[tokio::test] async fn epochs_by_number_aggregates_are_populated() { let app = TestApp::new(); let (status, bytes) = app.get_bytes("/epochs/2").await; assert_eq!( status, StatusCode::OK, "unexpected status {status} with body: {}", String::from_utf8_lossy(&bytes) ); let content: EpochContent = serde_json::from_slice(&bytes).expect("failed to parse epoch content"); // The synthetic chain places all blocks in epoch 2, so the precomputed // rolling aggregates must be non-zero. assert_eq!(content.epoch, 2); assert!(content.block_count > 0); assert!(content.tx_count > 0); assert!(content.first_block_time > 0); assert!(content.last_block_time >= content.first_block_time); }🤖 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/minibf/src/routes/epochs/mod.rs` around lines 381 - 400, Add a separate async test alongside epochs_by_number_happy_path that requests /epochs/2, verifies a successful response, deserializes EpochContent, and asserts epoch 2 has non-zero block_count and tx_count, plus populated first_block_time and last_block_time with last_block_time >= first_block_time.
🤖 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.
Nitpick comments:
In `@crates/minibf/src/lib.rs`:
- Around line 303-312: In the fallback branch of sum_active_stake_for_epoch,
bind the single result of stake_logs_sum_at_epoch(epoch + 1, chain_summary) and
use that value for both the is_some check and return. Preserve the existing
behavior for epochs without logs while eliminating the duplicate archive scan.
In `@crates/minibf/src/routes/epochs/mapping.rs`:
- Around line 202-211: Consolidate RollingStats derivation in
EpochContentModelBuilder::into_model: remove the redundant tx_count and output
fields from EpochContentModelBuilder, read both values from the single local
rolling value alongside blocks_minted and gathered_fees, and remove their
corresponding initializers in build_epoch_content.
In `@crates/minibf/src/routes/epochs/mod.rs`:
- Around line 381-400: Add a separate async test alongside
epochs_by_number_happy_path that requests /epochs/2, verifies a successful
response, deserializes EpochContent, and asserts epoch 2 has non-zero
block_count and tx_count, plus populated first_block_time and last_block_time
with last_block_time >= first_block_time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a61c5d6-3b16-42ea-b449-3af379304603
📒 Files selected for processing (6)
crates/cardano/src/model/epochs.rscrates/cardano/src/roll/epochs.rscrates/minibf/src/error.rscrates/minibf/src/lib.rscrates/minibf/src/routes/epochs/mapping.rscrates/minibf/src/routes/epochs/mod.rs
There was a problem hiding this comment.
Pull request overview
Adds Blockfrost-compatible epoch lookup and navigation endpoints to the minibf HTTP service, backed by archived EpochState logs and new per-epoch rolling aggregates computed during the Cardano roll pipeline to keep responses fast (especially for unpaginated /next and /previous listings).
Changes:
- Implement
/epochs/{number},/epochs/{number}/next, and/epochs/{number}/previous, mappingEpochState+ChainSummaryinto BlockfrostEpochContent. - Precompute additional per-epoch aggregates during roll (
tx_count,output,first_block_slot,last_block_slot) to avoid per-request block scanning. - Add epoch-number range validation and corresponding 400 error mapping.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/minibf/src/routes/epochs/mod.rs | Adds epoch endpoints, pagination logic, and tests; builds EpochContent from epoch state + chain summary. |
| crates/minibf/src/routes/epochs/mapping.rs | Introduces EpochContentModelBuilder and maps internal epoch data into Blockfrost EpochContent. |
| crates/minibf/src/lib.rs | Adds stake-log summation helper used to compute active_stake for epoch responses; wires new routes. |
| crates/minibf/src/error.rs | Adds InvalidEpochNumber error variant and Blockfrost-style 400 response body. |
| crates/cardano/src/roll/epochs.rs | Extends epoch rolling visitor to accumulate tx_count and gross output during roll. |
| crates/cardano/src/model/epochs.rs | Extends RollingStats and EpochStatsUpdate with new fields; adds CBOR codec for u128 output. |
Suppressed comments (1)
crates/minibf/src/routes/epochs/mod.rs:169
/epochs/{n}/previousonly checksn <= currentbut does not verify that the reference epoch actually exists in storage. If the epoch log is missing, this endpoint will silently omit epochs instead of returning 404 for an unknown reference epoch.
if epoch > current {
return Err(StatusCode::NOT_FOUND.into());
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The reference epoch must exist for the listing to be valid. | ||
| if epoch > current { | ||
| return Err(StatusCode::NOT_FOUND.into()); | ||
| } |
There was a problem hiding this comment.
The reference epoch in /next and /previous is more of a pagination cursor, than a returned resource. Even with max_history pruning, there is never a hole where n is gone while its neighbors survive with a gap. Each listed epoch still passes through load_epoch_state, so, IMO, the epoch > current guard is enough.
153e61d to
01202b9
Compare
|
@scarmuega I'm not sure if I could have caused the Stele test failures. I don't think so 👀 Based on which tests are passing and failing below, this feels like some flaky timing issue maybe? See below:Ubuntu:
https://github.com/txpipe/dolos/actions/runs/31087812293/job/92571414821?pr=1171 macOS:
https://github.com/txpipe/dolos/actions/runs/31087812293/job/92571414979?pr=1171 |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@michalrus test failures are mine. Concurrency of tests that came from two different branches broke the CI. I'll fix it shortly. |
da9d7db to
1ebb3d8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Add 23 missing ratified and canceled outcomes for Preview, Preprod, and Mainnet to `hacks::proposals::*`. These outcomes apply proposal-deposit refunds and treasury withdrawals at the correct epoch. This change requires a resync because Dolos records each outcome when it first processes the proposal.
The `latest` handler resolves the current epoch from the tip. It sums
the active stake from a live account scan, because the current epoch has
no stake log yet. The scan uses the same rules as the reward update.
This also gives the current epoch an active stake value through
`/epochs/{epoch}` and the `/next` and `/previous` listings.
|
@scarmuega I found discrepancies with the Blockfrost API ( |
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/minibf/src/routes/epochs/mod.rs (1)
188-191: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate that the reference epoch exists before listing adjacent epochs.
Both handlers only reject future epochs. If an in-range epoch has no archived
EpochState,/nextand/previousreturn adjacent epochs instead of the required 404 response.
crates/minibf/src/routes/epochs/mod.rs#L188-L191: Callload_epoch_stateforepochbefore building the next-page range.crates/minibf/src/routes/epochs/mod.rs#L217-L219: Call the same validation before building the previous-page range.- Add regression tests for an in-range epoch with no archive log.
🤖 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/minibf/src/routes/epochs/mod.rs` around lines 188 - 191, Validate the requested reference epoch with load_epoch_state before constructing adjacent-epoch ranges: update the /next handler at crates/minibf/src/routes/epochs/mod.rs:188-191 and the /previous handler at crates/minibf/src/routes/epochs/mod.rs:217-219 to return 404 when no archived EpochState exists, while preserving future-epoch rejection; add regression tests covering an in-range epoch without an archive log for both handlers.
🤖 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/minibf/src/routes/epochs/mod.rs`:
- Around line 188-191: Validate the requested reference epoch with
load_epoch_state before constructing adjacent-epoch ranges: update the /next
handler at crates/minibf/src/routes/epochs/mod.rs:188-191 and the /previous
handler at crates/minibf/src/routes/epochs/mod.rs:217-219 to return 404 when no
archived EpochState exists, while preserving future-epoch rejection; add
regression tests covering an in-range epoch without an archive log for both
handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 084ad942-542c-46c1-83dd-a24cbc0bb66e
📒 Files selected for processing (3)
crates/cardano/src/hacks.rscrates/minibf/src/lib.rscrates/minibf/src/routes/epochs/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/minibf/src/lib.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/minibf/src/routes/epochs/mod.rs:103
derive_current_active_stakerunsStakeSnapshot::load_globalson every request for/epochs/latestand for the current epoch, which (per its own docs) iterates everyAccountState(millions on mainnet) and everyPoolState. This can easily exceed typical HTTP timeouts and creates a trivial CPU/IO amplification vector.
Consider caching the computed active_stake_sum per current epoch (e.g., memoize in Facade/app state and refresh when the tip crosses an epoch boundary) so this scan is done at most once per epoch per process, not per request.
tokio::task::spawn_blocking(move || {
StakeSnapshot::load_globals::<D>(domain.state(), current, stake_epoch, protocol)
.map(|snapshot| snapshot.active_stake_sum)
})
crates/minibf/src/routes/epochs/mapping.rs:245
- The
as i32casts here can silently wrap/truncate on overflow (e.g., timestamps beyond 2038 or unexpectedly large epoch/tx counts), producing negative or otherwise invalid values in the API response. Using checked conversions will fail fast instead of emitting corrupted data.
let out = EpochContent {
epoch: state.number as i32,
start_time: start_time as i32,
end_time: end_time as i32,
first_block_time: first_block_time as i32,
|
The question is whether we should cache the |
Resolves #1076.
Resolves #1106.
Resolves #1084.
Resolves #1070.
Implement these Blockfrost endpoints:
/epochs/latest,/epochs/{number},/epochs/{number}/next, and/epochs/{number}/previous,then precompute the per-epoch block aggregates, so the unpaginated listings do not time out.
Commits
79ca655 implements
/epochs/{number},/next, and/previous. The endpoint maps the archived per-epochEpochStatelog to the BlockfrostEpochContentshape.block_countandfeescome from the rolling stats.start_timeandend_timecome from the chain summary.active_stakecomes from the sum of the per-poolStakeLogentries./nextand/previouslist the epochs after or before N in ascending order, with standard pagination. An epoch number abovei32::MAXreturns 400. An in-range but unknown epoch returns 404.[resync!] a7be715 precomputes
tx_count,output,first_block_time, andlast_block_timeonRollingStatsduring roll. The first commit derived these four fields at request time by scanning every block of the epoch. The unpaginated/nextand/previouslistings return up to 100 epochs, so this scanned about 400k blocks and passed the 15sblockfrost-teststimeout. Now the roll accumulates the aggregates likeblocks_mintedandgathered_fees, and the endpoints read them straight from the epoch log.outputis au128, because the per-epoch total can exceedu64.[resync!] c9bfc8b adds the missing proposal outcomes to
hack::proposals. Dolos does not run the ratification logic. It reads proposal outcomes from a hardcoded table instead. The table was missing 23 ratified and canceled outcomes for Preview, Preprod, and Mainnet. Without these outcomes, Dolos never refunds the proposal deposits, so the active stake and the rewards drift from the real values. We found differences of 2,000 and 3,000 ADA between Dolos and the original API. A resync is necessary, because Dolos records each outcome only when it first processes the proposal.976187f implements
/epochs/latest, which resolves the current epoch from the tip. It sums the active stake from a live account scan, because the current epoch has no stake log yet. The scan uses the same rules as the reward update. This also gives the current epoch an active stake value through/epochs/{epoch}and the/nextand/previouslistings.Resync required
Commits a7be715 and c9bfc8b require a resync. The list earlier in this description gives the details.
Testing
Tested on Preview with:
EBB tests were ignored:
{ "_comment": "epochs/0 -- first_block_time is 20s late: Blockfrost reports the Byron epoch boundary block (EBB) at the epoch's first slot, but EBBs never flow through Dolos' roll pipeline, so the precomputed first_block_slot points at the first regular block instead.", "id": "epochs-number-first_6f4c27191e8e" }Known limitation: Byron epoch boundary blocks (EBBs)
Byron EBBs do not flow through the roll pipeline. So
first_block_slotpoints at the epoch's first regular block, not the EBB. For every Byron epoch,first_block_timethen differs from Blockfrost. This affects epoch 0 on Preview and Preprod, and epochs 0 to 207 on mainnet. This is the same EBB gap that/epochs/{number}/blocksand/blocks/{block}already have. A separate EBB-ingestion fix must correct all three.See:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
400 Bad Requestresponse.