Conversation
📝 WalkthroughWalkthroughThis pull request implements Mithril snapshot bootstrap and ledger state import functionality. It introduces a complete Mithril client with snapshot discovery, download, and extraction; adds ledger state parsing for UTxOs, accounts, pools, DReps, governance, and snapshots; implements a resumable, phased import pipeline with checkpointing; expands the database layer with ImportCheckpoint, NetworkState, and SyncState models across SQLite, PostgreSQL, and MySQL; adds new CLI commands for Mithril bootstrap and sync; introduces RawBlock and bulk block insertion for efficient ledger loading; and updates node loading with database-integrated workflows. The changes enable rapid bootstrapping from Mithril snapshots and restoration of historical ledger state into the database. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
20 issues found across 43 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="ledgerstate/import_test.go">
<violation number="1" location="ledgerstate/import_test.go:64">
P2: state.Tip is dereferenced in the log output before the nil check, so a nil Tip would panic and skip the intended assertion. Check for nil before logging fields from state.Tip.</violation>
</file>
<file name="ledgerstate/utxo.go">
<violation number="1" location="ledgerstate/utxo.go:513">
P1: Potential index-out-of-bounds panic: `data[1+pos]` is not bounds-checked. If the CBOR data is truncated (no `0xff` terminator), `1+pos` could equal or exceed `len(data)`, causing a runtime panic. Add a bounds check before indexing.</violation>
</file>
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:152">
P2: The `readVarLen` method converts `uint64` to `int` which can silently produce a negative value for large VarLen-encoded numbers from malformed input. This negative value propagates to `make()` calls (e.g., `make([]ParsedAsset, numAssets)` or `make([]byte, n)`) causing a runtime panic. Consider adding a bounds check after the conversion.</violation>
<violation number="2" location="ledgerstate/mempack.go:607">
P1: The last asset's name will include trailing padding bytes from the flat buffer. The format pads Region E to word-size alignment, but the code takes `flat[nameOff:]` which includes those padding bytes. This corrupts the last asset name, which could cause incorrect asset identification downstream. Consider using the total name-region size (e.g., tracking the end of names from the last distinct offset boundary, or stripping trailing zero padding).</violation>
</file>
<file name="cmd/dingo/mithril.go">
<violation number="1" location="cmd/dingo/mithril.go:76">
P2: Use `cmd.Context()` instead of `context.Background()` so that the API calls respect signal-based cancellation (e.g., Ctrl+C).</violation>
<violation number="2" location="cmd/dingo/mithril.go:100">
P2: Unguarded string slicing `s.Digest[:16]` and `s.CreatedAt[:19]` will panic if the API returns values shorter than expected. Add bounds checks or use a helper that truncates safely.</violation>
<violation number="3" location="cmd/dingo/mithril.go:287">
P2: `errgroup.WithContext(context.Background())` makes the parallel operations non-cancelable from outside. Consider accepting a `context.Context` parameter in `runMithrilSync` and passing it to the errgroup.</violation>
</file>
<file name="ledgerstate/verify_test.go">
<violation number="1" location="ledgerstate/verify_test.go:48">
P2: Avoid calling require.NoError (FailNow) inside the httptest handler goroutine. Use a non-fatal error report or propagate the error back to the test goroutine so failures are recorded reliably.</violation>
</file>
<file name="mithril/client.go">
<violation number="1" location="mithril/client.go:132">
P2: Escape the snapshot digest before inserting it into the URL path; unescaped path characters can produce malformed requests or hit unintended endpoints.</violation>
</file>
<file name="database/plugin/metadata/sqlite/import.go">
<violation number="1" location="database/plugin/metadata/sqlite/import.go:108">
P2: When the pool already exists, `OnConflict` updates won’t populate `pool.ID`, so this assignment can set `PoolID` to 0 and create orphaned registration/owner rows. Fetch the ID before linking the registration when `pool.ID` is still zero.</violation>
</file>
<file name="ledgerstate/cbor_decode.go">
<violation number="1" location="ledgerstate/cbor_decode.go:94">
P2: Unterminated indefinite-length map silently returns partial results. If the `0xff` break code is never found (truncated data), the loop exits and the function returns `nil` error. This is inconsistent with `cborItemSize` which correctly returns an `"unterminated indef map"` error. Add a check after the loop.</violation>
<violation number="2" location="ledgerstate/cbor_decode.go:226">
P1: Missing bounds validation: `cborItemSize` for byte/text strings returns `hLen + int(arg)` without verifying the data is actually that long. With truncated CBOR, this causes callers (e.g., `decodeMapEntries`) to panic on out-of-bounds slicing when they do `data[pos : pos+keySize]`.</violation>
</file>
<file name="internal/node/load.go">
<violation number="1" location="internal/node/load.go:219">
P2: `LoadBlobsWithDB` is designed for bulk-loading Mithril snapshots but does not enable bulk-load pragmas (`SetBulkLoadPragmas`/`RestoreNormalPragmas`) like `LoadWithDB` does. Since `AddRawBlocks` writes to the metadata store, omitting these optimizations may significantly degrade import performance. Consider adding the same bulk-load pragma setup that `LoadWithDB` uses.</violation>
</file>
<file name="ledgerstate/snapshot.go">
<violation number="1" location="ledgerstate/snapshot.go:74">
P1: Bug: Lexicographic string sort of slot-number directory names gives wrong results when slot numbers have different digit counts (e.g., `"9" > "200"` in string order). This will select the wrong (not most recent) ledger state. Use a numeric sort instead, or use `sort.Slice` with `strconv.Atoi` comparison. The same bug is present in the `legacyFiles` sort below and in `FindUTxOTableFile`.</violation>
</file>
<file name="ledgerstate/telescope.go">
<violation number="1" location="ledgerstate/telescope.go:112">
P2: Add a length check before indexing items[1] to avoid panics on malformed telescope data.</violation>
<violation number="2" location="ledgerstate/telescope.go:150">
P2: Reject empty past-era summaries instead of appending a zero-value bound; this avoids silently producing incorrect epoch history when the CBOR is malformed.</violation>
</file>
<file name="ledgerstate/snapshots.go">
<violation number="1" location="ledgerstate/snapshots.go:35">
P2: The fee is described as optional, but the length check requires 4 elements and ss[3] is accessed unconditionally. Snapshots with only mark/set/go will error or panic. Allow len==3 and only decode fee when present.</violation>
</file>
<file name="ledgerstate/certstate.go">
<violation number="1" location="ledgerstate/certstate.go:615">
P2: `big.Int.Uint64()` is called without checking `IsUint64()` first. Per Go documentation, the result of `Uint64()` is undefined if the value cannot be represented as a `uint64` (negative or overflow). Malformed CBOR input could silently produce incorrect margin values. Guard both calls with `IsUint64()`.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:228">
P1: Bug: `setCheckpoint` never commits the transaction, so checkpoints are silently discarded and resume will never work. Add `return txn.Commit()` after the success log, matching the pattern used by every other write function in this file.</violation>
<violation number="2" location="ledgerstate/import.go:1021">
P2: Potential uint64 underflow in fallback path: when called with zero `EraBound{}` for the current era, `endBound.Epoch - startBound.Epoch` underflows if `startBound.Epoch > 0`, producing a garbage epoch length. Guard against this by checking that `endBound.Epoch > startBound.Epoch`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if _, err := cbor.Decode(data, &r); err == nil { | ||
| rat := r.ToBigRat() | ||
| if rat != nil { | ||
| num := rat.Num().Uint64() |
There was a problem hiding this comment.
P2: big.Int.Uint64() is called without checking IsUint64() first. Per Go documentation, the result of Uint64() is undefined if the value cannot be represented as a uint64 (negative or overflow). Malformed CBOR input could silently produce incorrect margin values. Guard both calls with IsUint64().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/certstate.go, line 615:
<comment>`big.Int.Uint64()` is called without checking `IsUint64()` first. Per Go documentation, the result of `Uint64()` is undefined if the value cannot be represented as a `uint64` (negative or overflow). Malformed CBOR input could silently produce incorrect margin values. Guard both calls with `IsUint64()`.</comment>
<file context>
@@ -0,0 +1,630 @@
+ if _, err := cbor.Decode(data, &r); err == nil {
+ rat := r.ToBigRat()
+ if rat != nil {
+ num := rat.Num().Uint64()
+ den := rat.Denom().Uint64()
+ if den > 0 {
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@cmd/dingo/mithril.go`:
- Line 287: The errgroup is currently created with
errgroup.WithContext(context.Background()), so CLI cancellation (SIGINT) won't
propagate; change runMithrilSync to accept a ctx context.Context (call site
should pass cmd.Context() or a signal-aware parent), replace
context.Background() with that ctx when calling errgroup.WithContext(ctx), and
thread the ctx into any spawned goroutines (e.g., the ledger-state import and
blob load) so they respect cancellation; update callers of runMithrilSync
accordingly.
In `@dingo.yaml.example`:
- Around line 219-221: The config default for the Mithril certificate
verification is insecure: change the `verifyCertificates` default from false to
true in the configuration example and any code that provides defaults (ensure
the symbol `verifyCertificates` now defaults to true), update associated
documentation/comments to state that verification is enabled by default and
users may opt out for faster bootstraps, and adjust any config-schema/validation
logic or tests that assume the previous false default so they reflect the new
secure default.
In `@internal/config/config.go`:
- Around line 332-336: Change the Mithril default to enable certificate
verification by setting the VerifyCertificates field to true in the
MithrilConfig default block (update the Mithril: MithrilConfig{ ... }
initializer to include VerifyCertificates: true). Also ensure any code that
relies on zero values or unmarshalling of MithrilConfig (e.g., constructors or
config loaders that treat absent fields specially) treats true as the
default/opt-out behavior and update relevant tests/docs referencing
VerifyCertificates or MithrilConfig.
In `@ledgerstate/cbor_decode.go`:
- Around line 188-226: cborItemSize can return a size larger than the available
buffer for definite-length strings and for composite types, so update
cborItemSize (and where it relies on cborArgument) to validate bounds: for major
types 2 and 3 after computing arg and hLen ensure hLen+int(arg) <= len(data) and
return an explicit truncated-data error if not; when handling indefinite strings
ensure each chunkSize is validated against remaining length before recursing and
that pos+chunkSize cannot exceed len(data); and in the array/map recursion paths
ensure each child chunkSize is checked against the remaining bytes before adding
to pos (returning an error on truncation) so callers like decodeMapEntries never
receive a too-large size.
- Around line 57-95: The indefinite-length map branch in DecodeMap stops parsing
when pos >= len(data) and currently returns entries even if the 0xff break code
was never seen; change the logic in that branch (the loop using pos,
cborItemSize, MapEntry, keyRaw, valRaw) to track whether the break code was
encountered (e.g., encounteredBreak boolean) and after the loop return a clear
error (including context like "truncated indefinite-length map: missing break
code 0xff") if the break code was not found instead of returning partial
entries; ensure the check happens before returning entries so truncated data is
treated as an error.
In `@ledgerstate/certstate.go`:
- Around line 85-151: parseCertStateConway currently only extracts PState and
DState and thus never populates result.DReps; detect and parse the VState
element (the small/empty map element, e.g. CBOR major type 5 or byte 0xa0)
similar to parseCertState3, call the existing parseVState (or the same VState
parsing helper used by parseCertState3) on that element and assign its output to
result.DReps; implement the detection by scanning certState for a map element
with small length (prefer the empty/small map) and handle errors the same way
you do for PState/DState (wrap and return).
In `@ledgerstate/import_test.go`:
- Around line 119-129: The test logs state.Tip.Slot before verifying state.Tip
is non-nil, risking a panic; move the nil check earlier by calling
require.NotNil(t, state.Tip) (or t.Fatalf/assert) before the t.Logf that
references state.Tip.Slot so that state.Tip is validated prior to access; update
the t.Logf invocation to use state.Tip.Slot only after require.NotNil(t,
state.Tip) succeeds (referencing the t.Logf and require.NotNil calls and the
state.Tip symbol).
In `@ledgerstate/import.go`:
- Line 70: The code reads cfg.State.Tip.Slot into slot without checking
cfg.State.Tip for nil; add a nil-check for cfg.State.Tip before accessing Slot
(e.g., if cfg.State.Tip == nil { /* handle origin snapshot: set slot to 0 or
return/err */ }) and decide the proper behavior for origin snapshots (use zero
value, skip import, or return an error). Update the assignment of slot (the
variable named slot) and any downstream logic that assumes a non-nil Tip
(references to cfg.State.Tip or slot) so they correctly handle the nil-Tip case.
- Around line 1001-1028: The fallback in resolveEraParams can underflow because
it subtracts endBound.Epoch - startBound.Epoch when endBound may be zero; change
the guard to explicitly verify endBound.Epoch >= startBound.Epoch (and ideally >
startBound.Epoch) before doing the subtraction and only compute slotSpan if
endBound.Slot >= startBound.Slot (or compute with safe signed arithmetic),
otherwise return (0,0). Update resolveEraParams to check endBound.Epoch and
endBound.Slot against startBound before computing epochSpan/slotSpan so no
unsigned underflow occurs.
In `@ledgerstate/snapshot.go`:
- Around line 72-83: The code currently uses lexicographic sorting
(sort.StringSlice) on slot-name slices (utxoHDDirs and legacyFiles and similarly
in FindUTxOTableFile), which yields incorrect "most recent" selection; replace
those sort.Sort(sort.Reverse(sort.StringSlice(...))) calls with a
numeric-descending sort helper (e.g., sortSlotNamesDesc) that parses slot names
to integers and sorts by numeric value, ensuring non-numeric entries are handled
deterministically (e.g., treat parse errors as -inf or 0) and then return the
first element; update places referencing utxoHDDirs, legacyFiles, and
FindUTxOTableFile to use this helper.
In `@ledgerstate/snapshots.go`:
- Around line 118-228: The three parsers (parseStakeMap, parseDelegationMap,
parsePoolParamsMap) currently drop malformed entries with bare continues, losing
data silently; add a skipped counter in each function (e.g., skipped := 0),
increment it at every place that currently does "continue" on parse errors
(inside the entry loop for entry.KeyRaw and entry.ValueRaw/parsing calls), and
after the loop return the partial result plus a non-nil error when skipped > 0
(e.g., fmt.Errorf("parsed map with %d malformed entries", skipped)) so callers
are informed of data loss; update callers if necessary to handle the returned
error.
In `@ledgerstate/utxo.go`:
- Around line 510-515: The loop reads data[1+pos] without checking bounds which
can panic if the decoder position reaches or exceeds the end of data (truncated
stream or missing 0xff); before accessing data[1+pos] in the loop that uses
decoder.Position(), guard with a bounds check (e.g., ensure pos+1 < len(data) or
use decoder.Remaining()/len check) and handle the truncated input by returning
an error instead of indexing out of range, so the code in the loop that checks
for the 0xff terminator safely exits or reports a malformed input.
In `@mithril/bootstrap_test.go`:
- Around line 99-116: The test handlers registered with mux.HandleFunc (e.g.,
the "/artifact/snapshots" handler that calls json.NewEncoder.Encode(snapshots)
and the "/download/snapshot.tar.zst" handler that writes archiveData) use
require.NoError inside the handler goroutine, which can panic/hang; change these
to a non-fatal handling strategy: either replace require.NoError(t, err) with
assert.NoError(t, err) so the assertion is non-fatal from the goroutine, or
write errors into the response via http.Error(w, err.Error(),
http.StatusInternalServerError) and assert the response in the main test
goroutine. Apply the same fix pattern to other occurrences noted
(TestBootstrapNoSnapshots, TestBootstrapNoLocations and similar handlers across
this file and certverify_test.go), ensuring no call to t.FailNow()/require.*
runs inside handler goroutines.
In `@mithril/download_test.go`:
- Around line 32-38: The httptest.NewServer handlers in mithril/download_test.go
call require.NoError(t, err) from handler goroutines which panics; update each
handler (the ones that create server :=
httptest.NewServer(http.HandlerFunc(...))) to not call t.FailNow from a non-test
goroutine: replace require.NoError(t, err) with assert.NoError(t, err)
(non-fatal) or simply ignore the write error (e.g., _, _ = w.Write(content));
apply this change to all httptest handler functions in the file.
🟡 Minor comments (10)
mithril/extract.go-138-150 (1)
138-150:⚠️ Potential issue | 🟡 MinorUnchecked
outFile.Close()error on the write path.On line 150,
outFile.Close()is called without checking the returned error. For files opened withO_WRONLY,Closecan surface delayed write errors (e.g., filesystem full, NFS flush failures). Ignoring this could silently produce a corrupt extraction.Proposed fix
- outFile.Close() + if err := outFile.Close(); err != nil { + return "", fmt.Errorf( + "closing file %s: %w", + target, + err, + ) + }ledgerstate/mempack_debug_test.go-10-146 (1)
10-146:⚠️ Potential issue | 🟡 MinorDebug test with hardcoded path should not be committed.
This test depends on a hardcoded absolute path (
/tmp/dingo-mithril-download/...), produces no assertions (onlyt.Log), and will always be skipped in CI. It appears to be a development/debugging artifact. Consider removing it before merge, or converting it into a proper unit test with embedded test fixtures.Additional issues:
- Test name says "FirstTwoEntries" but iterates up to 5.
cbor.Decodeerrors silently ignored on lines 67 and 82.cmd/dingo/mithril.go-97-106 (1)
97-106:⚠️ Potential issue | 🟡 MinorPotential panic on short
DigestorCreatedAtstrings.
s.Digest[:16](line 100) ands.CreatedAt[:19](line 104) will panic if the strings are shorter than expected. While the Mithril API should always return full-length values, a malformed response would crash the CLI.🛡️ Defensive slicing
for _, s := range snapshots { + digest := s.Digest + if len(digest) > 16 { + digest = digest[:16] + } + createdAt := s.CreatedAt + if len(createdAt) > 19 { + createdAt = createdAt[:19] + } fmt.Printf( "%-16s %-8d %-8d %12s %s\n", - s.Digest[:16], + digest, s.Beacon.Epoch, s.Beacon.ImmutableFileNumber, humanBytes(s.Size), - s.CreatedAt[:19], + createdAt, ) }mithril/bootstrap.go-297-319 (1)
297-319:⚠️ Potential issue | 🟡 Minor
Cleanupremoves the entire parent directory of the archive, which may contain unrelated files.If the user passes a pre-existing
DownloadDir(e.g., a data directory containing other files),os.RemoveAll(filepath.Dir(r.ArchivePath))will delete everything in it. Consider tracking only the files/dirs created by bootstrap (archive, extractDir, ancillaryDir) and removing those individually, or document thatDownloadDirmust be dedicated to the bootstrap process.mithril/bootstrap.go-168-173 (1)
168-173:⚠️ Potential issue | 🟡 Minor
snapshot.Digest[:16]panics if digest is shorter than 16 characters.While Mithril digests are typically 64-char hex strings, a malformed API response could produce a shorter value. Add a length guard or use a safe truncation helper.
Proposed fix
+func safePrefix(s string, n int) string { + if len(s) < n { + return s + } + return s[:n] +} + archiveFilename := fmt.Sprintf( "%s-%s.tar.zst", snapshot.Network, - snapshot.Digest[:16], + safePrefix(snapshot.Digest, 16), )Also applies to: 257-261
ledgerstate/mempack.go-606-615 (1)
606-615:⚠️ Potential issue | 🟡 MinorLast asset name may include trailing padding bytes.
For the last asset in the flat buffer (lines 606-614), the name extends to the end of
flat, which per the comment "may include padding." If downstream code compares asset names by exact byte match, trailing padding could cause mismatches. Consider trimming trailing zero bytes, or document this behavior for callers.ledgerstate/snapshot.go-226-231 (1)
226-231:⚠️ Potential issue | 🟡 MinorEra-bounds extraction error is silently swallowed.
boundsErris captured but never logged. IfextractAllEraBoundsfails,eraBoundsis silently nil, which degrades the import (no full epoch history) without any diagnostic output. Consider logging it:Proposed fix
eraBounds, boundsErr := extractAllEraBounds(telescopeData) if boundsErr != nil { // Non-fatal: log but continue without bounds + // (requires passing a logger or returning a warning) eraBounds = nil }Since there's no logger in
parseSnapshotData, consider returning the warning alongside the result, or accept a logger parameter.mithril/download.go-71-97 (1)
71-97:⚠️ Potential issue | 🟡 MinorProgress speed is inflated for resumed downloads.
pw.writtenincludesexistingSize(set at line 223), so the speed calculation at line 85 (float64(pw.written) / elapsed) includes bytes that were already on disk before this session started. For a resume of a 5 GB file needing 100 MB more, the speed would show ~5.1 GB/s after 1 second.Use only the bytes written in the current session:
Proposed fix
var speed float64 if elapsed > 0 { - speed = float64(pw.written) / elapsed + speed = float64(pw.written-pw.startOffset) / elapsed }This requires adding a
startOffset int64field toprogressWriter(set toexistingSizeat creation).ledgerstate/verify_test.go-34-61 (1)
34-61:⚠️ Potential issue | 🟡 Minor
requirein HTTP handler goroutine can cause test hangs or panics.
require.NoError(t, err)at line 48 (and similarly at lines 93-94, 134-135) callst.FailNow()from a goroutine spawned byhttptest.Server, not the test goroutine. Go'stestingpackage documents thatFailNowmust be called from the goroutine running the test function. In a non-test goroutine it can cause hangs or panics.Use
t.Error+return(or justhttp.Error(w, ...)) instead ofrequireinside the handler.Example fix for the genesis test handler
err := json.NewEncoder(w).Encode(cert) - require.NoError(t, err) + if err != nil { + t.Errorf("encoding cert: %v", err) + http.Error(w, err.Error(), 500) + return + }mithril/download.go-158-209 (1)
158-209:⚠️ Potential issue | 🟡 Minor
resp.ContentLengthmay be -1, producing incorrecttotalSizefor resumed downloads.When the server doesn't send a
Content-Lengthheader (chunked transfer),resp.ContentLengthis -1. At line 174,totalSize = existingSize + (-1)would be incorrect, leading to negative or misleading progress percentages.Consider guarding against this:
Proposed fix
case http.StatusPartialContent: // Resume supported - totalSize = existingSize + resp.ContentLength + if resp.ContentLength > 0 { + totalSize = existingSize + resp.ContentLength + } file, err = os.OpenFile(
🧹 Nitpick comments (22)
internal/node/load.go (4)
74-89: DB initialization logic is duplicated betweenLoadWithDBandLoadBlobsWithDB.Consider extracting the "open DB if nil" pattern into a small helper (e.g.,
ensureDB) to reduce duplication and keep both paths consistent. CurrentlyLoadWithDBapplies bulk-load pragmas whileLoadBlobsWithDBdoes not—if that's intentional for the blob-only path, a comment would help clarify.Also applies to: 201-217
276-288: Unwrapped errors returned from the inner loop.Per coding guidelines, errors should be wrapped with
fmt.Errorfand%wto provide context. The barereturnat lines 279 and 287 loses information about where the failure occurred during block iteration.Proposed fix
next, err := iter.Next() if err != nil { - return blocksCopied, immutableTip.Slot, err + return blocksCopied, immutableTip.Slot, fmt.Errorf( + "reading next block from immutable DB: %w", err, + ) } // No more blocks if next == nil { break } tmpBlock, err := gledger.NewBlockFromCbor(next.Type, next.Cbor) if err != nil { - return blocksCopied, immutableTip.Slot, err + return blocksCopied, immutableTip.Slot, fmt.Errorf( + "decoding block from CBOR: %w", err, + ) }As per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb".
359-385: Same unwrapped-error pattern incopyBlocksRaw.Lines 362, 377, and 384 return errors without context wrapping.
Proposed fix
next, err := iter.Next() if err != nil { - return blocksCopied, immutableTip.Slot, err + return blocksCopied, immutableTip.Slot, fmt.Errorf( + "reading next block from immutable DB: %w", err, + ) } if next == nil { break } // ... headerCbor, err := extractHeaderCbor(next.Cbor) if err != nil { - return blocksCopied, immutableTip.Slot, err + return blocksCopied, immutableTip.Slot, fmt.Errorf( + "extracting header CBOR: %w", err, + ) } header, err := gledger.NewBlockHeaderFromCbor( next.Type, headerCbor, ) if err != nil { - return blocksCopied, immutableTip.Slot, err + return blocksCopied, immutableTip.Slot, fmt.Errorf( + "decoding block header from CBOR: %w", err, + ) }As per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb".
308-310:slices.Deleteto clear a batch is less idiomatic than re-slicing.
slices.Delete(blockBatch, 0, len(blockBatch))works butblockBatch = blockBatch[:0]is the standard Go idiom for reusing the underlying array, is clearer in intent, and avoids the overhead ofslices.Delete's internal copy/zero logic.Proposed change (both sites)
- blockBatch = slices.Delete(blockBatch, 0, len(blockBatch)) + blockBatch = blockBatch[:0]Also applies to: 406-408
ledgerstate/certstate.go (3)
103-130: Heuristic component detection is fragile.Identifying PState as "largest array element" and DState as "largest map element" by raw byte length works for current encodings but could silently misidentify components if the encoding layout changes or if multiple elements share the same CBOR major type. Consider adding a comment documenting which node version / encoding this heuristic targets, or validating the identified components (e.g., checking expected sub-structure after selection).
609-630:big.Int.Uint64()silently truncates values exceedingmath.MaxUint64.At lines 615-616,
rat.Num().Uint64()andrat.Denom().Uint64()will silently return the low 64 bits if the value overflows. For pool margin rationals (fractions < 1) this is safe in practice, butIsUint64()could be checked first for correctness.
190-218: Silently skipping unparseable pool entries may hide data issues.Lines 201-205 and 210-212
continueon decode failures without logging. During bootstrap, silently dropping pools could lead to incomplete state. Consider at minimum logging at debug level when entries are skipped.ledger/forging/builder_test.go (1)
566-567: Remove unusedinputvariable assignment.
NewConstructorEncoderexists in gouroboros v0.155.0, but theinputvariable is never used. The test manually builds the transaction body viabodyMapinstead. The comment already acknowledges this, but the dead code assignment can be safely removed for clarity.Proposed cleanup
- input := cbor.NewConstructorEncoder(0, cbor.IndefLengthList{}) - _ = input // not used directly; we encode manually below -chain/chain.go (1)
244-333:AddRawBlockclosely mirrorsAddBlock— consider extracting shared logic.
AddRawBlockduplicates ~70 lines ofAddBlock(reconcile, header match, tip fit, persist, tip update, iterator notification, event emission). The only difference is the input source (field access vs. interface calls). This is a reasonable duplication for now given the different input types, but if more block-insertion variants appear, extracting a sharedaddBlockCommon(point, hash, prevHash, blockNumber, blockType, cbor, txn)helper would reduce maintenance burden.database/models/import_checkpoint.go (1)
40-46: Consider makingImportPhaseOrderimmutable.
ImportPhaseOrderis avarslice, so callers could accidentally mutate it at runtime (e.g.,appendor index assignment). If this is intended to be a fixed constant list, wrapping it as a function or using a compile-time check would be safer.♻️ Optional: return a copy from a function
-// ImportPhaseOrder defines the sequential order of import phases. -var ImportPhaseOrder = []string{ - ImportPhaseUTxO, - ImportPhaseCertState, - ImportPhaseSnapshots, - ImportPhasePParams, - ImportPhaseTip, -} +// importPhaseOrder defines the sequential order of import phases. +var importPhaseOrder = []string{ + ImportPhaseUTxO, + ImportPhaseCertState, + ImportPhaseSnapshots, + ImportPhasePParams, + ImportPhaseTip, +} + +// ImportPhaseOrder returns the sequential order of import phases. +func ImportPhaseOrder() []string { + return append([]string(nil), importPhaseOrder...) +}ledgerstate/mempack_test.go (1)
203-233: Hardcoded filesystem path is fragile and machine-specific.
/tmp/dingo-mithril-download/ancillary/ledger/104173430/tables/tvaris specific to a particular snapshot and developer machine. While theSkipguard prevents CI failures, this is effectively dead code for most developers.Consider reading the path from an environment variable (e.g.,
DINGO_TVAR_PATH) or removing this test in favor of the integration test inimport_test.gowhich fetches real data.mithril/client.go (2)
57-61:WithHTTPClientoverrides security defaults (timeout, redirect protection).When a caller provides a custom
*http.Client, the 30s timeout andhttpsOnlyRedirectcheck are silently lost. Consider documenting this clearly, or applying the redirect policy defensively after applying options:💡 Suggested approach
func NewClient( aggregatorURL string, opts ...ClientOption, ) *Client { c := &Client{ aggregatorURL: aggregatorURL, httpClient: &http.Client{ Timeout: 30 * time.Second, CheckRedirect: httpsOnlyRedirect, }, } for _, opt := range opts { opt(c) } + // Ensure redirect protection is always applied + if c.httpClient.CheckRedirect == nil { + c.httpClient.CheckRedirect = httpsOnlyRedirect + } return c }
128-152: Consider sanitizingdigest/hashpath parameters.
GetSnapshotandGetCertificateconcatenate user-supplied strings directly into the URL path. While these are expected to be hex digests, an adversarial input containing../or query characters could produce an unexpected request. A simple hex-validation orurl.PathEscapewould make this defensive.ledgerstate/utxo.go (2)
62-66: Try 4 indecodeTxInappears to be dead code.After Try 1 either returns early (success with
len(keyBytes) >= 34) or falls through (withlen(keyBytes) < 34orkeyBytes == nil), the conditionlen(keyBytes) >= 34at line 64 can never be true. This block is unreachable.
439-448:os.ReadFileloads entire tvar file into memory.UTxO-HD tvar files on mainnet can be very large. While the streaming decoder avoids duplicating the data, the initial
os.ReadFilestill requires holding the entire file in memory. Consider using memory-mapped I/O or a buffered reader for very large files. This may be acceptable for now if mainnet tvar files are within reasonable bounds.ledgerstate/verify_test.go (1)
80-107: Fragile URL path extraction can panic on unexpected request paths.Line 85 (
hash := path[len("/certificate/"):]) and line 126 will panic with an index-out-of-range if any request path doesn't start with/certificate/(e.g., health-check probes). While this is test code, usingstrings.TrimPrefixor checking the prefix first is safer and avoids confusing failures.Safer approach
- hash := path[len("/certificate/"):] + hash, ok := strings.CutPrefix(path, "/certificate/") + if !ok { + http.NotFound(w, r) + return + }ledgerstate/mempack.go (1)
186-194: Typo:babbageTxOutCompactRefScripis missing the trailing 't'.The constant name is
babbageTxOutCompactRefScripbut the corresponding function isdecodeTxOutCompactRefScript. Not a functional bug (the constant is used consistently), but it's a naming inconsistency that could confuse readers.Proposed fix
- babbageTxOutCompactRefScrip = 5 // CompactAddr + Value + Datum + Script + babbageTxOutCompactRefScript = 5 // CompactAddr + Value + Datum + ScriptAnd update the switch case at line 227 accordingly.
ledgerstate/snapshot.go (1)
169-179:os.ReadFileloads the entire snapshot into memory.Cardano ledger snapshots can be several GB. Loading the entire file at once (line 173) creates a large memory spike. While deferred CBOR decoding (via
cbor.RawMessage) avoids re-parsing, the full byte slice remains in memory until GC. Consider whether a streaming approach (e.g.,mmaporcbor.Decoderover a reader) would be feasible for the outer structure navigation.ledgerstate/import.go (1)
452-494: Accounts, pools, and DReps are imported one-at-a-time in a loop.
importAccounts,importPools, andimportDRepseach iterate over the entire collection calling individualstore.Import*methods in a loop within a single transaction. In contrast,importUTxOsuses a batch approach (store.ImportUtxoswith a slice). If the store supports batch imports for accounts/pools/DReps, using the same pattern would be significantly faster for large datasets.As per coding guidelines, prefer batch operations over N+1 patterns: "Use batch fetch queries (WHERE id IN ?) instead of N+1 queries in loops."
Also applies to: 497-571, 573-624
mithril/bootstrap.go (1)
376-443:verifyCertChainduplicates logic fromledgerstate.VerifyCertificateChain.Both functions walk the certificate chain, detect cycles, and terminate at genesis. The only addition in
verifyCertChainis the leaf digest binding check (lines 407-420). Maintaining two copies of the chain-walking logic risks them diverging. Consider extending theledgerstate.VerifyCertificateChainwith an optional digest-check parameter and calling it from here.mithril/types.go (2)
31-65: Consider extracting shared fields to reduce duplication.
SnapshotListItemandSnapshothave identical field sets. If the Mithril API is expected to diverge them over time, keeping them separate is fine, but embedding a shared base could reduce maintenance burden.♻️ Example using embedding
+// snapshotBase contains fields common to both list and detail +// snapshot responses. +type snapshotBase struct { + Digest string `json:"digest"` + Network string `json:"network"` + Beacon Beacon `json:"beacon"` + CertificateHash string `json:"certificate_hash"` + Size int64 `json:"size"` + AncillarySize int64 `json:"ancillary_size"` + CreatedAt string `json:"created_at"` + Locations []string `json:"locations"` + AncillaryLocations []string `json:"ancillary_locations"` + CompressionAlgorithm string `json:"compression_algorithm"` + CardanoNodeVersion string `json:"cardano_node_version"` +} + type SnapshotListItem struct { - Digest string `json:"digest"` - ... + snapshotBase } + type Snapshot struct { - Digest string `json:"digest"` - ... + snapshotBase }
46-48: Wrap errors returned byCreatedAtTimewith context.Both
CreatedAtTimemethods return the bare error fromtime.Parse. As per coding guidelines, errors should be wrapped withfmt.Errorfand the%wverb to provide call-site context.♻️ Proposed fix
+import "fmt" + func (s *SnapshotListItem) CreatedAtTime() (time.Time, error) { - return time.Parse(time.RFC3339Nano, s.CreatedAt) + t, err := time.Parse(time.RFC3339Nano, s.CreatedAt) + if err != nil { + return time.Time{}, fmt.Errorf( + "parsing CreatedAt: %w", + err, + ) + } + return t, nil } func (s *Snapshot) CreatedAtTime() (time.Time, error) { - return time.Parse(time.RFC3339Nano, s.CreatedAt) + t, err := time.Parse(time.RFC3339Nano, s.CreatedAt) + if err != nil { + return time.Time{}, fmt.Errorf( + "parsing CreatedAt: %w", + err, + ) + } + return t, nil }As per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb".
Also applies to: 67-70
wolf31o2
force-pushed
the
feat/mithril-bootstrap
branch
from
bba42f8 to
2142bfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In `@cmd/dingo/mithril.go`:
- Around line 510-518: The code building importKey uses
result.Snapshot.Digest[:16] which can panic if the digest is shorter than 16
chars; update the importKey construction in mithril.go to safely slice the
digest (e.g., take the first min(16, len(result.Snapshot.Digest))) or fall back
to the full digest when shorter, and then format it with state.Tip.Slot so
importKey is always built without risking an out-of-range slice on
result.Snapshot.Digest.
- Around line 495-518: The code dereferences state.Tip (state.Tip.Slot) without
a nil-check which can panic; add a nil guard: compute a safeSlot variable (e.g.,
safeSlot := uint64(0) or -1 sentinel) by checking if state.Tip != nil then
safeSlot = state.Tip.Slot, and use safeSlot in the logger.Info call (replace
state.Tip.Slot) and in the importKey fmt.Sprintf call (replace state.Tip.Slot)
so both uses are protected when state.Tip is nil.
- Around line 98-106: The code slices s.Digest[:16] and s.CreatedAt[:19] inside
the snapshots loop which can panic if those strings are shorter; change the
printing to use safe truncation (e.g., compute minLen := func(n int, s string)
int { if len(s) < n { return len(s) } return n } and use s.Digest[:minLen(16,
s.Digest)] and s.CreatedAt[:minLen(19, s.CreatedAt)] or a small helper
truncate(s, n) that returns s or s[:n]) so the fmt.Printf call in the snapshots
loop uses the truncated values instead of raw slices to avoid panics.
- Around line 355-361: The call to result.Cleanup(logger) is unconditional but
Bootstrap's contract says cleanup should be performed by the caller only when
cfg.Mithril.CleanupAfterLoad is true; change the unconditional call in the
Bootstrap caller to check cfg.Mithril.CleanupAfterLoad and only invoke
result.Cleanup(logger) when that flag is set, referencing the existing
cfg.Mithril.CleanupAfterLoad and result.Cleanup symbols in the surrounding
Bootstrap-related code.
In `@database/plugin/metadata/postgres/import.go`:
- Around line 83-124: ImportPool unconditionally calls db.Create(reg) which can
insert duplicate PoolRegistration rows on retries; change this to an idempotent
upsert or existence-check update. After setting reg.PoolID (and owners/relays),
replace the db.Create(reg) call in ImportPool with an upsert (e.g.,
db.Clauses(clause.OnConflict{Columns: /* use the unique registration constraint
columns */, DoNothing or DoUpdates: clause.AssignmentColumns(/* fields to update
*/)}).Create(reg)) or use FirstOrCreate/FirstOrInit+Save keyed on the
registration unique constraint so repeated ImportPool calls will not create
duplicates; ensure you reference the reg variable and the ImportPool method and
import/use gorm.io/gorm/clause as needed.
In `@database/plugin/metadata/sqlite/import.go`:
- Around line 84-140: ImportPool currently always calls db.Create(reg) which
creates duplicate PoolRegistration entries on retries; change the creation to an
upsert or guarded create: instead of unconditional db.Create(reg) use
db.Clauses(clause.OnConflict{Columns: []clause.Column{{Name: "pool_id"}, {Name:
"tx_hash"}, {Name: "cert_index"}}, DoNothing: true/DoUpdates: ...}).Create(reg)
or alternatively perform a pre-check (db.Where("pool_id = ? AND tx_hash = ? AND
cert_index = ?", reg.PoolID, reg.TxHash, reg.CertIndex).First(&existing) and
only create if not found) so ImportPool (and the reg variable) will not insert
duplicate PoolRegistration records on retry.
In `@ledgerstate/cbor_decode.go`:
- Around line 57-97: The indefinite-length map handling in cbor_decode.go
collects entries into entries but always returns an error after the loop; change
the control flow so finding the 0xff break code returns the parsed entries with
nil error instead of falling through. Specifically, in the loop inside the block
that checks if first == 0xbf, either replace the single-line break when
data[pos] == 0xff with "return entries, nil" or after the loop add an explicit
check "if pos < len(data) && data[pos] == 0xff { return entries, nil }" and only
return the existing "indefinite-length map: break code not found" error when the
break code truly isn't present; refer to variables/function names first, pos,
entries, cborItemSize, and MapEntry to locate the logic to change.
In `@ledgerstate/govstate.go`:
- Around line 417-426: The current block silently leaves prop.ActionType as 0 on
decode failures (decodeRawArray and cbor.Decode of govAction[0]), which
misclassifies actions as ParameterChange; change the logic in the function that
parses procedure so that if decodeRawArray(procedure[2]) fails or
cbor.Decode(govAction[0], &actionType) fails you return an error (propagate a
descriptive error) instead of silently leaving prop.ActionType as 0 —
alternatively, if you prefer not to fail, set prop.ActionType to a clear
sentinel (e.g., 0xFF) and document it; update callers to handle the new error or
sentinel accordingly.
In `@ledgerstate/mempack_debug_test.go`:
- Around line 65-68: The cbor.Decode calls that populate keyBytes and valBytes
are ignoring their error return; update the two calls to check the returned
error (from cbor.Decode(keyRaw, &keyBytes) and cbor.Decode(valRaw, &valBytes))
and handle failures (e.g., t.Fatalf or t.Errorf and return) so a decode failure
doesn't leave keyBytes/valBytes nil and produce misleading logs; ensure you
reference the variables keyBytes and valBytes in the error handling and include
the decode error in the test log message.
- Around line 51-56: The test reads mapData[1+pos] without bounds checking which
can panic if innerDecoder.Position() returns a value that makes 1+pos >=
len(mapData); update the loop in mempack_debug_test.go to compute idx := 1 +
innerDecoder.Position() and guard with if idx >= len(mapData) { t.Log("no break
byte available") ; break } (or t.Fatalf if you want the test to fail) before
accessing mapData[idx], so the check of the break byte is only performed when
the index is in-bounds.
In `@ledgerstate/snapshot.go`:
- Around line 520-524: The Origin tip decoding currently returns a zero-valued
&SnapshotTip{} (Slot:0, BlockHash:nil) which bypasses the nil guard in
ImportLedgerState; change the behavior in the decoder that handles the empty tip
array (the len(tipArr)==0 branch in snapshot.go) to return nil instead of
&SnapshotTip{} so ImportLedgerState's cfg.State.Tip == nil check correctly
detects Origin snapshots, and ensure any callers of SnapshotTip decoding expect
and handle a nil return for Origin.
In `@mithril/bootstrap.go`:
- Around line 169-173: The code slices snapshot.Digest with snapshot.Digest[:16]
which will panic if the digest is shorter than 16 bytes; replace those direct
slices with a safe truncation: compute a local variable like d :=
snapshot.Digest; if len(d) > 16 { d = d[:16] } and use d when building
archiveFilename (and the other filename occurrence that also uses
snapshot.Digest), ensuring both places (the archiveFilename construction and the
later use around the 298/295-299 area) use this safe truncation to avoid
out-of-range panics.
- Around line 336-357: Cleanup currently removes the parent directory of
ArchivePath which can delete user directories; change Cleanup (method on
BootstrapResult) to remove only the specific artifacts the Bootstrap created:
delete the archive file at r.ArchivePath (if set), and call os.RemoveAll on
r.ExtractDir and r.AncillaryDir (if those fields exist and are set) instead of
filepath.Dir(r.ArchivePath); if the struct tracks whether a temporary download
dir was created (e.g., a boolean like CreatedTempDir or DownloadDirCreated),
only call RemoveAll on the parent dir when that flag is true — otherwise avoid
removing the parent directory to prevent deleting user-provided folders.
In `@mithril/download.go`:
- Around line 71-97: The periodic BytesPerSecond calculation in
progressWriter.Write wrongly uses pw.written (which includes previous session
bytes) causing inflated speed during resumed downloads; add a startOffset int64
field to the progressWriter struct (set to existingSize when the writer is
created), and change the speed computation in progressWriter.Write (and any
periodic reporting) to use float64(pw.written - pw.startOffset) / elapsed
(matching the final report logic that uses pw.written - existingSize) while
guarding against negative values and zero elapsed.
- Around line 172-175: The calculation of totalSize in the
http.StatusPartialContent branch can be wrong when resp.ContentLength == -1;
update the logic in the switch case handling StatusPartialContent (where
totalSize, existingSize and resp.ContentLength are used) to guard for a missing
Content-Length by only adding resp.ContentLength when it is >= 0 (e.g., set
totalSize = existingSize + resp.ContentLength if resp.ContentLength >= 0,
otherwise set totalSize = -1 or leave unknown) and ensure progress reporting and
any subsequent uses of totalSize handle the unknown/-1 case correctly; adjust
the code around the file opening and progress handling in the same block to
account for this change.
🧹 Nitpick comments (19)
chain/chain.go (1)
258-333: Significant duplication withAddBlock— consider extracting shared logic.
AddRawBlockreproduces ~95% ofAddBlock's body (locking, reconcile, header check, tip-fit check, block construction, persistence, header removal, tip update, iterator notification, event publish). If the chain-integrity or event-emission logic changes, both methods must be updated in lockstep, which is error-prone.Consider extracting the common post-validation logic (persist, update tip, notify, emit) into a private helper that both methods call after constructing the
models.Block.Sketch of a shared helper
// addBlockInternal performs the common post-validation steps for both // AddBlock and AddRawBlock. Caller must hold c.mutex and c.manager.mutex. func (c *Chain) addBlockInternal( tmpBlock models.Block, tmpPoint ocommon.Point, txn *database.Txn, ) error { newBlockIndex := c.tipBlockIndex + 1 tmpBlock.ID = newBlockIndex if err := c.manager.addBlock(tmpBlock, txn, c.persistent); err != nil { return err } if !c.persistent { c.blocks = append(c.blocks, tmpPoint) } if len(c.headers) > 0 { c.headers = slices.Delete(c.headers, 0, 1) } c.currentTip = ochainsync.Tip{ Point: tmpPoint, BlockNumber: tmpBlock.Number, } c.tipBlockIndex = newBlockIndex c.waitingChanMutex.Lock() if c.waitingChan != nil { close(c.waitingChan) c.waitingChan = nil } c.waitingChanMutex.Unlock() if c.eventBus != nil { c.eventBus.Publish( ChainUpdateEventType, event.NewEvent( ChainUpdateEventType, ChainBlockEvent{ Point: tmpPoint, Block: tmpBlock, }, ), ) } return nil }chainselection/selector.go (2)
224-239:BlockDifferenceis an absolute block number, not a delta.When the best peer disconnects, the previous tip is unavailable (already deleted from
peerTipsat line 205).BlockDifferenceis computed assafeUint64ToInt64(newPeerTip.Tip.BlockNumber), which is the new peer's absolute block number, not a meaningful difference. The same pattern appears incleanupStalePeers(line 568-570).This could surprise event consumers expecting a true delta. Consider either:
- Saving the previous peer's tip before the
deleteat line 205, so a real diff can be computed, or- Documenting that
BlockDifferencemay represent an absolute value when no previous tip is available.Option 1: preserve previous tip before delete
func (cs *ChainSelector) RemovePeer(connId ouroboros.ConnectionId) { var switchEvent *event.Event func() { cs.mutex.Lock() defer cs.mutex.Unlock() + // Save tip before deletion for event payload + var previousTip ochainsync.Tip + if pt, ok := cs.peerTips[connId]; ok { + previousTip = pt.Tip + } delete(cs.peerTips, connId)Then use
previousTipin the event and computesafeBlockDiff(newPeerTip.Tip.BlockNumber, previousTip.BlockNumber).
559-574: SameBlockDifference/ missingPreviousTippattern asRemovePeer.In
cleanupStalePeers, the stale best peer is deleted frompeerTips(line 534) before the event is constructed, soPreviousTipis omitted andBlockDifferenceis an absolute value rather than a delta. If you address the suggestion inRemovePeer, apply the same fix here by saving the tip before deletion.ledger/forging/builder_test.go (1)
566-567: Dead code:inputis constructed but immediately discarded.
cbor.NewConstructorEncoder(0, cbor.IndefLengthList{})is called on line 566, but the result is never used — line 567 immediately blanks it with_ = input. If this is meant to verify that the API compiles, consolidate to a single line. Otherwise, remove both lines and construct the tagged input directly in thebodyMapas already done.Suggested cleanup
- input := cbor.NewConstructorEncoder(0, cbor.IndefLengthList{}) - _ = input // not used directly; we encode manually below + // Inputs are encoded as a tagged set (tag 258)ledgerstate/certstate.go (1)
219-239: Silentcontinueon parse errors loses diagnostic information.In
parseCertPoolParamsMap(and similarly inparseCredentialMap,parsePoolDeposits, andparseVState), malformed entries are silently skipped. During Mithril bootstrap debugging, it would be very helpful to know how many entries were skipped and why.Consider adding a counter or debug-level logging for skipped entries. This applies to the same pattern throughout the file (lines 304–307, 389–390, 547–549, 676–679).
internal/node/load.go (1)
195-239:LoadBlobsWithDBskips bulk-load optimizations thatLoadWithDBenables.
LoadWithDB(lines 90–109) checks forBulkLoadOptimizerand sets bulk-load pragmas (e.g., WAL mode, synchronous=OFF for SQLite) before importing blocks.LoadBlobsWithDBskips this entirely. Since blob-only loading can still involve writing thousands of blocks, it would benefit from the same optimization.♻️ Proposed addition after line 226
c := cm.PrimaryChain() if c == nil { return nil, errors.New("primary chain not available") } + // Enable bulk-load optimizations if available + if optimizer, ok := db.Metadata().(metadata.BulkLoadOptimizer); ok { + if err := optimizer.SetBulkLoadPragmas(); err != nil { + logger.Warn( + "failed to set bulk-load optimizations", + "error", err, + ) + } else { + defer func() { + if err := optimizer.RestoreNormalPragmas(); err != nil { + logger.Error( + "failed to restore normal settings", + "error", err, + ) + } + }() + } + } blocksCopied, immutableTipSlot, err := copyBlocksRaw(ledgerstate/mempack.go (1)
146-153:readVarLentruncates uint64 to int without overflow check.The
#nosec G115suppresses the gosec warning, but if a VarLen-encoded value exceedsmath.MaxInt(possible on 32-bit targets), the result silently wraps. This is used for byte-string lengths inreadLengthPrefixedBytes, so a malformed input could cause incorrect length values and subsequent out-of-bounds reads rather than a clean error.Consider adding an explicit overflow guard:
Proposed fix
func (r *mempackReader) readVarLen() (int, error) { result, err := r.readVarLenUint64() if err != nil { return 0, err } - // `#nosec` G115 - return int(result), nil + if result > uint64(^uint(0)>>1) { + return 0, fmt.Errorf( + "mempack: VarLen value %d overflows int", result, + ) + } + return int(result), nil }ledgerstate/utxo.go (3)
62-66: Try 4 is unreachable dead code.If Try 1's
cbor.Decodesucceeds withlen(keyBytes) >= 34, we return at line 46. If it fails,keyBytesremainsnil. If it succeeds withlen(keyBytes) < 34, the check at line 64 is false. In all cases, the conditionlen(keyBytes) >= 34at line 64 can never be true at this point.Proposed removal
- // Try 4: If we got a short byte string, it might still be - // a valid TxIn with known length - if len(keyBytes) >= 34 { - return decodeTxInFromBytes(keyBytes) - } - // Nothing worked — return diagnostic error
436-448:os.ReadFileloads the entire tvar file into memory.UTxO-HD tvar files can be very large (multi-GB on mainnet). Loading the entire file with
os.ReadFiledefeats the purpose of the streaming decode that follows. Consider using memory-mapped I/O (mmap) or a buffered reader to avoid the memory spike.
325-347:extractAddressKeysslices into the address without copying.
PaymentKeyandStakingKeyare set as sub-slices ofaddr. If the caller later modifiesaddr, these keys will be corrupted. Sinceaddrcomes from decoded data that may be reused across batches (e.g., the batch slice is reset withbatch[:0]), this could lead to subtle data corruption during streaming.However, looking at the flow:
ParsedUTxOvalues are appended to the batch and passed to the callback before the batch is reset. Theaddrslice comes from freshly allocated data in the mempack decoder (make([]byte, ...)). So the risk is low in the current code path but is fragile.mithril/certverify_test.go (1)
268-363:requirecalls insidehttptesthandlers run in a separate goroutine.
require.NoError/require.functions callt.FailNow(), which must only be called from the goroutine running the test. HTTP handlers inhttptestservers run in their own goroutine. If the assertion fails, this can cause a panic or test suite crash instead of a clean failure.Consider using
assert(which doesn't callFailNow) or manually writing the error response and returning. This applies to all handler functions in this file.ledgerstate/mempack_test.go (1)
203-206: Hardcoded tvar path duplicated across test files.The path
/tmp/dingo-mithril-download/ancillary/ledger/104173430/tables/tvaris repeated inmempack_debug_test.go(line 11) and here (line 204). Consider extracting it to a shared test constant or environment variable to avoid drift.cmd/dingo/mithril.go (1)
77-83: Usecmd.Context()instead ofcontext.Background()for CLI cancellation support.Both
mithrilListCommand(line 79) andmithrilShowCommand(line 144) usecontext.Background()for API calls. If a user cancels with Ctrl+C during a slow aggregator request, the call won't be interrupted. Usingcmd.Context()propagates cancellation.♻️ Proposed fix (line 79)
client := mithril.NewClient(aggregatorURL) snapshots, err := client.ListSnapshots( - context.Background(), + cmd.Context(), )ledgerstate/import.go (2)
505-537: N+1 insert pattern for accounts will be slow at scale.
importAccountscallsstore.ImportAccountonce per account inside a loop. With hundreds of thousands of accounts this is significantly slower than a bulk insert. The UTxO path already uses a batchedstore.ImportUtxos(plural). Consider adding a similarImportAccountsbulk method to the metadata store to align with the batching approach used for UTxOs.The same N+1 pattern applies to
importPools(lines 555–630) andimportDReps(lines 651–683).As per coding guidelines, "Use batch fetch queries (WHERE id IN ?) instead of N+1 queries in loops."
1162-1166: Swallowed non-nil error whengovStateis also non-nil — logic looks intentional but is subtle.Line 1162:
if govState == nil && err != nilmeans that if bothgovState != nilanderr != nil, the function falls through to line 1170 where the error is logged as a warning. This is the intended "best-effort" behavior, but the conditiongovState == nil && err != nilat line 1162 can be easily misread as "if there's an error, return it." A short inline comment would help future readers.ledgerstate/snapshot.go (1)
200-210:os.ReadFileloads the entire snapshot into memory.Cardano ledger state snapshots can be multiple GB. On memory-constrained nodes this single allocation could cause OOM. The subsequent CBOR decoding also needs random access, so streaming is non-trivial, but worth noting as a known limitation. A memory-mapped file or streaming CBOR decoder could be considered in a future iteration.
ledgerstate/telescope.go (1)
156-172: Redundant length check after empty guard.Line 163
if len(summary) >= 1is always true since line 156 already returned an error whenlen(summary) == 0. Not a bug, just unnecessary.Minor cleanup
if len(summary) == 0 { return nil, fmt.Errorf( "past era %s summary is empty", EraName(eraIndex), ) } var bound EraBound - if len(summary) >= 1 { - slot, epoch, bErr := parseBound(summary[0]) - if bErr != nil { - return nil, fmt.Errorf( - "parsing past era %s bound: %w", - EraName(eraIndex), bErr, - ) - } - bound = EraBound{Slot: slot, Epoch: epoch} + slot, epoch, bErr := parseBound(summary[0]) + if bErr != nil { + return nil, fmt.Errorf( + "parsing past era %s bound: %w", + EraName(eraIndex), bErr, + ) } + bound = EraBound{Slot: slot, Epoch: epoch}mithril/bootstrap.go (1)
414-481: Duplicated certificate chain walking logic betweenverifyCertChainandVerifyCertificateChain.The chain traversal (cycle detection, genesis/self-chain termination, depth limit) here closely mirrors
ledgerstate/verify.go:VerifyCertificateChain. The additional value here is the leaf digest binding check (lines 445–458). Consider extracting the shared chain-walk into a common helper and composing the digest check on top, to avoid divergence over time.mithril/types.go (1)
31-70:SnapshotListItemandSnapshotare structurally identical.These two types share every field and both have an identical
CreatedAtTime()helper. If the Mithril API guarantees the same shape for list vs. detail endpoints, consider embedding a shared base type to reduce duplication. If they're kept separate for forward-compatibility, a brief comment explaining why would help.
wolf31o2
force-pushed
the
feat/mithril-bootstrap
branch
from
2142bfa to
1ce3e2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
35-36:⚠️ Potential issue | 🟡 MinorMissing space in documentation text.
Line 35:
dingo.yaml.example.You can copy` — there's a missing space between the backtick-terminated filename and "You".-A sample configuration file is provided at `dingo.yaml.example`.You can copy and edit this file to configure Dingo for your local or production environment: +A sample configuration file is provided at `dingo.yaml.example`. You can copy and edit this file to configure Dingo for your local or production environment:
🤖 Fix all issues with AI agents
In `@cmd/dingo/mithril.go`:
- Around line 77-83: The list/show commands use context.Background() when
calling the Mithril client, so in mithrilListCommand and mithrilShowCommand
replace context.Background() with the command's context (cmd.Context()) when
invoking client.ListSnapshots and the corresponding show call; update any calls
like client.ListSnapshots(context.Background(), ...) and the show snapshot
request to use cmd.Context() so SIGINT/cancel will propagate to the HTTP
request.
In `@database/plugin/metadata/mysql/import.go`:
- Around line 123-154: ImportDrep currently upserts models.Drep by credential
but does not ensure drep.ID is populated (MySQL Create with OnConflict won't set
the existing row's PK), so subsequent creation of models.RegistrationDrep can
use a zero/incorrect DrepID and duplicate registrations on re-import; after the
upsert in ImportDrep, fetch the DRep row by credential (e.g.,
db.Where("credential = ?", drep.Credential).First(&drep)) to populate drep.ID,
set reg.DrepID = drep.ID, and then create the registration using conflict-safe
semantics (use clause.OnConflict{DoNothing: true} or an upsert on unique keys)
when calling db.Create/reg insertion so repeated imports are idempotent; keep
all operations on the resolved txn/db (resolveDB, ImportDrep, models.Drep,
models.RegistrationDrep).
- Around line 90-121: The MySQL upsert can leave pool.ID unset, so after the
upsert via db.Clauses(clause.OnConflict{...}).Create(pool) check if pool.ID == 0
and, if so, refetch the record by pool_key_hash to populate pool.ID before
assigning reg.PoolID and linking owners/relays; additionally, avoid duplicate
PoolRegistration rows by using conflict handling on the registration insert
(e.g., db.Clauses(clause.OnConflict{...}).Create(reg) or equivalent upsert for
the PoolRegistration model) so repeated imports won’t create duplicates — update
the import logic around the variables and functions pool, reg, PoolID,
pool_key_hash, and the db.Create calls to implement these fixes.
In `@database/plugin/metadata/postgres/import.go`:
- Around line 91-110: The upsert may leave pool.ID set to 0 on conflict; after
the db.Clauses(...).Create(pool) call (the result variable) check if pool.ID ==
0 and, if so, re-query the DB for the existing pool row (e.g., SELECT ... WHERE
pool_key_hash = pool.PoolKeyHash) to load and set pool.ID before assigning
reg.PoolID and iterating reg.Owners / reg.Relays; update the block around the
OnConflict upsert (variables: db, pool, result, clause.OnConflict) to perform
this re-fetch so owners/relays get the correct PoolID.
In `@ledgerstate/cbor_decode.go`:
- Around line 126-134: The definite-length map branch uses the CBOR-declared
count to allocate and iterate without bounds, risking OOM/panic; before calling
make([]MapEntry...) and starting the for-loop in the code around cborArgument,
cap the parsed count using the existing maxMapEntries constant (e.g., if count >
maxMapEntries then set count = maxMapEntries or return an error) and ensure the
loop iterates only up to that capped value so MapEntry allocation and iteration
cannot be driven by a maliciously large CBOR count; update references to count,
headerLen, and the loop that constructs entries accordingly.
- Around line 338-352: The switch handling major type 7 (simple values/floats)
returns fixed sizes for info values 24–27 without checking input length, which
can cause panics in callers like decodeMapEntries; update the case branches for
info 24,25,26,27 inside the major type 7 handler to validate that the remaining
buffer (len(data) - pos or equivalent) is at least the required size (2,3,5,9)
and return an explicit truncation/error instead of the size when it's too short;
ensure the function returns a clear error value (not a size) for truncated data
so callers like decodeMapEntries can handle it safely.
In `@ledgerstate/mempack.go`:
- Around line 615-624: The last-asset path populates assetName from
flat[nameOff:] which may include word-alignment padding bytes; update the code
that sets assetName (the branch using nameOff and flat) to trim trailing
null/padding bytes (e.g., remove trailing 0x00) or compute the true name length
from the structure instead of taking the entire suffix, so assetName contains
only the real name bytes (use bytes.TrimRight or locate the first zero
terminator before assigning to assetName).
- Around line 443-446: The script decoding currently calls
r.readLengthPrefixedBytes() directly and ignores the AlonzoScript tag
discriminator; change this to first read the discriminator byte/token (e.g.,
using r.readByte or r.readVarUint as appropriate) and switch on its value (0 =>
NativeScript, 1 => PlutusScript) before reading the actual packed script payload
(then call r.readLengthPrefixedBytes() or the appropriate reader for the inner
script); ensure you return an error for any unknown tag and update the variable
currently named script to hold the correctly-dispatched payload for the
AlonzoScript union (reference the existing symbols r.readLengthPrefixedBytes(),
the local variable script, and the AlonzoScript/NativeScript/PlutusScript union
types).
In `@ledgerstate/telescope.go`:
- Around line 314-317: In navigateTelescopeNested, when handling case tag == 0
the code accesses items[1] without ensuring len(items) >= 2; add a bounds check
at the start of the case (or before accessing items[1]) to return a clear error
if len(items) < 2, and ensure the same check is applied on recursive entry since
callers pass decoded rest from items[2]; update any error messages returned by
navigateTelescopeNested to reflect a malformed CBOR/insufficient items scenario
so malformed recursive inputs are rejected safely.
In `@ledgerstate/verify.go`:
- Around line 34-42: The VerifyCertificateChain wrapper currently drops snapshot
verification by passing an empty snapshotDigest; update the exported function
signature VerifyCertificateChain(ctx context.Context, client *mithril.Client,
certificateHash string, snapshotDigest string) and forward that snapshotDigest
to mithril.VerifyCertificateChain(ctx, client, certificateHash, snapshotDigest)
so the leaf certificate's snapshot_digest binding is validated; keep existing
context, client and certificateHash parameters and only add and propagate
snapshotDigest in the call to mithril.VerifyCertificateChain.
🧹 Nitpick comments (22)
ledger/forging/builder_test.go (1)
566-567: Remove dead code:inputvariable is unused.Lines 566–567 construct an
inputvariable that is never referenced. The comment acknowledges it is not used directly. Removing both lines reduces noise.Suggested cleanup
- input := cbor.NewConstructorEncoder(0, cbor.IndefLengthList{}) - _ = input // not used directly; we encode manually below - bodyMap := map[uint]any{ledgerstate/certstate.go (3)
86-172: Heuristic-based Conway CertState detection is fragile — consider adding structural validation.The approach of identifying PState, DState, and VState by relative byte-size of CBOR elements works for the known layouts, but a future encoding change could easily cause misidentification (e.g., a large VState DRep map exceeding the DState credential map). Consider adding a secondary validation step — for instance, after tentatively assigning PState/DState, attempt to decode each and swap assignments if the initial guess fails.
This isn't blocking since the code handles current known encodings, but it's worth hardening for forward compatibility.
220-239: Silently skipping malformed pool entries hides data loss.Lines 222-226 and 231-233
continueon decode errors without logging, which means missing pools in the import could go unnoticed. Consider emitting a debug/warn log or at least counting skipped entries so operators can detect unexpected data loss during import.Proposed approach
pools := make([]ParsedPool, 0, len(entries)) + var skipped int for _, entry := range entries { var poolKeyHash []byte if _, pErr := cbor.Decode( entry.KeyRaw, &poolKeyHash, ); pErr != nil { + skipped++ continue } pool, err := parsePoolParams( poolKeyHash, entry.ValueRaw, ) if err != nil { + skipped++ continue } pools = append(pools, *pool) } + if skipped > 0 { + // Consider logging: skipped N malformed pool entries + } return pools, nil
729-752: Credential type (key vs script) is discarded.
parseCredentialonly extracts the hash fromcred[1]and discardscred[0](the credential type: 0 = key-hash, 1 = script-hash). If downstream code ever needs to distinguish key-based from script-based staking credentials (e.g., for filtering or display), this information is lost.If this is intentional for the current use case, a brief comment clarifying that the type is not needed would help future readers.
chain/chain.go (1)
258-333: Significant duplication betweenAddRawBlockandAddBlock.
AddRawBlockis nearly identical toAddBlock(~70 lines of shared logic: reconcile, header check, tip fit check, block construction, persistence, header removal, tip update, iterator notification, event emission). The only difference is how fields are extracted (struct fields vs interface methods).Consider extracting the common post-validation logic into a private helper to reduce the maintenance burden. For example:
♻️ Sketch of shared helper
+// addBlockCommon contains the shared logic for AddBlock and +// AddRawBlock. Caller must hold c.mutex and c.manager.mutex. +func (c *Chain) addBlockCommon( + hash, prevHash, cborData []byte, + slot, blockNumber uint64, + blockType uint, + txn *database.Txn, +) error { + // Header match check, tip fit check, block construction, + // persistence, header removal, tip update, notification, + // and event emission — extracted from AddBlock. + ... +}chainselection/selector.go (1)
226-238:PreviousTipfield omitted inRemovePeerandcleanupStalePeersevents.Both
RemovePeer(here) andcleanupStalePeers(line 561) emitChainSwitchEventwithout aPreviousTip, whileEvaluateAndSwitch(line 430) populates it. This is understandable since the departing peer's tip data is deleted before event construction, but subscribers that rely onPreviousTipfor logging or metrics will silently get a zero-valued tip. Consider capturing the previous tip beforedelete(cs.peerTips, connId)if consistency matters to consumers.internal/node/load.go (3)
74-89: Duplicated database-initialization block.The
db == nilbootstrap (lines 75–89) is copy-pasted inLoadBlobsWithDB(lines 203–217). Extract a helper likeopenOrReuseDB(cfg, logger, db) (*database.Database, bool, error)returning whether the caller should close it.♻️ Sketch
func openOrReuseDB( cfg *config.Config, logger *slog.Logger, db *database.Database, ) (*database.Database, bool, error) { if db != nil { return db, false, nil } dbConfig := &database.Config{ DataDir: cfg.DatabasePath, Logger: logger, BlobPlugin: cfg.BlobPlugin, MetadataPlugin: cfg.MetadataPlugin, MaxConnections: cfg.DatabaseWorkers, } newDB, err := database.New(dbConfig) if err != nil { return nil, false, err } return newDB, true, nil // caller must Close }
244-323:copyBlocksandcopyBlocksRawshare ~70% structural duplication.The immutable DB open, tip read, iterator setup, batch loop, progress logging, and return pattern are near-identical. Consider extracting the common scaffolding into a generic helper that accepts a block-processing callback, differentiating only on how each block is decoded and added to the chain.
Also applies to: 330-421
159-181: 30-minute timeout may be insufficient for very large chains.For Cardano mainnet snapshots containing millions of blocks, the ledger processing pipeline could take longer than 30 minutes to catch up. Consider making this timeout configurable, or at a minimum logging periodic progress within the wait loop.
ledgerstate/telescope.go (1)
156-172: Redundantlen(summary) >= 1check.Line 156 already returns an error when
len(summary) == 0, making thelen(summary) >= 1guard at line 163 always true.ledgerstate/snapshot.go (1)
200-210:ParseSnapshotreads the entire file into memory.
os.ReadFileloads the complete snapshot into a single[]byte. Cardano mainnet ledger snapshots can be several GB, which could cause OOM on memory-constrained systems. For the initial implementation this may be acceptable, but consider noting this as a known limitation or using memory-mapped I/O for production use.ledgerstate/mempack_debug_test.go (1)
10-15: Debug test with hardcoded filesystem path — consider gating or documenting.This test reads from a hardcoded
/tmp/dingo-mithril-download/...path. While it correctly skips when absent, this path is machine-specific. Consider adding a build tag (e.g.,//go:build debug) or an env-var gate (similar toDINGO_INTEGRATION_TESTinimport_test.go) to prevent it from appearing in normalgo testoutput.mithril/extract.go (1)
130-155: Considerdefer outFile.Close()for safer resource handling.
outFile.Close()is called inline at Line 148, which means a panic duringio.Copy(however unlikely) would leak the file descriptor. Usingdeferwith an error check is a more idiomatic Go pattern:Suggested approach
outFile, err := os.OpenFile( target, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o640, ) if err != nil { return "", fmt.Errorf( "creating file %s: %w", target, err, ) } // Limit copy to prevent decompression bombs - _, copyErr := io.Copy( + _, err = io.Copy( outFile, io.LimitReader(tr, header.Size), ) - outFile.Close() - if copyErr != nil { + closeErr := outFile.Close() + if err != nil { return "", fmt.Errorf( "extracting file %s: %w", target, - copyErr, + err, + ) + } + if closeErr != nil { + return "", fmt.Errorf( + "closing file %s: %w", + target, + closeErr, ) }This also checks the
Close()error, which can indicate a failed flush to disk. As per coding guidelines, errors should be wrapped with%w.README.md (1)
170-188: Consider linking to Mithril endpoint documentation for future reference.The aggregator URLs in the README are current and match official Mithril documentation; however, linking to https://mithril.network/doc/manual/develop/references/ would help readers find the latest endpoints if they change in the future and reduce maintenance burden.
ledgerstate/mempack_test.go (1)
203-234: Hardcoded absolute path makes this test non-portable.The path
/tmp/dingo-mithril-download/ancillary/ledger/104173430/tables/tvaris machine-specific and snapshot-version-specific. Consider sourcing the path from an environment variable (e.g.,DINGO_TVAR_PATH) so CI and other developers can run the test without guessing the expected location.♻️ Suggested improvement
func TestParseTvarFileIfAvailable(t *testing.T) { - tvarPath := "/tmp/dingo-mithril-download/ancillary/" + - "ledger/104173430/tables/tvar" + tvarPath := os.Getenv("DINGO_TVAR_PATH") + if tvarPath == "" { + tvarPath = "/tmp/dingo-mithril-download/ancillary/" + + "ledger/104173430/tables/tvar" + } if _, err := os.Stat(tvarPath); os.IsNotExist(err) { t.Skip("tvar file not available, skipping") }mithril/client.go (1)
198-239: Response body leak if caller ignores the returnedReadCloser.
doGetreturns anio.ReadCloserthe caller must close. All current call sites usedefer body.Close()— good. However, consider documenting this contract more explicitly or returning the decoded result directly for simpler APIs.Also,
io.LimitReadersilently truncates atmaxResponseByteswithout error — if a response exceeds 10 MiB, the JSON decoder will see an unexpected EOF rather than a descriptive error. This is an acceptable trade-off for safety, but worth noting.ledgerstate/verify_test.go (1)
34-62: Consider usingt.Cleanupfor server teardown (consistency with other test files).The
mithril/client_test.gousest.Cleanup(server.Close)via thenewTestServerhelper, while these tests usedefer server.Close(). Both work, butt.Cleanupis preferred as it runs even if at.Fatal/requirefails before the defer is registered.ledgerstate/import.go (1)
502-534: All accounts imported in a single transaction — consider batching for large imports.
importAccountsopens one transaction and inserts all accounts sequentially. For mainnet snapshots with ~1.3M+ stake accounts, this creates a very large transaction that may cause memory pressure and lock contention, especially on SQLite. The UTxO import path already uses per-batch transactions — consider a similar pattern here.The same concern applies to
importPools(lines 549–631) andimportDReps(lines 646–685), though their cardinalities are typically smaller.mithril/bootstrap.go (1)
183-201: Only the first download location is tried.
snapshot.Locations[0]is used without fallback to other locations if the download fails. For robustness, consider iterating throughsnapshot.Locationson transient failures — though this can be deferred.ledgerstate/utxo.go (2)
62-66: Try 4 appears to be dead code.If Try 1 (line 44) succeeds with
len(keyBytes) >= 34, we return at line 46. If it succeeds withlen(keyBytes) < 34, then Try 4'slen(keyBytes) >= 34is false. If Try 1'scbor.Decodefails,keyBytesremains nil/empty. In all cases, the condition at line 64 cannot be true.
444-447:os.ReadFileloads the entire tvar file into memory.UTxO-HD tvar files on mainnet can be several GB. While downstream processing is streamed in batches, the full file content is held in memory. This is pragmatic for now but consider memory-mapped I/O (
mmap) or incremental reads for mainnet-scale files if memory becomes a constraint.mithril/types.go (1)
31-65:SnapshotListItemandSnapshothave identical fields.These two types share all 11 fields verbatim. Consider embedding a common base type to reduce duplication, unless you expect the Mithril API to diverge them in the future.
| // Script is MemPack (Script era) - length-prefixed for now | ||
| script, err := r.readLengthPrefixedBytes() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("reading Script: %w", err) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
cardano-ledger mempack Script era encoding babbage conway plutus
💡 Result:
- In the Haskell ledger,
Script era(for Alonzo-family eras) is represented asAlonzoScript era = NativeScript | PlutusScript, and there areMemPackinstances forAlonzoScriptwheneverPlutusScript eraandNativeScript erahaveMemPackinstances. (cardano-ledger.cardano.intersectmbo.org) PlutusScripthas era-specificMemPackinstances (notably forBabbageEraandConwayEra). The docs also note that theMemPack (PlutusScript AlonzoEra)instance uses a “zero tag” for forward compatibility with future Plutus script variants, but that it “currently … is not used at all”, because reference scripts were introduced in Babbage, and (for now)MemPackis only used forTxOuts. (cardano-ledger.cardano.intersectmbo.org)- Conway extends the set of Plutus script purposes beyond the earlier spending/minting/certifying/rewarding purposes by adding voting and proposing purposes (reflected in
ConwayPlutusPurposeand theVotingPurpose/ProposingPurposepatterns). (cardano-ledger.cardano.intersectmbo.org) - Plutus “ledger language versions” are tied to eras and available transaction features: Plutus V1 (introduced in Alonzo) can’t be used with Babbage-only features like inline datums; Plutus V2 (introduced in Babbage) can’t be used with Conway-introduced features like DRep registration. (plutus.cardano.intersectmbo.org)
- At the encoding level, Conway also introduced tagged encoding for sets (a CBOR representation change vs legacy array encoding), and libraries expose “use tag” toggles for Conway-era compatibility (example shown for Plutus script sets). (cardano-c.readthedocs.io)
Citations:
- 1: https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-conway/Cardano-Ledger-Conway-Scripts.html?utm_source=openai
- 2: https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-conway/Cardano-Ledger-Conway-Scripts.html?utm_source=openai
- 3: https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-conway/Cardano-Ledger-Conway-Scripts.html?utm_source=openai
- 4: https://plutus.cardano.intersectmbo.org/docs/working-with-scripts/ledger-language-version?utm_source=openai
- 5: https://cardano-c.readthedocs.io/en/latest/api/witness_set/plutus_v1_script_set.html?utm_source=openai
🌐 Web query:
cardano-ledger AlonzoScript MemPack encoding discriminator tag NativeScript PlutusScript
💡 Result:
In cardano-ledger’s Alonzo-era MemPack encoding, the “discriminator” is a Tag written before the payload:
-
AlonzoScript era:NativeScript script⇒packTagM Tag 0thenpackM scriptPlutusScript script⇒packTagM Tag 1thenpackM script
(and decoding dispatches onTag 0vsTag 1accordingly). [1]
-
PlutusScript AlonzoEra(which, in Alonzo, only hasAlonzoPlutusV1):AlonzoPlutusV1 script⇒packTagM Tag 0thenpackM script
This explicitTag 0is kept for forward compatibility with future Plutus script variants. [2]
Sources
[1] Cardano.Ledger.Alonzo.Scripts (source view), MemPack (AlonzoScript era) packM/unpackM implementation (tags 0/1). (cardano-ledger.cardano.intersectmbo.org)
[2] Cardano.Ledger.Alonzo.Scripts (source view), MemPack (PlutusScript AlonzoEra) packM (tag 0) + comment on forward compatibility. (cardano-ledger.cardano.intersectmbo.org)
Citations:
- 1: https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-alonzo/src/Cardano.Ledger.Alonzo.Scripts.html
- 2: https://cardano-ledger.cardano.intersectmbo.org/cardano-ledger-alonzo/src/Cardano.Ledger.Alonzo.Scripts.html
🏁 Script executed:
cd /tmp && git ls-files 'ledgerstate/mempack.go' | head -5Repository: blinklabs-io/dingo
Length of output: 130
🏁 Script executed:
cat -n ledgerstate/mempack.go | sed -n '430,460p'Repository: blinklabs-io/dingo
Length of output: 1128
🏁 Script executed:
# Check for any tag reading around the Script decoding area
rg -A 10 -B 10 "readLengthPrefixedBytes" ledgerstate/mempack.go | head -50Repository: blinklabs-io/dingo
Length of output: 1223
Script decoding must read tag discriminator for AlonzoScript union type.
Cardano's MemPack encoding for AlonzoScript uses tag discriminators:
NativeScript→ Tag 0 + packed scriptPlutusScript→ Tag 1 + packed script
The current code reads only length-prefixed bytes without consuming the tag, which will silently misinterpret the script type and misalign the stream. The "length-prefixed for now" comment indicates this is incomplete. Add tag-based dispatch before reading the script payload.
🤖 Prompt for AI Agents
In `@ledgerstate/mempack.go` around lines 443 - 446, The script decoding currently
calls r.readLengthPrefixedBytes() directly and ignores the AlonzoScript tag
discriminator; change this to first read the discriminator byte/token (e.g.,
using r.readByte or r.readVarUint as appropriate) and switch on its value (0 =>
NativeScript, 1 => PlutusScript) before reading the actual packed script payload
(then call r.readLengthPrefixedBytes() or the appropriate reader for the inner
script); ensure you return an error for any unknown tag and update the variable
currently named script to hold the correctly-dispatched payload for the
AlonzoScript union (reference the existing symbols r.readLengthPrefixedBytes(),
the local variable script, and the AlonzoScript/NativeScript/PlutusScript union
types).
1ce3e2f to
49c10bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/node/load.go (1)
293-305:⚠️ Potential issue | 🟡 MinorSeveral raw errors returned without wrapping in
copyBlocks.Lines 297 and 305 return errors from
iter.Next()andgledger.NewBlockFromCbor()without any context wrapping. This makes it harder to trace where the failure occurred.Proposed fix
next, err := iter.Next() if err != nil { - return blocksCopied, immutableTip.Slot, err + return blocksCopied, immutableTip.Slot, fmt.Errorf( + "reading next block: %w", err, + ) }tmpBlock, err := gledger.NewBlockFromCbor(next.Type, next.Cbor) if err != nil { - return blocksCopied, immutableTip.Slot, err + return blocksCopied, immutableTip.Slot, fmt.Errorf( + "decoding block CBOR: %w", err, + ) }As per coding guidelines, "Wrap errors with context using fmt.Errorf with %w verb".
🤖 Fix all issues with AI agents
In `@database/plugin/metadata/sqlite/import.go`:
- Around line 141-174: ImportDrep can leave drep.ID as 0 on SQLite upsert
conflicts; add the same post-upsert ID re-fetch used in Postgres/MySQL. After
the db.Create(drep) upsert in ImportDrep, if drep.ID == 0 (or RowsAffected ==
0), query the DB for the existing DRep by its unique credential and populate
drep.ID (e.g., use db.First(&drep, "credential = ?", drep.Credential) or
equivalent) before creating the registration; reference the ImportDrep function
and the drep.ID field to locate where to insert this re-fetch.
In `@internal/node/load.go`:
- Around line 377-403: In copyBlocksRaw, the calls iter.Next(),
extractHeaderCbor(next.Cbor), and gledger.NewBlockHeaderFromCbor(...) return raw
errors that should be wrapped with context; update the error returns in the loop
so each returns fmt.Errorf(... %w, err) with a clear message (e.g., "iter.Next
failed", "extractHeaderCbor failed for slot X", "NewBlockHeaderFromCbor failed
for type Y") while preserving the existing return values (blocksCopied,
immutableTip.Slot, wrappedErr) and reference the symbols iter.Next,
extractHeaderCbor, and gledger.NewBlockHeaderFromCbor to locate the sites to
change.
- Around line 74-89: The code returns raw errors from database.New in the Load
function (and similarly in LoadBlobsWithDB), so wrap those errors with
contextual messages using fmt.Errorf and the %w verb; locate the calls to
database.New (e.g., the db, err = database.New(...) in Load and the analogous
call in LoadBlobsWithDB) and change the error handling to return
fmt.Errorf("creating database: %w", err) (or a similarly descriptive message)
instead of returning err directly, preserving the existing defer db.Close()
behavior.
In `@ledgerstate/certstate.go`:
- Around line 219-239: The code currently silently skips unparseable entries in
parseCertPoolParamsMap and parseCredentialMap; change these handlers to record
failures instead of dropping them silently by (1) logging each decode/parse
error with contextual details (e.g., include entry.KeyRaw or poolKeyHash and the
error) using the module's logger (referencing functions parseCertPoolParamsMap
and parsePoolParams for pool entries and parseCredentialMap for credentials) and
(2) optionally surface a skipped-count to callers by updating the function
signatures to return an additional skippedCount (or an error) so callers can
detect data loss; implement both logging at the continue points and add/return a
skipped counter where these continues occur.
In `@ledgerstate/import.go`:
- Around line 1264-1271: isEpochDuplicate only matches SQLite's error string and
misses MySQL/Postgres duplicates; update it to detect unique-constraint errors
portably by: check errors.Is(err, gorm.ErrDuplicatedKey) if your GORM version
exposes that, and additionally use errors.As to detect driver error types (e.g.,
*mysql.MySQLError with Number 1062, *pq.Error with Code "23505") and/or fallback
to checking MySQL/Postgres duplicate-key substrings ("Duplicate entry",
"duplicate key value violates unique constraint") so both isEpochDuplicate and
SetEpoch (where raw db.Create is used) correctly treat duplicates across
SQLite/MySQL/Postgres. Reference symbols: isEpochDuplicate and SetEpoch.
In `@ledgerstate/snapshot.go`:
- Around line 91-103: The sort comparator for legacyFiles incorrectly passes
filenames with suffixes into strconv.ParseUint, causing parse failures; add a
helper function (e.g., stripLedgerSuffix) that trims known suffixes like
".lstate" and "_snapshot" and use it inside the slices.SortFunc comparator
before calling strconv.ParseUint so na/nb parse the numeric slot correctly, then
return filepath.Join(ledgerDir, legacyFiles[0]) as before; remember to import
strings if not already present.
In `@mithril/bootstrap.go`:
- Around line 468-482: The current leaf certificate check in the block guarding
isLeaf and snapshotDigest uses
cert.ProtocolMessage.MessageParts["snapshot_digest"] (certDigest) but silently
accepts a missing/empty certDigest; change the logic in the isLeaf &&
snapshotDigest != "" branch to treat an empty certDigest as a verification
failure by returning an error that the leaf certificate is missing
snapshot_digest, and otherwise still compare certDigest to snapshotDigest and
return the existing mismatch error if they differ (refer to
cert.ProtocolMessage.MessageParts["snapshot_digest"], certDigest, isLeaf, and
snapshotDigest to locate and modify the check).
- Around line 156-167: Bootstrap currently creates a temp dir when
cfg.DownloadDir is empty using os.MkdirTemp (downloadDir) but can leak it if
later steps fail; update Bootstrap so the created temp dir is cleaned up on
error by implementing a named return with a success boolean (or similar guard)
and a deferred cleanup that removes downloadDir unless success is set true, or
alternatively return a partial BootstrapResult that carries Cleanup() so caller
can remove it; reference the Bootstrap function, the downloadDir variable, and
BootstrapResult/Cleanup() to locate and modify the allocation path around
os.MkdirTemp.
In `@mithril/client.go`:
- Around line 67-82: NewClient currently stores aggregatorURL verbatim which
allows a trailing slash to produce double slashes when building endpoints (see
uses in request URL concatenation). In NewClient, sanitize aggregatorURL by
removing any trailing slash (e.g., use strings.TrimSuffix or strings.TrimRight
with "/") before saving to the Client struct so subsequent concatenations like
aggregatorURL + "/artifact/..." do not yield double slashes; preserve a lone
scheme/root correctly (i.e., only strip a trailing "/" not nullify the URL).
In `@mithril/download.go`:
- Around line 199-206: The 416 (http.StatusRequestedRangeNotSatisfiable) branch
currently returns immediately, bypassing the ExpectedSize verification and
accepting potentially corrupt or wrong-size files; update the case in the
download function to perform the same size check used later (the
ExpectedSize/file-size verification block) before returning: stat destPath,
compare its size to ExpectedSize (or reuse the existing verification logic
already present later in this function), and if sizes mismatch log an error via
cfg.Logger and return an error instead of returning destPath,nil; only return
destPath,nil when the on-disk file size matches ExpectedSize.
In `@mithril/extract.go`:
- Around line 86-108: The HasPrefix check incorrectly rejects a tar entry like
"." because target equals destDir (no trailing separator); update the validation
around target (the variable built from filepath.Join(destDir,
filepath.FromSlash(name))) to allow either an exact match to
filepath.Clean(destDir) OR a HasPrefix starting with
filepath.Clean(destDir)+string(filepath.Separator). Keep the existing use of
path.Clean(header.Name) and validRelPath(name) but replace the single
strings.HasPrefix(...) conditional with a check that accepts target ==
filepath.Clean(destDir) || strings.HasPrefix(target,
filepath.Clean(destDir)+string(filepath.Separator)) to avoid false "path escapes
destination" errors.
In `@mithril/types.go`:
- Around line 162-172: The IsChainingToItself method currently returns true when
Certificate.Hash == Certificate.PreviousHash even if both are empty; update
IsChainingToItself (on type Certificate) to first guard against empty Hash or
PreviousHash and return false if either is empty, then compare equality—this
prevents malformed certificates with empty fields from being treated as
root/genesis in VerifyCertificateChain.
In `@README.md`:
- Around line 170-172: The README sets CARDANO_NETWORK but the mithril-client
commands shown do not use it; either remove the "export CARDANO_NETWORK=preview"
line or add a clarifying comment that CARDANO_NETWORK is required only for the
subsequent dingo command (not mithril-client), and explicitly mention which
variables are consumed by mithril-client (AGGREGATOR_ENDPOINT and
GENESIS_VERIFICATION_KEY) so readers know which env vars to set; update the
README section near the existing AGGREGATOR_ENDPOINT line to reflect this change
and reference the CARDANO_NETWORK and dingo usage if you keep the export.
🧹 Nitpick comments (21)
chain/chain.go (1)
258-333: Consider extracting shared logic withAddBlockto reduce duplication.
AddRawBlockduplicates nearly all ofAddBlock's body (lock ordering, reconcile, header/tip checks, block persistence, tip update, waiter notification, event emission). A shared private helper (e.g.,addBlockInternal) accepting the already-constructedmodels.Blockand pre-computed point would let both public methods delegate to it, keeping the validation and state-management logic in one place.ledger/forging/builder_test.go (1)
566-567: Remove unused constructor call on lines 566–567.The
inputvariable is created withNewConstructorEncoderbut never used—it's immediately assigned to_on the next line. The transaction body is built manually viabodyMapbelow. This dead code can be safely removed.Remove unused code
- input := cbor.NewConstructorEncoder(0, cbor.IndefLengthList{}) - _ = input // not used directly; we encode manually below - bodyMap := map[uint]any{internal/config/config.go (2)
58-60: Minor grammar nit in comment.There's a missing period/comma between "plugins" and "This".
Proposed fix
-// ErrPluginListRequested is returned when the user requests to list available -// plugins This is not an error condition but a successful operation that -// displays plugin information +// ErrPluginListRequested is returned when the user requests to list +// available plugins. This is not an error condition but a successful +// operation that displays plugin information.
402-472: Duplicated blob/metadata config extraction logic.The blob config processing block (lines 403-436) and the metadata config processing block (lines 438-472) are nearly identical in structure — both extract a
pluginkey, build a config map withmap[any]any→map[string]anyconversion, and merge. Consider extracting a helper likeextractPluginConfig(raw map[string]any) (string, map[string]map[string]any, error)to reduce duplication.mithril/client.go (1)
200-203: Parameterurlshadows the importednet/urlpackage.The parameter name
urlon line 203 shadows thenet/urlimport used on line 133 (url.PathEscape). While it doesn't cause a bug here (theurlimport is only used in caller methods, not indoGet), it reduces clarity and could cause issues ifdoGetever needsurl.Parseor similar.Proposed fix
func (c *Client) doGet( ctx context.Context, - url string, + reqURL string, ) (io.ReadCloser, error) { req, err := http.NewRequestWithContext( ctx, http.MethodGet, - url, + reqURL, nil, )internal/node/load.go (1)
195-257: Significant duplication betweenLoadWithDBandLoadBlobsWithDBfor DB init and bulk-load setup.The database initialization (lines 201-217) and bulk-load optimizer setup (lines 219-235) in
LoadBlobsWithDBare nearly identical toLoadWithDB(lines 74-109). Consider extracting a shared helper likeopenAndOptimizeDB(cfg, logger, db) (*database.Database, func(), error)that returns the db and a cleanup function.ledgerstate/certstate.go (1)
86-172: Heuristic-based Conway CertState parsing — document assumptions and add safeguard.The Conway parser identifies PState/DState/VState by CBOR type and size heuristics (largest array → PState, largest map → DState, remaining smaller map → VState). While pragmatic for the observed node 10.x encoding, this is fragile against future era changes.
Consider adding a log warning (or returning a diagnostic) when the element count is neither 3 nor 6, since the heuristic was designed around those two layouts. This would help surface parsing issues early rather than silently producing incomplete data.
mithril/download_test.go (1)
47-50:requireinOnProgresscallback is safe here but worth a note.The
OnProgresscallback at line 49 usesrequire.GreaterOrEqual. This is safe becauseDownloadSnapshotcalls the callback synchronously fromio.Copyon the test goroutine. If the implementation ever invokes callbacks from a different goroutine, this would need to switch toassert.ledgerstate/verify.go (1)
83-130: Consider validating the checksum file contains valid hex.If the
.checksumfile contains non-hex garbage,strings.EqualFoldwill always report a mismatch without indicating the checksum file itself is malformed. Decoding the expected hex first would give a more actionable error message.Suggested improvement
expectedHex := strings.TrimSpace(string(checksumData)) if expectedHex == "" { return nil // Empty checksum file, skip verification } + if _, err := hex.DecodeString(expectedHex); err != nil { + return fmt.Errorf( + "invalid hex in checksum file %s: %w", + checksumPath, err, + ) + }(Would require adding
"encoding/hex"to imports.)ledgerstate/mempack_debug_test.go (1)
10-15: Debug test depends on hardcoded/tmppath — consider a build tag.This test always runs in CI (where it will be skipped), adding noise to test output. A
//go:build manualor similar build tag would keep it out ofgo test ./...runs while remaining easily invocable during development.mithril/certverify_test.go (1)
280-376: Consider adding asnapshot_digesttocert_leafto exercise the full digest verification path.The
cert_leafin this test has noProtocolMessage.MessageParts["snapshot_digest"], soVerifyCertificateChainsilently skips the digest binding check (perbootstrap.goline ~475:if certDigest != ""guard). Adding the digest would validate that the end-to-end flow correctly binds the snapshot to its certificate.certs := map[string]Certificate{ "cert_leaf": { Hash: "cert_leaf", PreviousHash: "cert_genesis", MultiSignature: "sig", + ProtocolMessage: ProtocolMessage{ + MessageParts: map[string]string{ + "snapshot_digest": "abc123def456789012345678", + }, + }, },ledgerstate/mempack_test.go (1)
203-234: Same/tmphardcoded path concern as the debug test — consider a shared constant or build tag.This test and
TestDebugFirstTwoEntriesinmempack_debug_test.goboth reference the same hardcoded path. Extracting it to a package-level constant would ensure they stay in sync and make it easier to update.cmd/dingo/mithril.go (1)
346-366:loadResultdereference on Line 364 is safe only ifLoadBlobsWithDBnever returns(nil, nil).After
g.Wait()succeeds,loadResultis expected to be non-nil, but there's no guard. IfLoadBlobsWithDBever returned a nil result without an error,loadResult.BlocksCopiedwould panic.Defensive guard
if len(recentBlocks) > 0 { chainTip := recentBlocks[0] // ... + blocksCopied := int64(0) + if loadResult != nil { + blocksCopied = loadResult.BlocksCopied + } logger.Info( "metadata tip updated to chain tip", "component", "mithril", "slot", chainTip.Slot, - "blocks_loaded", loadResult.BlocksCopied, + "blocks_loaded", blocksCopied, )ledgerstate/snapshots.go (1)
339-343: Silent skip onhex.DecodeStringfailure for a self-encoded key.Since
poolHexwas encoded viahex.EncodeToStringat line 319,hex.DecodeStringat line 340 should never fail here. However, silently continuing discards a pool's aggregated stake with no warning, which is inconsistent with the warning pattern used in the parse functions above. Consider at least logging or counting skips for parity.ledgerstate/import.go (2)
1100-1111: RedundantepochSpan > 0check; consider guardingslotSpansubtraction.After the
endBound.Epoch <= startBound.Epochguard on line 1101,epochSpanat line 1104 is guaranteed> 0, making the check on line 1105 redundant. More importantly,endBound.Slot - startBound.Sloton line 1106 could underflow if the slot bounds are inconsistent (unlikely but unguarded), whereas the epoch bounds are guarded.Proposed simplification
// Fallback: compute from bounds if we have both start and end. if endBound.Epoch <= startBound.Epoch { return 0, 0 } + if endBound.Slot < startBound.Slot { + return 0, 0 + } epochSpan := endBound.Epoch - startBound.Epoch - if epochSpan > 0 { - slotSpan := endBound.Slot - startBound.Slot - return 0, uint(slotSpan / epochSpan) - } - - return 0, 0 + slotSpan := endBound.Slot - startBound.Slot + return 0, uint(slotSpan / epochSpan)
490-534: Single transaction for all accounts may be long-lived on mainnet.On mainnet there can be hundreds of thousands of staking accounts.
importAccountswraps all inserts in a single transaction (line 503), which could hold a write lock for a long time depending on the backend. The same applies toimportPoolsandimportDReps, though those have fewer entries.This is acceptable for a one-time bootstrap import, but worth noting if the import path is ever used more generally.
ledgerstate/snapshot.go (1)
200-210:ParseSnapshotreads entire file into memory.
os.ReadFileloads the full ledger state file at once. For mainnet snapshots this can be several GB. This is acceptable for a bootstrap operation, but worth noting for memory-constrained environments. A streaming approach could be considered in a future iteration if needed.ledgerstate/utxo.go (1)
319-341:extractAddressKeysslices reference the originaladdrbacking array.The slices
addr[1:29]andaddr[29:57]share memory with the input buffer. If the caller later overwrites or reuses that buffer (e.g., in a streaming decode loop), thePaymentKey/StakingKeyvalues would be silently corrupted. This is safe if the decodedaddris freshly allocated by the CBOR decoder per entry (which appears to be the case for both MemPack and CBOR paths), but worth noting.mithril/bootstrap.go (1)
183-201: Only the first download location is tried; no fallback to alternate mirrors.
snapshot.Locations[0]is always used. If that CDN endpoint is down, the bootstrap fails even though other locations may be available. Same pattern at line 309 for ancillary data.This is acceptable for an initial implementation, but consider iterating over
Locationson transient failures for improved resilience.ledgerstate/govstate.go (1)
332-347: Defensive check onfieldslength is good, but consider validatinggovIdfield sizes.The function validates array lengths throughout. One observation:
prop.TxHash(line 365) is expected to be 32 bytes per the struct comment, but the decoded value is not length-checked. A malformed CBOR payload could produce a hash of unexpected length that downstream consumers may not handle.This is low risk since CBOR data comes from a verified Mithril snapshot, but worth noting if this parser is reused for untrusted inputs.
mithril/types.go (1)
31-70:SnapshotListItemandSnapshotare structurally identical.Both structs have exactly the same fields and JSON tags. If they're not expected to diverge, consider embedding a shared base type or using a single type with a type alias. If they will diverge (e.g., the detail endpoint adds fields), this is fine as-is.
wolf31o2
force-pushed
the
feat/mithril-bootstrap
branch
from
49c10bb to
d1e3f64
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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)
internal/node/load.go (1)
66-68:⚠️ Potential issue | 🟡 MinorUnwrapped error from
LoadCardanoNodeConfigWithFallback.Line 67 returns the raw error without context wrapping.
Proposed fix
if err != nil { - return err + return fmt.Errorf("loading cardano node config: %w", err) }As per coding guidelines, "Wrap errors with context using fmt.Errorf with %w verb".
🤖 Fix all issues with AI agents
In `@database/plugin/metadata/mysql/import.go`:
- Around line 127-135: The OnConflict clause around the Create(reg) call is
ineffective because PoolRegistration and RegistrationDrep lack a composite
unique key; fix by either (A) make the conflict meaningful: add a composite
unique constraint to the models (e.g., on PoolRegistration add a unique index
combining PoolID + AddedSlot + CertificateID/CertIndex and on RegistrationDrep
add a unique index combining DrepID + AddedSlot + CertificateID/CertIndex), add
a DB migration to create those unique indexes, and keep the
db.Clauses(clause.OnConflict{DoNothing: true}).Create(reg) usage so duplicate
inserts are prevented at the DB level; or (B) remove the misleading OnConflict
clause from the Create(reg) call and update comments/docs to state idempotency
is only provided by ImportCheckpoint. Locate the Create(reg) call and the
PoolRegistration/RegistrationDrep model definitions to apply the model
tags/migration or to remove the clause and documentation accordingly.
In `@ledgerstate/utxo.go`:
- Around line 433-442: ParseUTxOsFromFile currently uses os.ReadFile (in
ParseUTxOsFromFile) which loads the whole tvar into memory; change it to a
streaming approach (or memory-map) similar to ParseUTxOsStreaming: open the file
with os.Open, create an io.Reader (or mmap.ReaderAt) and feed that into the CBOR
streaming/decoder logic used by ParseUTxOsStreaming so the top-level map and
batched entries are decoded incrementally; ensure you replace the os.ReadFile
usage, close the file when done, preserve the existing UTxOCallback batching
semantics, and propagate errors from open/decode operations.
🧹 Nitpick comments (22)
chain/chain.go (3)
258-333: Consider extracting shared logic withAddBlockto reduce duplication.
AddRawBlockduplicates nearly the entire body ofAddBlock(lock acquisition, reconcile, header check, tip check, block construction, persist, tip update, notify waiters, emit event). The only difference is how fields are extracted (fromRawBlockfields vs.ledger.Blockmethods). A shared internal helper (e.g.,addBlockInternal) accepting the already-extracted values would eliminate this duplication and ensure future changes (e.g., new validations) are applied consistently.
320-331: Per-block event emission may bottleneck bulk Mithril imports.During a Mithril bootstrap,
AddRawBlockswill callAddRawBlockfor potentially millions of blocks, each publishing aChainUpdateEvent. If any subscriber does non-trivial work per event, this becomes a hot path. Consider either batching events (one event per batch of 50) or providing an option to suppress events during bulk load and emit a single "bulk load complete" event afterward.
266-268: Errors returned without wrapping context.Per coding guidelines, errors should be wrapped with
fmt.Errorfand%w. For example:if err := c.reconcile(); err != nil { return fmt.Errorf("reconcile: %w", err) }This applies to the
reconcile()call (Line 266) andaddBlock()call (Line 300). The existingAddBlockhas the same gap, so this is not a regression — but worth addressing here since this is new code.As per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb".
Also applies to: 300-301
mithril/extract_test.go (1)
175-197: Good edge-case coverage forvalidRelPath.Consider adding a test case for
"."whichpath.Cleanproduces from entries like"./". This path currently passesvalidRelPathbut then causes a false "path escapes destination" error inExtractArchive(as noted in the existing review onextract.golines 86-110).{"current dir dot", ".", false},mithril/extract.go (1)
190-203:validRelPathshould also reject".".While the
"."path doesn't cause a runtime error inExtractArchive(it safely creates the already-existing destDir), it represents a root-directory tar entry that should arguably be skipped rather than processed. Rejecting it invalidRelPathor adding aname == "."skip before the switch would be cleaner.♻️ Suggested approach in ExtractArchive
name := path.Clean(header.Name) + if name == "." { + continue // skip root directory entry + } if !validRelPath(name) {mithril/types.go (1)
31-65:SnapshotListItemandSnapshothave identical fields.These two structs share the exact same field set. If they're not expected to diverge, consider using a single type or embedding a shared base to reduce duplication. If they're kept separate intentionally (e.g., API may evolve them independently), a brief comment explaining why would help future readers.
ledgerstate/cbor_decode.go (1)
28-38: Consider wrapping the error indecodeRawArrayfor context.Per coding guidelines, errors should be wrapped with context using
fmt.Errorfwith%w. The rawcbor.Decodeerror on line 31 loses the context that this was specifically an array decode attempt.♻️ Suggested fix
var items []cbor.RawMessage if _, err := cbor.Decode(data, &items); err != nil { - return nil, err + return nil, fmt.Errorf("decoding CBOR array: %w", err) }As per coding guidelines,
**/*.go: Wrap errors with context using fmt.Errorf with %w verb.ledgerstate/certstate.go (2)
97-194: Heuristic-based Conway element detection is fragile but pragmatically acceptable.The "largest array = PState, largest map = DState" heuristic works for the known Conway 6-element structure, but could misidentify elements if future eras change relative sizes (e.g., a very large VState DRep map). The approach is well-documented in comments. Consider adding a log/warning when using this path so operators know the heuristic was applied.
460-487:parsePoolDepositssilently drops unparseable entries, unlike other parsers.Other map parsers (
parseCertPoolParamsMap,parseCredentialMap,parseStakeMap, etc.) track askippedcounter and return a warning.parsePoolDepositsuses barecontinueat lines 473 and 480 with no visibility into data loss. Consider aligning with the warning pattern used elsewhere for consistency.♻️ Suggested fix
func parsePoolDeposits(data []byte) map[string]uint64 { entries, err := decodeMapEntries(data) if err != nil { return nil } deposits := make(map[string]uint64, len(entries)) + var skipped int for _, entry := range entries { var keyHash []byte if _, err := cbor.Decode( entry.KeyRaw, &keyHash, ); err != nil { + skipped++ continue } var amount uint64 if _, err := cbor.Decode( entry.ValueRaw, &amount, ); err != nil { + skipped++ continue } deposits[hex.EncodeToString(keyHash)] = amount } return deposits }Note: The function currently returns only a map with no error. To fully align, the signature would need to return an error as well, but that's a larger change. At minimum, tracking the count internally could be useful for debugging.
internal/node/load.go (1)
150-181: Consider wrapping the error fromcopyBlocksfor additional context in the caller.Line 155 returns
errfromcopyBlockswithout additional context fromLoadWithDB. WhilecopyBlocksdoes wrap its own errors, addingLoadWithDBcontext would help with traceability in log output.Proposed fix
blocksCopied, immutableTipSlot, err := copyBlocks( logger, immutableDir, c, ) if err != nil { - return err + return fmt.Errorf("loading blocks: %w", err) }As per coding guidelines, "Wrap errors with context using fmt.Errorf with %w verb".
ledgerstate/telescope.go (1)
32-58: Consider extracting the shared format-detection logic to reduce duplication.Both
extractAllEraBoundsandnavigateTelescoperepeat the same pattern: decode the raw array, checklen >= 2, try to decode the first item asuint64to distinguish flat vs. nested format. This could be factored into a small helper (e.g.,detectTelescopeFormat) to keep the two entry points DRY.Not blocking since both implementations are correct and well-bounded.
Also applies to: 247-276
ledgerstate/snapshot.go (1)
213-223:ParseSnapshotreads the entire file into memory — acceptable for bootstrap but worth noting.
os.ReadFileloads the full ledger state file at once. Ledger state files can range from tens of MB to a few hundred MB. For a one-time bootstrap operation this is acceptable, but be aware of memory pressure if this is ever called in a memory-constrained environment.ledgerstate/govstate.go (1)
437-452: Anchor parsing is intentionally best-effort — consider adding a brief comment.The anchor
[url, hash]decoding silently swallows errors (lines 439, 441-445, 447-451), while other fields inparseGovActionStateare strict. This is reasonable since anchor is non-critical metadata, but a short inline comment explaining why would help future readers.ledgerstate/import_test.go (1)
180-190: Consider releasing the transaction before the finalfmt.Fprintf.The
txn.Release()is deferred (line 182), but thefmt.Fprintfblock at lines 197-208 accessesstate.Tipfields outside of any transaction context. This is fine sincestateis a local variable, not database state — but it's worth noting thattxnis held open longer than necessary across the final logging block.mithril/certverify_test.go (1)
280-383: Good end-to-end bootstrap verification test.This test thoroughly validates the full bootstrap flow with certificate verification enabled, including snapshot listing, certificate fetching, and archive download.
One minor inconsistency: this file uses
defer server.Close()whilemithril/client_test.gousest.Cleanup(server.Close). Consider aligning on one pattern within themithrilpackage for consistency.ledgerstate/mempack_test.go (1)
203-233: Hardcoded absolute path will rot — consider using an env var ort.TempDir()fixture.The path
/tmp/dingo-mithril-download/ancillary/ledger/104173430/tables/tvarcontains a hardcoded epoch/slot number that will become stale. While theos.Stat+t.Skipmakes this safe, it's effectively dead code for CI and other developers. Consider gating on an env var (e.g.,DINGO_TVAR_PATH) or removing this test in favor of the integration test inimport_test.gothat already exercises tvar parsing with a live snapshot.♻️ Suggested approach
func TestParseTvarFileIfAvailable(t *testing.T) { - tvarPath := "/tmp/dingo-mithril-download/ancillary/" + - "ledger/104173430/tables/tvar" - if _, err := os.Stat(tvarPath); os.IsNotExist(err) { - t.Skip("tvar file not available, skipping") + tvarPath := os.Getenv("DINGO_TVAR_PATH") + if tvarPath == "" { + t.Skip("set DINGO_TVAR_PATH to run this test") }ledgerstate/import.go (1)
498-543: Single transaction for all accounts may cause issues with large datasets.
importAccountsopens one transaction (line 512) and iterates through every account inside it. On mainnet, there can be hundreds of thousands of stake accounts. A single long-running transaction can cause lock contention, high memory usage, and risk timeouts depending on the backend.Consider batching (similar to how
importUTxOsusesbatchCallback) — e.g., commit every N accounts and open a new transaction.The same pattern applies to
importPools(lines 558-640) andimportDReps(lines 655-694), though those datasets are smaller in practice.mithril/client.go (1)
59-83:WithHTTPClientbypasses the HTTPS-only redirect policy.When a caller uses
WithHTTPClient, the custom client replaces the default one including itshttpsOnlyRedirectcheck. This is the expected functional-option pattern, but callers should be aware that security-sensitive defaults are lost. A brief doc comment onWithHTTPClientnoting this would help.📝 Suggested doc addition
-// WithHTTPClient sets a custom *http.Client for the Mithril client. +// WithHTTPClient sets a custom *http.Client for the Mithril client. +// Note: this replaces the default client, including the HTTPS-only +// redirect policy. The caller is responsible for configuring +// appropriate security settings on the provided client. func WithHTTPClient(hc *http.Client) ClientOption {cmd/dingo/mithril.go (1)
346-370: Tip is updated unconditionally even if the blob store loaded zero blocks.If
recentBlockshas entries butblocksCopiedis 0 (load found nothing new), the tip is still overwritten. This is likely fine for bootstrap (the blocks are already there from a previous run), but the log message at line 364-369 could be misleading in that scenario.mithril/download.go (1)
150-153: Newhttp.Clientcreated per download — no connection reuse.Each
DownloadSnapshotcall creates a freshhttp.Client, preventing TCP/TLS connection reuse across multiple downloads (main + ancillary). Consider accepting an optional*http.ClientinDownloadConfigor reusing the Mithril client's HTTP client.ledgerstate/utxo.go (1)
381-389:ledger.NewBlake2b224(nil)called twice per TxOut — consider a package-level zero value.Lines 383 and 387 each allocate a new zero-hash for comparison. For millions of UTxOs, this creates unnecessary allocations.
♻️ Proposed optimization
+var zeroBlake2b224 = ledger.NewBlake2b224(nil) + func parseCborTxOut( ... ) (*ParsedUTxO, error) { ... pkh := addr.PaymentKeyHash() - if pkh != ledger.NewBlake2b224(nil) { + if pkh != zeroBlake2b224 { result.PaymentKey = pkh.Bytes() } skh := addr.StakeKeyHash() - if skh != ledger.NewBlake2b224(nil) { + if skh != zeroBlake2b224 { result.StakingKey = skh.Bytes() }mithril/bootstrap.go (1)
192-210: Only the first download location is used — no fallback on failure.
snapshot.Locations[0]is used at line 196, butLocationscan contain multiple mirrors. If the first location is unavailable or slow, the download fails without trying alternatives.Consider iterating through
snapshot.Locationson download failure:♻️ Proposed fallback approach
- } else { - var dlErr error - archivePath, dlErr = DownloadSnapshot( - ctx, DownloadConfig{ - URL: snapshot.Locations[0], - DestDir: downloadDir, - Filename: archiveFilename, - ExpectedSize: snapshot.Size, - Logger: cfg.Logger, - OnProgress: cfg.OnProgress, - }, - ) - if dlErr != nil { - return nil, fmt.Errorf( - "downloading snapshot: %w", - dlErr, - ) - } + } else { + var dlErr error + for i, loc := range snapshot.Locations { + archivePath, dlErr = DownloadSnapshot( + ctx, DownloadConfig{ + URL: loc, + DestDir: downloadDir, + Filename: archiveFilename, + ExpectedSize: snapshot.Size, + Logger: cfg.Logger, + OnProgress: cfg.OnProgress, + }, + ) + if dlErr == nil { + break + } + cfg.Logger.Warn( + "download location failed, trying next", + "component", "mithril", + "location", i, + "error", dlErr, + ) + } + if dlErr != nil { + return nil, fmt.Errorf( + "downloading snapshot (all locations failed): %w", + dlErr, + ) + } }The same applies to
downloadAncillaryat line 319.
| func ParseUTxOsFromFile( | ||
| path string, | ||
| era int, | ||
| callback UTxOCallback, | ||
| ) (int, error) { | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("reading tvar file: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
os.ReadFile loads entire tvar file into memory — mainnet files can be multi-GB.
ParseUTxOsFromFile reads the entire UTxO-HD tvar file into memory at line 438. On mainnet, these files can be several gigabytes. The streaming approach used for the map contents (batched callbacks) is negated by loading the entire file upfront.
Consider using memory-mapped I/O (mmap) or a buffered streaming approach that reads the file in chunks, similar to how ParseUTxOsStreaming processes the CBOR map incrementally.
🤖 Prompt for AI Agents
In `@ledgerstate/utxo.go` around lines 433 - 442, ParseUTxOsFromFile currently
uses os.ReadFile (in ParseUTxOsFromFile) which loads the whole tvar into memory;
change it to a streaming approach (or memory-map) similar to
ParseUTxOsStreaming: open the file with os.Open, create an io.Reader (or
mmap.ReaderAt) and feed that into the CBOR streaming/decoder logic used by
ParseUTxOsStreaming so the top-level map and batched entries are decoded
incrementally; ensure you replace the os.ReadFile usage, close the file when
done, preserve the existing UTxOCallback batching semantics, and propagate
errors from open/decode operations.
bb7a6a3 to
e7f3e12
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
13 issues found across 50 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ledgerstate/snapshot.go">
<violation number="1" location="ledgerstate/snapshot.go:273">
P2: Comment says "log but continue without bounds" but the error `boundsErr` is silently discarded — no logging actually occurs. Either add actual logging (e.g., `slog.Warn`) or update the comment to reflect that the error is intentionally ignored.</violation>
</file>
<file name="ledgerstate/cbor_decode.go">
<violation number="1" location="ledgerstate/cbor_decode.go:253">
P1: Integer overflow: `int(arg)` can wrap negative for a crafted large `uint64`, causing the bounds check `size > len(data)` to silently pass. Validate that `arg` fits in a non-negative `int` before casting, or compare in `uint64` space.</violation>
</file>
<file name="ledgerstate/certstate.go">
<violation number="1" location="ledgerstate/certstate.go:157">
P2: Conway-era DReps may be silently dropped. `parseCertStateConway` passes raw DRep map bytes to `parseVState`, but `parseVState` expects a VState *array* `[drepMap, ccHotKeys, ...]` and accesses `vs[0]` as the inner DRep map. When a flat map is passed, `decodeRawElements` will decompose it differently, so `decodeMapEntries(vs[0])` will operate on a single map value rather than the whole DRep map. The `vErr == nil && len(dreps) > 0` guard silently absorbs the failure.
Consider parsing the DRep map directly in the Conway path instead of delegating to `parseVState`:
```go
if isMap && (dIdx < 0 || len(elem) < dSize) {
// In Conway flat layout, elem IS the drep map directly
drepEntries, dErr := decodeMapEntries(elem)
if dErr == nil && len(drepEntries) > 0 {
// parse drep entries directly...
}
}
```</violation>
<violation number="2" location="ledgerstate/certstate.go:782">
P2: Credential type (key vs. script) is silently discarded. `parseCredential` parses `[type, hash]` but only returns `hash`, losing the distinction between key-based (type 0) and script-based (type 1) credentials. If downstream consumers need to reconstruct addresses or store credential types, this information is permanently lost at parse time. Consider returning a struct with both fields or encoding the type in the returned byte slice.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:1006">
P2: When `EpochLengthFunc` is unavailable for the current (last) era, `resolveEraParams` silently returns `(0, 0)` because the zero-value `endBound` always triggers the early return. This causes every epoch in the last era to have `startSlot = lastBound.Slot` and `lengthInSlots = 0`, silently corrupting epoch data critical for the slot clock. Consider returning an error from `resolveEraParams` when it cannot determine valid parameters, or at minimum logging a warning before using the zero fallback.</violation>
<violation number="2" location="ledgerstate/import.go:1156">
P2: The docstring claims this imports "constitution, committee, proposals" but committee members (`govState.Committee`) are parsed by `ParseGovState` and then silently discarded — no import, no log. Consider either adding a log message that committee import is not yet supported, or updating the docstring to reflect what is actually imported.</violation>
</file>
<file name="internal/node/load.go">
<violation number="1" location="internal/node/load.go:254">
P2: Unwrapped error return from `copyBlocksRaw` — inconsistent with the sibling `LoadWithDB` which wraps the same pattern with `fmt.Errorf("loading blocks: %w", err)`. The project convention (per AGENTS.md) is to always wrap errors with context.</violation>
</file>
<file name="mithril/bootstrap.go">
<violation number="1" location="mithril/bootstrap.go:392">
P2: Ancillary archive file (`.tar.zst`) is never cleaned up. `downloadAncillary` downloads the archive but only returns the extraction directory path. The archive file path is not stored in `BootstrapResult`, so `Cleanup()` cannot remove it. Consider adding an `AncillaryArchivePath` field to `BootstrapResult` and including it in the cleanup paths.</violation>
</file>
<file name="ledgerstate/govstate.go">
<violation number="1" location="ledgerstate/govstate.go:163">
P2: Validate the constitution policy hash length and surface decode errors; silently ignoring non‑28‑byte values can hide malformed governance state.</violation>
<violation number="2" location="ledgerstate/govstate.go:217">
P2: Do not treat a malformed StrictMaybe committee (tag present but missing body) as SNothing; return an error for unexpected tags or missing body to avoid silently dropping committee data.</violation>
</file>
<file name="cmd/dingo/mithril.go">
<violation number="1" location="cmd/dingo/mithril.go:469">
P2: Errors from `FindLedgerStateFile` are silently swallowed. A permission-denied or I/O error would be indistinguishable from "file not found", potentially causing the ledger state import to be silently skipped. Consider logging the error at debug/warn level.</violation>
</file>
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:608">
P2: Out-of-bounds asset name offsets are silently ignored (resulting in a nil/empty asset name), while out-of-bounds policy ID offsets correctly return an error. This inconsistency could hide data corruption. Consider returning an error when name offsets are out of bounds, consistent with the policy ID bounds check.</violation>
<violation number="2" location="ledgerstate/mempack.go:628">
P2: Trimming trailing null bytes from the last asset name may corrupt names that legitimately end with `0x00`. Cardano asset names are arbitrary byte strings. Consider using the difference between the name region's start (derived from the last entry in Region D) and the total concatenated name length to determine exact boundaries, rather than stripping nulls heuristically.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return 0, err | ||
| } | ||
| // #nosec G115 | ||
| size := hLen + int(arg) |
There was a problem hiding this comment.
P1: Integer overflow: int(arg) can wrap negative for a crafted large uint64, causing the bounds check size > len(data) to silently pass. Validate that arg fits in a non-negative int before casting, or compare in uint64 space.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/cbor_decode.go, line 253:
<comment>Integer overflow: `int(arg)` can wrap negative for a crafted large `uint64`, causing the bounds check `size > len(data)` to silently pass. Validate that `arg` fits in a non-negative `int` before casting, or compare in `uint64` space.</comment>
<file context>
@@ -0,0 +1,421 @@
+ return 0, err
+ }
+ // #nosec G115
+ size := hLen + int(arg)
+ if size > len(data) {
+ return 0, fmt.Errorf(
</file context>
| c := &ParsedConstitution{} | ||
|
|
||
| var url string | ||
| if _, err := cbor.Decode( |
There was a problem hiding this comment.
P2: Validate the constitution policy hash length and surface decode errors; silently ignoring non‑28‑byte values can hide malformed governance state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/govstate.go, line 163:
<comment>Validate the constitution policy hash length and surface decode errors; silently ignoring non‑28‑byte values can hide malformed governance state.</comment>
<file context>
@@ -0,0 +1,474 @@
+ c := &ParsedConstitution{}
+
+ var url string
+ if _, err := cbor.Decode(
+ anchor[0], &url,
+ ); err != nil {
</file context>
ledgerstate/mempack.go
Outdated
| if nameStart > nameEnd { | ||
| nameStart, nameEnd = nameEnd, nameStart | ||
| } | ||
| if nameStart >= 0 && |
There was a problem hiding this comment.
P2: Out-of-bounds asset name offsets are silently ignored (resulting in a nil/empty asset name), while out-of-bounds policy ID offsets correctly return an error. This inconsistency could hide data corruption. Consider returning an error when name offsets are out of bounds, consistent with the policy ID bounds check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 608:
<comment>Out-of-bounds asset name offsets are silently ignored (resulting in a nil/empty asset name), while out-of-bounds policy ID offsets correctly return an error. This inconsistency could hide data corruption. Consider returning an error when name offsets are out of bounds, consistent with the policy ID bounds check.</comment>
<file context>
@@ -0,0 +1,792 @@
+ if nameStart > nameEnd {
+ nameStart, nameEnd = nameEnd, nameStart
+ }
+ if nameStart >= 0 &&
+ nameStart <= len(flat) &&
+ nameEnd <= len(flat) {
</file context>
e7f3e12 to
3da4464
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 50 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:629">
P2: Trimming trailing null bytes from the last asset name could silently truncate legitimate asset names that end with `0x00`. Cardano asset names are arbitrary byte sequences and may contain null bytes.
Consider computing the actual end of Region E from the known region sizes (quantities + offsets + policy IDs) and the total flat buffer length, then determining the padding size from word-alignment math rather than stripping nulls.</violation>
</file>
<file name="ledgerstate/snapshot.go">
<violation number="1" location="ledgerstate/snapshot.go:76">
P2: Silent `strconv.ParseUint` error handling in sort comparisons: non-numeric names would parse as slot 0, potentially selecting the wrong file. Consider logging a warning or pre-filtering non-numeric names before sorting.</violation>
</file>
<file name="ledgerstate/cbor_decode.go">
<violation number="1" location="ledgerstate/cbor_decode.go:263">
P2: Integer overflow: `hLen + int(arg)` can wrap negative when `arg` is near `math.MaxInt`. Although `arg` is bounded by `math.MaxInt`, adding `hLen` (1–9) to `int(arg)` can still overflow, producing a negative `size` that passes the `size > len(data)` guard and causes panics in upstream slice operations.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:1006">
P1: `resolveEraParams` silently returns `(0, 0)` for the last era when `CardanoNodeConfig` is nil or `EpochLengthFunc` fails, because `EraBound{}` is passed as `endBound`. This causes all last-era epochs to be stored with the same start slot and zero-length, corrupting the slot clock. Consider returning an error or at least logging a warning when the epoch length can't be resolved for a non-empty epoch range.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Prefer UTxO-HD format (newer) | ||
| if len(utxoHDDirs) > 0 { | ||
| slices.SortFunc(utxoHDDirs, func(a, b string) int { | ||
| na, _ := strconv.ParseUint(a, 10, 64) |
There was a problem hiding this comment.
P2: Silent strconv.ParseUint error handling in sort comparisons: non-numeric names would parse as slot 0, potentially selecting the wrong file. Consider logging a warning or pre-filtering non-numeric names before sorting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/snapshot.go, line 76:
<comment>Silent `strconv.ParseUint` error handling in sort comparisons: non-numeric names would parse as slot 0, potentially selecting the wrong file. Consider logging a warning or pre-filtering non-numeric names before sorting.</comment>
<file context>
@@ -0,0 +1,753 @@
+ // Prefer UTxO-HD format (newer)
+ if len(utxoHDDirs) > 0 {
+ slices.SortFunc(utxoHDDirs, func(a, b string) int {
+ na, _ := strconv.ParseUint(a, 10, 64)
+ nb, _ := strconv.ParseUint(b, 10, 64)
+ if na > nb {
</file context>
| ) | ||
| } | ||
| // #nosec G115 | ||
| size := hLen + int(arg) |
There was a problem hiding this comment.
P2: Integer overflow: hLen + int(arg) can wrap negative when arg is near math.MaxInt. Although arg is bounded by math.MaxInt, adding hLen (1–9) to int(arg) can still overflow, producing a negative size that passes the size > len(data) guard and causes panics in upstream slice operations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/cbor_decode.go, line 263:
<comment>Integer overflow: `hLen + int(arg)` can wrap negative when `arg` is near `math.MaxInt`. Although `arg` is bounded by `math.MaxInt`, adding `hLen` (1–9) to `int(arg)` can still overflow, producing a negative `size` that passes the `size > len(data)` guard and causes panics in upstream slice operations.</comment>
<file context>
@@ -0,0 +1,431 @@
+ )
+ }
+ // #nosec G115
+ size := hLen + int(arg)
+ if size > len(data) {
+ return 0, fmt.Errorf(
</file context>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@chain/chain.go`:
- Around line 336-359: AddRawBlocks currently returns raw errors from txn.Do
which loses batch context; update the error handling after calling txn.Do in
AddRawBlocks to wrap the returned error using fmt.Errorf and %w, including
identifying batch context (e.g., batchOffset and batchSize or the batch range)
in the message so callers can see which block batch failed; ensure the wrapped
error is returned in place of the raw err from txn.Do.
- Around line 259-289: Add guards in Chain.AddRawBlock to validate RawBlock
fields before doing header/tip comparisons: ensure rb.Hash is non-empty
(len(rb.Hash) > 0) before comparing to c.headers[0].Hash(), and when
c.tipBlockIndex >= initialBlockIndex ensure rb.PrevHash is non-empty before
comparing to c.currentTip.Point.Hash; return a clear error (e.g., a new
invalid-block error) if either hash is empty so we never proceed with malformed
blocks into the chain state.
In `@database/plugin/metadata/sqlite/import.go`:
- Around line 128-136: The OnConflict{DoNothing: true} used in the Create(reg)
call (db.Clauses(clause.OnConflict{DoNothing: true}).Create(reg)) is ineffective
because PoolRegistration lacks a unique constraint; either add a DB-level unique
constraint to the PoolRegistration model (e.g., a composite unique index on
PoolID + CertificateID or whatever fields uniquely identify a registration) and
run the migration so the OnConflict works, or remove the misleading OnConflict
clause and rely solely on ImportCheckpoint for idempotency; update the
PoolRegistration model definition and migration code (or update the
import-create code to drop the OnConflict) so behavior is correct and not
silently allowing duplicates.
In `@ledgerstate/cbor_decode.go`:
- Around line 272-342: The array and map handling in cborItemSize currently
casts arg (from cborArgument) to int without bounds checking, which can wrap for
extremely large CBOR lengths; before casting arg to int in both the array (case
4) and map (case 5) non-indefinite branches, validate that arg <= MaxInt (the
same guard used for strings) and return an error if it exceeds MaxInt to prevent
overflow and unsafe slicing when iterating with the loop that calls
cborItemSize; update the checks immediately after retrieving arg, hLen, err from
cborArgument.
🧹 Nitpick comments (7)
README.md (2)
35-36: Fix the awkward sentence transition.Line 35 ends with a colon suggesting a continuation, but line 36 starts a new sentence about environment variables. The transition is unclear — what does "This behavior" refer to?
✍️ Proposed fix for clarity
-A sample configuration file is provided at `dingo.yaml.example`. You can copy and edit this file to configure Dingo for your local or production environment: -This behavior can be changed via the following environment variables: +A sample configuration file is provided at `dingo.yaml.example`. You can copy and edit this file to configure Dingo for your local or production environment. + +Dingo's behavior can be modified via the following environment variables:
119-126: Clarify that performance timings are approximate.The performance table shows specific timing values that will vary based on hardware, network conditions, and snapshot age. Consider adding a note that these are example/approximate values to set expectations appropriately.
📊 Suggested clarification
-**Performance** (preview network, ~4M blocks): +**Performance** (example timings for preview network, ~4M blocks):Or add a note after the table:
| **Total** | **~50 min** | + +*Note: Timings are approximate and will vary based on hardware, network speed, and snapshot size.*internal/node/load.go (2)
160-183: Minor timer leak withtime.After.
time.Aftercreates a timer that isn't garbage collected until it fires. If the function returns successfully before the 30-minute timeout, the timer continues to exist. For a single long-running operation this is typically acceptable, but usingtime.NewTimerwith a deferred stop is more resource-efficient.♻️ Suggested improvement
- timeout := time.After(30 * time.Minute) + timeout := time.NewTimer(30 * time.Minute) + defer timeout.Stop() for { select { case <-ticker.C: tip := ls.Tip() if tip.Point.Slot >= immutableTipSlot { logger.Info( "finished processing blocks from immutable DB", "blocks_copied", blocksCopied, ) return nil } - case <-timeout: + case <-timeout.C: return fmt.Errorf(
203-239: Consider extracting common database setup logic.The database initialization and bulk-load optimization setup (lines 203-239) duplicates the pattern in
LoadWithDB(lines 76-111). While acceptable given the different return types, extracting this to a helper could reduce duplication.ledgerstate/import_test.go (1)
17-49: Add a timeout to avoid hanging integration runs.Using
context.WithTimeoutkeeps CI from stalling on network issues.💡 Suggested change
import ( "context" "fmt" "log/slog" "os" "testing" + "time" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout( + context.Background(), 30*time.Minute, + ) + t.Cleanup(cancel)cmd/dingo/mithril.go (1)
387-404: Consider using a constant for humanBytes thresholds.The implementation is correct. Minor suggestion: extracting the unit constants (KB, MB, GB) to package level would improve reusability if this utility is needed elsewhere.
mithril/download.go (1)
150-153: Consider adding a configurable timeout for connection establishment.The
http.ClienthasTimeout: 0for large downloads (correct), but this also disables connection timeout. For very slow or unresponsive servers, the download could hang indefinitely waiting to connect.💡 Optional enhancement
Consider using a custom
TransportwithDialContexttimeout while keeping the overall request timeout at 0:client := &http.Client{ Timeout: 0, // No timeout for large downloads CheckRedirect: httpsOnlyRedirect, Transport: &http.Transport{ DialContext: (&net.Dialer{ Timeout: 30 * time.Second, }).DialContext, }, }
chain/chain.go
Outdated
| func (c *Chain) AddRawBlock(rb RawBlock, txn *database.Txn) error { | ||
| if c == nil { | ||
| return errors.New("chain is nil") | ||
| } | ||
| c.mutex.Lock() | ||
| defer c.mutex.Unlock() | ||
| c.manager.mutex.Lock() | ||
| defer c.manager.mutex.Unlock() | ||
| if err := c.reconcile(); err != nil { | ||
| return fmt.Errorf("reconcile: %w", err) | ||
| } | ||
| // Check that the new block matches our first header, if any | ||
| if len(c.headers) > 0 { | ||
| firstHeader := c.headers[0] | ||
| if string(rb.Hash) != string(firstHeader.Hash().Bytes()) { | ||
| return NewBlockNotMatchHeaderError( | ||
| hex.EncodeToString(rb.Hash), | ||
| firstHeader.Hash().String(), | ||
| ) | ||
| } | ||
| } | ||
| // Check that this block fits on the current chain tip | ||
| if c.tipBlockIndex >= initialBlockIndex { | ||
| if string(rb.PrevHash) != string(c.currentTip.Point.Hash) { | ||
| return NewBlockNotFitChainTipError( | ||
| hex.EncodeToString(rb.Hash), | ||
| hex.EncodeToString(rb.PrevHash), | ||
| hex.EncodeToString(c.currentTip.Point.Hash), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate RawBlock hashes before insertion to avoid corrupt chain state.
AddRawBlock accepts raw slices; with an empty Hash (or empty PrevHash when a prior tip/header exists), an invalid block can be persisted and later chain operations may misbehave. Add minimal guards before the header/tip checks.
🔧 Suggested guard rails
if err := c.reconcile(); err != nil {
return fmt.Errorf("reconcile: %w", err)
}
+if len(rb.Hash) == 0 {
+ return errors.New("raw block hash is empty")
+}
+if (c.tipBlockIndex >= initialBlockIndex || len(c.headers) > 0) && len(rb.PrevHash) == 0 {
+ return errors.New("raw block prev hash is empty")
+}
// Check that the new block matches our first header, if any
if len(c.headers) > 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *Chain) AddRawBlock(rb RawBlock, txn *database.Txn) error { | |
| if c == nil { | |
| return errors.New("chain is nil") | |
| } | |
| c.mutex.Lock() | |
| defer c.mutex.Unlock() | |
| c.manager.mutex.Lock() | |
| defer c.manager.mutex.Unlock() | |
| if err := c.reconcile(); err != nil { | |
| return fmt.Errorf("reconcile: %w", err) | |
| } | |
| // Check that the new block matches our first header, if any | |
| if len(c.headers) > 0 { | |
| firstHeader := c.headers[0] | |
| if string(rb.Hash) != string(firstHeader.Hash().Bytes()) { | |
| return NewBlockNotMatchHeaderError( | |
| hex.EncodeToString(rb.Hash), | |
| firstHeader.Hash().String(), | |
| ) | |
| } | |
| } | |
| // Check that this block fits on the current chain tip | |
| if c.tipBlockIndex >= initialBlockIndex { | |
| if string(rb.PrevHash) != string(c.currentTip.Point.Hash) { | |
| return NewBlockNotFitChainTipError( | |
| hex.EncodeToString(rb.Hash), | |
| hex.EncodeToString(rb.PrevHash), | |
| hex.EncodeToString(c.currentTip.Point.Hash), | |
| ) | |
| } | |
| } | |
| func (c *Chain) AddRawBlock(rb RawBlock, txn *database.Txn) error { | |
| if c == nil { | |
| return errors.New("chain is nil") | |
| } | |
| c.mutex.Lock() | |
| defer c.mutex.Unlock() | |
| c.manager.mutex.Lock() | |
| defer c.manager.mutex.Unlock() | |
| if err := c.reconcile(); err != nil { | |
| return fmt.Errorf("reconcile: %w", err) | |
| } | |
| if len(rb.Hash) == 0 { | |
| return errors.New("raw block hash is empty") | |
| } | |
| if (c.tipBlockIndex >= initialBlockIndex || len(c.headers) > 0) && len(rb.PrevHash) == 0 { | |
| return errors.New("raw block prev hash is empty") | |
| } | |
| // Check that the new block matches our first header, if any | |
| if len(c.headers) > 0 { | |
| firstHeader := c.headers[0] | |
| if string(rb.Hash) != string(firstHeader.Hash().Bytes()) { | |
| return NewBlockNotMatchHeaderError( | |
| hex.EncodeToString(rb.Hash), | |
| firstHeader.Hash().String(), | |
| ) | |
| } | |
| } | |
| // Check that this block fits on the current chain tip | |
| if c.tipBlockIndex >= initialBlockIndex { | |
| if string(rb.PrevHash) != string(c.currentTip.Point.Hash) { | |
| return NewBlockNotFitChainTipError( | |
| hex.EncodeToString(rb.Hash), | |
| hex.EncodeToString(rb.PrevHash), | |
| hex.EncodeToString(c.currentTip.Point.Hash), | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@chain/chain.go` around lines 259 - 289, Add guards in Chain.AddRawBlock to
validate RawBlock fields before doing header/tip comparisons: ensure rb.Hash is
non-empty (len(rb.Hash) > 0) before comparing to c.headers[0].Hash(), and when
c.tipBlockIndex >= initialBlockIndex ensure rb.PrevHash is non-empty before
comparing to c.currentTip.Point.Hash; return a clear error (e.g., a new
invalid-block error) if either hash is empty so we never proceed with malformed
blocks into the chain state.
| // AddRawBlocks adds a batch of pre-extracted blocks to the chain. | ||
| func (c *Chain) AddRawBlocks(blocks []RawBlock) error { | ||
| if c == nil { | ||
| return errors.New("chain is nil") | ||
| } | ||
| batchOffset := 0 | ||
| for { | ||
| batchSize := min(50, len(blocks)-batchOffset) | ||
| if batchSize == 0 { | ||
| break | ||
| } | ||
| txn := c.manager.db.BlobTxn(true) | ||
| err := txn.Do(func(txn *database.Txn) error { | ||
| for _, rb := range blocks[batchOffset : batchOffset+batchSize] { | ||
| if err := c.AddRawBlock(rb, txn); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| batchOffset += batchSize |
There was a problem hiding this comment.
Wrap batch errors with context.
AddRawBlocks returns raw errors from txn.Do, which drops batch context. Wrap the error for easier triage.
🔧 Suggested fix
if err != nil {
- return err
+ return fmt.Errorf("add raw blocks batch starting at %d: %w", batchOffset, err)
}As per coding guidelines, Wrap errors with context using fmt.Errorf with %w verb.
🤖 Prompt for AI Agents
In `@chain/chain.go` around lines 336 - 359, AddRawBlocks currently returns raw
errors from txn.Do which loses batch context; update the error handling after
calling txn.Do in AddRawBlocks to wrap the returned error using fmt.Errorf and
%w, including identifying batch context (e.g., batchOffset and batchSize or the
batch range) in the message so callers can see which block batch failed; ensure
the wrapped error is returned in place of the raw err from txn.Do.
| // Create registration record (ignore duplicates on retry) | ||
| if result := db.Clauses(clause.OnConflict{ | ||
| DoNothing: true, | ||
| }).Create(reg); result.Error != nil { | ||
| return fmt.Errorf( | ||
| "import pool registration: %w", | ||
| result.Error, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -A 30 "type PoolRegistration struct" --type goRepository: blinklabs-io/dingo
Length of output: 1853
🏁 Script executed:
# Search for pool_registration table creation/migration
fd . database/ -type f -name "*.go" | xargs rg "pool_registration" -l | head -20Repository: blinklabs-io/dingo
Length of output: 703
🏁 Script executed:
# Check ImportCheckpoint usage
rg -B 5 -A 10 "ImportCheckpoint" database/plugin/metadata/sqlite/import.goRepository: blinklabs-io/dingo
Length of output: 1088
🏁 Script executed:
# Check import.go around the PoolRegistration create to understand the flow
rg -B 15 -A 5 "Create pool registration record\|Create\(reg\)" database/plugin/metadata/sqlite/import.goRepository: blinklabs-io/dingo
Length of output: 44
🏁 Script executed:
# Look for migrations or schema definitions for pool_registration
rg -l "pool_registration\|PoolRegistration" database/ | xargs rg "CREATE TABLE\|unique\|UniqueIndex" -B 2 -A 5Repository: blinklabs-io/dingo
Length of output: 46
🏁 Script executed:
# View the import.go file around line 128
sed -n '100,150p' database/plugin/metadata/sqlite/import.goRepository: blinklabs-io/dingo
Length of output: 1254
🏁 Script executed:
# Search for migration files
find database -type f -name "*migrat*" -o -name "*schema*" | head -20Repository: blinklabs-io/dingo
Length of output: 44
🏁 Script executed:
# Search for AutoMigrate or table creation
rg "AutoMigrate|CreateTable" database/ -A 3Repository: blinklabs-io/dingo
Length of output: 14241
🏁 Script executed:
# Check the Pool model for unique constraints
rg -A 40 "type Pool struct" database/models/pool.goRepository: blinklabs-io/dingo
Length of output: 1695
🏁 Script executed:
# Get the complete PoolRegistration struct definition including all fields and tags
sed -n '27,55p' database/models/pool.goRepository: blinklabs-io/dingo
Length of output: 1198
🏁 Script executed:
# Get the complete PoolRegistration struct - more lines
sed -n '47,75p' database/models/pool.goRepository: blinklabs-io/dingo
Length of output: 1057
OnConflict{DoNothing: true} will not prevent duplicates without a unique constraint on PoolRegistration.
The PoolRegistration model has no unique constraints—only an auto-incrementing ID and several regular indexes. GORM's OnConflict{DoNothing: true} requires a unique constraint to detect conflicts. As currently written, the code silently fails to prevent duplicates on retry.
Idempotency is genuinely provided by ImportCheckpoint tracking import progress. If database-level duplicate prevention is needed, add a unique constraint (e.g., composite index on PoolID + CertificateID or other identifying fields) to PoolRegistration. Otherwise, remove the misleading OnConflict clause and rely solely on ImportCheckpoint.
🤖 Prompt for AI Agents
In `@database/plugin/metadata/sqlite/import.go` around lines 128 - 136, The
OnConflict{DoNothing: true} used in the Create(reg) call
(db.Clauses(clause.OnConflict{DoNothing: true}).Create(reg)) is ineffective
because PoolRegistration lacks a unique constraint; either add a DB-level unique
constraint to the PoolRegistration model (e.g., a composite unique index on
PoolID + CertificateID or whatever fields uniquely identify a registration) and
run the migration so the OnConflict works, or remove the misleading OnConflict
clause and rely solely on ImportCheckpoint for idempotency; update the
PoolRegistration model definition and migration code (or update the
import-create code to drop the OnConflict) so behavior is correct and not
silently allowing duplicates.
92b9af7 to
e5417dc
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@internal/node/load.go`:
- Around line 392-398: The skip logic in copyBlocks currently compares next.Slot
(ImmutableDB metadata) to chainTip.Point.Slot which can diverge from the decoded
block header slot used elsewhere (tmpBlock.SlotNumber()); change the check to
decode the first block's header (or reuse the already-decoded tmpBlock/header)
and compare header.SlotNumber() (or tmpBlock.SlotNumber()) against
chainTip.Point.Slot so both resume paths use the same source of truth; update
the code around the next == nil / blocksCopied == 0 branch in copyBlocks to
perform the header decode prior to the skip-check and use header.SlotNumber()
instead of next.Slot to decide whether to continue.
In `@ledgerstate/govstate.go`:
- Around line 300-340: parseProposals currently returns nil, nil when the
decoded container has fewer than 2 elements which can be ambiguous; change this
to return an empty []ParsedGovProposal (not nil) and nil error for the "no
proposals" case so callers get an empty slice instead of nil, and add a short
comment noting that a single-element container is treated as "no OMap -> no
proposals"; keep the existing error return path for actual decode failures and
leave errors.Join(propErrs...) as-is.
In `@ledgerstate/utxo.go`:
- Around line 126-132: The current fallback treats any byte slice with len(data)
>= 34 as a valid binary "hash + LE index", which can silently mis-parse
malformed CBOR; update the fallback in ledgerstate/utxo.go so it only accepts
the binary path when the remaining bytes after the initial 34 are empty (i.e.,
len(data) == 34) or pass an additional sanity check (for example verify the
32-byte txHash is not all-zero or contains expected entropy) and otherwise
return an error (or log a warning) instead of silently returning garbage; locate
the block using the variables/data sequence (data, txHash, idx) and change the
condition and add a warning/error path when extra/unexpected bytes or
implausible txHash are present.
- Around line 347-412: parseCborTxOut currently only extracts DatumHash via a
type assertion and omits inline Datum and ScriptRef; update parseCborTxOut to
also detect and extract inline datum and reference script similar to
parseMempackTxOut by using type assertions on txOut for Datum() and ScriptRef()
methods (in addition to the existing DatumHash() assertion), setting
ParsedUTxO.Datum with the decoded datum bytes when Datum() returns non-nil and
setting ParsedUTxO.ScriptRef when ScriptRef() returns a non-nil reference (use
the same byte extraction/limits logic used for DatumHash and other fields to
avoid huge allocations), ensuring you reference the txOut variable, the
ParsedUTxO struct fields (Datum, DatumHash, ScriptRef), and the
Datum()/ScriptRef()/DatumHash() method names when making the changes.
In `@mithril/bootstrap.go`:
- Around line 259-291: The code skips downloading when
hasLedgerFiles(candidateDir) is true but never sets ancillaryArchivePath, so
Cleanup() can't remove a leftover archive; update the branch for the
already-extracted path to locate and assign the existing ancillary archive path
(ancillaryArchivePath) so Cleanup can remove it — e.g., add logic in the
hasLedgerFiles true branch to call a new helper (or reuse logic from
downloadAncillary) to find the existing archive file and set
ancillaryArchivePath, or change hasLedgerFiles/downloadAncillary to return the
archive path alongside the extraction check; ensure you reference
ancillaryArchivePath, hasLedgerFiles, downloadAncillary and Cleanup in the fix.
In `@mithril/download.go`:
- Around line 166-176: When handling the http.StatusOK branch in the download
switch, reset existingSize to 0 before creating/truncating the destination file
so the progressWriter (constructed later with written: existingSize and
startOffset: existingSize) doesn't carry a stale partial-file size; update the
http.StatusOK case in the function that checks resp.StatusCode (references:
resp.StatusCode, http.StatusOK, existingSize, os.Create, destPath,
progressWriter, BytesDownloaded, startOffset) to set existingSize = 0 so
progress callbacks report correct bytes and percent.
In `@README.md`:
- Around line 35-36: The sentence "This behavior can be changed via the
following environment variables:" is misplaced immediately after the sentence
introducing `dingo.yaml.example` and breaks the text flow; fix by either moving
that sentence to follow the environment variables list or insert a paragraph
break so the config-file introduction and the env-var note are separate—update
the README so the sentence follows the relevant env var section or is separated
into its own paragraph for clear context.
🧹 Nitpick comments (17)
chainselection/selector.go (1)
224-238:PreviousTipomitted fromChainSwitchEventinRemovePeer— intentional but worth noting.When the best peer disconnects, it's already removed from
peerTips(line 205) before the event is constructed, so the previous tip is unavailable. This meansPreviousTipwill be the zero value. If subscribers rely onPreviousTipfor anything meaningful (e.g., logging the slot delta), they'll see misleading data. Consider capturing the previous peer's tip before thedeleteon line 205 if this matters to consumers.internal/node/load.go (2)
160-183: 30-minute timeout for ledger catch-up may be insufficient for mainnet.For a full mainnet bootstrap (millions of blocks), the ledger processing pipeline could take significantly longer than 30 minutes to consume all queued chain events. Consider making this timeout configurable or scaling it based on the number of blocks copied.
197-261: DB config construction is duplicated betweenLoadWithDBandLoadBlobsWithDB.Lines 206–213 repeat the exact same
database.Configsetup as lines 78–85. Consider extracting a small helper to reduce drift risk.ledgerstate/snapshots.go (1)
302-358: Consider sorting the output for deterministic results.
AggregatePoolStakeiteratespoolMap(a Go map) which has non-deterministic iteration order. While this likely doesn't affect correctness (downstream code presumably handles unordered slices), deterministic output simplifies debugging and testing.ledgerstate/snapshot.go (3)
270-277: Silently discarding era-bounds extraction error.
boundsErris explicitly suppressed with_ = boundsErr. If bounds extraction fails,EraBoundswill benil, and the comment notes a fallback to "single-epoch path." However, since incorrect epoch history can affect SlotToTime calculations and downstream functionality, consider returning the error as a warning (similar to the warnings pattern insnapshots.go) or addingeraBoundsErrtoRawLedgerStateso callers can assess the quality of the parsed result.
362-388: Version-peeling loop is well-designed but could benefit from a depth limit.The loop correctly handles nested version wrappers (
[v1, [v2, [tip, NES, ...]]]) and flat wrappers ([v, tip, NES, ...]). In practice, Cardano snapshots have at most 1-2 version layers. Consider adding a depth counter (e.g., max 5 iterations) as a defensive measure against pathological input causing excessive recursion.
696-709: Same version-peeling pattern forPraosState— consider extracting a shared helper.This loop mirrors the version-unwrapping logic in
parseCurrentEra(lines 362-388). A shared helper likepeelVersionWrappers(data [][]byte) [][]bytecould reduce duplication and apply a consistent depth limit.ledgerstate/import_test.go (1)
156-175: Minor:lastProgresscapture may have a data race ifOnProgressis called concurrently.
lastProgress = p(Line 165) is a plain struct assignment in a closure. IfImportLedgerStateever callsOnProgressfrom a background goroutine, this is a race with the read at Line 175. Currently this is likely synchronous, but consider usingatomicor a mutex for safety, or documenting the synchronous contract.cmd/dingo/serve.go (2)
46-72: Swallowing alldatabase.Newerrors may mask real problems.Line 57-60: any
database.Newfailure (permissions, corrupt WAL, disk full, etc.) is treated as "fresh start" and silently ignored. The node would then proceed tonode.Run, which likely opens the same DB and fails with a less actionable error.Consider distinguishing "database doesn't exist" from other errors, or at least logging a warning.
♻️ Suggested improvement
db, err := database.New(&database.Config{ DataDir: cfg.DatabasePath, Logger: logger, BlobPlugin: cfg.BlobPlugin, MetadataPlugin: cfg.MetadataPlugin, MaxConnections: 1, }) if err != nil { - // Database may not exist yet (fresh start) — that's fine. + // Database may not exist yet (fresh start). + // Log at debug level so real errors are not completely invisible. + logger.Debug( + "could not open database for sync check", + "error", err, + ) return nil }
63-66:GetSyncStateerror is also silently swallowed.Line 64 treats any error from
GetSyncStatethe same as "no entry found." If the sync_state table exists but the query fails for a real reason (e.g., I/O error), the node starts without the safety check. The same debug-log suggestion applies here.ledgerstate/import.go (1)
498-543: Single transaction for all accounts may strain the database on mainnet.
importAccountsopens one transaction and inserts every account serially. On mainnet there can be millions of stake accounts, which means one very long-lived write transaction. The UTxO path already demonstrates a good batching pattern with per-batch commits. Consider applying a similar batched-commit approach here (and inimportPools/importDReps) to reduce memory pressure and lock duration.mithril/types.go (1)
31-65:SnapshotListItemandSnapshotare structurally identical.These two types share every field. If the Mithril API guarantees the same shape for both endpoints, consider embedding a shared struct or using a type alias to reduce duplication. That said, keeping them separate is defensible if you expect the schemas to diverge.
cmd/dingo/mithril.go (1)
638-758:fetchGapBlocksonly tries the first bootstrap peer with no retry.If the first (and only) bootstrap peer is unreachable or stalls, the gap-block fetch fails the entire sync. Consider iterating over
netInfo.BootstrapPeerswith a fallback, or at minimum documenting this limitation.database/models/import_checkpoint.go (1)
41-48: ExportedImportPhaseOrderslice is mutable — consider making it unexported or a function.Any caller can accidentally (or intentionally) mutate
ImportPhaseOrderviaappendor index assignment, which would corrupt phase ordering globally. A safer pattern is to either unexport it or return a copy from a function.♻️ Suggested change
-// ImportPhaseOrder defines the sequential order of import phases. -var ImportPhaseOrder = []string{ +// importPhaseOrder defines the sequential order of import phases. +var importPhaseOrder = []string{ ImportPhaseUTxO, ImportPhaseCertState, ImportPhaseSnapshots, ImportPhasePParams, ImportPhaseGovState, ImportPhaseTip, } + +// ImportPhaseOrder returns the sequential order of import phases. +func ImportPhaseOrder() []string { + out := make([]string, len(importPhaseOrder)) + copy(out, importPhaseOrder) + return out +}ledgerstate/utxo.go (1)
174-262: Duplicated batch-processing logic betweenParseUTxOsStreamingandparseIndefiniteUTxOMap.The batch accumulation, callback invocation, and final flush pattern (lines 201–259 and 505–572) are nearly identical. Consider extracting a shared helper to reduce duplication.
♻️ Sketch of a shared helper
// batchProcessor handles batch accumulation and callback. type batchProcessor struct { batch []ParsedUTxO callback UTxOCallback total int } func (bp *batchProcessor) add(u ParsedUTxO) error { bp.batch = append(bp.batch, u) bp.total++ if len(bp.batch) >= utxoBatchSize { if err := bp.callback(bp.batch); err != nil { return fmt.Errorf( "UTxO callback error at entry %d: %w", bp.total, err, ) } bp.batch = bp.batch[:0] } return nil } func (bp *batchProcessor) flush() error { if len(bp.batch) > 0 { if err := bp.callback(bp.batch); err != nil { return fmt.Errorf( "UTxO callback error at final batch: %w", err, ) } } return nil }Also applies to: 488-575
database/plugin/metadata/store.go (1)
613-626: Consider naming thestringparameters for readability.
SetSyncState(string, string, types.Txn)— it's unclear from the signature alone which string is the key vs. value. While other methods in this interface also use bare types, the two adjacentstringparams make this particularly easy to mix up.♻️ Suggested change
- GetSyncState(string, types.Txn) (string, error) - SetSyncState(string, string, types.Txn) error - DeleteSyncState(string, types.Txn) error + GetSyncState(key string, txn types.Txn) (string, error) + SetSyncState(key string, value string, txn types.Txn) error + DeleteSyncState(key string, txn types.Txn) errormithril/bootstrap.go (1)
82-101: Missing validation forcfg.NetworkwhenAggregatorURLis also empty.If both
cfg.Networkandcfg.AggregatorURLare empty,AggregatorURLForNetwork("")will fail with a somewhat opaque error. Consider validating the config upfront.♻️ Suggested early validation
func Bootstrap( ctx context.Context, cfg BootstrapConfig, ) (*BootstrapResult, error) { if cfg.Logger == nil { cfg.Logger = slog.Default() } + if cfg.Network == "" && cfg.AggregatorURL == "" { + return nil, errors.New( + "either Network or AggregatorURL must be set", + ) + } // Resolve aggregator URL
| func parseProposals(data []byte) ( | ||
| []ParsedGovProposal, error, | ||
| ) { | ||
| container, err := decodeRawArray(data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf( | ||
| "decoding proposals container: %w", err, | ||
| ) | ||
| } | ||
| if len(container) < 2 { | ||
| // No proposals or unexpected format | ||
| return nil, nil | ||
| } | ||
|
|
||
| // The OMap is the second element: [roots, omap]. | ||
| // OMap encodes as a flat array of GovActionState values. | ||
| proposalSeq := container[1] | ||
|
|
||
| items, err := decodeRawArray(proposalSeq) | ||
| if err != nil { | ||
| return nil, fmt.Errorf( | ||
| "decoding proposals OMap: %w", err, | ||
| ) | ||
| } | ||
|
|
||
| proposals := make([]ParsedGovProposal, 0, len(items)) | ||
| var propErrs []error | ||
| for _, item := range items { | ||
| prop, err := parseGovActionState(item) | ||
| if err != nil { | ||
| propErrs = append(propErrs, err) | ||
| continue | ||
| } | ||
| if prop != nil { | ||
| proposals = append(proposals, *prop) | ||
| } | ||
| } | ||
|
|
||
| // Return parsed proposals even if some failed | ||
| return proposals, errors.Join(propErrs...) | ||
| } |
There was a problem hiding this comment.
parseProposals returns nil, nil for containers with fewer than 2 elements.
A 1-element container (roots without OMap) would silently succeed with no proposals. If this is expected for empty proposal sets, it's fine. If a 1-element container indicates corruption, a warning might be useful.
🤖 Prompt for AI Agents
In `@ledgerstate/govstate.go` around lines 300 - 340, parseProposals currently
returns nil, nil when the decoded container has fewer than 2 elements which can
be ambiguous; change this to return an empty []ParsedGovProposal (not nil) and
nil error for the "no proposals" case so callers get an empty slice instead of
nil, and add a short comment noting that a single-element container is treated
as "no OMap -> no proposals"; keep the existing error return path for actual
decode failures and leave errors.Join(propErrs...) as-is.
ledgerstate/utxo.go
Outdated
| // Fall back to binary format for other lengths | ||
| if len(data) >= 34 { | ||
| txHash := make([]byte, 32) | ||
| copy(txHash, data[:32]) | ||
| idx := uint32(data[32]) | uint32(data[33])<<8 | ||
| return txHash, idx, nil | ||
| } |
There was a problem hiding this comment.
Binary fallback for len(data) >= 34 may silently mis-parse non-binary data.
After the CBOR array decode attempt fails, any byte slice ≥ 34 bytes is interpreted as binary hash + LE index. If the data is actually malformed CBOR (not binary MemPack), this will silently produce a garbage TxHash and output index without any error.
Consider logging a warning or only accepting this path if there's additional validation (e.g., checking that remaining bytes after 34 are empty, or that the resulting hash looks plausible).
🤖 Prompt for AI Agents
In `@ledgerstate/utxo.go` around lines 126 - 132, The current fallback treats any
byte slice with len(data) >= 34 as a valid binary "hash + LE index", which can
silently mis-parse malformed CBOR; update the fallback in ledgerstate/utxo.go so
it only accepts the binary path when the remaining bytes after the initial 34
are empty (i.e., len(data) == 34) or pass an additional sanity check (for
example verify the 32-byte txHash is not all-zero or contains expected entropy)
and otherwise return an error (or log a warning) instead of silently returning
garbage; locate the block using the variables/data sequence (data, txHash, idx)
and change the condition and add a warning/error path when extra/unexpected
bytes or implausible txHash are present.
| A sample configuration file is provided at `dingo.yaml.example`. You can copy and edit this file to configure Dingo for your local or production environment: | ||
| This behavior can be changed via the following environment variables: |
There was a problem hiding this comment.
Minor text flow issue.
Line 35 introduces the sample config file, but line 36 starts with "This behavior can be changed..." which originally referred to the preceding paragraph about YAML config and env vars. The new sentence creates a disconnect. Consider moving it after the environment variables list, or adding a paragraph break.
🤖 Prompt for AI Agents
In `@README.md` around lines 35 - 36, The sentence "This behavior can be changed
via the following environment variables:" is misplaced immediately after the
sentence introducing `dingo.yaml.example` and breaks the text flow; fix by
either moving that sentence to follow the environment variables list or insert a
paragraph break so the config-file introduction and the env-var note are
separate—update the README so the sentence follows the relevant env var section
or is separated into its own paragraph for clear context.
There was a problem hiding this comment.
8 issues found across 58 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/dingo/mithril.go">
<violation number="1" location="cmd/dingo/mithril.go:1000">
P1: Bug: `seenCbor` map is scoped per-transaction but `findNthOccurrence` searches the entire block CBOR. If two transactions in the same block produce outputs with identical CBOR, the counter resets and both will resolve to the same byte position (the first occurrence), producing an incorrect offset for the second transaction's output. Move `seenCbor` declaration before the transaction loop so it accumulates across all transactions in the block.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:516">
P2: Unlike `importUTxOs` which streams records in batches, `importAccounts` processes all accounts (potentially ~1.4M on mainnet) in a single database transaction. Consider adding batching similar to the UTxO import to reduce memory pressure and transaction lock duration.</violation>
<violation number="2" location="ledgerstate/import.go:1324">
P2: Duplicate detection in `isEpochDuplicate` uses fragile string matching against DB-driver error messages. Consider using `errors.Is(err, gorm.ErrDuplicatedKey)` for a driver-independent check that won't break if error message formats change.</violation>
</file>
<file name="mithril/bootstrap.go">
<violation number="1" location="mithril/bootstrap.go:240">
P2: Goroutine leak / hang risk: if main archive extraction fails, `defer ancWg.Wait()` blocks the return until the ancillary download goroutine finishes, but nothing cancels that download. This can cause the function to hang for a long time (minutes to hours depending on archive size). Create a cancellable derived context and cancel it on error so the goroutine's download aborts promptly.</violation>
</file>
<file name="database/plugin/metadata/mysql/import.go">
<violation number="1" location="database/plugin/metadata/mysql/import.go:127">
P2: Inconsistent with `ImportPool` pattern: `reg.DrepCredential` is not linked to `drep.Credential` before creating the registration. In `ImportPool`, `reg.PoolID` and related fields are explicitly set after the upsert. The same pattern should apply here to ensure the registration is always linked to the DRep, regardless of what the caller passes.</violation>
</file>
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:610">
P2: Silent failure on malformed data: if the flat buffer is shorter than expected (`remainder < 0`) or no padding value matches, `lastNameLen` silently defaults to 0 instead of returning an error. This could produce incorrect asset name data without any indication of corruption. Consider returning an error when `remainder < 0` or when no padding candidate matches.</violation>
<violation number="2" location="ledgerstate/mempack.go:672">
P1: Missing bounds check: `flat[e.nameOff:namesContentEnd]` can panic if `namesContentEnd > len(flat)` due to malformed data. The non-last-asset branch correctly checks `nameEnd <= len(flat)` before slicing, but this branch does not. Add a bounds check like `e.nameOff < namesContentEnd && namesContentEnd <= len(flat)` to match the defensive style of the other branch.</violation>
</file>
<file name="cmd/dingo/serve.go">
<violation number="1" location="cmd/dingo/serve.go:57">
P2: Ignoring database.New errors can leak a partially opened DB and skip the sync-state guard even when initialization failed for non-“missing DB” reasons. Handle the error and close any opened DB instead of returning nil.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
cmd/dingo/mithril.go
Outdated
| var txHashArr [32]byte | ||
| copy(txHashArr[:], txHash.Bytes()) | ||
| produced := tx.Produced() | ||
| seenCbor := make(map[string]int) |
There was a problem hiding this comment.
P1: Bug: seenCbor map is scoped per-transaction but findNthOccurrence searches the entire block CBOR. If two transactions in the same block produce outputs with identical CBOR, the counter resets and both will resolve to the same byte position (the first occurrence), producing an incorrect offset for the second transaction's output. Move seenCbor declaration before the transaction loop so it accumulates across all transactions in the block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/dingo/mithril.go, line 1000:
<comment>Bug: `seenCbor` map is scoped per-transaction but `findNthOccurrence` searches the entire block CBOR. If two transactions in the same block produce outputs with identical CBOR, the counter resets and both will resolve to the same byte position (the first occurrence), producing an incorrect offset for the second transaction's output. Move `seenCbor` declaration before the transaction loop so it accumulates across all transactions in the block.</comment>
<file context>
@@ -0,0 +1,1123 @@
+ var txHashArr [32]byte
+ copy(txHashArr[:], txHash.Bytes())
+ produced := tx.Produced()
+ seenCbor := make(map[string]int)
+ for _, utxo := range produced {
+ outCbor := utxo.Output.Cbor()
</file context>
ledgerstate/mempack.go
Outdated
| } else { | ||
| // Last asset: name extends to the computed | ||
| // end of names content (excluding padding). | ||
| if e.nameOff < namesContentEnd { |
There was a problem hiding this comment.
P1: Missing bounds check: flat[e.nameOff:namesContentEnd] can panic if namesContentEnd > len(flat) due to malformed data. The non-last-asset branch correctly checks nameEnd <= len(flat) before slicing, but this branch does not. Add a bounds check like e.nameOff < namesContentEnd && namesContentEnd <= len(flat) to match the defensive style of the other branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 672:
<comment>Missing bounds check: `flat[e.nameOff:namesContentEnd]` can panic if `namesContentEnd > len(flat)` due to malformed data. The non-last-asset branch correctly checks `nameEnd <= len(flat)` before slicing, but this branch does not. Add a bounds check like `e.nameOff < namesContentEnd && namesContentEnd <= len(flat)` to match the defensive style of the other branch.</comment>
<file context>
@@ -0,0 +1,838 @@
+ } else {
+ // Last asset: name extends to the computed
+ // end of names content (excluding padding).
+ if e.nameOff < namesContentEnd {
+ nameLen := namesContentEnd - e.nameOff
+ if nameLen > 0 {
</file context>
| txn := cfg.Database.MetadataTxn(true) | ||
| defer txn.Release() | ||
|
|
||
| for _, acct := range accounts { |
There was a problem hiding this comment.
P2: Unlike importUTxOs which streams records in batches, importAccounts processes all accounts (potentially ~1.4M on mainnet) in a single database transaction. Consider adding batching similar to the UTxO import to reduce memory pressure and transaction lock duration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/import.go, line 516:
<comment>Unlike `importUTxOs` which streams records in batches, `importAccounts` processes all accounts (potentially ~1.4M on mainnet) in a single database transaction. Consider adding batching similar to the UTxO import to reduce memory pressure and transaction lock duration.</comment>
<file context>
@@ -0,0 +1,1337 @@
+ txn := cfg.Database.MetadataTxn(true)
+ defer txn.Release()
+
+ for _, acct := range accounts {
+ select {
+ case <-ctx.Done():
</file context>
| // idempotent epoch generation when resuming a partially | ||
| // completed import. Handles SQLite, MySQL, and PostgreSQL | ||
| // duplicate key error formats. | ||
| func isEpochDuplicate(err error) bool { |
There was a problem hiding this comment.
P2: Duplicate detection in isEpochDuplicate uses fragile string matching against DB-driver error messages. Consider using errors.Is(err, gorm.ErrDuplicatedKey) for a driver-independent check that won't break if error message formats change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/import.go, line 1324:
<comment>Duplicate detection in `isEpochDuplicate` uses fragile string matching against DB-driver error messages. Consider using `errors.Is(err, gorm.ErrDuplicatedKey)` for a driver-independent check that won't break if error message formats change.</comment>
<file context>
@@ -0,0 +1,1337 @@
+// idempotent epoch generation when resuming a partially
+// completed import. Handles SQLite, MySQL, and PostgreSQL
+// duplicate key error formats.
+func isEpochDuplicate(err error) bool {
+ if err == nil {
+ return false
</file context>
| // Always wait for the goroutine before returning, even on error, | ||
| // to prevent goroutine leaks and races with temp dir cleanup. | ||
| var ancWg sync.WaitGroup | ||
| defer ancWg.Wait() |
There was a problem hiding this comment.
P2: Goroutine leak / hang risk: if main archive extraction fails, defer ancWg.Wait() blocks the return until the ancillary download goroutine finishes, but nothing cancels that download. This can cause the function to hang for a long time (minutes to hours depending on archive size). Create a cancellable derived context and cancel it on error so the goroutine's download aborts promptly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mithril/bootstrap.go, line 240:
<comment>Goroutine leak / hang risk: if main archive extraction fails, `defer ancWg.Wait()` blocks the return until the ancillary download goroutine finishes, but nothing cancels that download. This can cause the function to hang for a long time (minutes to hours depending on archive size). Create a cancellable derived context and cancel it on error so the goroutine's download aborts promptly.</comment>
<file context>
@@ -0,0 +1,668 @@
+ // Always wait for the goroutine before returning, even on error,
+ // to prevent goroutine leaks and races with temp dir cleanup.
+ var ancWg sync.WaitGroup
+ defer ancWg.Wait()
+ if len(snapshot.AncillaryLocations) > 0 {
+ ancWg.Add(1)
</file context>
| reg.Relays[i].PoolID = pool.ID | ||
| } | ||
|
|
||
| // Create registration record. Idempotency is handled at the |
There was a problem hiding this comment.
P2: Inconsistent with ImportPool pattern: reg.DrepCredential is not linked to drep.Credential before creating the registration. In ImportPool, reg.PoolID and related fields are explicitly set after the upsert. The same pattern should apply here to ensure the registration is always linked to the DRep, regardless of what the caller passes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At database/plugin/metadata/mysql/import.go, line 127:
<comment>Inconsistent with `ImportPool` pattern: `reg.DrepCredential` is not linked to `drep.Credential` before creating the registration. In `ImportPool`, `reg.PoolID` and related fields are explicitly set after the upsert. The same pattern should apply here to ensure the registration is always linked to the DRep, regardless of what the caller passes.</comment>
<file context>
@@ -0,0 +1,230 @@
+ reg.Relays[i].PoolID = pool.ID
+ }
+
+ // Create registration record. Idempotency is handled at the
+ // application level via ImportCheckpoint, not by DB constraints.
+ if result := db.Create(reg); result.Error != nil {
</file context>
| numUniquePolicies := len(uniquePidOffs) | ||
| base := nameOffsetsEnd + numUniquePolicies*28 + knownNameBytes | ||
| remainder := len(flat) - base // = lastNameLen + padding | ||
| lastNameLen := 0 |
There was a problem hiding this comment.
P2: Silent failure on malformed data: if the flat buffer is shorter than expected (remainder < 0) or no padding value matches, lastNameLen silently defaults to 0 instead of returning an error. This could produce incorrect asset name data without any indication of corruption. Consider returning an error when remainder < 0 or when no padding candidate matches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 610:
<comment>Silent failure on malformed data: if the flat buffer is shorter than expected (`remainder < 0`) or no padding value matches, `lastNameLen` silently defaults to 0 instead of returning an error. This could produce incorrect asset name data without any indication of corruption. Consider returning an error when `remainder < 0` or when no padding candidate matches.</comment>
<file context>
@@ -0,0 +1,838 @@
+ numUniquePolicies := len(uniquePidOffs)
+ base := nameOffsetsEnd + numUniquePolicies*28 + knownNameBytes
+ remainder := len(flat) - base // = lastNameLen + padding
+ lastNameLen := 0
+ if numAssets > 0 && remainder > 0 {
+ for pad := range 8 {
</file context>
e667073 to
7948983
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
6 issues found across 54 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ledgerstate/utxo.go">
<violation number="1" location="ledgerstate/utxo.go:486">
P3: Significant code duplication between `parseIndefiniteUTxOMap` and `ParseUTxOsStreaming`. The batch accumulation, `parseUTxOEntry` call, flush-on-threshold, and final-flush logic are duplicated nearly verbatim (~40 lines). Consider extracting the shared batch processing into a helper function that accepts a "next key/value" iterator to reduce maintenance burden.</violation>
</file>
<file name="chain/chain.go">
<violation number="1" location="chain/chain.go:294">
P2: Exported `AddRawBlock` skips `waitingChan` notification, which can cause chain iterators to hang if called directly (not through `AddRawBlocks`). The method's godoc should document this contract requirement, or the method should be unexported (like `addBlockLocked`) to prevent misuse. Consider either:
1. Making it unexported (`addRawBlock`) since it has a caller-responsibility contract, or
2. Adding explicit documentation: "Callers must notify waitingChan after the enclosing transaction commits."</violation>
</file>
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:672">
P1: Missing bounds check on last asset name extraction can panic on malformed input. Unlike the non-last asset branch which validates `nameStart <= len(flat) && nameEnd <= len(flat)`, this branch slices `flat[e.nameOff:namesContentEnd]` without checking either offset against `len(flat)`. If `entries[last].nameOff` (read as a uint16 from the flat buffer) is corrupted, this will panic with an out-of-bounds slice.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:1153">
P2: In the fallback path, `resolveEraParams` returns `slotLength = 0` while computing a valid `epochLength`. The caller only guards against `epochLength == 0` before calling `store.SetEpoch`, so epochs get persisted with a zero slot length. This could break slot-to-wall-clock-time mapping in the slot clock. Consider either logging a warning when using the fallback slotLength, or adding a guard in the caller similar to the `epochLength == 0` check.</violation>
</file>
<file name="ledgerstate/govstate.go">
<violation number="1" location="ledgerstate/govstate.go:165">
P2: Silent error swallowing on policy hash decode: if `cbor.Decode` fails, the error is discarded and `PolicyHash` silently defaults to nil. CBOR null already decodes successfully into an empty `[]byte`, so a decode *failure* here indicates genuinely unexpected data. Consider returning or collecting it as a warning (consistent with the committee/proposals best-effort pattern).</violation>
</file>
<file name="internal/node/load.go">
<violation number="1" location="internal/node/load.go:197">
P2: The database initialization and bulk-load pragma setup pattern is duplicated between `LoadWithDB` and `LoadBlobsWithDB`. Consider extracting a helper like `ensureDB(cfg, logger, db) (*database.Database, bool, error)` (returning whether a cleanup/close is needed) and a `withBulkLoadPragmas(db, logger)` helper to reduce maintenance burden. If either function's setup logic changes (e.g., new config fields, different pragma handling), the other could easily get out of sync.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ledgerstate/mempack.go
Outdated
| } else { | ||
| // Last asset: name extends to the computed | ||
| // end of names content (excluding padding). | ||
| if e.nameOff < namesContentEnd { |
There was a problem hiding this comment.
P1: Missing bounds check on last asset name extraction can panic on malformed input. Unlike the non-last asset branch which validates nameStart <= len(flat) && nameEnd <= len(flat), this branch slices flat[e.nameOff:namesContentEnd] without checking either offset against len(flat). If entries[last].nameOff (read as a uint16 from the flat buffer) is corrupted, this will panic with an out-of-bounds slice.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 672:
<comment>Missing bounds check on last asset name extraction can panic on malformed input. Unlike the non-last asset branch which validates `nameStart <= len(flat) && nameEnd <= len(flat)`, this branch slices `flat[e.nameOff:namesContentEnd]` without checking either offset against `len(flat)`. If `entries[last].nameOff` (read as a uint16 from the flat buffer) is corrupted, this will panic with an out-of-bounds slice.</comment>
<file context>
@@ -0,0 +1,838 @@
+ } else {
+ // Last asset: name extends to the computed
+ // end of names content (excluding padding).
+ if e.nameOff < namesContentEnd {
+ nameLen := namesContentEnd - e.nameOff
+ if nameLen > 0 {
</file context>
ledgerstate/import.go
Outdated
| } | ||
| epochSpan := endBound.Epoch - startBound.Epoch | ||
| slotSpan := endBound.Slot - startBound.Slot | ||
| return 0, uint(slotSpan / epochSpan) |
There was a problem hiding this comment.
P2: In the fallback path, resolveEraParams returns slotLength = 0 while computing a valid epochLength. The caller only guards against epochLength == 0 before calling store.SetEpoch, so epochs get persisted with a zero slot length. This could break slot-to-wall-clock-time mapping in the slot clock. Consider either logging a warning when using the fallback slotLength, or adding a guard in the caller similar to the epochLength == 0 check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/import.go, line 1153:
<comment>In the fallback path, `resolveEraParams` returns `slotLength = 0` while computing a valid `epochLength`. The caller only guards against `epochLength == 0` before calling `store.SetEpoch`, so epochs get persisted with a zero slot length. This could break slot-to-wall-clock-time mapping in the slot clock. Consider either logging a warning when using the fallback slotLength, or adding a guard in the caller similar to the `epochLength == 0` check.</comment>
<file context>
@@ -0,0 +1,1337 @@
+ }
+ epochSpan := endBound.Epoch - startBound.Epoch
+ slotSpan := endBound.Slot - startBound.Slot
+ return 0, uint(slotSpan / epochSpan)
+}
+
</file context>
| c := &ParsedConstitution{} | ||
|
|
||
| var url string | ||
| if _, err := cbor.Decode( |
There was a problem hiding this comment.
P2: Silent error swallowing on policy hash decode: if cbor.Decode fails, the error is discarded and PolicyHash silently defaults to nil. CBOR null already decodes successfully into an empty []byte, so a decode failure here indicates genuinely unexpected data. Consider returning or collecting it as a warning (consistent with the committee/proposals best-effort pattern).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/govstate.go, line 165:
<comment>Silent error swallowing on policy hash decode: if `cbor.Decode` fails, the error is discarded and `PolicyHash` silently defaults to nil. CBOR null already decodes successfully into an empty `[]byte`, so a decode *failure* here indicates genuinely unexpected data. Consider returning or collecting it as a warning (consistent with the committee/proposals best-effort pattern).</comment>
<file context>
@@ -0,0 +1,491 @@
+ c := &ParsedConstitution{}
+
+ var url string
+ if _, err := cbor.Decode(
+ anchor[0], &url,
+ ); err != nil {
</file context>
| // parseIndefiniteUTxOMap streams UTxO entries from an | ||
| // indefinite-length CBOR map (0xbf ... 0xff). Each entry is a | ||
| // TxIn key and TxOut value decoded using the existing parsers. | ||
| func parseIndefiniteUTxOMap( |
There was a problem hiding this comment.
P3: Significant code duplication between parseIndefiniteUTxOMap and ParseUTxOsStreaming. The batch accumulation, parseUTxOEntry call, flush-on-threshold, and final-flush logic are duplicated nearly verbatim (~40 lines). Consider extracting the shared batch processing into a helper function that accepts a "next key/value" iterator to reduce maintenance burden.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/utxo.go, line 486:
<comment>Significant code duplication between `parseIndefiniteUTxOMap` and `ParseUTxOsStreaming`. The batch accumulation, `parseUTxOEntry` call, flush-on-threshold, and final-flush logic are duplicated nearly verbatim (~40 lines). Consider extracting the shared batch processing into a helper function that accepts a "next key/value" iterator to reduce maintenance burden.</comment>
<file context>
@@ -0,0 +1,602 @@
+// parseIndefiniteUTxOMap streams UTxO entries from an
+// indefinite-length CBOR map (0xbf ... 0xff). Each entry is a
+// TxIn key and TxOut value decoded using the existing parsers.
+func parseIndefiniteUTxOMap(
+ data []byte,
+ callback UTxOCallback,
</file context>
7948983 to
2bd061c
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
9 issues found across 54 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mithril/download.go">
<violation number="1" location="mithril/download.go:183">
P2: Resume downloads should validate Content-Range before appending. As written, any 206 response is appended without verifying the start offset, which can corrupt the archive if the server ignores the requested range or returns a mismatched offset.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:573">
P2: Unsafe `uint64` → `int64` conversion for `SetFrac64`: if `MarginNum` or `MarginDen` exceeds `math.MaxInt64`, the resulting `big.Rat` will be incorrect (negative value). Use `new(big.Rat).SetFrac(new(big.Int).SetUint64(...), new(big.Int).SetUint64(...))` to handle the full `uint64` range safely.</violation>
</file>
<file name="ledgerstate/cbor_decode.go">
<violation number="1" location="ledgerstate/cbor_decode.go:435">
P2: When decoding an integer-keyed map, sparse keys (e.g., `{0, 2, 5}`) silently produce a compact result where positional indices no longer match key values. Callers like `parsePState` access elements by index expecting `result[i]` to correspond to key `i`. Consider validating that keys are contiguous `0..n-1`, or filling gaps with nil entries up to `max(key)+1`.</violation>
</file>
<file name="ledgerstate/certstate.go">
<violation number="1" location="ledgerstate/certstate.go:282">
P1: `parseDState` uses `decodeRawArray` which only handles CBOR arrays, but Conway-era DState may be encoded as an int-keyed map (record encoding). Both `parsePState` and `parseVState` already use `decodeRawElements` to handle both formats. This inconsistency would cause DState parsing to fail on Conway-era snapshots when reached via the 3-element `parseCertState3` path, resulting in lost stake account data.</violation>
</file>
<file name="ledgerstate/utxo.go">
<violation number="1" location="ledgerstate/utxo.go:252">
P1: `ParseUTxOsStreaming` silently returns 0 entries for indefinite-length CBOR maps. The function only checks the `count` from `DecodeMapHeader`, which is 0 for indefinite-length maps (0xbf header). Consider checking the `indef` return value (second return from `DecodeMapHeader`) and either delegating to `parseIndefiniteUTxOMap` or returning an explicit error.</violation>
</file>
<file name="ledgerstate/snapshots.go">
<violation number="1" location="ledgerstate/snapshots.go:278">
P2: Redundant struct copy: `parsePoolParams` already returns `*ParsedPoolParams`, so copying each field into a new identical struct is unnecessary and fragile — any new fields added to `ParsedPoolParams` will be silently dropped here.</violation>
</file>
<file name="database/immutable/immutable.go">
<violation number="1" location="database/immutable/immutable.go:81">
P1: `upperBound := len(chunkNames) - 1` allows an empty chunk list to fall through without error, so callers can panic when indexing `chunkNames[0]` (e.g., GetBlock). Add an explicit empty-slice guard to preserve the previous error behavior.</violation>
</file>
<file name="mithril/client.go">
<violation number="1" location="mithril/client.go:64">
P2: Guard against a nil custom HTTP client to avoid a nil pointer panic when `doGet` calls `c.httpClient.Do`.</violation>
</file>
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:612">
P1: Bug: last asset name in `decodeFlatMultiAsset` will include trailing padding bytes. The padding-determination loop at line 530 always matches on the first iteration (`pad=0`) because for 8-byte-aligned `flat` buffers, `(8 - contentSize%8)%8 == pad` is satisfied for every `pad` value 0–7. This causes `lastNameLen` to be `remainder` (= actual name length + padding), so the extracted name will have up to 7 trailing garbage bytes.
Consider an alternative approach: compute the names region start as the minimum `nameOff`, then compute total names content as `len(flat) - namesRegionStart - padding`. Or strip trailing zero bytes from the last name, since MemPack padding uses zero fill.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ledgerstate/certstate.go
Outdated
| // - ptrs: map[Ptr]Credential (legacy, skipped) | ||
| // - various other fields | ||
| func parseDState(data []byte) ([]ParsedAccount, error) { | ||
| ds, err := decodeRawArray(data) |
There was a problem hiding this comment.
P1: parseDState uses decodeRawArray which only handles CBOR arrays, but Conway-era DState may be encoded as an int-keyed map (record encoding). Both parsePState and parseVState already use decodeRawElements to handle both formats. This inconsistency would cause DState parsing to fail on Conway-era snapshots when reached via the 3-element parseCertState3 path, resulting in lost stake account data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/certstate.go, line 282:
<comment>`parseDState` uses `decodeRawArray` which only handles CBOR arrays, but Conway-era DState may be encoded as an int-keyed map (record encoding). Both `parsePState` and `parseVState` already use `decodeRawElements` to handle both formats. This inconsistency would cause DState parsing to fail on Conway-era snapshots when reached via the 3-element `parseCertState3` path, resulting in lost stake account data.</comment>
<file context>
@@ -0,0 +1,860 @@
+// - ptrs: map[Ptr]Credential (legacy, skipped)
+// - various other fields
+func parseDState(data []byte) ([]ParsedAccount, error) {
+ ds, err := decodeRawArray(data)
+ if err != nil {
+ return nil, fmt.Errorf("decoding DState: %w", err)
</file context>
| ) | ||
| } | ||
|
|
||
| count, _, _, err := decoder.DecodeMapHeader() |
There was a problem hiding this comment.
P1: ParseUTxOsStreaming silently returns 0 entries for indefinite-length CBOR maps. The function only checks the count from DecodeMapHeader, which is 0 for indefinite-length maps (0xbf header). Consider checking the indef return value (second return from DecodeMapHeader) and either delegating to parseIndefiniteUTxOMap or returning an explicit error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/utxo.go, line 252:
<comment>`ParseUTxOsStreaming` silently returns 0 entries for indefinite-length CBOR maps. The function only checks the `count` from `DecodeMapHeader`, which is 0 for indefinite-length maps (0xbf header). Consider checking the `indef` return value (second return from `DecodeMapHeader`) and either delegating to `parseIndefiniteUTxOMap` or returning an explicit error.</comment>
<file context>
@@ -0,0 +1,596 @@
+ )
+ }
+
+ count, _, _, err := decoder.DecodeMapHeader()
+ if err != nil {
+ return 0, fmt.Errorf(
</file context>
| remainder := len(flat) - base // = lastNameLen + padding | ||
| lastNameLen := 0 | ||
| if numAssets > 0 && remainder > 0 { | ||
| for pad := range 8 { |
There was a problem hiding this comment.
P1: Bug: last asset name in decodeFlatMultiAsset will include trailing padding bytes. The padding-determination loop at line 530 always matches on the first iteration (pad=0) because for 8-byte-aligned flat buffers, (8 - contentSize%8)%8 == pad is satisfied for every pad value 0–7. This causes lastNameLen to be remainder (= actual name length + padding), so the extracted name will have up to 7 trailing garbage bytes.
Consider an alternative approach: compute the names region start as the minimum nameOff, then compute total names content as len(flat) - namesRegionStart - padding. Or strip trailing zero bytes from the last name, since MemPack padding uses zero fill.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 612:
<comment>Bug: last asset name in `decodeFlatMultiAsset` will include trailing padding bytes. The padding-determination loop at line 530 always matches on the first iteration (`pad=0`) because for 8-byte-aligned `flat` buffers, `(8 - contentSize%8)%8 == pad` is satisfied for every `pad` value 0–7. This causes `lastNameLen` to be `remainder` (= actual name length + padding), so the extracted name will have up to 7 trailing garbage bytes.
Consider an alternative approach: compute the names region start as the minimum `nameOff`, then compute total names content as `len(flat) - namesRegionStart - padding`. Or strip trailing zero bytes from the last name, since MemPack padding uses zero fill.</comment>
<file context>
@@ -0,0 +1,840 @@
+ remainder := len(flat) - base // = lastNameLen + padding
+ lastNameLen := 0
+ if numAssets > 0 && remainder > 0 {
+ for pad := range 8 {
+ candidate := remainder - pad
+ if candidate < 0 {
</file context>
| err, | ||
| ) | ||
| } | ||
| case http.StatusPartialContent: |
There was a problem hiding this comment.
P2: Resume downloads should validate Content-Range before appending. As written, any 206 response is appended without verifying the start offset, which can corrupt the archive if the server ignores the requested range or returns a mismatched offset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mithril/download.go, line 183:
<comment>Resume downloads should validate Content-Range before appending. As written, any 206 response is appended without verifying the start offset, which can corrupt the archive if the server ignores the requested range or returns a mismatched offset.</comment>
<file context>
@@ -0,0 +1,315 @@
+ err,
+ )
+ }
+ case http.StatusPartialContent:
+ // Resume supported
+ if resp.ContentLength > 0 {
</file context>
ledgerstate/import.go
Outdated
| var margin *types.Rat | ||
| if pool.MarginDen > 0 { | ||
| // #nosec G115 | ||
| r := new(big.Rat).SetFrac64( |
There was a problem hiding this comment.
P2: Unsafe uint64 → int64 conversion for SetFrac64: if MarginNum or MarginDen exceeds math.MaxInt64, the resulting big.Rat will be incorrect (negative value). Use new(big.Rat).SetFrac(new(big.Int).SetUint64(...), new(big.Int).SetUint64(...)) to handle the full uint64 range safely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/import.go, line 573:
<comment>Unsafe `uint64` → `int64` conversion for `SetFrac64`: if `MarginNum` or `MarginDen` exceeds `math.MaxInt64`, the resulting `big.Rat` will be incorrect (negative value). Use `new(big.Rat).SetFrac(new(big.Int).SetUint64(...), new(big.Int).SetUint64(...))` to handle the full `uint64` range safely.</comment>
<file context>
@@ -0,0 +1,1345 @@
+ var margin *types.Rat
+ if pool.MarginDen > 0 {
+ // #nosec G115
+ r := new(big.Rat).SetFrac64(
+ int64(pool.MarginNum),
+ int64(pool.MarginDen),
</file context>
ledgerstate/cbor_decode.go
Outdated
| for k := range m { | ||
| keys = append(keys, k) | ||
| } | ||
| slices.Sort(keys) |
There was a problem hiding this comment.
P2: When decoding an integer-keyed map, sparse keys (e.g., {0, 2, 5}) silently produce a compact result where positional indices no longer match key values. Callers like parsePState access elements by index expecting result[i] to correspond to key i. Consider validating that keys are contiguous 0..n-1, or filling gaps with nil entries up to max(key)+1.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/cbor_decode.go, line 435:
<comment>When decoding an integer-keyed map, sparse keys (e.g., `{0, 2, 5}`) silently produce a compact result where positional indices no longer match key values. Callers like `parsePState` access elements by index expecting `result[i]` to correspond to key `i`. Consider validating that keys are contiguous `0..n-1`, or filling gaps with nil entries up to `max(key)+1`.</comment>
<file context>
@@ -0,0 +1,442 @@
+ for k := range m {
+ keys = append(keys, k)
+ }
+ slices.Sort(keys)
+
+ result := make([][]byte, len(keys))
</file context>
ledgerstate/snapshots.go
Outdated
| continue | ||
| } | ||
|
|
||
| params := &ParsedPoolParams{ |
There was a problem hiding this comment.
P2: Redundant struct copy: parsePoolParams already returns *ParsedPoolParams, so copying each field into a new identical struct is unnecessary and fragile — any new fields added to ParsedPoolParams will be silently dropped here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/snapshots.go, line 278:
<comment>Redundant struct copy: `parsePoolParams` already returns `*ParsedPoolParams`, so copying each field into a new identical struct is unnecessary and fragile — any new fields added to `ParsedPoolParams` will be silently dropped here.</comment>
<file context>
@@ -0,0 +1,358 @@
+ continue
+ }
+
+ params := &ParsedPoolParams{
+ PoolKeyHash: pool.PoolKeyHash,
+ VrfKeyHash: pool.VrfKeyHash,
</file context>
mithril/client.go
Outdated
| // so callers should configure their own redirect policy if needed. | ||
| func WithHTTPClient(hc *http.Client) ClientOption { | ||
| return func(c *Client) { | ||
| c.httpClient = hc |
There was a problem hiding this comment.
P2: Guard against a nil custom HTTP client to avoid a nil pointer panic when doGet calls c.httpClient.Do.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mithril/client.go, line 64:
<comment>Guard against a nil custom HTTP client to avoid a nil pointer panic when `doGet` calls `c.httpClient.Do`.</comment>
<file context>
@@ -0,0 +1,254 @@
+// so callers should configure their own redirect policy if needed.
+func WithHTTPClient(hc *http.Client) ClientOption {
+ return func(c *Client) {
+ c.httpClient = hc
+ }
+}
</file context>
2bd061c to
0d655be
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
5 issues found across 54 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="database/plugin/metadata/mysql/import.go">
<violation number="1" location="database/plugin/metadata/mysql/import.go:129">
P1: Missing `OnConflict{DoNothing: true}` on pool registration insert. The SQLite implementation defensively ignores duplicate registrations on retry, but this MySQL version will fail with a duplicate key error if a crash occurs between creating the registration and updating the ImportCheckpoint. Add `clause.OnConflict{DoNothing: true}` for crash-recovery safety.</violation>
</file>
<file name="mithril/download.go">
<violation number="1" location="mithril/download.go:273">
P2: When the restart path replaces `resp.Body` with `resp2.Body`, the original deferred close still targets the old body. The new response body is never closed, which can leak HTTP connections. Add a defer/close for `resp2.Body` after assigning it.</violation>
</file>
<file name="cmd/dingo/mithril.go">
<violation number="1" location="cmd/dingo/mithril.go:1006">
P2: The `seenCbor` counter is reset per transaction but `findNthOccurrence` searches the entire block CBOR. If two transactions in the same block produce outputs with identical CBOR byte patterns, the offset for the second transaction's output will incorrectly point to the first transaction's output.
Consider tracking `seenCbor` at the block level (move the map initialization before the outer `for _, tx := range ...` loop) so that the nth-occurrence counter accounts for all prior matches in the block.</violation>
</file>
<file name="mithril/client.go">
<violation number="1" location="mithril/client.go:64">
P2: Guard against nil in WithHTTPClient; assigning a nil *http.Client will cause a panic when doGet calls c.httpClient.Do.</violation>
</file>
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:651">
P2: Defensive swap in asset name extraction reads the wrong memory region when name offsets are descending. If `entries[i].nameOff > entries[i+1].nameOff`, the swap causes the code to read bytes starting at `entries[i+1].nameOff` (the *next* entry's name region) instead of `entries[i].nameOff` (this entry's actual name). The name length calculation is correct (absolute difference), but the bytes read are wrong. Consider computing the length separately and always reading from `e.nameOff`:
```go
nameLen := entries[i+1].nameOff - e.nameOff
if nameLen < 0 { nameLen = -nameLen }
assetName = flat[e.nameOff : e.nameOff+nameLen]
Alternatively, if offsets are guaranteed ascending, remove the swap entirely to avoid masking bugs.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
|
|
||
| // Create registration record. Idempotency is handled at the | ||
| // application level via ImportCheckpoint, not by DB constraints. | ||
| if result := db.Create(reg); result.Error != nil { |
There was a problem hiding this comment.
P1: Missing OnConflict{DoNothing: true} on pool registration insert. The SQLite implementation defensively ignores duplicate registrations on retry, but this MySQL version will fail with a duplicate key error if a crash occurs between creating the registration and updating the ImportCheckpoint. Add clause.OnConflict{DoNothing: true} for crash-recovery safety.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At database/plugin/metadata/mysql/import.go, line 129:
<comment>Missing `OnConflict{DoNothing: true}` on pool registration insert. The SQLite implementation defensively ignores duplicate registrations on retry, but this MySQL version will fail with a duplicate key error if a crash occurs between creating the registration and updating the ImportCheckpoint. Add `clause.OnConflict{DoNothing: true}` for crash-recovery safety.</comment>
<file context>
@@ -0,0 +1,230 @@
+
+ // Create registration record. Idempotency is handled at the
+ // application level via ImportCheckpoint, not by DB constraints.
+ if result := db.Create(reg); result.Error != nil {
+ return fmt.Errorf(
+ "import pool registration: %w",
</file context>
cmd/dingo/mithril.go
Outdated
| var txHashArr [32]byte | ||
| copy(txHashArr[:], txHash.Bytes()) | ||
| produced := tx.Produced() | ||
| seenCbor := make(map[string]int) |
There was a problem hiding this comment.
P2: The seenCbor counter is reset per transaction but findNthOccurrence searches the entire block CBOR. If two transactions in the same block produce outputs with identical CBOR byte patterns, the offset for the second transaction's output will incorrectly point to the first transaction's output.
Consider tracking seenCbor at the block level (move the map initialization before the outer for _, tx := range ... loop) so that the nth-occurrence counter accounts for all prior matches in the block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/dingo/mithril.go, line 1006:
<comment>The `seenCbor` counter is reset per transaction but `findNthOccurrence` searches the entire block CBOR. If two transactions in the same block produce outputs with identical CBOR byte patterns, the offset for the second transaction's output will incorrectly point to the first transaction's output.
Consider tracking `seenCbor` at the block level (move the map initialization before the outer `for _, tx := range ...` loop) so that the nth-occurrence counter accounts for all prior matches in the block.</comment>
<file context>
@@ -0,0 +1,1129 @@
+ var txHashArr [32]byte
+ copy(txHashArr[:], txHash.Bytes())
+ produced := tx.Produced()
+ seenCbor := make(map[string]int)
+ for _, utxo := range produced {
+ outCbor := utxo.Output.Cbor()
</file context>
mithril/client.go
Outdated
| // so callers should configure their own redirect policy if needed. | ||
| func WithHTTPClient(hc *http.Client) ClientOption { | ||
| return func(c *Client) { | ||
| c.httpClient = hc |
There was a problem hiding this comment.
P2: Guard against nil in WithHTTPClient; assigning a nil *http.Client will cause a panic when doGet calls c.httpClient.Do.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mithril/client.go, line 64:
<comment>Guard against nil in WithHTTPClient; assigning a nil *http.Client will cause a panic when doGet calls c.httpClient.Do.</comment>
<file context>
@@ -0,0 +1,254 @@
+// so callers should configure their own redirect policy if needed.
+func WithHTTPClient(hc *http.Client) ClientOption {
+ return func(c *Client) {
+ c.httpClient = hc
+ }
+}
</file context>
| // Names are sorted descending. Adjacent entries' | ||
| // offsets bound each name. | ||
| var assetName []byte | ||
| if i+1 < numAssets { |
There was a problem hiding this comment.
P2: Defensive swap in asset name extraction reads the wrong memory region when name offsets are descending. If entries[i].nameOff > entries[i+1].nameOff, the swap causes the code to read bytes starting at entries[i+1].nameOff (the next entry's name region) instead of entries[i].nameOff (this entry's actual name). The name length calculation is correct (absolute difference), but the bytes read are wrong. Consider computing the length separately and always reading from e.nameOff:
nameLen := entries[i+1].nameOff - e.nameOff
if nameLen < 0 { nameLen = -nameLen }
assetName = flat[e.nameOff : e.nameOff+nameLen]Alternatively, if offsets are guaranteed ascending, remove the swap entirely to avoid masking bugs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 651:
<comment>Defensive swap in asset name extraction reads the wrong memory region when name offsets are descending. If `entries[i].nameOff > entries[i+1].nameOff`, the swap causes the code to read bytes starting at `entries[i+1].nameOff` (the *next* entry's name region) instead of `entries[i].nameOff` (this entry's actual name). The name length calculation is correct (absolute difference), but the bytes read are wrong. Consider computing the length separately and always reading from `e.nameOff`:
```go
nameLen := entries[i+1].nameOff - e.nameOff
if nameLen < 0 { nameLen = -nameLen }
assetName = flat[e.nameOff : e.nameOff+nameLen]
Alternatively, if offsets are guaranteed ascending, remove the swap entirely to avoid masking bugs.
@@ -0,0 +1,840 @@ + // Names are sorted descending. Adjacent entries' + // offsets bound each name. + var assetName []byte + if i+1 < numAssets { + nameStart := e.nameOff + nameEnd := entries[i+1].nameOff ```
wolf31o2
force-pushed
the
feat/mithril-bootstrap
branch
from
0d655be to
0e05ac7
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
7 issues found across 54 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:660">
P2: Inconsistent bounds validation: out-of-bounds asset name offsets are silently ignored (producing nil names), while out-of-bounds policy ID offsets correctly return an error. For data integrity, both should return an error on invalid offsets to avoid silently importing assets with missing names from corrupted snapshots.</violation>
</file>
<file name="ledgerstate/certstate.go">
<violation number="1" location="ledgerstate/certstate.go:218">
P2: `parsePStateConway` returns the first map element >100 bytes instead of finding the largest map element, inconsistent with the pattern used in `parseCertStateConway`. If the PState array contains multiple sizable map elements (pool params, retiring pools map, deposits map), this could select the wrong one. Consider tracking the largest map and returning it after the loop, matching the `parseCertStateConway` approach.</violation>
</file>
<file name="ledgerstate/snapshots.go">
<violation number="1" location="ledgerstate/snapshots.go:316">
P2: Delegator count increments even when a credential has no stake entry, which can overcount delegators relative to the aggregated stake. Only count delegators when the stake entry exists.</violation>
</file>
<file name="ledgerstate/govstate.go">
<violation number="1" location="ledgerstate/govstate.go:191">
P2: Validate constitution anchor hash length before assigning it; otherwise malformed hashes (not 32 bytes) are accepted as valid state.</violation>
</file>
<file name="ledgerstate/utxo.go">
<violation number="1" location="ledgerstate/utxo.go:429">
P2: The CBOR TxOut path does not extract inline `Datum` or `ScriptRef` from the transaction output, unlike the MemPack path which populates both. Babbage+ era UTxOs with inline datums or reference scripts will have those `ParsedUTxO` fields silently empty when parsed through this code path.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:272">
P2: `importAccounts` loads all accounts in a single transaction without batching. On mainnet (~1.5M+ accounts), this can cause excessive memory usage, long lock durations, or transaction timeouts. Unlike `importUTxOs` which commits in batches, a failure here loses all progress. Consider adding batch commits (e.g., every 10k–50k accounts) consistent with the UTxO import pattern.</violation>
<violation number="2" location="ledgerstate/import.go:1339">
P2: `isEpochDuplicate` uses overly broad MySQL/PostgreSQL error string matching. The patterns `"Duplicate entry"` (MySQL) and `"duplicate key value violates unique constraint"` (PostgreSQL) match duplicate key errors from *any* table, not just the epoch table. This could silently swallow real constraint violations from other tables if `SetEpoch` has side effects. Consider narrowing the match to include the table/constraint name (e.g., `"Duplicate entry" ... "epoch"` for MySQL, and including the constraint name for PostgreSQL).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ledgerstate/mempack.go
Outdated
| if i+1 < numAssets { | ||
| nameLen := entries[i+1].nameOff - e.nameOff | ||
| nameEnd := e.nameOff + nameLen | ||
| if nameLen > 0 && |
There was a problem hiding this comment.
P2: Inconsistent bounds validation: out-of-bounds asset name offsets are silently ignored (producing nil names), while out-of-bounds policy ID offsets correctly return an error. For data integrity, both should return an error on invalid offsets to avoid silently importing assets with missing names from corrupted snapshots.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 660:
<comment>Inconsistent bounds validation: out-of-bounds asset name offsets are silently ignored (producing nil names), while out-of-bounds policy ID offsets correctly return an error. For data integrity, both should return an error on invalid offsets to avoid silently importing assets with missing names from corrupted snapshots.</comment>
<file context>
@@ -0,0 +1,840 @@
+ if i+1 < numAssets {
+ nameLen := entries[i+1].nameOff - e.nameOff
+ nameEnd := e.nameOff + nameLen
+ if nameLen > 0 &&
+ e.nameOff <= len(flat) &&
+ nameEnd <= len(flat) {
</file context>
ledgerstate/certstate.go
Outdated
| // Pool params maps are always significantly larger than | ||
| // the other PState fields (counters, small arrays). | ||
| const minPoolParamsMapSize = 100 | ||
| if isMap && len(elem) > minPoolParamsMapSize { |
There was a problem hiding this comment.
P2: parsePStateConway returns the first map element >100 bytes instead of finding the largest map element, inconsistent with the pattern used in parseCertStateConway. If the PState array contains multiple sizable map elements (pool params, retiring pools map, deposits map), this could select the wrong one. Consider tracking the largest map and returning it after the loop, matching the parseCertStateConway approach.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/certstate.go, line 218:
<comment>`parsePStateConway` returns the first map element >100 bytes instead of finding the largest map element, inconsistent with the pattern used in `parseCertStateConway`. If the PState array contains multiple sizable map elements (pool params, retiring pools map, deposits map), this could select the wrong one. Consider tracking the largest map and returning it after the loop, matching the `parseCertStateConway` approach.</comment>
<file context>
@@ -0,0 +1,860 @@
+ // Pool params maps are always significantly larger than
+ // the other PState fields (counters, small arrays).
+ const minPoolParamsMapSize = 100
+ if isMap && len(elem) > minPoolParamsMapSize {
+ // This is the pool params map
+ return parseCertPoolParamsMap(elem)
</file context>
| } | ||
|
|
||
| // Add this credential's stake to the pool total | ||
| if stake, ok := snap.Stake[credHex]; ok { |
There was a problem hiding this comment.
P2: Delegator count increments even when a credential has no stake entry, which can overcount delegators relative to the aggregated stake. Only count delegators when the stake entry exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/snapshots.go, line 316:
<comment>Delegator count increments even when a credential has no stake entry, which can overcount delegators relative to the aggregated stake. Only count delegators when the stake entry exists.</comment>
<file context>
@@ -0,0 +1,347 @@
+ }
+
+ // Add this credential's stake to the pool total
+ if stake, ok := snap.Stake[credHex]; ok {
+ agg.totalStake += stake
+ }
</file context>
| "decoding constitution hash: %w", err, | ||
| ) | ||
| } | ||
| c.AnchorHash = hash |
There was a problem hiding this comment.
P2: Validate constitution anchor hash length before assigning it; otherwise malformed hashes (not 32 bytes) are accepted as valid state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/govstate.go, line 191:
<comment>Validate constitution anchor hash length before assigning it; otherwise malformed hashes (not 32 bytes) are accepted as valid state.</comment>
<file context>
@@ -0,0 +1,506 @@
+ "decoding constitution hash: %w", err,
+ )
+ }
+ c.AnchorHash = hash
+
+ // Decode optional script hash (null or bytes(28)).
</file context>
ledgerstate/utxo.go
Outdated
| result.StakingKey = skh.Bytes() | ||
| } | ||
|
|
||
| if datumOutput, ok := txOut.(interface { |
There was a problem hiding this comment.
P2: The CBOR TxOut path does not extract inline Datum or ScriptRef from the transaction output, unlike the MemPack path which populates both. Babbage+ era UTxOs with inline datums or reference scripts will have those ParsedUTxO fields silently empty when parsed through this code path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/utxo.go, line 429:
<comment>The CBOR TxOut path does not extract inline `Datum` or `ScriptRef` from the transaction output, unlike the MemPack path which populates both. Babbage+ era UTxOs with inline datums or reference scripts will have those `ParsedUTxO` fields silently empty when parsed through this code path.</comment>
<file context>
@@ -0,0 +1,604 @@
+ result.StakingKey = skh.Bytes()
+ }
+
+ if datumOutput, ok := txOut.(interface {
+ DatumHash() *lcommon.Blake2b256
+ }); ok {
</file context>
| return nil | ||
| } | ||
| store := cfg.Database.Metadata() | ||
| txn := cfg.Database.MetadataTxn(true) |
There was a problem hiding this comment.
P2: importAccounts loads all accounts in a single transaction without batching. On mainnet (~1.5M+ accounts), this can cause excessive memory usage, long lock durations, or transaction timeouts. Unlike importUTxOs which commits in batches, a failure here loses all progress. Consider adding batch commits (e.g., every 10k–50k accounts) consistent with the UTxO import pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/import.go, line 272:
<comment>`importAccounts` loads all accounts in a single transaction without batching. On mainnet (~1.5M+ accounts), this can cause excessive memory usage, long lock durations, or transaction timeouts. Unlike `importUTxOs` which commits in batches, a failure here loses all progress. Consider adding batch commits (e.g., every 10k–50k accounts) consistent with the UTxO import pattern.</comment>
<file context>
@@ -0,0 +1,1344 @@
+ return nil
+ }
+ store := cfg.Database.Metadata()
+ txn := cfg.Database.MetadataTxn(true)
+ defer txn.Release()
+
</file context>
ledgerstate/import.go
Outdated
| return strings.Contains( | ||
| msg, "UNIQUE constraint failed: epoch", | ||
| ) || | ||
| strings.Contains(msg, "Duplicate entry") || |
There was a problem hiding this comment.
P2: isEpochDuplicate uses overly broad MySQL/PostgreSQL error string matching. The patterns "Duplicate entry" (MySQL) and "duplicate key value violates unique constraint" (PostgreSQL) match duplicate key errors from any table, not just the epoch table. This could silently swallow real constraint violations from other tables if SetEpoch has side effects. Consider narrowing the match to include the table/constraint name (e.g., "Duplicate entry" ... "epoch" for MySQL, and including the constraint name for PostgreSQL).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/import.go, line 1339:
<comment>`isEpochDuplicate` uses overly broad MySQL/PostgreSQL error string matching. The patterns `"Duplicate entry"` (MySQL) and `"duplicate key value violates unique constraint"` (PostgreSQL) match duplicate key errors from *any* table, not just the epoch table. This could silently swallow real constraint violations from other tables if `SetEpoch` has side effects. Consider narrowing the match to include the table/constraint name (e.g., `"Duplicate entry" ... "epoch"` for MySQL, and including the constraint name for PostgreSQL).</comment>
<file context>
@@ -0,0 +1,1344 @@
+ return strings.Contains(
+ msg, "UNIQUE constraint failed: epoch",
+ ) ||
+ strings.Contains(msg, "Duplicate entry") ||
+ strings.Contains(
+ msg,
</file context>
0ac5bbd to
47c341b
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
6 issues found across 54 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ledgerstate/certstate.go">
<violation number="1" location="ledgerstate/certstate.go:356">
P2: When the UMElem value fails to decode, the account is silently added with zeroed reward/delegation data (Reward=0, nil PoolKeyHash, nil DRepCred) instead of being tracked as a partial parse. This could mask data quality issues during import — accounts may appear to have zero rewards and no delegation when the actual data was simply unparseable. Consider incrementing `skipped` (or a separate counter) and including it in the warning so operators can diagnose import problems.</violation>
</file>
<file name="ledgerstate/snapshot.go">
<violation number="1" location="ledgerstate/snapshot.go:700">
P2: PraosState version peeling doesn't handle the "flat" format. The ShelleyLedgerState peeling loop (line ~380) handles both wrapped (`[version, [inner...]]`) and flat (`[version, field1, field2, ...]`) formats, but the PraosState loop here only handles the wrapped case (`len == 2`). If a flat-versioned PraosState is encountered, the version element won't be stripped, and `praosState[4]` will read the wrong field, returning an incorrect epoch nonce.
Consider mirroring the ShelleyLedgerState pattern to also handle flat version prefixes.</violation>
</file>
<file name="ledgerstate/verify.go">
<violation number="1" location="ledgerstate/verify.go:48">
P3: `VerifySnapshotDigest` is unused outside of tests, so it adds dead code to the production build. If this helper isn’t part of the intended public API yet, consider removing it or wiring it into the bootstrap flow.</violation>
<violation number="2" location="ledgerstate/verify.go:83">
P3: `VerifyChecksumFile` is unused outside tests, so it’s dead code in the production build. If it isn’t meant to be part of the public API yet, consider removing it or integrating it into the ledger import path.</violation>
</file>
<file name="mithril/download.go">
<violation number="1" location="mithril/download.go:303">
P2: When the server responds 416, the code treats the local file as complete unless `ExpectedSize` is provided. If the local file is larger than the remote snapshot, this path will incorrectly accept a corrupted file. Use the `Content-Range` total (or fail) to verify the local size even when `ExpectedSize` is unset.</violation>
</file>
<file name="cmd/dingo/serve.go">
<violation number="1" location="cmd/dingo/serve.go:57">
P2: Do not ignore all database initialization errors here. Returning nil masks real storage/plugin failures and disables the sync-state guard when the database is misconfigured or corrupted. Propagate the error so `serve` fails fast and reports the root cause.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| // UMElem = [RDPair, StakePool, DRep, ...] | ||
| var elem []cbor.RawMessage | ||
| if _, err := cbor.Decode( |
There was a problem hiding this comment.
P2: When the UMElem value fails to decode, the account is silently added with zeroed reward/delegation data (Reward=0, nil PoolKeyHash, nil DRepCred) instead of being tracked as a partial parse. This could mask data quality issues during import — accounts may appear to have zero rewards and no delegation when the actual data was simply unparseable. Consider incrementing skipped (or a separate counter) and including it in the warning so operators can diagnose import problems.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/certstate.go, line 356:
<comment>When the UMElem value fails to decode, the account is silently added with zeroed reward/delegation data (Reward=0, nil PoolKeyHash, nil DRepCred) instead of being tracked as a partial parse. This could mask data quality issues during import — accounts may appear to have zero rewards and no delegation when the actual data was simply unparseable. Consider incrementing `skipped` (or a separate counter) and including it in the warning so operators can diagnose import problems.</comment>
<file context>
@@ -0,0 +1,864 @@
+
+ // UMElem = [RDPair, StakePool, DRep, ...]
+ var elem []cbor.RawMessage
+ if _, err := cbor.Decode(
+ entry.ValueRaw, &elem,
+ ); err != nil {
</file context>
ledgerstate/snapshot.go
Outdated
| } | ||
|
|
||
| // Peel version wrapper(s) | ||
| for len(praosState) == 2 { |
There was a problem hiding this comment.
P2: PraosState version peeling doesn't handle the "flat" format. The ShelleyLedgerState peeling loop (line ~380) handles both wrapped ([version, [inner...]]) and flat ([version, field1, field2, ...]) formats, but the PraosState loop here only handles the wrapped case (len == 2). If a flat-versioned PraosState is encountered, the version element won't be stripped, and praosState[4] will read the wrong field, returning an incorrect epoch nonce.
Consider mirroring the ShelleyLedgerState pattern to also handle flat version prefixes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/snapshot.go, line 700:
<comment>PraosState version peeling doesn't handle the "flat" format. The ShelleyLedgerState peeling loop (line ~380) handles both wrapped (`[version, [inner...]]`) and flat (`[version, field1, field2, ...]`) formats, but the PraosState loop here only handles the wrapped case (`len == 2`). If a flat-versioned PraosState is encountered, the version element won't be stripped, and `praosState[4]` will read the wrong field, returning an incorrect epoch nonce.
Consider mirroring the ShelleyLedgerState pattern to also handle flat version prefixes.</comment>
<file context>
@@ -0,0 +1,756 @@
+ }
+
+ // Peel version wrapper(s)
+ for len(praosState) == 2 {
+ var ver uint64
+ if _, decErr := cbor.Decode(
</file context>
mithril/download.go
Outdated
| case http.StatusRequestedRangeNotSatisfiable: | ||
| // The file is already complete — verify size before | ||
| // accepting. | ||
| if cfg.ExpectedSize > 0 { |
There was a problem hiding this comment.
P2: When the server responds 416, the code treats the local file as complete unless ExpectedSize is provided. If the local file is larger than the remote snapshot, this path will incorrectly accept a corrupted file. Use the Content-Range total (or fail) to verify the local size even when ExpectedSize is unset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mithril/download.go, line 303:
<comment>When the server responds 416, the code treats the local file as complete unless `ExpectedSize` is provided. If the local file is larger than the remote snapshot, this path will incorrectly accept a corrupted file. Use the `Content-Range` total (or fail) to verify the local size even when `ExpectedSize` is unset.</comment>
<file context>
@@ -0,0 +1,410 @@
+ case http.StatusRequestedRangeNotSatisfiable:
+ // The file is already complete — verify size before
+ // accepting.
+ if cfg.ExpectedSize > 0 {
+ fi, err := os.Stat(destPath)
+ if err != nil {
</file context>
|
|
||
| // VerifyChecksumFile verifies the CRC32 checksum of a ledger state | ||
| // file against its companion .checksum file. | ||
| func VerifyChecksumFile(lstatePath string) error { |
There was a problem hiding this comment.
P3: VerifyChecksumFile is unused outside tests, so it’s dead code in the production build. If it isn’t meant to be part of the public API yet, consider removing it or integrating it into the ledger import path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/verify.go, line 83:
<comment>`VerifyChecksumFile` is unused outside tests, so it’s dead code in the production build. If it isn’t meant to be part of the public API yet, consider removing it or integrating it into the ledger import path.</comment>
<file context>
@@ -0,0 +1,136 @@
+
+// VerifyChecksumFile verifies the CRC32 checksum of a ledger state
+// file against its companion .checksum file.
+func VerifyChecksumFile(lstatePath string) error {
+ checksumPath := lstatePath + ".checksum"
+
</file context>
| // VerifySnapshotDigest computes the SHA-256 digest of a snapshot | ||
| // archive file and compares it against the expected digest from the | ||
| // Mithril aggregator. | ||
| func VerifySnapshotDigest( |
There was a problem hiding this comment.
P3: VerifySnapshotDigest is unused outside of tests, so it adds dead code to the production build. If this helper isn’t part of the intended public API yet, consider removing it or wiring it into the bootstrap flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/verify.go, line 48:
<comment>`VerifySnapshotDigest` is unused outside of tests, so it adds dead code to the production build. If this helper isn’t part of the intended public API yet, consider removing it or wiring it into the bootstrap flow.</comment>
<file context>
@@ -0,0 +1,136 @@
+// VerifySnapshotDigest computes the SHA-256 digest of a snapshot
+// archive file and compares it against the expected digest from the
+// Mithril aggregator.
+func VerifySnapshotDigest(
+ archivePath string,
+ expectedDigest string,
</file context>
47c341b to
1bf3e57
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
9 issues found across 54 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ledgerstate/mempack.go">
<violation number="1" location="ledgerstate/mempack.go:616">
P2: Silent fallback when padding alignment calculation fails: if no valid padding matches (e.g., truncated/corrupt flat buffer where `remainder < 0` or no alignment found), `lastNameLen` stays 0 and the last asset's name is silently empty. Consider returning an error when no valid padding is found, to surface data corruption rather than importing incorrect asset names.</violation>
</file>
<file name="ledgerstate/import.go">
<violation number="1" location="ledgerstate/import.go:1355">
P2: The `isEpochDuplicate` error detection uses loose substring matching that could match UNIQUE constraint violations from unrelated tables (e.g., `epoch_summaries`). The SQLite pattern `"UNIQUE constraint failed: epoch"` would match any table starting with `epoch`, and the MySQL pattern `"Duplicate entry"` matches any table. Consider tightening the match to the specific table name (e.g., `"UNIQUE constraint failed: epochs."`) or using typed error code checks from the database drivers.</violation>
</file>
<file name="ledgerstate/govstate.go">
<violation number="1" location="ledgerstate/govstate.go:26">
P3: Go naming convention: `AnchorUrl` should be `AnchorURL`. Per Go's [CodeReviewComments](https://go.dev/wiki/CodeReviewComments#initialisms), well-known initialisms like URL should be all-caps in exported identifiers. This applies to both `ParsedConstitution.AnchorUrl` and `ParsedGovProposal.AnchorUrl`. The project's own linting setup (`golangci-lint` / `stylecheck` ST1003) would typically flag this.</violation>
</file>
<file name="ledgerstate/snapshot.go">
<violation number="1" location="ledgerstate/snapshot.go:377">
P2: Inconsistent error handling: PraosState version-peeling silently `break`s on decode failure instead of returning an error like the identical ShelleyLedgerState loop does. If the wrapped inner data is corrupt, this masks the real error and produces a confusing 'PraosState has 2 elements, expected at least 5' message downstream.</violation>
</file>
<file name="database/immutable/immutable.go">
<violation number="1" location="database/immutable/immutable.go:87">
P1: getChunkNamesFromPoint can now return an empty slice when point.Slot is beyond the last chunk, which will panic in GetBlock (chunkNames[0]). Add a guard when lowerBound >= len(chunkNames) to return a proper error instead of an empty slice.</violation>
</file>
<file name="ledgerstate/utxo.go">
<violation number="1" location="ledgerstate/utxo.go:580">
P2: `UTxOToModel` does not map `DatumHash`, `Datum`, or `ScriptRef` from `ParsedUTxO` to the model. These fields are populated by both `parseMempackTxOut` and `parseCborTxOut`. If the `models.Utxo` struct supports these fields, they should be set here; otherwise, there's no value in parsing them upstream.</violation>
</file>
<file name="cmd/dingo/mithril.go">
<violation number="1" location="cmd/dingo/mithril.go:347">
P2: `LoadBlobsWithDB` is run inside an `errgroup` but doesn't receive the errgroup's derived context (`gctx`). If the parallel `importLedgerState` goroutine fails, the blob loading will continue running since it can't observe the context cancellation. Consider propagating the context to enable cooperative cancellation.</violation>
<violation number="2" location="cmd/dingo/mithril.go:883">
P2: `postProcessUtxoOffsets` does not accept a `context.Context` and cannot be cancelled. This function iterates over all ImmutableDB blocks (millions on mainnet) without any cancellation check. Add a `ctx context.Context` parameter and check `ctx.Err()` periodically (e.g., each batch commit) to allow graceful shutdown.</violation>
</file>
<file name="chain/chain.go">
<violation number="1" location="chain/chain.go:411">
P2: Bug: `waitingChan` iterator notification is skipped when `eventBus` is nil. The condition `if len(pendingEvents) > 0` ties iterator notification to event generation, but events are only created when `eventBus != nil`. If `eventBus` is nil, blocks are added but waiting `ChainIterator`s are never woken up. The notification should be unconditional after a successful commit, matching the pattern in `addBlockLocked`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| numUniquePolicies := len(uniquePidOffs) | ||
| base := nameOffsetsEnd + numUniquePolicies*28 + knownNameBytes | ||
| remainder := len(flat) - base // = lastNameLen + padding | ||
| lastNameLen := 0 |
There was a problem hiding this comment.
P2: Silent fallback when padding alignment calculation fails: if no valid padding matches (e.g., truncated/corrupt flat buffer where remainder < 0 or no alignment found), lastNameLen stays 0 and the last asset's name is silently empty. Consider returning an error when no valid padding is found, to surface data corruption rather than importing incorrect asset names.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/mempack.go, line 616:
<comment>Silent fallback when padding alignment calculation fails: if no valid padding matches (e.g., truncated/corrupt flat buffer where `remainder < 0` or no alignment found), `lastNameLen` stays 0 and the last asset's name is silently empty. Consider returning an error when no valid padding is found, to surface data corruption rather than importing incorrect asset names.</comment>
<file context>
@@ -0,0 +1,854 @@
+ numUniquePolicies := len(uniquePidOffs)
+ base := nameOffsetsEnd + numUniquePolicies*28 + knownNameBytes
+ remainder := len(flat) - base // = lastNameLen + padding
+ lastNameLen := 0
+ if numAssets > 0 && remainder > 0 {
+ for pad := range 8 {
</file context>
ledgerstate/import.go
Outdated
| } | ||
| msg := err.Error() | ||
| return strings.Contains( | ||
| msg, "UNIQUE constraint failed: epoch", |
There was a problem hiding this comment.
P2: The isEpochDuplicate error detection uses loose substring matching that could match UNIQUE constraint violations from unrelated tables (e.g., epoch_summaries). The SQLite pattern "UNIQUE constraint failed: epoch" would match any table starting with epoch, and the MySQL pattern "Duplicate entry" matches any table. Consider tightening the match to the specific table name (e.g., "UNIQUE constraint failed: epochs.") or using typed error code checks from the database drivers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/import.go, line 1355:
<comment>The `isEpochDuplicate` error detection uses loose substring matching that could match UNIQUE constraint violations from unrelated tables (e.g., `epoch_summaries`). The SQLite pattern `"UNIQUE constraint failed: epoch"` would match any table starting with `epoch`, and the MySQL pattern `"Duplicate entry"` matches any table. Consider tightening the match to the specific table name (e.g., `"UNIQUE constraint failed: epochs."`) or using typed error code checks from the database drivers.</comment>
<file context>
@@ -0,0 +1,1362 @@
+ }
+ msg := err.Error()
+ return strings.Contains(
+ msg, "UNIQUE constraint failed: epoch",
+ ) ||
+ strings.Contains(msg, "Duplicate entry") ||
</file context>
| inner, innerErr := decodeRawArray( | ||
| shelleyState[1], | ||
| ) | ||
| if innerErr != nil { |
There was a problem hiding this comment.
P2: Inconsistent error handling: PraosState version-peeling silently breaks on decode failure instead of returning an error like the identical ShelleyLedgerState loop does. If the wrapped inner data is corrupt, this masks the real error and produces a confusing 'PraosState has 2 elements, expected at least 5' message downstream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/snapshot.go, line 377:
<comment>Inconsistent error handling: PraosState version-peeling silently `break`s on decode failure instead of returning an error like the identical ShelleyLedgerState loop does. If the wrapped inner data is corrupt, this masks the real error and produces a confusing 'PraosState has 2 elements, expected at least 5' message downstream.</comment>
<file context>
@@ -0,0 +1,763 @@
+ inner, innerErr := decodeRawArray(
+ shelleyState[1],
+ )
+ if innerErr != nil {
+ return nil, fmt.Errorf(
+ "decoding ShelleyLedgerState "+
</file context>
|
|
||
| // UTxOToModel converts a ParsedUTxO to a Dingo database Utxo model. | ||
| func UTxOToModel(u *ParsedUTxO, slot uint64) models.Utxo { | ||
| utxo := models.Utxo{ |
There was a problem hiding this comment.
P2: UTxOToModel does not map DatumHash, Datum, or ScriptRef from ParsedUTxO to the model. These fields are populated by both parseMempackTxOut and parseCborTxOut. If the models.Utxo struct supports these fields, they should be set here; otherwise, there's no value in parsing them upstream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/utxo.go, line 580:
<comment>`UTxOToModel` does not map `DatumHash`, `Datum`, or `ScriptRef` from `ParsedUTxO` to the model. These fields are populated by both `parseMempackTxOut` and `parseCborTxOut`. If the `models.Utxo` struct supports these fields, they should be set here; otherwise, there's no value in parsing them upstream.</comment>
<file context>
@@ -0,0 +1,602 @@
+
+// UTxOToModel converts a ParsedUTxO to a Dingo database Utxo model.
+func UTxOToModel(u *ParsedUTxO, slot uint64) models.Utxo {
+ utxo := models.Utxo{
+ TxId: u.TxHash,
+ OutputIdx: u.OutputIndex,
</file context>
| "immutable_dir", result.ImmutableDir, | ||
| ) | ||
| var loadErr error | ||
| loadResult, loadErr = node.LoadBlobsWithDB( |
There was a problem hiding this comment.
P2: LoadBlobsWithDB is run inside an errgroup but doesn't receive the errgroup's derived context (gctx). If the parallel importLedgerState goroutine fails, the blob loading will continue running since it can't observe the context cancellation. Consider propagating the context to enable cooperative cancellation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/dingo/mithril.go, line 347:
<comment>`LoadBlobsWithDB` is run inside an `errgroup` but doesn't receive the errgroup's derived context (`gctx`). If the parallel `importLedgerState` goroutine fails, the blob loading will continue running since it can't observe the context cancellation. Consider propagating the context to enable cooperative cancellation.</comment>
<file context>
@@ -0,0 +1,1129 @@
+ "immutable_dir", result.ImmutableDir,
+ )
+ var loadErr error
+ loadResult, loadErr = node.LoadBlobsWithDB(
+ cfg, logger, result.ImmutableDir, db,
+ )
</file context>
| // | ||
| // Fallback (Byron and other eras): parses the full block via | ||
| // NewBlockFromCbor and locates output CBOR via byte search. | ||
| func postProcessUtxoOffsets( |
There was a problem hiding this comment.
P2: postProcessUtxoOffsets does not accept a context.Context and cannot be cancelled. This function iterates over all ImmutableDB blocks (millions on mainnet) without any cancellation check. Add a ctx context.Context parameter and check ctx.Err() periodically (e.g., each batch commit) to allow graceful shutdown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/dingo/mithril.go, line 883:
<comment>`postProcessUtxoOffsets` does not accept a `context.Context` and cannot be cancelled. This function iterates over all ImmutableDB blocks (millions on mainnet) without any cancellation check. Add a `ctx context.Context` parameter and check `ctx.Err()` periodically (e.g., each batch commit) to allow graceful shutdown.</comment>
<file context>
@@ -0,0 +1,1129 @@
+//
+// Fallback (Byron and other eras): parses the full block via
+// NewBlockFromCbor and locates output CBOR via byte search.
+func postProcessUtxoOffsets(
+ db *database.Database,
+ logger *slog.Logger,
</file context>
chain/chain.go
Outdated
| } | ||
| // Notify waiting iterators and publish events only after | ||
| // the transaction has committed successfully. | ||
| if len(pendingEvents) > 0 { |
There was a problem hiding this comment.
P2: Bug: waitingChan iterator notification is skipped when eventBus is nil. The condition if len(pendingEvents) > 0 ties iterator notification to event generation, but events are only created when eventBus != nil. If eventBus is nil, blocks are added but waiting ChainIterators are never woken up. The notification should be unconditional after a successful commit, matching the pattern in addBlockLocked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At chain/chain.go, line 411:
<comment>Bug: `waitingChan` iterator notification is skipped when `eventBus` is nil. The condition `if len(pendingEvents) > 0` ties iterator notification to event generation, but events are only created when `eventBus != nil`. If `eventBus` is nil, blocks are added but waiting `ChainIterator`s are never woken up. The notification should be unconditional after a successful commit, matching the pattern in `addBlockLocked`.</comment>
<file context>
@@ -277,6 +277,155 @@ func (c *Chain) AddBlocks(blocks []ledger.Block) error {
+ }
+ // Notify waiting iterators and publish events only after
+ // the transaction has committed successfully.
+ if len(pendingEvents) > 0 {
+ c.waitingChanMutex.Lock()
+ if c.waitingChan != nil {
</file context>
|
|
||
| // ParsedConstitution holds decoded constitution data. | ||
| type ParsedConstitution struct { | ||
| AnchorUrl string |
There was a problem hiding this comment.
P3: Go naming convention: AnchorUrl should be AnchorURL. Per Go's CodeReviewComments, well-known initialisms like URL should be all-caps in exported identifiers. This applies to both ParsedConstitution.AnchorUrl and ParsedGovProposal.AnchorUrl. The project's own linting setup (golangci-lint / stylecheck ST1003) would typically flag this.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ledgerstate/govstate.go, line 26:
<comment>Go naming convention: `AnchorUrl` should be `AnchorURL`. Per Go's [CodeReviewComments](https://go.dev/wiki/CodeReviewComments#initialisms), well-known initialisms like URL should be all-caps in exported identifiers. This applies to both `ParsedConstitution.AnchorUrl` and `ParsedGovProposal.AnchorUrl`. The project's own linting setup (`golangci-lint` / `stylecheck` ST1003) would typically flag this.</comment>
<file context>
@@ -0,0 +1,513 @@
+
+// ParsedConstitution holds decoded constitution data.
+type ParsedConstitution struct {
+ AnchorUrl string
+ AnchorHash []byte // 32 bytes
+ PolicyHash []byte // 28 bytes, nil if no guardrails script
</file context>
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feat/mithril-bootstrap
branch
from
1bf3e57 to
55fc5d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (21)
mithril/types.go (1)
31-65: Consider extracting a shared base type forSnapshotListItemandSnapshot.These two structs have identical fields. If the Mithril API ever adds detail-only fields to
Snapshot, a shared embedded struct would reduce duplication while still allowing divergence.type snapshotBase struct { Digest string `json:"digest"` Network string `json:"network"` // ... shared fields } type SnapshotListItem struct { snapshotBase } type Snapshot struct { snapshotBase }Not blocking — they may intentionally diverge as the API matures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mithril/types.go` around lines 31 - 65, Both SnapshotListItem and Snapshot duplicate the same fields; extract a shared embedded struct (e.g., snapshotBase) containing the common fields and replace the two concrete types to embed that base (retain Beacon, CreatedAt parsing method by moving or keeping CreatedAt on snapshotBase so SnapshotListItem.CreatedAtTime() continues to work). Update type declarations SnapshotListItem and Snapshot to embed snapshotBase and adjust any code references to use the embedded fields (Digest, Network, CreatedAt, etc.) so behavior is unchanged.mithril/bootstrap.go (2)
646-667:hasFileRecursiveonly checks one level of subdirectories, not truly recursive.The function name suggests arbitrary depth, but it only checks the immediate directory and its direct children. This is fine for the known UTxO-HD layout (
ledger/<slot>/state), but the name is slightly misleading.♻️ Consider renaming
-func hasFileRecursive(dir string, name string) bool { +func hasFileShallow(dir string, name string) bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mithril/bootstrap.go` around lines 646 - 667, The function hasFileRecursive is misleading because it only checks dir and one level of subdirectories; rename hasFileRecursive to hasFileInDirOrImmediateSubdirs (or an equivalent name) and update all callers to use the new name, or alternatively implement true recursion inside hasFileRecursive by recursing into subdirectories (update function signature and tests accordingly) so the name matches behavior; target the function named hasFileRecursive and any callers referencing it.
236-283: Data race concern in ancillary goroutine — but safe due to WaitGroup synchronization.
ancillaryDirandancillaryArchivePathare written from the goroutine (lines 255–256, 281–282) and read afterancWg.Wait()(line 317). The happens-before guarantee fromWaitGroup.Wait()makes this safe, but the pattern is subtle. A brief comment noting this would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mithril/bootstrap.go` around lines 236 - 283, The writes to ancillaryDir and ancillaryArchivePath happen inside the ancillary goroutine and are read after ancWg.Wait(); add a short comment near the ancWg variable/goroutine (referencing ancWg, ancWg.Wait(), ancillaryDir, ancillaryArchivePath, and the anonymous goroutine) stating that the WaitGroup.Wait provides the necessary happens-before synchronization so those assignments are safe and visible to the reader; keep the comment brief and located where ancWg is declared or the goroutine is launched.internal/node/load.go (2)
177-207: Consider making the 30-minute timeout configurable or at least a named constant.The 30-minute ledger catch-up timeout is hardcoded. For large mainnet snapshots this may be tight; for tests it's unnecessarily long. Extracting it as a constant (or a
Configfield with a default) would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/node/load.go` around lines 177 - 207, The hardcoded 30-minute timeout in the ledger catch-up loop should be made configurable: replace the literal time.After(30 * time.Minute) with a named constant or a Config field (e.g., LedgerCatchupTimeout) and use that value when creating timeout; update the surrounding logic in the function that calls copyBlocks / uses immutableTipSlot and ls.Tip to read the default from a package-level constant if no config provided, and ensure the new symbol (e.g., ledgerCatchupTimeout or cfg.LedgerCatchupTimeout) is used in the timeout channel creation and documented where defaults are set.
221-257:LoadBlobsWithDBacceptsctxbut the full-decodeLoadWithDBdoes not support cancellation.
copyBlocksRaw(used byLoadBlobsWithDB) checksctx.Err()after each batch, butcopyBlocks(used byLoadWithDB) has no context parameter and cannot be cancelled. If this is intentional (full load is always run to completion), consider documenting why. Otherwise, threading a context throughcopyBlockswould make the two paths consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/node/load.go` around lines 221 - 257, LoadBlobsWithDB accepts ctx and uses copyBlocksRaw which checks ctx, but LoadWithDB calls copyBlocks (no ctx) so full-decode can't be cancelled; update copyBlocks to accept a context parameter and propagate ctx from LoadWithDB into the new copyBlocks(ctx, ...) so it can check ctx.Err() between batches just like copyBlocksRaw, and update all callers (including LoadWithDB and any tests) to pass through the context; alternatively, if cancellation is intentionally unsupported, add a clear comment on LoadWithDB and copyBlocks explaining why cancellation is omitted and ensure signatures remain consistent.database/models/import_checkpoint.go (1)
41-48:ImportPhaseOrderis an exported mutable slice.Any caller can accidentally mutate
ImportPhaseOrder(e.g.,appendor element reassignment), which would breakIsPhaseCompletedfor all consumers. Consider making it unexported or returning a copy from a function.♻️ Option: make it a function
-var ImportPhaseOrder = []string{ +var importPhaseOrder = []string{ ImportPhaseUTxO, ImportPhaseCertState, ImportPhaseSnapshots, ImportPhasePParams, ImportPhaseGovState, ImportPhaseTip, } + +func ImportPhaseOrder() []string { + out := make([]string, len(importPhaseOrder)) + copy(out, importPhaseOrder) + return out +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/models/import_checkpoint.go` around lines 41 - 48, ImportPhaseOrder is an exported mutable slice; make it immutable by renaming the variable to an unexported importPhaseOrder and add an exported accessor function ImportPhaseOrder() that returns a new slice copy (e.g., copy := make([]string,len(importPhaseOrder)); copy(copy, importPhaseOrder); return copy). Update consumers such as IsPhaseCompleted to call the accessor instead of referencing the variable directly so callers cannot mutate the underlying slice.mithril/download.go (1)
200-203: No idle/read timeout on the download HTTP client.
Timeout: 0disables the overall deadline, which is correct for large multi-GB downloads. However, there's no protection against a stalled TCP connection where the server stops sending data. Consider adding aTransportwithResponseHeaderTimeoutor wrappingresp.Bodywith a deadline-resetting reader so that a completely stalled transfer eventually fails rather than hanging indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mithril/download.go` around lines 200 - 203, The HTTP client created in variable client in download.go has Timeout: 0 which leaves transfers vulnerable to stalled TCP connections; modify the client to use a custom http.Transport (assigned to client.Transport) that sets sensible timeouts such as ResponseHeaderTimeout and IdleConnTimeout (and optionally TLSHandshakeTimeout) to ensure stalled servers fail fast, and/or ensure the download reader returned from resp.Body is wrapped with a deadline-resetting or context-aware reader to enforce per-chunk read deadlines; update the client creation code (where client := &http.Client{...} and CheckRedirect: httpsOnlyRedirect is set) to install this Transport and/or wrap resp.Body in the downloader function so long-running large downloads still fail on complete stalls.ledgerstate/utxo.go (1)
589-598:hex.EncodeToStringresult stored as[]byte— consider storing asstringif the model supports it.Line 595:
NameHex: []byte(hex.EncodeToString(a.Name))converts the hex string to bytes. This works but is a roundabout allocation. Verify thatmodels.Asset.NameHexis[]byteand notstring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/utxo.go` around lines 589 - 598, The code currently sets NameHex with []byte(hex.EncodeToString(a.Name)) inside the assets conversion loop; please check the type of models.Asset.NameHex and adjust the assignment accordingly: if NameHex is a string, assign hex.EncodeToString(a.Name) directly (in the utxo.Assets append), otherwise (if NameHex is []byte) keep the conversion but consider using a single conversion expression; update the models.Asset usage in the conversion loop (the utxo.Assets append where PolicyId/Name/NameHex/Amount/Fingerprint are set) to match the actual field type.database/sync_state.go (1)
19-61: Consider wrapping errors with context at the Database layer.The methods delegate directly to
d.metadatawithout adding context. While the underlying metadata implementations already wrap their errors, adding a thin layer here would help callers distinguish which layer failed. That said, this follows the same pattern as other Database methods (e.g.,GetCommitTimestamp), so it's consistent with the existing codebase.As per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/sync_state.go` around lines 19 - 61, The Database layer should wrap errors with context before returning; update GetSyncState, SetSyncState, DeleteSyncState, and ClearSyncState to capture the result and error from calls to d.metadata (both nil-txn and txn.Metadata() paths), and if err != nil return a wrapped error using fmt.Errorf with %w (e.g. fmt.Errorf("Database.GetSyncState: %w", err)); ensure GetSyncState returns the zero-value string plus the wrapped error on failure, and the other methods return the wrapped error directly while returning the original value on success.ledgerstate/certstate.go (1)
97-196: Heuristic-based Conway CertState parsing is fragile but pragmatically necessary.The strategy of identifying PState (largest array) and DState (largest map) by CBOR byte size works for known layouts but could misidentify components if future node versions change element sizes or add new large structures. The comments documenting the expected layout are helpful.
One subtle point on lines 158-164: the VState DRep map search requires
len(dreps) > 0to accept a candidate map. If the DRep registry is genuinely empty on-chain, no element will match andresult.DRepswill remain nil. This is likely fine for disambiguation purposes, but worth a brief comment clarifying that an empty DRep registry is indistinguishable from "not found" in this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/certstate.go` around lines 97 - 196, The VState DRep map search in parseCertStateConway uses parseDRepMap and only accepts candidates when len(dreps) > 0, which makes an actually-empty on-chain DRep registry indistinguishable from “not found”; update the VState parsing block to include a short clarifying comment next to the parseDRepMap call (or alternatively explicitly set result.DReps = []DRep{} when vErr == nil and len(dreps) == 0 if you want to treat an empty registry as present) so readers see the intended semantics for parseDRepMap and result.DReps.ledgerstate/import.go (2)
564-658: Pool import runs in a single transaction with no batching.Unlike
importAccounts(which batches at 10k) andimportUTxOs(which uses batch callbacks),importPoolscommits all pools in a single transaction. On mainnet with ~3,000+ pools this is likely fine, but the pattern is inconsistent with the rest of the import pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/import.go` around lines 564 - 658, importPools currently opens one long metadata transaction and imports all pools in it; change it to batch the work into smaller transactions (like importAccounts does) by iterating pools in chunks (e.g., batchSize = 1000 or reuse 10_000 if desired), and for each batch create a new txn via cfg.Database.MetadataTxn(true), use txn.Release()/txn.Commit() per-batch, call store.ImportPool(model, reg, txn.Metadata()) for each pool in the batch, handle ctx cancellation between batches, and return errors immediately on ImportPool failures; ensure you remove the single long-lived txn and instead create/release/commit a transaction per batch to match the rest of the import pipeline.
1344-1370: Replace string matching with GORM's portablegorm.ErrDuplicatedKeyerror constant.GORM v1.31.1 provides
gorm.ErrDuplicatedKeythat can be checked viaerrors.Is(err, gorm.ErrDuplicatedKey)whenTranslateError: trueis configured on the database connection. This eliminates the fragility of parsing error message strings across SQLite, MySQL, and PostgreSQL driver versions.The current string-matching approach will silently break if any driver changes its error format. Either migrate to use GORM's error translation mechanism, or add tests that verify these error strings remain stable across driver versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/import.go` around lines 1344 - 1370, Replace fragile string matching in isEpochDuplicate by first checking errors.Is(err, gorm.ErrDuplicatedKey) (importing "errors" and "gorm.io/gorm") which will be true when DB is configured with TranslateError: true; if you want to be backward compatible, keep the existing string-based checks as a fallback after the errors.Is check. Update isEpochDuplicate to reference gorm.ErrDuplicatedKey and ensure imports are added.ledgerstate/govstate.go (1)
140-142: Three-return-value pattern is unconventional but works here.
parseConstitutionreturning(*ParsedConstitution, error, error)for (result, warning, fatal_error) is unusual in Go. A more idiomatic approach would be a result struct with aWarnings []errorfield, or a named error type. Since this is a private helper with a single call site, it's acceptable — just worth noting if more callers are added later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/govstate.go` around lines 140 - 142, parseConstitution currently returns (*ParsedConstitution, error, error) (result, warning, fatal) which is unconventional; change it to return a single result type that carries warnings instead—e.g. define a small result struct (ParseConstitutionResult) containing Parsed *ParsedConstitution and Warnings []error, then update parseConstitution to return (ParseConstitutionResult, error) and adjust its single caller to read result. This keeps the fatal error as the returned error and moves the warning into result.Warnings for idiomatic handling while preserving behavior; update references to parseConstitution and ParsedConstitution accordingly.ledgerstate/import_test.go (1)
197-208: Consider usingt.Logfinstead offmt.Fprintf(os.Stderr, ...).The rest of the test uses
t.Logffor output. This final summary block writes directly to stderr, which bypasses Go's test output buffering (e.g., output won't be suppressed bygo testwithout-v). If the intent is to always show this summary, it's fine — but it's inconsistent with the rest of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/import_test.go` around lines 197 - 208, The test prints a final summary using fmt.Fprintf(os.Stderr, ...) which bypasses Go test logging; replace that call with t.Logf(...) so output goes through the test logger (use the same format string and the same values: ledgerstate.EraName(state.EraIndex), state.Epoch, state.Tip.Slot, state.Tip.BlockHash) to be consistent with other logs in the file and respect `go test` output buffering.ledgerstate/snapshot.go (3)
216-223:ParseSnapshotreads the entire file into memory.
os.ReadFile(path)loads the full ledger state, which can be several GB on mainnet. This is acceptable for a one-time bootstrap operation, but worth documenting in the function's doc comment so callers are aware of the memory requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/snapshot.go` around lines 216 - 223, The ParseSnapshot function reads the entire snapshot file into memory using os.ReadFile which can be multiple GB; add a doc comment above ParseSnapshot explaining that it loads the full file into memory, warn callers about high memory usage for large mainnet snapshots, note that it delegates to parseSnapshotData and that callers should use alternatives if streaming/low-memory behavior is required.
365-391: Version-wrapper peeling loop is clever but could benefit from a depth guard.The loop peels nested version prefixes (
[version, [version2, [...]]]) until it reaches the actual data. While finite CBOR size guarantees termination, a small iteration cap (e.g., 10) would guard against pathological inputs and make the intent clearer.🛡️ Suggested guard
+ const maxVersionLayers = 10 + layers := 0 for len(shelleyState) >= 2 { + if layers >= maxVersionLayers { + return nil, fmt.Errorf( + "exceeded %d version wrapper layers", + maxVersionLayers, + ) + } var ssVersion uint64 if _, decErr := cbor.Decode( shelleyState[0], &ssVersion, ); decErr != nil { break } + layers++🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/snapshot.go` around lines 365 - 391, The loop that peels nested version wrappers from shelleyState (using ssVersion and decodeRawArray) needs a defensive depth guard to avoid pathological infinite or excessive nesting; add a small maxDepth constant (e.g., 10), track iterations with a counter, increment each loop iteration, and if the counter exceeds maxDepth return a clear error (e.g., "too many nested version wrappers") instead of continuing forever. Ensure the guard is applied around the existing for loop that inspects shelleyState and ssVersion so normal short nesting still behaves the same but overly deep inputs fail fast.
444-456: Silent zero-default for treasury/reserves whenAccountStatehas fewer than 2 elements.If
len(acctState) < 2, bothtreasuryandreservessilently default to 0. For a bootstrap import, this could mask a malformed snapshot. Consider logging a warning or returning an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledgerstate/snapshot.go` around lines 444 - 456, The code decodes AccountState into acctState and silently leaves treasury and reserves as zero when len(acctState) < 2; change this to validate that acctState has at least 2 elements after cbor.Decode and return a descriptive error (e.g., fmt.Errorf("invalid AccountState: expected >=2 elements, got %d", len(acctState))) instead of allowing silent zero defaults so callers of this snapshot parsing (the acctState/treasury/reserves logic) will fail fast on malformed snapshots.cmd/dingo/mithril.go (4)
909-912: Consider documentingbatchBlockstuning rationale.
batchBlocks = 100directly controls blob-store write amplification and memory pressure. A short comment explaining why 100 was chosen (or that it's empirically tuned) would help future maintainers adjust it confidently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dingo/mithril.go` around lines 909 - 912, Add a short explanatory comment next to the const batchBlocks = 100 declaration in mithril.go describing why 100 was chosen (e.g., empirically tuned), what it controls (blob-store write amplification and memory pressure), and guidance for adjusting it (trade-offs: larger values reduce IO but increase memory and write amplification; smaller values lower memory but increase IO). Reference the constant name batchBlocks so maintainers can quickly find and tune it.
231-246: Missing error context on aggregator URL resolution.Line 244 returns the bare error from
AggregatorURLForNetwork. Per coding guidelines, wrap with context:Proposed fix
aggregatorURL, err = mithril.AggregatorURLForNetwork( network, ) if err != nil { - return err + return fmt.Errorf("resolving aggregator URL: %w", err) }The same pattern appears in
mithrilListCommand(line 84) andmithrilShowCommand(line 157). As per coding guidelines,**/*.go: Wrap errors with context using fmt.Errorf with %w verb.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dingo/mithril.go` around lines 231 - 246, Wrap the bare error returned from mithril.AggregatorURLForNetwork in runMithrilSync using fmt.Errorf and the %w verb to add context (e.g., "failed to resolve aggregator URL for network %s: %w", network, err); do the same for the identical calls in mithrilListCommand and mithrilShowCommand so each returns a wrapped error that includes the network and a clear failure message instead of the raw error.
636-654: Unwrapped error return fromImportLedgerState.Line 653 returns the error from
ImportLedgerStatewithout adding context. As per coding guidelines, errors should be wrapped withfmt.Errorfand%w.Proposed fix
); err != nil { - return 0, nil, err + return 0, nil, fmt.Errorf("importing ledger state: %w", err) }As per coding guidelines,
**/*.go: Wrap errors with context using fmt.Errorf with %w verb.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dingo/mithril.go` around lines 636 - 654, The error returned from ledgerstate.ImportLedgerState is returned directly; wrap it with fmt.Errorf using %w to add context before returning. In the block where ledgerstate.ImportLedgerState(...) is called (inside the function handling the Mithril import), replace the bare return of err with something like returning fmt.Errorf("failed to import ledger state: %w", err) so callers get contextual information; ensure you import "fmt" if not already present.
62-132: Network-default logic duplicated across commands.The
if network == "" { network = "preview" }pattern appears inmithrilListCommand(line 73),mithrilShowCommand(line 146),mithrilSyncCommand(line 221), andsyncCommand(line 517). Consider extracting a small helper (e.g.,resolveNetwork(cfg) string) to centralize the default and avoid silent divergence if the default ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dingo/mithril.go` around lines 62 - 132, Duplicate network-default logic exists in mithrilListCommand, mithrilShowCommand, mithrilSyncCommand and syncCommand; extract a small helper (e.g., resolveNetwork(cfg *config.Type) string) that returns cfg.Network if non-empty or "preview" otherwise, replace the inline if network == "" { network = "preview" } blocks in those functions with a call to resolveNetwork(cfg), and update any callers to use the returned value; keep the helper name and signature unique and referenced in each of the four functions (mithrilListCommand, mithrilShowCommand, mithrilSyncCommand, syncCommand) so the default is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@database/plugin/metadata/sqlite/network_state.go`:
- Around line 58-77: GetNetworkState is performing a read-only query but calls
resolveDB; change it to use resolveReadDB() so the method uses the
read-optimized connection pool (the same pattern used by
GetSyncState/GetTip/GetAccount). Replace the call to resolveDB(txn) with
resolveReadDB(txn) (which returns d.ReadDB()) inside GetNetworkState and keep
the existing error handling and return behavior unchanged.
In `@ledgerstate/import.go`:
- Around line 558-561: The final batch commit in ledgerstate/import.go currently
returns txn.Commit() raw when inBatch > 0; change it to capture the error from
txn.Commit(), wrap it with context using fmt.Errorf and the %w verb (e.g.,
"commit final batch: %w"), and return the wrapped error; reference the inBatch
check and the txn.Commit() call and ensure you import fmt if not already
imported.
- Around line 514-561: importAccounts leaks intermediate MetadataTxn resources
because after committing a batch the old txn is replaced without calling
txn.Release(); before creating a new txn in the loop (the assignment txn =
cfg.Database.MetadataTxn(true) in importAccounts), call txn.Release() on the
committed transaction to free resources; keep the existing defer txn.Release()
for the final txn, and follow the same pattern used in importUTxOs
(create/release per batch) so every created MetadataTxn is either Release()d
after Commit() or by the existing defer.
In `@ledgerstate/snapshot.go`:
- Around line 583-603: The legacy fallback in parseTip that decodes a 2-element
[slot, hash] from tipArr assumes tipArr has at least 2 items; add an explicit
length validation on tipArr before decoding (e.g., require len(tipArr) == 2 for
the legacy branch) and return a clear error (or log a warning) if the array
length is unexpected so we don't treat a third element (like blockNo) as the
hash; update the logic around SnapshotTip construction in parseTip to only
decode tipArr[0] and tipArr[1] when that length check passes.
In `@ledgerstate/utxo.go`:
- Around line 578-601: UTxOToModel currently drops datum and script reference
data from ParsedUTxO; update UTxOToModel to copy the ParsedUTxO.Cbor into
models.Utxo.Cbor and persist relationships for datum and scripts: when
ParsedUTxO.DatumHash/Datum is present, create or link a models.Datum (populate
hash and raw CBOR/data) and set the Utxo's datum reference fields accordingly;
when ParsedUTxO.ScriptRef is present, create or link the appropriate
WitnessScripts model entries and set the Utxo's script reference fields; mirror
the approach used in UtxoLedgerToModel for populating Cbor and establishing
Datum/WitnessScripts associations so snapshot imports do not discard datum or
script data.
---
Duplicate comments:
In `@chain/chain.go`:
- Around line 375-426: The txn.Do error returned inside AddRawBlocks should be
wrapped with contextual batch information before returning; replace the bare
"return err" after calling txn.Do (the call assigned to err variable from
txn.Do) with a wrapped error using fmt.Errorf and the %w verb that includes
batchOffset and batchSize (or an equivalent descriptor) so callers get batch
context; ensure you import fmt if not present and keep the original error
wrapped (e.g., fmt.Errorf("AddRawBlocks batch offset=%d size=%d: %w",
batchOffset, batchSize, err)).
- Around line 297-373: The function addRawBlock lacks validation for empty
rb.Hash or rb.PrevHash which lets malformed RawBlock values pass the header/tip
comparisons; add an explicit guard early in addRawBlock (before the header and
tip checks, after reconcile) that checks if len(rb.Hash) == 0 or
len(rb.PrevHash) == 0 and return a clear error (e.g., errors.New("raw block has
empty Hash/PrevHash") or a domain-specific NewMalformedBlockError) so malformed
blocks are rejected before persisting or mutating c.headers/c.currentTip.
In `@ledgerstate/mempack.go`:
- Around line 448-451: The Script decoding currently uses
r.readLengthPrefixedBytes() which returns the entire MemPack blob including the
AlonzoScript tag, so update the decoder in the Script/ScriptRef handling to
first read and consume the tag discriminator (e.g., call r.readByte() or a
length-prefixed tag reader), dispatch on that tag to separate NativeScript vs
PlutusScript payloads, then read/assign only the decoded script payload into
ScriptRef (or if you intend ScriptRef to remain an opaque blob add a
comment/docstring on the ScriptRef field and decoder to state that it includes
the tag). Ensure you modify where readLengthPrefixedBytes() is invoked in this
path and reference the ScriptRef field and the decoder function that calls
r.readLengthPrefixedBytes() so downstream consumers see the tag-stripped payload
or documented behavior.
- Around line 541-559: The function decodeFlatMultiAsset lacks an upper-bound
check on numAssets so multiplications like numAssets*8 can overflow or cause
excessive allocations; before computing
quantitiesEnd/pidOffsetsEnd/nameOffsetsEnd, validate numAssets against a safe
maximum derived from the input length and integer limits (e.g., ensure numAssets
is non-negative and numAssets <= len(flat)/minimumBytesPerAsset and also check
using a larger integer type or compare against math.MaxInt/8 to prevent
overflow), and return an error if it exceeds that cap; update the checks around
quantitiesEnd, pidOffsetsEnd and nameOffsetsEnd in decodeFlatMultiAsset to use
the overflow-safe calculations and bail early on invalid numAssets.
In `@ledgerstate/snapshot.go`:
- Around line 53-89: The code collects directory names into utxoHDDirs without
ensuring they are numeric, so strconv.ParseUint later treats non-numeric names
as 0; fix by validating names before appending: in the loop that detects
statePath, call strconv.ParseUint(name, 10, 64) and only append name (or its
numeric value) if err == nil; alternatively store parsed uint64s in a parallel
slice or map and then use slices.SortFunc on those numeric values (or sort by
the parsed numbers) so non-numeric directories are ignored by functions using
ParseUint (references: utxoHDDirs, ParseUint, slices.SortFunc, ledgerDir).
In `@ledgerstate/snapshots.go`:
- Around line 294-299: AggregatePoolStake currently dereferences snap (e.g.,
snap.Delegations) without guarding against a nil ParsedSnapShot; add an early
nil check at the start of AggregatePoolStake that returns an empty
[]*models.PoolStakeSnapshot (or nil per project convention) if snap == nil to
avoid a panic. Locate the AggregatePoolStake function and add the guard before
any access to snap.Delegations or other snap fields, referencing the
ParsedSnapShot type and the snap parameter.
In `@mithril/client.go`:
- Around line 189-202: GetLatestSnapshot currently assumes the aggregator
returns snapshots ordered; update it to be deterministic by fetching the slice
from ListSnapshots then sorting it client-side before selecting the latest: sort
by SnapshotListItem.Beacon.Epoch descending and use
SnapshotListItem.Beacon.ImmutableFileNumber descending as a tie-breaker, then
return the first element; ensure you import and use sort.Slice (or equivalent)
and keep error handling from ListSnapshots intact.
In `@README.md`:
- Around line 35-36: Clarify the linkage between the sample config file and the
env-var sentence: replace or follow the sentence mentioning `dingo.yaml.example`
with a short transitional sentence such as "Configuration may also be controlled
via environment variables, which can override or supplement settings in
dingo.yaml.example." and then keep "This behavior can be changed via the
following environment variables:" so the antecedent of "This behavior" clearly
refers to using environment variables to alter configuration; target the README
lines mentioning `dingo.yaml.example` and the sentence "This behavior can be
changed via the following environment variables:" when applying the change.
---
Nitpick comments:
In `@cmd/dingo/mithril.go`:
- Around line 909-912: Add a short explanatory comment next to the const
batchBlocks = 100 declaration in mithril.go describing why 100 was chosen (e.g.,
empirically tuned), what it controls (blob-store write amplification and memory
pressure), and guidance for adjusting it (trade-offs: larger values reduce IO
but increase memory and write amplification; smaller values lower memory but
increase IO). Reference the constant name batchBlocks so maintainers can quickly
find and tune it.
- Around line 231-246: Wrap the bare error returned from
mithril.AggregatorURLForNetwork in runMithrilSync using fmt.Errorf and the %w
verb to add context (e.g., "failed to resolve aggregator URL for network %s:
%w", network, err); do the same for the identical calls in mithrilListCommand
and mithrilShowCommand so each returns a wrapped error that includes the network
and a clear failure message instead of the raw error.
- Around line 636-654: The error returned from ledgerstate.ImportLedgerState is
returned directly; wrap it with fmt.Errorf using %w to add context before
returning. In the block where ledgerstate.ImportLedgerState(...) is called
(inside the function handling the Mithril import), replace the bare return of
err with something like returning fmt.Errorf("failed to import ledger state:
%w", err) so callers get contextual information; ensure you import "fmt" if not
already present.
- Around line 62-132: Duplicate network-default logic exists in
mithrilListCommand, mithrilShowCommand, mithrilSyncCommand and syncCommand;
extract a small helper (e.g., resolveNetwork(cfg *config.Type) string) that
returns cfg.Network if non-empty or "preview" otherwise, replace the inline if
network == "" { network = "preview" } blocks in those functions with a call to
resolveNetwork(cfg), and update any callers to use the returned value; keep the
helper name and signature unique and referenced in each of the four functions
(mithrilListCommand, mithrilShowCommand, mithrilSyncCommand, syncCommand) so the
default is centralized.
In `@database/models/import_checkpoint.go`:
- Around line 41-48: ImportPhaseOrder is an exported mutable slice; make it
immutable by renaming the variable to an unexported importPhaseOrder and add an
exported accessor function ImportPhaseOrder() that returns a new slice copy
(e.g., copy := make([]string,len(importPhaseOrder)); copy(copy,
importPhaseOrder); return copy). Update consumers such as IsPhaseCompleted to
call the accessor instead of referencing the variable directly so callers cannot
mutate the underlying slice.
In `@database/sync_state.go`:
- Around line 19-61: The Database layer should wrap errors with context before
returning; update GetSyncState, SetSyncState, DeleteSyncState, and
ClearSyncState to capture the result and error from calls to d.metadata (both
nil-txn and txn.Metadata() paths), and if err != nil return a wrapped error
using fmt.Errorf with %w (e.g. fmt.Errorf("Database.GetSyncState: %w", err));
ensure GetSyncState returns the zero-value string plus the wrapped error on
failure, and the other methods return the wrapped error directly while returning
the original value on success.
In `@internal/node/load.go`:
- Around line 177-207: The hardcoded 30-minute timeout in the ledger catch-up
loop should be made configurable: replace the literal time.After(30 *
time.Minute) with a named constant or a Config field (e.g.,
LedgerCatchupTimeout) and use that value when creating timeout; update the
surrounding logic in the function that calls copyBlocks / uses immutableTipSlot
and ls.Tip to read the default from a package-level constant if no config
provided, and ensure the new symbol (e.g., ledgerCatchupTimeout or
cfg.LedgerCatchupTimeout) is used in the timeout channel creation and documented
where defaults are set.
- Around line 221-257: LoadBlobsWithDB accepts ctx and uses copyBlocksRaw which
checks ctx, but LoadWithDB calls copyBlocks (no ctx) so full-decode can't be
cancelled; update copyBlocks to accept a context parameter and propagate ctx
from LoadWithDB into the new copyBlocks(ctx, ...) so it can check ctx.Err()
between batches just like copyBlocksRaw, and update all callers (including
LoadWithDB and any tests) to pass through the context; alternatively, if
cancellation is intentionally unsupported, add a clear comment on LoadWithDB and
copyBlocks explaining why cancellation is omitted and ensure signatures remain
consistent.
In `@ledgerstate/certstate.go`:
- Around line 97-196: The VState DRep map search in parseCertStateConway uses
parseDRepMap and only accepts candidates when len(dreps) > 0, which makes an
actually-empty on-chain DRep registry indistinguishable from “not found”; update
the VState parsing block to include a short clarifying comment next to the
parseDRepMap call (or alternatively explicitly set result.DReps = []DRep{} when
vErr == nil and len(dreps) == 0 if you want to treat an empty registry as
present) so readers see the intended semantics for parseDRepMap and
result.DReps.
In `@ledgerstate/govstate.go`:
- Around line 140-142: parseConstitution currently returns (*ParsedConstitution,
error, error) (result, warning, fatal) which is unconventional; change it to
return a single result type that carries warnings instead—e.g. define a small
result struct (ParseConstitutionResult) containing Parsed *ParsedConstitution
and Warnings []error, then update parseConstitution to return
(ParseConstitutionResult, error) and adjust its single caller to read result.
This keeps the fatal error as the returned error and moves the warning into
result.Warnings for idiomatic handling while preserving behavior; update
references to parseConstitution and ParsedConstitution accordingly.
In `@ledgerstate/import_test.go`:
- Around line 197-208: The test prints a final summary using
fmt.Fprintf(os.Stderr, ...) which bypasses Go test logging; replace that call
with t.Logf(...) so output goes through the test logger (use the same format
string and the same values: ledgerstate.EraName(state.EraIndex), state.Epoch,
state.Tip.Slot, state.Tip.BlockHash) to be consistent with other logs in the
file and respect `go test` output buffering.
In `@ledgerstate/import.go`:
- Around line 564-658: importPools currently opens one long metadata transaction
and imports all pools in it; change it to batch the work into smaller
transactions (like importAccounts does) by iterating pools in chunks (e.g.,
batchSize = 1000 or reuse 10_000 if desired), and for each batch create a new
txn via cfg.Database.MetadataTxn(true), use txn.Release()/txn.Commit()
per-batch, call store.ImportPool(model, reg, txn.Metadata()) for each pool in
the batch, handle ctx cancellation between batches, and return errors
immediately on ImportPool failures; ensure you remove the single long-lived txn
and instead create/release/commit a transaction per batch to match the rest of
the import pipeline.
- Around line 1344-1370: Replace fragile string matching in isEpochDuplicate by
first checking errors.Is(err, gorm.ErrDuplicatedKey) (importing "errors" and
"gorm.io/gorm") which will be true when DB is configured with TranslateError:
true; if you want to be backward compatible, keep the existing string-based
checks as a fallback after the errors.Is check. Update isEpochDuplicate to
reference gorm.ErrDuplicatedKey and ensure imports are added.
In `@ledgerstate/snapshot.go`:
- Around line 216-223: The ParseSnapshot function reads the entire snapshot file
into memory using os.ReadFile which can be multiple GB; add a doc comment above
ParseSnapshot explaining that it loads the full file into memory, warn callers
about high memory usage for large mainnet snapshots, note that it delegates to
parseSnapshotData and that callers should use alternatives if
streaming/low-memory behavior is required.
- Around line 365-391: The loop that peels nested version wrappers from
shelleyState (using ssVersion and decodeRawArray) needs a defensive depth guard
to avoid pathological infinite or excessive nesting; add a small maxDepth
constant (e.g., 10), track iterations with a counter, increment each loop
iteration, and if the counter exceeds maxDepth return a clear error (e.g., "too
many nested version wrappers") instead of continuing forever. Ensure the guard
is applied around the existing for loop that inspects shelleyState and ssVersion
so normal short nesting still behaves the same but overly deep inputs fail fast.
- Around line 444-456: The code decodes AccountState into acctState and silently
leaves treasury and reserves as zero when len(acctState) < 2; change this to
validate that acctState has at least 2 elements after cbor.Decode and return a
descriptive error (e.g., fmt.Errorf("invalid AccountState: expected >=2
elements, got %d", len(acctState))) instead of allowing silent zero defaults so
callers of this snapshot parsing (the acctState/treasury/reserves logic) will
fail fast on malformed snapshots.
In `@ledgerstate/utxo.go`:
- Around line 589-598: The code currently sets NameHex with
[]byte(hex.EncodeToString(a.Name)) inside the assets conversion loop; please
check the type of models.Asset.NameHex and adjust the assignment accordingly: if
NameHex is a string, assign hex.EncodeToString(a.Name) directly (in the
utxo.Assets append), otherwise (if NameHex is []byte) keep the conversion but
consider using a single conversion expression; update the models.Asset usage in
the conversion loop (the utxo.Assets append where
PolicyId/Name/NameHex/Amount/Fingerprint are set) to match the actual field
type.
In `@mithril/bootstrap.go`:
- Around line 646-667: The function hasFileRecursive is misleading because it
only checks dir and one level of subdirectories; rename hasFileRecursive to
hasFileInDirOrImmediateSubdirs (or an equivalent name) and update all callers to
use the new name, or alternatively implement true recursion inside
hasFileRecursive by recursing into subdirectories (update function signature and
tests accordingly) so the name matches behavior; target the function named
hasFileRecursive and any callers referencing it.
- Around line 236-283: The writes to ancillaryDir and ancillaryArchivePath
happen inside the ancillary goroutine and are read after ancWg.Wait(); add a
short comment near the ancWg variable/goroutine (referencing ancWg,
ancWg.Wait(), ancillaryDir, ancillaryArchivePath, and the anonymous goroutine)
stating that the WaitGroup.Wait provides the necessary happens-before
synchronization so those assignments are safe and visible to the reader; keep
the comment brief and located where ancWg is declared or the goroutine is
launched.
In `@mithril/download.go`:
- Around line 200-203: The HTTP client created in variable client in download.go
has Timeout: 0 which leaves transfers vulnerable to stalled TCP connections;
modify the client to use a custom http.Transport (assigned to client.Transport)
that sets sensible timeouts such as ResponseHeaderTimeout and IdleConnTimeout
(and optionally TLSHandshakeTimeout) to ensure stalled servers fail fast, and/or
ensure the download reader returned from resp.Body is wrapped with a
deadline-resetting or context-aware reader to enforce per-chunk read deadlines;
update the client creation code (where client := &http.Client{...} and
CheckRedirect: httpsOnlyRedirect is set) to install this Transport and/or wrap
resp.Body in the downloader function so long-running large downloads still fail
on complete stalls.
In `@mithril/types.go`:
- Around line 31-65: Both SnapshotListItem and Snapshot duplicate the same
fields; extract a shared embedded struct (e.g., snapshotBase) containing the
common fields and replace the two concrete types to embed that base (retain
Beacon, CreatedAt parsing method by moving or keeping CreatedAt on snapshotBase
so SnapshotListItem.CreatedAtTime() continues to work). Update type declarations
SnapshotListItem and Snapshot to embed snapshotBase and adjust any code
references to use the embedded fields (Digest, Network, CreatedAt, etc.) so
behavior is unchanged.
| func (d *MetadataStoreSqlite) GetNetworkState( | ||
| txn types.Txn, | ||
| ) (*models.NetworkState, error) { | ||
| db, err := d.resolveDB(txn) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var state models.NetworkState | ||
| result := db.Order("slot DESC").First(&state) | ||
| if result.Error != nil { | ||
| if errors.Is(result.Error, gorm.ErrRecordNotFound) { | ||
| return nil, nil | ||
| } | ||
| return nil, fmt.Errorf( | ||
| "get network state: %w", | ||
| result.Error, | ||
| ) | ||
| } | ||
| return &state, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the sync_state.go file and check GetSyncState
fd -t f "sync_state.go" database/plugin/metadata/sqlite/Repository: blinklabs-io/dingo
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check the GetSyncState method in sync_state.go
rg -n "GetSyncState" database/plugin/metadata/sqlite/sync_state.go -A 15 | head -30Repository: blinklabs-io/dingo
Length of output: 573
🏁 Script executed:
#!/bin/bash
# Check all Get methods in the SQLite metadata plugin and their DB resolution
fd -t f "\.go$" database/plugin/metadata/sqlite/ --exec rg -l "func.*MetadataStoreSqlite.*Get"Repository: blinklabs-io/dingo
Length of output: 984
🏁 Script executed:
#!/bin/bash
# Check all Get methods in network_state.go for their DB resolution patterns
rg -n "func.*MetadataStoreSqlite.*Get" database/plugin/metadata/sqlite/network_state.go -A 10Repository: blinklabs-io/dingo
Length of output: 417
🏁 Script executed:
#!/bin/bash
# Check patterns across multiple SQLite files - see what Get methods use
rg -n "func.*MetadataStoreSqlite.*Get" database/plugin/metadata/sqlite/ -A 8 | grep -E "(func|resolve(Read)?DB)" | head -50Repository: blinklabs-io/dingo
Length of output: 4487
🏁 Script executed:
#!/bin/bash
# Get a broader overview of resolve methods usage patterns in Get methods
for file in database/plugin/metadata/sqlite/{sync_state,epoch,tip,account,pparams}.go; do
if [ -f "$file" ]; then
echo "=== $(basename $file) ==="
rg -n "func.*MetadataStoreSqlite.*Get" "$file" -A 8 | grep -E "(func|resolve(Read)?DB)"
fi
doneRepository: blinklabs-io/dingo
Length of output: 804
🏁 Script executed:
#!/bin/bash
# Check GetEpoch to see if there's a pattern/reason for some Get methods using resolveDB
rg -n "func.*MetadataStoreSqlite.*GetEpoch" database/plugin/metadata/sqlite/epoch.go -A 15 | head -40Repository: blinklabs-io/dingo
Length of output: 1064
🏁 Script executed:
#!/bin/bash
# Check GetTransactionHashesAfterSlot and GetPoolByVrfKeyHash - the other methods using resolveDB
rg -n "GetTransactionHashesAfterSlot|GetPoolByVrfKeyHash" database/plugin/metadata/sqlite/ -A 8 | head -40Repository: blinklabs-io/dingo
Length of output: 1790
🏁 Script executed:
#!/bin/bash
# Check what resolveDB and resolveReadDB do - are there different implementations?
fd -t f "\.go$" database/plugin/metadata/sqlite/ --exec rg -l "func.*resolveDB|func.*resolveReadDB" | head -5Repository: blinklabs-io/dingo
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Find the definitions of these methods
rg -n "func \(d \*MetadataStoreSqlite\) resolve(Read)?DB" database/plugin/metadata/sqlite/ -A 8Repository: blinklabs-io/dingo
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Get the full implementations of both methods
rg -n "func \(d \*MetadataStoreSqlite\) resolve(Read)?DB" database/plugin/metadata/sqlite/transaction.go -A 20Repository: blinklabs-io/dingo
Length of output: 1235
GetNetworkState should use resolveReadDB instead of resolveDB.
This is a read-only query that should use the read-optimized connection pool via resolveReadDB(), consistent with other Get* methods in the codebase (e.g., GetSyncState, GetTip, GetAccount). The resolveReadDB() method returns d.ReadDB() which uses a separate read connection pool optimized for file-based databases, while resolveDB() returns the standard write-capable connection pool.
♻️ Proposed fix
func (d *MetadataStoreSqlite) GetNetworkState(
txn types.Txn,
) (*models.NetworkState, error) {
- db, err := d.resolveDB(txn)
+ db, err := d.resolveReadDB(txn)
if err != nil {
return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (d *MetadataStoreSqlite) GetNetworkState( | |
| txn types.Txn, | |
| ) (*models.NetworkState, error) { | |
| db, err := d.resolveDB(txn) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var state models.NetworkState | |
| result := db.Order("slot DESC").First(&state) | |
| if result.Error != nil { | |
| if errors.Is(result.Error, gorm.ErrRecordNotFound) { | |
| return nil, nil | |
| } | |
| return nil, fmt.Errorf( | |
| "get network state: %w", | |
| result.Error, | |
| ) | |
| } | |
| return &state, nil | |
| } | |
| func (d *MetadataStoreSqlite) GetNetworkState( | |
| txn types.Txn, | |
| ) (*models.NetworkState, error) { | |
| db, err := d.resolveReadDB(txn) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var state models.NetworkState | |
| result := db.Order("slot DESC").First(&state) | |
| if result.Error != nil { | |
| if errors.Is(result.Error, gorm.ErrRecordNotFound) { | |
| return nil, nil | |
| } | |
| return nil, fmt.Errorf( | |
| "get network state: %w", | |
| result.Error, | |
| ) | |
| } | |
| return &state, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@database/plugin/metadata/sqlite/network_state.go` around lines 58 - 77,
GetNetworkState is performing a read-only query but calls resolveDB; change it
to use resolveReadDB() so the method uses the read-optimized connection pool
(the same pattern used by GetSyncState/GetTip/GetAccount). Replace the call to
resolveDB(txn) with resolveReadDB(txn) (which returns d.ReadDB()) inside
GetNetworkState and keep the existing error handling and return behavior
unchanged.
| store := cfg.Database.Metadata() | ||
| txn := cfg.Database.MetadataTxn(true) | ||
| defer func() { txn.Release() }() | ||
|
|
||
| inBatch := 0 | ||
| for _, acct := range accounts { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| default: | ||
| } | ||
|
|
||
| model := &models.Account{ | ||
| StakingKey: acct.StakingKey, | ||
| Pool: acct.PoolKeyHash, | ||
| Drep: acct.DRepCred, | ||
| AddedSlot: slot, | ||
| Reward: types.Uint64(acct.Reward), | ||
| Active: acct.Active, | ||
| } | ||
|
|
||
| if err := store.ImportAccount( | ||
| model, txn.Metadata(), | ||
| ); err != nil { | ||
| return fmt.Errorf( | ||
| "importing account %x: %w", | ||
| acct.StakingKey, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| inBatch++ | ||
| if inBatch >= accountBatchSize { | ||
| if err := txn.Commit(); err != nil { | ||
| return fmt.Errorf( | ||
| "committing account batch: %w", | ||
| err, | ||
| ) | ||
| } | ||
| txn = cfg.Database.MetadataTxn(true) | ||
| inBatch = 0 | ||
| } | ||
| } | ||
|
|
||
| if inBatch > 0 { | ||
| return txn.Commit() | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Intermediate transactions leak: committed batches are never Release()d.
When importAccounts commits a batch at Line 547 and creates a new transaction at Line 553, the old (committed) transaction is never released — only the final txn is released by the defer on Line 516. If Release() returns resources to a pool (e.g., database connections), this leaks one resource per batch.
The importUTxOs function handles this correctly (Lines 341-342) by creating a new txn with defer txn.Release() inside each batch callback invocation.
🐛 Proposed fix: release old transaction before creating a new one
inBatch++
if inBatch >= accountBatchSize {
if err := txn.Commit(); err != nil {
return fmt.Errorf(
"committing account batch: %w",
err,
)
}
+ txn.Release()
txn = cfg.Database.MetadataTxn(true)
inBatch = 0
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ledgerstate/import.go` around lines 514 - 561, importAccounts leaks
intermediate MetadataTxn resources because after committing a batch the old txn
is replaced without calling txn.Release(); before creating a new txn in the loop
(the assignment txn = cfg.Database.MetadataTxn(true) in importAccounts), call
txn.Release() on the committed transaction to free resources; keep the existing
defer txn.Release() for the final txn, and follow the same pattern used in
importUTxOs (create/release per batch) so every created MetadataTxn is either
Release()d after Commit() or by the existing defer.
| if inBatch > 0 { | ||
| return txn.Commit() | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Final batch commit doesn't wrap errors with context.
When inBatch > 0, the return on Line 559 propagates the raw txn.Commit() error without wrapping. This is inconsistent with the wrapped error at Line 548 and makes it harder to identify where a commit failure occurred in logs.
Proposed fix
if inBatch > 0 {
- return txn.Commit()
+ if err := txn.Commit(); err != nil {
+ return fmt.Errorf(
+ "committing final account batch: %w",
+ err,
+ )
+ }
}
return nilAs per coding guidelines: "Wrap errors with context using fmt.Errorf with %w verb".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if inBatch > 0 { | |
| return txn.Commit() | |
| } | |
| return nil | |
| if inBatch > 0 { | |
| if err := txn.Commit(); err != nil { | |
| return fmt.Errorf( | |
| "committing final account batch: %w", | |
| err, | |
| ) | |
| } | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ledgerstate/import.go` around lines 558 - 561, The final batch commit in
ledgerstate/import.go currently returns txn.Commit() raw when inBatch > 0;
change it to capture the error from txn.Commit(), wrap it with context using
fmt.Errorf and the %w verb (e.g., "commit final batch: %w"), and return the
wrapped error; reference the inBatch check and the txn.Commit() call and ensure
you import fmt if not already imported.
| // Legacy format: [slot, hash] directly | ||
| var slot uint64 | ||
| if _, err := cbor.Decode( | ||
| tipArr[0], &slot, | ||
| ); err != nil { | ||
| return nil, fmt.Errorf( | ||
| "decoding tip slot: %w", err, | ||
| ) | ||
| } | ||
| var blockHash []byte | ||
| if _, err := cbor.Decode( | ||
| tipArr[1], &blockHash, | ||
| ); err != nil { | ||
| return nil, fmt.Errorf( | ||
| "decoding tip hash: %w", err, | ||
| ) | ||
| } | ||
| return &SnapshotTip{ | ||
| Slot: slot, | ||
| BlockHash: blockHash, | ||
| }, nil |
There was a problem hiding this comment.
Fallback parseTip path assumes 2-element [slot, hash] format but doesn't validate length.
When len(tipArr) > 1 and it's not the WithOrigin At case (len == 1), the code falls through to the legacy path and interprets tipArr[0] as slot and tipArr[1] as hash. If tipArr has 3+ elements in an unexpected encoding (e.g., [slot, blockNo, hash] without WithOrigin wrapping), tipArr[1] would be blockNo, not the block hash. Consider adding an explicit length check or logging a warning for unexpected tip array lengths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ledgerstate/snapshot.go` around lines 583 - 603, The legacy fallback in
parseTip that decodes a 2-element [slot, hash] from tipArr assumes tipArr has at
least 2 items; add an explicit length validation on tipArr before decoding
(e.g., require len(tipArr) == 2 for the legacy branch) and return a clear error
(or log a warning) if the array length is unexpected so we don't treat a third
element (like blockNo) as the hash; update the logic around SnapshotTip
construction in parseTip to only decode tipArr[0] and tipArr[1] when that length
check passes.
| // UTxOToModel converts a ParsedUTxO to a Dingo database Utxo model. | ||
| func UTxOToModel(u *ParsedUTxO, slot uint64) models.Utxo { | ||
| utxo := models.Utxo{ | ||
| TxId: u.TxHash, | ||
| OutputIdx: u.OutputIndex, | ||
| PaymentKey: u.PaymentKey, | ||
| StakingKey: u.StakingKey, | ||
| Amount: types.Uint64(u.Amount), | ||
| AddedSlot: slot, | ||
| } | ||
|
|
||
| // Convert assets | ||
| for _, a := range u.Assets { | ||
| fingerprint := lcommon.NewAssetFingerprint(a.PolicyId, a.Name) | ||
| utxo.Assets = append(utxo.Assets, models.Asset{ | ||
| PolicyId: a.PolicyId, | ||
| Name: a.Name, | ||
| NameHex: []byte(hex.EncodeToString(a.Name)), | ||
| Amount: types.Uint64(a.Amount), | ||
| Fingerprint: []byte(fingerprint.String()), | ||
| }) | ||
| } | ||
|
|
||
| return utxo |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the Utxo model has Datum/DatumHash/ScriptRef fields
rg -n 'Datum|ScriptRef' --type=go -C2 database/models/utxo.goRepository: blinklabs-io/dingo
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Read the Utxo model definition
echo "=== models.Utxo structure ==="
cat database/models/utxo.go
echo -e "\n=== ParsedUTxO definition ==="
rg -n 'type ParsedUTxO' --type=go -A 30 ledgerstate/utxo.go | head -50Repository: blinklabs-io/dingo
Length of output: 3115
🏁 Script executed:
#!/bin/bash
# Get ParsedUTxO definition more clearly
rg -n 'type ParsedUTxO struct' --type=go -A 20 ledgerstate/utxo.goRepository: blinklabs-io/dingo
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search more broadly for ParsedUTxO
rg -n 'ParsedUTxO' --type=go -B 2 -A 15 ledgerstate/utxo.go | head -80Repository: blinklabs-io/dingo
Length of output: 2485
🏁 Script executed:
#!/bin/bash
# Find the ParsedUTxO type definition
rg -n 'type ParsedUTxO' --type=go ledgerstate/utxo.go -A 15Repository: blinklabs-io/dingo
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for ParsedUTxO definition across all Go files
rg -n 'type ParsedUTxO' --type=go -A 15Repository: blinklabs-io/dingo
Length of output: 984
🏁 Script executed:
#!/bin/bash
# Verify parseCborTxOut also populates these fields
rg -n 'parseCborTxOut' --type=go -A 30 ledgerstate/utxo.go | head -60Repository: blinklabs-io/dingo
Length of output: 1795
🏁 Script executed:
#!/bin/bash
# Get the full parseCborTxOut function
rg -n 'func parseCborTxOut' --type=go -A 50 ledgerstate/utxo.goRepository: blinklabs-io/dingo
Length of output: 1433
🏁 Script executed:
#!/bin/bash
# Get full parseCborTxOut including end
sed -n '382,460p' ledgerstate/utxo.goRepository: blinklabs-io/dingo
Length of output: 2029
🏁 Script executed:
#!/bin/bash
# Check the UTxOToModel function implementation
sed -n '578,610p' ledgerstate/utxo.go
# Also search for where UTxOToModel is called
echo -e "\n=== Where UTxOToModel is called ==="
rg -n 'UTxOToModel' --type=go ledgerstate/utxo.goRepository: blinklabs-io/dingo
Length of output: 933
🏁 Script executed:
#!/bin/bash
# Find where UTxOToModel is called
rg -n 'UTxOToModel' --type=go -B 3 -A 3 | grep -v ledgerstate/utxo.go
# Also check for any Mithril-related code
echo -e "\n=== Mithril-related code ==="
rg -n 'Mithril|snapshot' --type=go | head -20Repository: blinklabs-io/dingo
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Check the import.go context around where UTxOToModel is called
sed -n '320,360p' ledgerstate/import.goRepository: blinklabs-io/dingo
Length of output: 757
🏁 Script executed:
#!/bin/bash
# Check if models.Utxo is supposed to have Datum/ScriptRef fields
# Look for any discussion or TODOs about this
rg -n 'Datum|ScriptRef' --type=go database/models/
# Also check if there's any storage for Datum/ScriptRef elsewhere
echo -e "\n=== Asset model to see storage pattern ==="
sed -n '1,100p' database/models/asset.goRepository: blinklabs-io/dingo
Length of output: 3188
🏁 Script executed:
#!/bin/bash
# Check if Utxo model has relationships to Datum or WitnessScript
echo "=== Datum model ==="
cat database/models/datum.go
echo -e "\n=== WitnessScript model ==="
cat database/models/witness_script.go
# Check if Utxo has any reference to these
echo -e "\n=== Checking for relationships in Utxo ==="
grep -n 'Datum\|Script' database/models/utxo.goRepository: blinklabs-io/dingo
Length of output: 2736
🏁 Script executed:
#!/bin/bash
# Check if ParsedUTxO has a Cbor field or raw bytes we could use
rg -n 'type ParsedUTxO' --type=go -A 15 ledgerstate/types.go
# Check how UtxoLedgerToModel (the other conversion function) handles Cbor
echo -e "\n=== Check UtxoLedgerToModel in models ==="
sed -n '55,95p' database/models/utxo.go
# Check if Cbor field is ever used/populated elsewhere
echo -e "\n=== Where Cbor field is set ==="
rg -n '\.Cbor\s*=' --type=goRepository: blinklabs-io/dingo
Length of output: 1755
🏁 Script executed:
#!/bin/bash
# Check the import flow more carefully - where does ParsedUTxO come from in the Mithril case?
echo "=== Check the import function signature ==="
sed -n '300,330p' ledgerstate/import.go
# Check if there's a raw CBOR available in the parsing flow
echo -e "\n=== Check parseUTxOEntry to see what data is available ==="
sed -n '305,330p' ledgerstate/utxo.go
# Also check if ParsedUTxO should have a Cbor field or if it's being reconstructed
echo -e "\n=== Check if there's CBOR in the snapshot/import context ==="
rg -n 'parseUTxOEntry|processBatchedUTxOs' --type=go -B 2 -A 15 ledgerstate/utxo.go | head -100Repository: blinklabs-io/dingo
Length of output: 4021
UTxOToModel silently discards datum and script reference data during snapshot imports.
ParsedUTxO has DatumHash, Datum, and ScriptRef fields that both parseMempackTxOut and parseCborTxOut populate from parsed snapshot/ledger state data. However, UTxOToModel does not:
- Set the
Cborfield onmodels.Utxo(which would preserve the complete TxOut representation) - Establish any relationship to the separate
DatumorWitnessScriptsmodels that exist in the database
This contrasts with UtxoLedgerToModel (database/models/utxo.go), which does set the Cbor field. For Mithril snapshot imports via ledgerstate/import.go, the datum and script data available during parsing is silently dropped during the conversion to the database model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ledgerstate/utxo.go` around lines 578 - 601, UTxOToModel currently drops
datum and script reference data from ParsedUTxO; update UTxOToModel to copy the
ParsedUTxO.Cbor into models.Utxo.Cbor and persist relationships for datum and
scripts: when ParsedUTxO.DatumHash/Datum is present, create or link a
models.Datum (populate hash and raw CBOR/data) and set the Utxo's datum
reference fields accordingly; when ParsedUTxO.ScriptRef is present, create or
link the appropriate WitnessScripts model entries and set the Utxo's script
reference fields; mirror the approach used in UtxoLedgerToModel for populating
Cbor and establishing Datum/WitnessScripts associations so snapshot imports do
not discard datum or script data.
1 participant
Summary by cubic
Adds a Mithril bootstrap client and CLI for one‑command fast sync from certified snapshots. Speeds up first run with bulk ledger import, header‑only block load, resumable downloads, and a startup guard that blocks serving on incomplete syncs.
New Features
Migration
Written for commit 55fc5d4. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
dingo sync --mithrilcommand to download and import ledger snapshots directly from Mithril aggregator with automatic ledger state and gap-block handling.--networkflag to specify Cardano network (preview, preprod, mainnet) across all commands.mithrilsubcommand group withlist,show, andsyncoperations for snapshot management.Improvements
Documentation