feat(localstatequery): decode chain-dep-state opcert counters - #1898
Conversation
Implement the DebugChainDepState result decoder to expose the authoritative on-chain operational-certificate counters a synced node enforces (block-issuer key hash -> highest accepted opcert issue number), plus a GetOpCertCounters convenience helper. This lets any node-to-client consumer read the counters over LSQ without depending on a node-specific API. The decoder is version-discriminated and handles both the Praos (Babbage era and later, including Conway) and TPraos (Shelley through Alonzo) serialisations, reusing the existing WithOriginSlot and ledger/common Nonce types. Table-driven decode tests cover both protocols, chain origin, empty counters, the query envelope, and unknown-version rejection; a build-tagged integration test provides the live-node spot-check harness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
📝 WalkthroughWalkthroughAdds typed decoding for LocalStateQuery chain-dependent state across Praos and TPraos serializations, including slots, nonces, and operational-certificate counters. Adds client documentation and a 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (1)
protocol/localstatequery/chain_dep_state_test.go (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep fixture constants within the 80-column limit.
Lines 28 and 31 exceed the configured limit. Split each compile-time string literal across concatenated lines and run
golines.As per coding guidelines, “Enforce 80-column line length limit using
golines.”🤖 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 `@protocol/localstatequery/chain_dep_state_test.go` around lines 28 - 31, Wrap the fixture string literals assigned to poolKeyHashHex and poolKeyHashHex2 across concatenated lines so every line stays within 80 columns, then run golines to enforce the formatting.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 `@protocol/localstatequery/chain_dep_state_test.go`:
- Around line 33-44: Replace the manual fatal checks in blake224 and the related
tests in protocol/localstatequery/chain_dep_state_test.go:33-44 with testify
require assertions for fixture/precondition failures and assert for checks that
should continue. Apply the same conversion to environment, connection, and
result assertions in
protocol/localstatequery/chain_dep_state_integration_test.go:45-56, importing
testify assert/require as needed.
In `@protocol/localstatequery/chain_dep_state.go`:
- Around line 156-162: Update the TPraos decode branch around ChainDepState
result assignment to explicitly clear all four Praos-only nonce pointers,
including EpochNonce and the other nonce fields, before returning. Add a
regression test that reuses a result after a Praos decode and verifies every
Praos-only nonce is nil for the subsequent TPraos decode.
- Around line 96-106: Update DebugChainDepStateResult.UnmarshalCBOR to call
r.SetCbor(data) before decoding and use cbor.DecodeStoreCbor for decoding.
Preserve the existing temporary array decoding and field-copy behavior so Cbor()
returns the exact received wire bytes.
---
Nitpick comments:
In `@protocol/localstatequery/chain_dep_state_test.go`:
- Around line 28-31: Wrap the fixture string literals assigned to poolKeyHashHex
and poolKeyHashHex2 across concatenated lines so every line stays within 80
columns, then run golines to enforce the formatting.
🪄 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: 620e7ef6-5218-4676-b8c2-bdf5f23483a5
📒 Files selected for processing (6)
protocol/localstatequery/README.mdprotocol/localstatequery/chain_dep_state.goprotocol/localstatequery/chain_dep_state_integration_test.goprotocol/localstatequery/chain_dep_state_test.goprotocol/localstatequery/client.goprotocol/localstatequery/queries.go
| func blake224(t *testing.T, s string) ledger.Blake2b224 { | ||
| t.Helper() | ||
| var h ledger.Blake2b224 | ||
| raw, err := hex.DecodeString(s) | ||
| if err != nil { | ||
| t.Fatalf("decode key hash %q: %s", s, err) | ||
| } | ||
| if len(raw) != len(h) { | ||
| t.Fatalf("key hash %q: expected %d bytes, got %d", s, len(h), len(raw)) | ||
| } | ||
| copy(h[:], raw) | ||
| return h |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'github\.com/stretchr/testify/(assert|require)' \
--glob '*.go' --glob 'go.mod' --glob 'go.sum' .Repository: blinklabs-io/gouroboros
Length of output: 13214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- module dependency ---\n'
rg -n 'github\.com/stretchr/testify' go.mod go.sum || true
printf '\n--- target files imports and assertions ---\n'
for f in \
protocol/localstatequery/chain_dep_state_test.go \
protocol/localstatequery/chain_dep_state_integration_test.go
do
echo "### $f"
sed -n '1,220p' "$f" | cat -n
echo "-- assert/require usage --"
rg -n 'testify/(assert|require)|assert\.|require\.|\.(Fatal|Error|Fatalf|Errorf|Skip|Skipf)\(' "$f" || true
echo "-- t.Fatal direct usage --"
rg -n '\b(t\.Fatal|t\.Fatalf|t\.Error|t\.Errorf|t\.Skip|t\.Skipf)\(' "$f" || true
doneRepository: blinklabs-io/gouroboros
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- module dependency ---'
rg -n 'github\.com/stretchr/testify' go.mod go.sum || true
printf '%s\n' ''
printf '%s\n' '--- target files imports and assertions ---'
for f in \
protocol/localstatequery/chain_dep_state_test.go \
protocol/localstatequery/chain_dep_state_integration_test.go
do
printf '%s\n' "### $f"
sed -n '1,220p' "$f" | cat -n
printf '%s\n' '-- testify usage --'
rg -n 'testify/(assert|require)|assert\.|require\.|\.(Fatal|Error|Fatalf|Errorf|Skip|Skipf)\(' "$f" || true
printf '%s\n' '-- testing.T.Fatal/error/skip direct usage --'
rg -n '\b(t\.Fatal|t\.Fatalf|t\.Error|t\.Errorf|t\.Skip|t\.Skipf)\(' "$f" || true
doneRepository: blinklabs-io/gouroboros
Length of output: 19835
Use testify assertions in both new test files. testify is already a module dependency, and this project’s test guideline requires fatal assertions with require and continuing assertions with assert.
protocol/localstatequery/chain_dep_state_test.go: convert fixture/precondition assertions torequire, and assertions intended to continue testing toassert.protocol/localstatequery/chain_dep_state_integration_test.go: convert environment/connection/result assertions torequire/assertas appropriate.
📍 Affects 2 files
protocol/localstatequery/chain_dep_state_test.go#L33-L44(this comment)protocol/localstatequery/chain_dep_state_integration_test.go#L45-L56
🤖 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 `@protocol/localstatequery/chain_dep_state_test.go` around lines 33 - 44,
Replace the manual fatal checks in blake224 and the related tests in
protocol/localstatequery/chain_dep_state_test.go:33-44 with testify require
assertions for fixture/precondition failures and assert for checks that should
continue. Apply the same conversion to environment, connection, and result
assertions in
protocol/localstatequery/chain_dep_state_integration_test.go:45-56, importing
testify assert/require as needed.
Source: Coding guidelines
| func (r *DebugChainDepStateResult) UnmarshalCBOR(data []byte) error { | ||
| // Both protocols wrap the state in `encodeVersion N`, which serialises as | ||
| // a 2-element array [version, innerState]. The version selects the layout | ||
| // of innerState. | ||
| var outer struct { | ||
| cbor.StructAsArray | ||
| Version uint64 | ||
| Inner cbor.RawMessage | ||
| } | ||
| if _, err := cbor.Decode(data, &outer); err != nil { | ||
| return err |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A12 -B3 \
'DecodeStoreCbor|SetCbor\(|Cbor\(\)' \
cbor protocol --glob '*.go'Repository: blinklabs-io/gouroboros
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact decoder context and nearby usage without running repository code.
sed -n '1,230p' protocol/localstatequery/chain_dep_state.go
printf '\n--- matching QueryWrapper usages ---\n'
rg -n "DebugChainDepState|ChainDepStateQuery|ChainDepStateResult|SetCbor|Cbor\\(\\)" protocol/localstatequery --glob '*.go'Repository: blinklabs-io/gouroboros
Length of output: 12148
Preserve the received CBOR bytes on DebugChainDepStateResult.
The decoder receives CBOR in UnmarshalCBOR but never stores it, and it decodes the structs into temporary values before copying fields. Add cbor.DecodeStoreCbor, call r.SetCbor(data) before decoding, and keep the current array/field copy semantics so Cbor() returns the exact wire bytes.
🤖 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 `@protocol/localstatequery/chain_dep_state.go` around lines 96 - 106, Update
DebugChainDepStateResult.UnmarshalCBOR to call r.SetCbor(data) before decoding
and use cbor.DecodeStoreCbor for decoding. Preserve the existing temporary array
decoding and field-copy behavior so Cbor() returns the exact received wire
bytes.
Source: Coding guidelines
| r.Protocol = ChainDepStateProtocolTPraos | ||
| r.LastSlot = t.LastSlot | ||
| r.OpCertCounters = t.PrtclState.OpCertCounters | ||
| r.EvolvingNonce = t.PrtclState.EvolvingNonce | ||
| r.CandidateNonce = t.PrtclState.CandidateNonce | ||
| // EpochNonce and the other Praos-only nonces do not exist in TPraos; | ||
| // they are left nil. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear Praos-only fields for TPraos decodes.
Reusing a result after a Praos decode leaves the four nonce pointers populated: this branch assigns TPraos fields but never sets them to nil. That contradicts the documented TPraos contract and can expose stale nonce data. Clear all four pointers in this branch and add a reuse regression test.
🤖 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 `@protocol/localstatequery/chain_dep_state.go` around lines 156 - 162, Update
the TPraos decode branch around ChainDepState result assignment to explicitly
clear all four Praos-only nonce pointers, including EpochNonce and the other
nonce fields, before returning. Add a regression test that reuses a result after
a Praos decode and verifies every Praos-only nonce is nil for the subsequent
TPraos decode.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Clear the Praos-only nonce pointers (EpochNonce, PreviousEpochNonce, LabNonce, LastEpochBlockNonce) in the TPraos decode branch so decoding into a reused result cannot expose stale Praos values; add a reuse regression test. - Convert the new decode and integration tests to testify require/assert per the project testing guideline. - Wrap the fixture key-hash constants to stay within the 80-column limit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
|
Thanks @coderabbitai and @cubic-dev-ai. Addressed in 0e86431:
Declined, with reasoning:
|
@chrisguiney Looks good — the commit matches what you described, and the code already reflects it in the diffs above:
Everything lines up. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
|
Tip For best results, initiate chat on the files or code changes.
Your rationale for not retaining raw CBOR on this read-only LocalStateQuery result is sound; that suggestion does not apply here. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
2 participants
Summary
Implements a typed LocalStateQuery path to read the authoritative on-chain operational-certificate counter a synced node enforces, so any node-to-client consumer (operator,
dingoctl, etc.) can read it over an NtC socket without depending on a node-specific API.Closes #1890. Also implements the previously-
anyDebugChainDepStateresult decoder (closes the #865 TODO).What changed
DebugChainDepStateresult decoder (chain_dep_state.go): the query result is now decoded into a typedDebugChainDepStateResultinstead of ananystub. It exposes:OpCertCounters map[ledger.Blake2b224]uint64— block-issuer key hash (pool cold-key hash) → highest accepted opcert issue number (the counter the node enforces).LastSlot, plus the evolving/candidate nonces and (Praos-only) epoch nonces, so callers can combine withslotsPerKESPeriodfromGetGenesisConfigto derive KES-period info.OpCertCounter(poolKeyHash)per-pool lookup helper.GetOpCertCounters()client helper: typed convenience wrapper returning just the counter map.WithOriginSlotandledger/commonNoncetypes:encodeVersion 0, 8-element state.encodeVersion 1, nestedPrtclState.Wire format provenance
Layouts were taken from the upstream Haskell instances (ouroboros-consensus
Praos/TPraos, cardano-ledgerNonce/PrtclState, cardano-slottingWithOrigin), not inferred.WithOrigin SlotNo([0]/[1,slot]) andNonce([0]/[1,hash]) match this repo's existingWithOriginSlotandlcommon.Nonce.Tests
Table-driven decode tests over wire fixtures: Praos golden (incl. exact-byte pin), TPraos golden, chain-origin/empty counters, the
buildShelleyQueryenvelope, and unknown-version rejection, plus the lookup helper.A build-tagged integration test (
-tags=chain_dep_state_integration,GOUROBOROS_NTC_SOCKET/GOUROBOROS_NETWORK_MAGIC) provides the live-node spot-check harness for the "matches the counter a synced node enforces" acceptance criterion — this environment has no node access, so that spot-check is left to run against a synced node.make lint(0 issues),make format(no changes),go vet ./..., and the package test suite all pass.Notes
ROADMAP.md,docs/plans/lsq.md) don't exist in the repo, so the new capability is documented in the packageREADME.mdinstead.#864(DebugNewEpochState) is intentionally left as-is — ChainDepState is the authoritative enforcement source for the counter and fully satisfies the acceptance criteria.🤖 Generated with Claude Code
Summary by cubic
Adds a typed
DebugChainDepStatedecoder and aGetOpCertCounters()helper to read authoritative on-chain opcert counters over LocalStateQuery, with robust decoding across Praos and TPraos and safe reuse of result structs.New Features
DebugChainDepStateResultwithOpCertCounters(pool cold-keyledger.Blake2b224→ highest opcert issue number),LastSlot, evolving/candidate/epoch nonces, andOpCertCounter()lookup.encodeVersion 0) and TPraos (encodeVersion 1), reusingWithOriginSlotandledger/commonNonce.GetOpCertCounters()convenience wrapper returning only the counter map.Bug Fixes
testifyassert/require.Written for commit 0e86431. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests