fix(grpc): populate block_ref in AnyUtxoData via block_by_tx_hash#1000
Open
islamaliev wants to merge 2 commits into
Open
fix(grpc): populate block_ref in AnyUtxoData via block_by_tx_hash#1000islamaliev wants to merge 2 commits into
islamaliev wants to merge 2 commits into
Conversation
`into_u5c_utxo` returned `block_ref: None` for every UTxO surfaced by `read_utxos` / `search_utxos`, leaving downstream consumers unable to associate UTxOs with the blocks that created them. The field was added as a placeholder in txpipe#813 (u5c 0.18.1 upgrade) but the reverse lookup was never wired up. Mirror the pattern already used by `read_tx`: call `AsyncQueryFacade::block_by_tx_hash`, decode the block, and build a `ChainPoint { slot, hash, height, timestamp }`. Lookup failures (missing tx in archive, decode error) `warn!` with the tx hash and fall back to `None` — matching the existing posture for datum-decode errors in the same function. Applied to both v1alpha and v1beta. No new indexing is needed; `slot_by_tx_hash` already exists across redb3 and fjall.
📝 WalkthroughWalkthroughBoth gRPC v1alpha and v1beta query endpoints now populate the ChangesBlock reference population in UTXO responses
sequenceDiagram
participant Client
participant GRPC_Handler as gRPC Handler (v1alpha/v1beta)
participant BlockRefs as serve::grpc::block_refs::fetch_block_refs
participant AsyncQuery as AsyncQueryFacade::block_meta_by_tx_hash
participant Archive as Archive/Index
Client->>GRPC_Handler: request read_utxos/search_utxos
GRPC_Handler->>BlockRefs: fetch_block_refs(&domain, utxo_keys)
BlockRefs->>AsyncQuery: block_meta_by_tx_hash(tx_hash)
AsyncQuery->>Archive: load & decode block bytes
Archive-->>AsyncQuery: block bytes / metadata
AsyncQuery-->>BlockRefs: BlockRefMeta (slot, hash, height, tx_index)
BlockRefs-->>GRPC_Handler: Map<TxHash, BlockRefData>
GRPC_Handler->>GRPC_Handler: to_chain_point + into_u5c_utxo(with block_ref)
GRPC_Handler-->>Client: UTXO response with AnyUtxoData.block_ref
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/serve/grpc/v1alpha/query.rs (1)
799-799: 💤 Low valueConsider logging when tx hash is not found in archive.
The
Ok(None)case (tx not found) silently falls back toblock_ref: None, while other failure paths log warnings. The PR description indicates "missing tx in archive" should also log a warning for consistency.💡 Optional: Add warning for missing tx
- Ok(None) => None, + Ok(None) => { + warn!( + tx_hash = hex::encode(txo.0), + "block_by_tx_hash returned None for UTxO" + ); + None + }🤖 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 `@src/serve/grpc/v1alpha/query.rs` at line 799, The match arm currently returns Ok(None) => None silently, leaving block_ref: None without a log; update this branch to emit a warning when the tx hash is not found in the archive (i.e., when Ok(None) is matched) using the same logger/macro used by the other failure paths in this file so it matches existing logs, then return None as before — locate the match arm showing "Ok(None) => None" and add a warn call referencing the missing tx/hash context before returning.
🤖 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 `@src/serve/grpc/v1alpha/query.rs`:
- Line 799: The match arm currently returns Ok(None) => None silently, leaving
block_ref: None without a log; update this branch to emit a warning when the tx
hash is not found in the archive (i.e., when Ok(None) is matched) using the same
logger/macro used by the other failure paths in this file so it matches existing
logs, then return None as before — locate the match arm showing "Ok(None) =>
None" and add a warn call referencing the missing tx/hash context before
returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad2af15f-803c-4619-8535-f87c49dcd2f3
📒 Files selected for processing (2)
src/serve/grpc/v1alpha/query.rssrc/serve/grpc/v1beta/query.rs
…e decode Addresses review feedback on the per-UTxO block_ref population added in the previous commit: - Introduce `AsyncQueryFacade::block_meta_by_tx_hash` returning chain-point metadata (`BlockRefMeta`) decoded once inside the blocking task. Eliminates the redundant `MultiEraBlock::decode` previously performed in the gRPC caller. (review A-01) - Hoist `load_era_summary` to the request handler scope via a new shared `serve::grpc::block_refs` module. Previously `get_slot_timestamp` reloaded the era summary from state on every UTxO; now it runs once per request. (review A-02) - Deduplicate block lookups by source tx hash. A `search_utxos` page whose UTxOs cluster on M unique txs now does M lookups instead of N. (review A-03) - Propagate storage failures as `Status::internal` instead of swallowing them. Block-decode failures (indicating archive corruption) degrade to `debug!` rather than per-UTxO `warn!` log spam. (review A-04, A-05) - Extract the entire flow into `serve::grpc::block_refs::fetch_block_refs`, shared by both v1alpha and v1beta. Each version maps the chain-agnostic `BlockRefData` to its own proto-specific `ChainPoint` via a tiny `to_chain_point` helper. (review A-06) `into_u5c_utxo` now takes the pre-resolved `Option<ChainPoint>` as a parameter rather than performing its own lookup, keeping the per-UTxO path cheap. Tests deferred: no harness exists for `read_utxos`/`search_utxos`. The `minibf::TestDomainBuilder` pattern can seed a `ToyDomain` via `import_blocks`, and `FaultyToyDomain` covers error branches — happy to add unit tests if maintainers prefer them in-PR.
Author
|
Pushed a follow-up commit (b3b1495) addressing the bulk of the review feedback on the original Tier-1 patch:
The two Local quality gates against the new HEAD:
Tests still deferred per the original PR question. The reviewer noted that |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/serve/grpc/v1alpha/query.rs (1)
873-883: ⚡ Quick winAdd regression tests for
block_refpopulation in UTxO responses.Line 873 and Line 927 introduce new behavior (prefetch + per-item mapping), but there’s no direct test here covering: populated
block_ref, missing archive entry (None), and lookup failure propagation semantics.Also applies to: 927-937
🤖 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 `@src/serve/grpc/v1alpha/query.rs` around lines 873 - 883, Add regression tests covering the new block_ref behavior introduced by fetch_block_refs and per-item mapping into_u5c_utxo: write tests that (1) verify a populated block_ref is returned when fetch_block_refs yields an entry (use to_chain_point expectation), (2) verify block_ref is None when the archive lookup returns no entry, and (3) verify lookup failures propagate as a gRPC/internal error (map_err path). Target the code paths invoking crate::serve::grpc::block_refs::fetch_block_refs, the loop that calls into_u5c_utxo, and the to_chain_point conversion so the tests exercise both the prefetch map usage and the per-item mapping/error handling.
🤖 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 `@src/serve/grpc/v1alpha/query.rs`:
- Around line 873-883: Add regression tests covering the new block_ref behavior
introduced by fetch_block_refs and per-item mapping into_u5c_utxo: write tests
that (1) verify a populated block_ref is returned when fetch_block_refs yields
an entry (use to_chain_point expectation), (2) verify block_ref is None when the
archive lookup returns no entry, and (3) verify lookup failures propagate as a
gRPC/internal error (map_err path). Target the code paths invoking
crate::serve::grpc::block_refs::fetch_block_refs, the loop that calls
into_u5c_utxo, and the to_chain_point conversion so the tests exercise both the
prefetch map usage and the per-item mapping/error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83ccb622-d1a9-4b48-bfc4-eff4e21d129e
📒 Files selected for processing (5)
crates/core/src/async_query.rssrc/serve/grpc/block_refs.rssrc/serve/grpc/mod.rssrc/serve/grpc/v1alpha/query.rssrc/serve/grpc/v1beta/query.rs
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.
What
into_u5c_utxo(in bothsrc/serve/grpc/v1alpha/query.rsandsrc/serve/grpc/v1beta/query.rs) now populatesAnyUtxoData.block_refinstead of returningNone. Every UTxO returned byread_utxosandsearch_utxoswill now carry the block (slot/hash/height/timestamp) that produced it.Why
The
block_reffield was added toAnyUtxoDataas part of the u5c 0.18.1 upgrade in #813 (commit2b6de0bc), but it landed as a placeholder —block_ref: None— and the reverse lookup was never wired up. Consumers querying UTxOs throughread_utxos/search_utxostherefore lose all block-level metadata for those UTxOs.A downstream library (
pogun-chainfollower) shipped afilter_maptourniquet to silently drop these items so the rest of the pipeline doesn't fall over. Populating the field at the source makes that workaround unnecessary.How
Mirror the pattern already used by
read_tx(a few functions down in the same file): callAsyncQueryFacade::block_by_tx_hash(txo.0.to_vec()), decode the block withMultiEraBlock::decode, and build aChainPoint { slot, hash, height, timestamp }. The reverse lookup is backed by the existingslot_by_tx_hashindex — no new indexing is needed; it is already implemented across both storage backends (dolos-redb3,dolos-fjall).Lookup failures (tx not in archive, decode error, query error)
warn!with the tx hash and fall back toblock_ref: None. This mirrors the existing posture ininto_u5c_utxofor datum-decode errors — graceful degradation rather than failing the whole call.Applied to both
v1alphaandv1beta(near-identical files).Performance tradeoff (please weigh in)
into_u5c_utxois invoked once per UTxO. With this patch, asearch_utxospage of N UTxOs costs N reverse-lookups + N fullMultiEraBlock::decodecalls. When the UTxOs cluster on M < N source txs (typical — multi-output txs), N − M of those decodes are redundant.This PR takes the Tier 1 path: per-UTxO lookup, with a
TODOcomment near the new code marking the spot for future batching. Correctness over performance, acceptable cost for typical query sizes.Tier 2 (deferred): refactor
read_utxos/search_utxosto gather unique tx hashes from the page first, batch-fetch aHashMap<TxHash, ChainPoint>, and thread it intointo_u5c_utxovia a signature change. Bigger diff, better cost profile.Happy to land Tier 2 inline before merge if reviewers prefer it, or to track it as a follow-up issue — deferring to maintainer judgment.
Tests
No existing test harness exercises
read_utxos/search_utxosagainst fixture blocks — the in-file test module only covers genesis/era-summary RPCs against the in-memoryToyDomain, which doesn't have meaningful archive integration forblock_by_tx_hashto returnSome. I didn't want to invent a heavy fixture/seeded-chain scaffolding without first asking whether maintainers want one. Please advise:tests/against a seeded snapshot, or a unit test with a richer mock domain)?Quality gates (locally, against
a5dc18f)cargo fmt --check— the two touched files are clean. (Pre-existing fmt diffs in unrelated files exist onmainbecauserustfmt.tomlenableswrap_comments = true, which only applies on nightly. Untouched.)cargo clippy --all-targets --all-features -- -D warnings— clean.cargo build --workspace --all-targets --all-features— clean.cargo test --workspace --all-targets— 540 passed, 0 failed (matches CI shape —ci.ymldoes not pass--all-features).cargo test --workspace --all-featuresshows 14 pre-existing failures indolos-cardanoproptests (model::accounts::prop_tests::*,model::epochs::prop_tests::*,model::pools::prop_tests::pool_registration_roundtrip, etc.). I verified these fail on baremain@a5dc18fwithout my changes, so they're unrelated to this PR.Scope
v1alphaandv1betaupdated.block_ref: Option<ChainPoint>already exists onAnyUtxoData).Summary by CodeRabbit