[IC-272][CP-341] Async CheckTx support and Unified mempool support#90
Open
kakysha wants to merge 46 commits into
Open
[IC-272][CP-341] Async CheckTx support and Unified mempool support#90kakysha wants to merge 46 commits into
kakysha wants to merge 46 commits into
Conversation
…xcept Commit Commit is different cause it invokes mempool.Update, which in turns calls RecheckTx on every mempool tx -> needs to be serialized before checking new incoming txns
…el to bloc kexecution + Copy() method in rootmultiStore
so initial_height type is serialized into string and not int
…ktx-proxy-mempool
…r `initial_height` (#67) * fix(genesis): align SDK genesis with BFT genesis to use cmtjson so initial_height type is serialized into string and not int * fix(devnetify): delete genesisHash from StateDB after changing genesis
…ring devnetify or export) since I added cms.Copy() when we re-set CheckState, if we have special storeLoader set that will apply store upgrades, we need to use them instead of `LoadLatestVersion()` + ignore new mempool TxNotFound error type to not panic during `DeliverGenTxs()` while `InitChain()`
…es for mempool error
…tion before commit
…lock + before we were not removing it in case antehandlers error
to support function errors
…/parallel-checktx-proxy-mempool
…n for correct visualization of traces
…eseries with high cardinality tags
github-actions
Bot
added
C:x/feegrant
C:Store
C:x/circuit
C:x/evidence
C:x/genutil
C:x/nft
C:x/upgrade
labels
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds global ABCI mutexing, isolates CheckTx by copying the root multistore, refactors BaseApp ante/mempool/span handling and tx lifecycle, deletes stale genesis hash during testnetify, and updates InjectiveLabs CometBFT dependency pins. ChangesABCI Concurrency and State Isolation
Sequence DiagramsequenceDiagram
participant ABCI
participant BaseApp as BaseApp.runTxWithMultiStore
participant Store as RootMultiStore.Copy
participant AnteHandler as Ante Handler
participant Mempool
participant Tracer as Tracing Span
ABCI->>BaseApp: runTxWithMultiStore
BaseApp->>Tracer: SpanFromContext (metricsRunTxSpan)
alt execModeCheck
BaseApp->>Store: Copy multistore
BaseApp->>BaseApp: create cached branch + set AnteMutexes
end
BaseApp->>AnteHandler: cacheTxContext (anteCtx, msCache)
alt Ante success
AnteHandler->>BaseApp: commit msCache (timed by execMode)
else Ante failure
alt execModeReCheck && !ErrWrongSequence
AnteHandler->>Mempool: RemoveTx
end
end
alt execModeFinalize
BaseApp->>Mempool: deferred RemoveTx
end
BaseApp->>Tracer: Reassign span to runTx span
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/start.go (1)
998-1003:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the exact genesis bytes you just wrote.
This path re-encodes
genDocafterAppGenesis.SaveAsalready wrote the file. Because the hash mismatch you are fixing is byte-sensitive, using a second marshaling path here can leavegenesisDocinstateDBout of sync with the on-disk genesis again. Reuse the file bytes fromgenFilePathor a shared helper for both writes.Proposed fix
- b, err := cmtjson.MarshalIndent(genDoc, "", " ") + b, err := os.ReadFile(genFilePath) if err != nil { return nil, err } if err := stateDB.SetSync([]byte("genesisDoc"), b); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/start.go` around lines 998 - 1003, The code re-marshals genDoc with cmtjson.MarshalIndent before writing to stateDB, which can produce byte-different output than the bytes written by AppGenesis.SaveAs; instead, persist the exact bytes that were written to disk: retrieve the genesis file bytes (e.g., read the file at genFilePath that AppGenesis.SaveAs wrote, or change AppGenesis.SaveAs to return the written []byte) and pass those exact bytes into stateDB.SetSync([]byte("genesisDoc"), fileBytes) rather than re-encoding genDoc with cmtjson.MarshalIndent.baseapp/baseapp.go (1)
1020-1035:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn ante events on ante failure instead of dropping them.
When ante fails, Line 1034 returns
nilante events even though events are already collected. This loses useful CheckTx/ReCheck diagnostics.Suggested fix
- events := ctx.EventManager().Events() + events := ctx.EventManager().Events() + anteEvents = events.ToABCIEvents() ... - return gInfo, nil, nil, err + return gInfo, nil, anteEvents, err } ... - anteEvents = events.ToABCIEvents()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baseapp/baseapp.go` around lines 1020 - 1035, The ante failure branch currently discards collected ante events by returning nil; preserve and return the collected events variable (events or anteEvents as used nearby) instead of nil so diagnostics are not lost. In the error handling after the AnteHandler (the block that checks err, mode == execModeReCheck, sdkerrors.ErrWrongSequence and potentially calls app.mempool.Remove(tx)), change the final return from "return gInfo, nil, nil, err" to return the collected ante events (e.g., "return gInfo, nil, anteEvents, err" or the local events variable) so ante events from ctx.EventManager().Events() are propagated back to callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baseapp/baseapp.go`:
- Around line 1042-1048: In CheckTx (execModeCheck) the code calls
msCache.Write() unconditionally after app.mempool.Insert, which can panic if
app.anteHandler == nil because msCache is never initialized; fix by guarding the
write with a nil-check (e.g., if msCache != nil { msCache.Write() }) or ensure
msCache is created earlier when needed—update the block containing
execModeCheck, app.mempool.Insert, and msCache.Write to only call msCache.Write
when msCache is non-nil.
- Around line 967-969: The mempool removal error is being wrapped into the
existing accumulator `err` using `errorsmod.Wrapf(err, ...)`, which returns nil
when `err` is nil and thus drops `mempoolErr`; update the finalize path so you
wrap or join `mempoolErr` itself (or use errorsmod.Join) instead of wrapping
`err` — locate the block around `app.mempool.Remove(tx)` and replace the `err =
errorsmod.Wrapf(err, "mempool remove err: %s", mempoolErr.Error())` logic with
code that preserves `mempoolErr` (e.g., `err = errorsmod.Wrapf(mempoolErr,
"mempool remove err: %s", mempoolErr.Error())` or `err = errorsmod.Join(err,
mempoolErr)`) so mempool removal failures are not silently discarded.
In `@server/start.go`:
- Around line 789-795: The stateDB opened by cmtcfg.DefaultDBProvider is not
closed on intermediate error paths (it currently defers closing later), so move
the close immediately after the successful open: after stateDB, err :=
cmtcfg.DefaultDBProvider(...) and the err check, call defer stateDB.Close() (or
ensure stateDB.Close() is invoked on every subsequent early return such as after
stateDB.DeleteSync and any addrbook/blockstore setup failures) so the DB handle
is always closed on error; reference stateDB, cmtcfg.DefaultDBProvider, and
stateDB.DeleteSync to locate the code to change.
In `@store/rootmulti/store.go`:
- Around line 130-132: The code does unchecked type assertions on
copyRs.stores[key] (e.g., when assigning dstKV) which can panic if the
destination store is missing or the wrong type; update the logic around
copyRs.stores[key] and the block that assigns dstKV/srcKV in the function
containing copyStore so you first check existence (ok := copyRs.stores[key]) and
type-assert safely (dstKV, ok := value.(types.KVStore)), and if either check
fails return a descriptive error instead of panicking; apply the same guarded
checks for the other similar block referenced (lines around the second copyStore
call).
---
Outside diff comments:
In `@baseapp/baseapp.go`:
- Around line 1020-1035: The ante failure branch currently discards collected
ante events by returning nil; preserve and return the collected events variable
(events or anteEvents as used nearby) instead of nil so diagnostics are not
lost. In the error handling after the AnteHandler (the block that checks err,
mode == execModeReCheck, sdkerrors.ErrWrongSequence and potentially calls
app.mempool.Remove(tx)), change the final return from "return gInfo, nil, nil,
err" to return the collected ante events (e.g., "return gInfo, nil, anteEvents,
err" or the local events variable) so ante events from
ctx.EventManager().Events() are propagated back to callers.
In `@server/start.go`:
- Around line 998-1003: The code re-marshals genDoc with cmtjson.MarshalIndent
before writing to stateDB, which can produce byte-different output than the
bytes written by AppGenesis.SaveAs; instead, persist the exact bytes that were
written to disk: retrieve the genesis file bytes (e.g., read the file at
genFilePath that AppGenesis.SaveAs wrote, or change AppGenesis.SaveAs to return
the written []byte) and pass those exact bytes into
stateDB.SetSync([]byte("genesisDoc"), fileBytes) rather than re-encoding genDoc
with cmtjson.MarshalIndent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fec5f8a0-3b01-46c1-b203-698666e1f2c7
⛔ Files ignored due to path filters (9)
go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumstore/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
baseapp/abci.gobaseapp/baseapp.gogo.modserver/start.gosimapp/go.modstore/go.modstore/rootmulti/store.gotests/go.modx/circuit/go.modx/evidence/go.modx/feegrant/go.modx/genutil/types/genesis.gox/nft/go.modx/upgrade/go.mod
Comment on lines
+967
to
+969
| if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil && !errors.Is(mempoolErr, mempool.ErrTxNotFound) { | ||
| err = errorsmod.Wrapf(err, "mempool remove err: %s", mempoolErr.Error()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Wrapf nil behavior and current project patterns for combining errors.
# Expected: evidence that Wrapf(nil, ...) yields nil (or non-nil), and preferred join/wrap pattern.
rg -n -C2 'func Wrapf\(|Wrapf\(' --type=go
rg -n -C2 'errors\.Join\(|mempool remove err' --type=goRepository: InjectiveLabs/cosmos-sdk
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact surrounding code around baseapp/baseapp.go:967-969
FILE="baseapp/baseapp.go"
echo "=== Context around $FILE:967-969 ==="
sed -n '930,1010p' "$FILE" | nl -ba | sed -n '1,120p'
# 2) Identify how err is declared/initialized in the same function/scope
echo "=== Find 'err :=' / 'var err' nearby ==="
rg -n --no-heading "(\berr\b\s*(:=|=)\s*nil|\bvar\s+err\b|\berr\s*:=)" "$FILE" | tail -n 30
# 3) Locate errorsmod.Wrapf definition / implementation (to understand nil base behavior)
echo "=== Find errorsmod Wrapf implementation in repo ==="
rg -n --no-heading "func .*Wrapf|Wrapf\(" -S "types/errors|errorsmod|errorsmod\.Wrapf" . || true
# 4) Narrow: where is errorsmod imported from?
echo "=== Find errorsmod import path in baseapp/baseapp.go ==="
rg -n --no-heading "errorsmod" "$FILE" | head -n 20
# 5) See how errors.Join is used in this repo (patterns for combining errors)
echo "=== Search for errors.Join usage in repo ==="
rg -n --no-heading "errors\.Join\(" --type=go . | head -n 50Repository: InjectiveLabs/cosmos-sdk
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="baseapp/baseapp.go"
echo "=== Find exact location of mempool.Remove(tx) around the reported lines ==="
rg -n "mempool\.Remove\(tx\)" "$FILE"
echo "=== Show surrounding code with line numbers (no nl available) ==="
awk 'NR>=940 && NR<=990 {printf "%6d\t%s\n", NR, $0}' "$FILE" | sed -n '1,60p'
echo "=== Show a wider window to understand how err is declared/accumulated ==="
awk 'NR>=880 && NR<=1030 {printf "%6d\t%s\n", NR, $0}' "$FILE" > /tmp/baseapp_slice.txt
sed -n '1,200p' /tmp/baseapp_slice.txt
echo "=== Find where err is declared/initialized within this vicinity ==="
rg -n --no-heading "(\berr\s*[:=]=|var\s+err\s+error|\berr\s*:\=|if\s+err\s*:=)" "$FILE" | rg -n "9(3[0-9]|4[0-9]|5[0-9]|6[0-9]|7[0-9]|8[0-9]|9[0-9])" || true
echo "=== Locate errorsmod import in baseapp/baseapp.go ==="
rg -n "errorsmod" "$FILE" | head -n 40Repository: InjectiveLabs/cosmos-sdk
Length of output: 10392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Try to locate the cosmossdk.io/errors package in the module cache and inspect Wrapf implementation.
echo "=== Locate cosmossdk.io/errors in module cache ==="
if command -v go >/dev/null 2>&1; then
GOPATH="$(go env GOPATH)"
echo "GOPATH=$GOPATH"
# Print candidate dirs containing Wrapf in that module, if present
rg -n --hidden --no-mmap "func\s+Wrapf\s*\(" "$GOPATH/pkg/mod" -S cosmossdk.io/errors >/dev/null 2>&1 || true
# Ask go where the package lives (best-effort)
go list -f '{{.Dir}}' cosmossdk.io/errors 2>/dev/null || true
fi
# 2) If go list succeeded, try to print Wrapf source (best-effort).
PKG_DIR="$(go list -f '{{.Dir}}' cosmossdk.io/errors 2>/dev/null || true)"
if [ -n "${PKG_DIR:-}" ] && [ -d "$PKG_DIR" ]; then
echo "=== Inspect Wrapf implementation at $PKG_DIR ==="
rg -n "func\s+Wrapf\s*\(" "$PKG_DIR" -S
# show surrounding implementation lines for Wrapf
FILE="$(rg -n "func\s+Wrapf\s*\(" "$PKG_DIR" -S --files | head -n 1 || true)"
if [ -n "${FILE:-}" ]; then
echo "=== Wrapf file: $FILE ==="
sed -n '1,220p' "$FILE" | head -n 200 || true
# narrower window around Wrapf
LINE="$(rg -n "func\s+Wrapf\s*\(" "$FILE" -S | head -n 1 | cut -d: -f1)"
if [ -n "${LINE:-}" ]; then
START=$((LINE-20))
END=$((LINE+60))
sed -n "${START},${END}p" "$FILE" | nl -ba | head -n 120 || true
fi
fi
fi
# 3) Search this repo for patterns of combining errors or wrapping possibly-nil base errors.
echo "=== Search this repo for errors.Join usage ==="
rg -n --no-heading "errors\.Join\(" --type=go . | head -n 50 || true
echo "=== Search this repo for errorsmod.Wrapf usage where base is possibly an error variable ==="
# Focus on calls of the form errorsmod.Wrapf(err,
rg -n --no-heading "errorsmod\.Wrapf\(err\s*," --type=go . | head -n 50 || true
# Also check for any Wrapf(nil, ...) patterns.
rg -n --no-heading "errorsmod\.Wrapf\(\s*nil\s*," --type=go . || trueRepository: InjectiveLabs/cosmos-sdk
Length of output: 11853
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the Wrapf implementation in cosmossdk.io/errors@v1.0.1
PKG_DIR="$(go list -f '{{.Dir}}' cosmossdk.io/errors 2>/dev/null || true)"
echo "PKG_DIR=$PKG_DIR"
# Print the exact Wrapf implementation (and surrounding nil-handling)
if [ -n "$PKG_DIR" ] && [ -f "$PKG_DIR/errors.go" ]; then
echo "=== errors.go: Wrapf implementation context ==="
awk 'NR>=160 && NR<=260 {print NR ":" $0}' "$PKG_DIR/errors.go" | sed -n '1,120p'
else
echo "Could not find $PKG_DIR/errors.go"
exit 1
fi
# Also capture any helpers like Wrap/Join used by that Wrapf
echo "=== Search for Join usage in this package ==="
rg -n --no-heading "func\s+Join\s*\(|Join\(" "$PKG_DIR" || true
# Check how this repo combines errors (limited output)
echo "=== Sample errors.Join usage in repo ==="
rg -n --no-heading "errors\.Join\(" --type=go . | head -n 50 || true
echo "=== Sample errorsmod.Wrapf(err, ...) usage where err might be nil ==="
rg -n --no-heading "errorsmod\.Wrapf\(\s*err\s*," --type=go . | head -n 50 || trueRepository: InjectiveLabs/cosmos-sdk
Length of output: 7633
Fix mempool error wrapping in baseapp finalize path
errorsmod.Wrapf returns nil when the wrapped error is nil, so when the current err accumulator is nil, mempool removal failures are dropped at lines 967-969. Wrap/join mempoolErr instead to preserve the failure.
Suggested fix
- if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil && !errors.Is(mempoolErr, mempool.ErrTxNotFound) {
- err = errorsmod.Wrapf(err, "mempool remove err: %s", mempoolErr.Error())
+ if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil && !errors.Is(mempoolErr, mempool.ErrTxNotFound) {
+ if err != nil {
+ err = errors.Join(err, errorsmod.Wrapf(mempoolErr, "mempool remove err"))
+ } else {
+ err = errorsmod.Wrapf(mempoolErr, "mempool remove 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.
Suggested change
| if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil && !errors.Is(mempoolErr, mempool.ErrTxNotFound) { | |
| err = errorsmod.Wrapf(err, "mempool remove err: %s", mempoolErr.Error()) | |
| } | |
| if mempoolErr := app.mempool.Remove(tx); mempoolErr != nil && !errors.Is(mempoolErr, mempool.ErrTxNotFound) { | |
| if err != nil { | |
| err = errors.Join(err, errorsmod.Wrapf(mempoolErr, "mempool remove err")) | |
| } else { | |
| err = errorsmod.Wrapf(mempoolErr, "mempool remove err") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp/baseapp.go` around lines 967 - 969, The mempool removal error is
being wrapped into the existing accumulator `err` using `errorsmod.Wrapf(err,
...)`, which returns nil when `err` is nil and thus drops `mempoolErr`; update
the finalize path so you wrap or join `mempoolErr` itself (or use
errorsmod.Join) instead of wrapping `err` — locate the block around
`app.mempool.Remove(tx)` and replace the `err = errorsmod.Wrapf(err, "mempool
remove err: %s", mempoolErr.Error())` logic with code that preserves
`mempoolErr` (e.g., `err = errorsmod.Wrapf(mempoolErr, "mempool remove err: %s",
mempoolErr.Error())` or `err = errorsmod.Join(err, mempoolErr)`) so mempool
removal failures are not silently discarded.
Comment on lines
1042
to
1048
| if mode == execModeCheck { | ||
| err = app.mempool.Insert(ctx, tx) | ||
| if err != nil { | ||
| return gInfo, nil, anteEvents, err | ||
| } | ||
| } else if mode == execModeFinalize { | ||
| err = app.mempool.Remove(tx) | ||
| if err != nil && !errors.Is(err, mempool.ErrTxNotFound) { | ||
| return gInfo, nil, anteEvents, | ||
| fmt.Errorf("failed to remove tx from mempool: %w", err) | ||
| } else { | ||
| msCache.Write() // commit ante changes since we succeeded on tx insertion into mempool | ||
| } |
There was a problem hiding this comment.
Prevent nil msCache write when no ante handler is configured.
In CheckTx mode, msCache is only initialized inside the ante block. If app.anteHandler == nil, Line 1047 can panic on msCache.Write().
Suggested fix
if mode == execModeCheck {
err = app.mempool.Insert(ctx, tx)
if err != nil {
return gInfo, nil, anteEvents, err
- } else {
+ } else if msCache != nil {
msCache.Write() // commit ante changes since we succeeded on tx insertion into mempool
}
}📝 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.
Suggested change
| if mode == execModeCheck { | |
| err = app.mempool.Insert(ctx, tx) | |
| if err != nil { | |
| return gInfo, nil, anteEvents, err | |
| } | |
| } else if mode == execModeFinalize { | |
| err = app.mempool.Remove(tx) | |
| if err != nil && !errors.Is(err, mempool.ErrTxNotFound) { | |
| return gInfo, nil, anteEvents, | |
| fmt.Errorf("failed to remove tx from mempool: %w", err) | |
| } else { | |
| msCache.Write() // commit ante changes since we succeeded on tx insertion into mempool | |
| } | |
| if mode == execModeCheck { | |
| err = app.mempool.Insert(ctx, tx) | |
| if err != nil { | |
| return gInfo, nil, anteEvents, err | |
| } else if msCache != nil { | |
| msCache.Write() // commit ante changes since we succeeded on tx insertion into mempool | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp/baseapp.go` around lines 1042 - 1048, In CheckTx (execModeCheck) the
code calls msCache.Write() unconditionally after app.mempool.Insert, which can
panic if app.anteHandler == nil because msCache is never initialized; fix by
guarding the write with a nil-check (e.g., if msCache != nil { msCache.Write()
}) or ensure msCache is created earlier when needed—update the block containing
execModeCheck, app.mempool.Insert, and msCache.Write to only call msCache.Write
when msCache is non-nil.
Comment on lines
+789
to
+795
| stateDB, err := cmtcfg.DefaultDBProvider(&cmtcfg.DBContext{ID: "state", Config: config}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := stateDB.DeleteSync([]byte("genesisDocHash")); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Close stateDB on all error paths.
stateDB now opens at Line 789, but its defer still sits later at Line 819. Any failure during the addrbook or blockstore setup leaks the DB handle. Move the close immediately after the successful open.
Proposed fix
stateDB, err := cmtcfg.DefaultDBProvider(&cmtcfg.DBContext{ID: "state", Config: config})
if err != nil {
return nil, err
}
+defer stateDB.Close()
+
if err := stateDB.DeleteSync([]byte("genesisDocHash")); err != nil {
return nil, err
}
...
- defer stateDB.Close()📝 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.
Suggested change
| stateDB, err := cmtcfg.DefaultDBProvider(&cmtcfg.DBContext{ID: "state", Config: config}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if err := stateDB.DeleteSync([]byte("genesisDocHash")); err != nil { | |
| return nil, err | |
| } | |
| stateDB, err := cmtcfg.DefaultDBProvider(&cmtcfg.DBContext{ID: "state", Config: config}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer stateDB.Close() | |
| if err := stateDB.DeleteSync([]byte("genesisDocHash")); err != nil { | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/start.go` around lines 789 - 795, The stateDB opened by
cmtcfg.DefaultDBProvider is not closed on intermediate error paths (it currently
defers closing later), so move the close immediately after the successful open:
after stateDB, err := cmtcfg.DefaultDBProvider(...) and the err check, call
defer stateDB.Close() (or ensure stateDB.Close() is invoked on every subsequent
early return such as after stateDB.DeleteSync and any addrbook/blockstore setup
failures) so the DB handle is always closed on error; reference stateDB,
cmtcfg.DefaultDBProvider, and stateDB.DeleteSync to locate the code to change.
Comment on lines
+130
to
+132
| srcKV := src.(types.KVStore) | ||
| dstKV := copyRs.stores[key].(types.KVStore) | ||
| copyStore(srcKV, dstKV) |
There was a problem hiding this comment.
Guard destination store lookups before type assertions.
copyRs.stores[key] is asserted without existence/type checks. If a custom storeLoader does not mount the expected non-disk store, this panics at runtime instead of returning an error.
Suggested fix
- srcKV := src.(types.KVStore)
- dstKV := copyRs.stores[key].(types.KVStore)
+ srcKV := src.(types.KVStore)
+ dstStore, ok := copyRs.stores[key]
+ if !ok {
+ return nil, fmt.Errorf("missing destination memory store for key %s", key.Name())
+ }
+ dstKV, ok := dstStore.(types.KVStore)
+ if !ok {
+ return nil, fmt.Errorf("destination store for key %s is not KVStore: %T", key.Name(), dstStore)
+ }
copyStore(srcKV, dstKV)- srcKV := src.(types.ObjKVStore)
- dstKV := copyRs.stores[key].(types.ObjKVStore)
+ srcKV := src.(types.ObjKVStore)
+ dstStore, ok := copyRs.stores[key]
+ if !ok {
+ return nil, fmt.Errorf("missing destination object store for key %s", key.Name())
+ }
+ dstKV, ok := dstStore.(types.ObjKVStore)
+ if !ok {
+ return nil, fmt.Errorf("destination store for key %s is not ObjKVStore: %T", key.Name(), dstStore)
+ }
copyStore(srcKV, dstKV)Also applies to: 138-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@store/rootmulti/store.go` around lines 130 - 132, The code does unchecked
type assertions on copyRs.stores[key] (e.g., when assigning dstKV) which can
panic if the destination store is missing or the wrong type; update the logic
around copyRs.stores[key] and the block that assigns dstKV/srcKV in the function
containing copyStore so you first check existence (ok := copyRs.stores[key]) and
type-assert safely (dstKV, ok := value.(types.KVStore)), and if either check
fails return a descriptive error instead of panicking; apply the same guarded
checks for the other similar block referenced (lines around the second copyStore
call).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
main PR: https://github.com/InjectiveLabs/injective-core/pull/2375
Summary by CodeRabbit
Improvements
Bug Fixes
Chores