fix: check tx issue and add unsafe-local-validator for local upgrade testing#112
Merged
AdriaCarrera merged 7 commits intomainfrom Feb 26, 2026
Merged
fix: check tx issue and add unsafe-local-validator for local upgrade testing#112AdriaCarrera merged 7 commits intomainfrom
AdriaCarrera merged 7 commits intomainfrom
Conversation
AdriaCarrera
commented
Feb 25, 2026
| // use Cosmos-SDK fork to enable Ledger functionality | ||
| github.com/cosmos/cosmos-sdk => github.com/xrplevm/cosmos-sdk v0.53.6-xrplevm.1 | ||
| // cosmos evm private fork | ||
| github.com/cosmos/evm => github.com/xrplevm/evm-sec-papyrus v0.6.0-xrplevm.3 |
Contributor
Author
There was a problem hiding this comment.
You can find the changes in this release here:
xrplevm/evm-sec-papyrus@f9cdf28
📝 WalkthroughWalkthroughAdds EVM keeper coin-info initialization, introduces a new in-place testnet command that mutates consensus and application state to add/replace a local validator, registers the command in the CLI, bumps an evm module replace directive, and adds a local upgrade test script. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "testnetUnsafeStartLocalValidatorCmd"
participant Parser as "getCommandArgs"
participant AppCreator as "newTestingApp"
participant Consensus as "updateConsensusState"
participant AppState as "updateApplicationState"
participant Storage as "DB / BlockStore"
User->>CLI: invoke with flags (operator, pubkey, privkey, accounts, replace, upgrade)
CLI->>Parser: parse & validate inputs
Parser-->>CLI: valArgs
CLI->>AppCreator: create testing app (db, logger)
AppCreator->>Storage: load existing app state
AppCreator-->>CLI: initialized app
CLI->>Consensus: modify consensus state (validators, last commits)
Consensus->>Storage: load/save validator info & blockstore
Consensus-->>CLI: consensus persisted
CLI->>AppState: apply application state changes (staking, distribution, slashing, bank, gov, schedule upgrade)
AppState->>Storage: persist state changes
AppState-->>CLI: app state updated
CLI->>User: local validator started / upgrade scheduled
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 8
🧹 Nitpick comments (1)
cmd/exrpd/cmd/in-place-testnet.go (1)
165-170: Validate replaced consensus address format during argument parsing.Line 165-170 only checks non-empty input. Failing fast on malformed consensus addresses here would avoid late, opaque replacement failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/in-place-testnet.go` around lines 165 - 170, The code only checks for an empty replacedConsensusAddress but does not validate its format; update the argument parsing where replacedConsensusAddress is read (the code that calls appOpts.Get(flagReplacedConsensusAddress) and assigns args.replacedConsensusAddress) to parse/validate the bech32 consensus address using the project’s consensus address parser (e.g., sdk.ConsAddressFromBech32 or the equivalent validator function) and return a clear error when parsing fails so malformed addresses are rejected early rather than causing opaque failures later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/exrpd/cmd/in-place-testnet.go`:
- Around line 86-88: The help/example embeds real-looking base64 private-key
material; remove any secret-like literals and replace them with clearly labelled
placeholders (e.g. REPLACE_WITH_VALIDATOR_PRIVKEY,
REPLACE_WITH_VALIDATOR_PUBKEY, REPLACE_WITH_OPERATOR_ADDR) in the example
command lines so the flags --validator-privkey, --validator-pubkey,
--replace-validator, --replaced-operator-address and
--replaced-consensus-address are preserved but do not contain sensitive-looking
values; update both occurrences in the simd testnet unsafe-start-local-validator
example to use placeholders or clearly dummy values (e.g. short non-secret
strings) instead of the long base64 blob.
- Around line 563-565: The three calls to saveValidatorsInfo(stateDB,
state.LastBlockHeight, valInfo) etc. ignore returned errors; change each call to
capture the error (e.g., err := saveValidatorsInfo(...)), check if err != nil
and handle it consistently (log the error with context including the target
block height and valInfo, and return or propagate the error to abort the
in-place testnet setup) so failed writes to stateDB don't leave consensus
metadata inconsistent; update any enclosing function signature to return an
error if necessary (or use the existing error handling path) and ensure
saveValidatorsInfo's error is not discarded.
- Around line 129-132: The check for an empty validator private key uses the
unrelated variable err and wraps it causing a possible runtime panic; in the
function where validatorPrivKey is read from appOpts.Get(flagValidatorPrivKey)
(and err was previously used for pubkey validation), replace the return that
does fmt.Errorf("invalid validator private key %w", err) with a fresh error that
does not wrap err (e.g. fmt.Errorf("invalid validator private key") or
fmt.Errorf("invalid validator private key: %s", validatorPrivKey)) so the
empty-key branch does not reference the unrelated err variable (update the
return where validatorPrivKey == "" to use the new error).
In `@go.mod`:
- Line 288: The go.mod contains an unresolvable replace directive
"github.com/cosmos/evm => github.com/xrplevm/evm-sec-papyrus v0.6.0-xrplevm.4"
which causes module resolution to fail; fix it by either restoring access to
that repository (so git ls-remote succeeds), changing the replace to a reachable
alternative module/version that provides the same API, or removing the replace
directive entirely if it's no longer required—update the replace line
accordingly and run go mod tidy to verify the dependency graph resolves.
In `@scripts/test-local-upgrade.sh`:
- Around line 49-57: Update the usage examples printed by the script to
reference the correct filename test-local-upgrade.sh: replace occurrences of
"./test-upgrade.sh" in the echo that prints the example command and the echo
that prints the export example so both show "./test-local-upgrade.sh" (and
update the export line text from "&& ./test-upgrade.sh" to "&&
./test-local-upgrade.sh"); verify the strings in the echo statements inside
scripts/test-local-upgrade.sh around the usage-printing block.
- Around line 26-31: The dependency check loop that iterates "for cmd in exrpd
jq lz4;" misses tools used later; update that command list to include "wget" and
"tar" so the script validates all required binaries (the loop using variable cmd
and the command -v check) and ensure the existing error message still references
the missing tool name for clarity.
- Around line 1-2: Add strict shell mode to the script so failures abort early:
at the top of scripts/test-local-upgrade.sh (immediately after the existing
shebang), enable "set -euo pipefail" and optionally "IFS=$'\n\t'" so any failing
commands (including the call to unsafe-start-local-validator) or unset variables
cause the script to exit rather than continue silently.
- Around line 69-73: The script uses unquoted variable expansions in commands
wget and the pipeline to tar, which can break on paths containing spaces or glob
characters; update the wget invocation to quote $GENESIS_URL and $NODE_HOME
(e.g., wget -O "$NODE_HOME/config/genesis.json" "$GENESIS_URL") and quote the
snapshot/tar pipeline by using "$SNAPSHOT_PATH" and "$NODE_HOME" in the lz4/tar
command (e.g., lz4 -c -d "$SNAPSHOT_PATH" | tar -x -C "$NODE_HOME") so that
GENESIS_URL, NODE_HOME and SNAPSHOT_PATH are safely handled.
---
Nitpick comments:
In `@cmd/exrpd/cmd/in-place-testnet.go`:
- Around line 165-170: The code only checks for an empty
replacedConsensusAddress but does not validate its format; update the argument
parsing where replacedConsensusAddress is read (the code that calls
appOpts.Get(flagReplacedConsensusAddress) and assigns
args.replacedConsensusAddress) to parse/validate the bech32 consensus address
using the project’s consensus address parser (e.g., sdk.ConsAddressFromBech32 or
the equivalent validator function) and return a clear error when parsing fails
so malformed addresses are rejected early rather than causing opaque failures
later.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
app/app.gocmd/exrpd/cmd/in-place-testnet.gocmd/exrpd/cmd/root.gogo.modscripts/test-local-upgrade.sh
GuillemGarciaDev
approved these changes
Feb 25, 2026
JordiParraCrespo
approved these changes
Feb 25, 2026
| cmd := server.StartCmd(ac.newTestingApp, xrplevm.DefaultNodeHome) | ||
| cmd.Use = "unsafe-start-local-validator" | ||
| cmd.Short = "Updates chain's application and consensus state with provided validator info and starts the node" | ||
| cmd.Long = `The unsafe-start-local-validator command modifies both application and consensus stores within a local mainnet node and starts the node, |
Contributor
There was a problem hiding this comment.
The local mainnet will depen on if it is created from mainnet or testnet
| }, | ||
| Commission: stakingtypes.Commission{ | ||
| CommissionRates: stakingtypes.CommissionRates{ | ||
| Rate: math.LegacyMustNewDecFromStr("0.05"), |
Contributor
There was a problem hiding this comment.
Are this values aligned with the current chain's values?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cmd/exrpd/cmd/in-place-testnet.go (4)
204-207: Consider renaminggaiaApptoxrplevmAppfor clarity.The variable name
gaiaAppappears to be a leftover from copying Cosmos SDK's Gaia code. Renaming it would improve readability in the xrplevm context.Proposed fix
app := a.newApp(logger, db, traceStore, appOpts) - gaiaApp, ok := app.(*xrplevm.App) + xrplevmApp, ok := app.(*xrplevm.App) if !ok { panic(errors.New("invalid xrplevm application")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/in-place-testnet.go` around lines 204 - 207, Rename the leftover variable gaiaApp to xrplevmApp to better reflect the concrete type xrplevm.App: locate the assignment "gaiaApp, ok := app.(*xrplevm.App)" and change the identifier to "xrplevmApp" (and update any subsequent uses of gaiaApp in this scope) so the variable name matches the type and improves readability.
396-403: Inconsistent error handling: panics here but returns errors elsewhere.This block uses
panic()for an upgrade scheduling failure, while the rest of the function returns errors. This inconsistency makes error handling unpredictable.Proposed fix
if args.upgradeVersion != "" { upgradeHeight := app.LastBlockHeight() + 5 upgradePlan := upgradetypes.Plan{ Height: upgradeHeight, Name: args.upgradeVersion, Info: "Testnet upgrade to " + args.upgradeVersion, } if err := app.UpgradeKeeper.ScheduleUpgrade(appCtx, upgradePlan); err != nil { - panic( - fmt.Errorf( - "failed to schedule upgrade %s during BeginBlock at height %d: %w", - upgradePlan.Name, upgradeHeight, err, - ), - ) + return fmt.Errorf( + "failed to schedule upgrade %s at height %d: %w", + upgradePlan.Name, upgradeHeight, err, + ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/in-place-testnet.go` around lines 396 - 403, The ScheduleUpgrade failure is currently panicking inside the BeginBlock flow; replace the panic with consistent error propagation by returning a wrapped error instead. In the block that calls app.UpgradeKeeper.ScheduleUpgrade(appCtx, upgradePlan) (using variables appCtx, upgradePlan, upgradeHeight), capture the error and return fmt.Errorf("failed to schedule upgrade %s during BeginBlock at height %d: %w", upgradePlan.Name, upgradeHeight, err) rather than calling panic, so the surrounding function returns an error consistent with other code paths.
308-320: Multiple critical state mutations have errors silently suppressed.
SetValidator,SetValidatorByPowerIndex, andSetLastValidatorPowerall have//nolint:errcheckdirectives. While these operations rarely fail, suppressing errors during validator state setup could mask issues and leave state inconsistent.Consider checking these errors, especially since
SetValidatorByConsAddrat line 310 already has proper error handling.Proposed fix
// Add our validator to power and last validators store - //nolint:errcheck - app.StakingKeeper.SetValidator(appCtx, newVal) + if err := app.StakingKeeper.SetValidator(appCtx, newVal); err != nil { + return err + } err = app.StakingKeeper.SetValidatorByConsAddr(appCtx, newVal) if err != nil { return err } - //nolint:errcheck - app.StakingKeeper.SetValidatorByPowerIndex(appCtx, newVal) - //nolint:errcheck - app.StakingKeeper.SetLastValidatorPower(appCtx, newValAddr, valVotingPower) + if err := app.StakingKeeper.SetValidatorByPowerIndex(appCtx, newVal); err != nil { + return err + } + if err := app.StakingKeeper.SetLastValidatorPower(appCtx, newValAddr, valVotingPower); err != nil { + return err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/in-place-testnet.go` around lines 308 - 320, The validator setup calls suppress errors for SetValidator, SetValidatorByPowerIndex, and SetLastValidatorPower; remove the //nolint:errcheck suppressions and propagate any returned errors like you do for SetValidatorByConsAddr: call app.StakingKeeper.SetValidator(appCtx, newVal), capture its error into err and return it if non-nil; do the same for app.StakingKeeper.SetValidatorByPowerIndex(appCtx, newVal) and app.StakingKeeper.SetLastValidatorPower(appCtx, newValAddr, valVotingPower), ensuring each call checks err and returns on failure before proceeding to Hooks().AfterValidatorCreated.
438-443: Deferred close error overwrites any prior error, potentially losing context.If
stateStore.Save(state)fails at line 557 and thenstateStore.Close()also fails, the original error is overwritten by the close error. Consider using a named return or error joining.Proposed fix using error joining
defer func() { if derr := stateStore.Close(); derr != nil { logger.Error("Failed to close consensus state db", "err", derr) - err = derr + err = errors.Join(err, derr) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/in-place-testnet.go` around lines 438 - 443, The deferred close handler for stateStore currently replaces any existing error with the close error (`stateStore.Close()` sets `err = derr`), which loses earlier errors (e.g., from `stateStore.Save(state)`); change the defer to preserve both errors by joining or wrapping them instead of overwriting — capture the close error from `stateStore.Close()` as `derr`, and if the named return error `err` is non-nil combine them (for example with `errors.Join(err, derr)` or by wrapping: `err = fmt.Errorf("%w; close error: %v", err, derr)`), and keep the `logger.Error("Failed to close consensus state db", "err", derr)` logging; update the defer surrounding `stateStore.Close()` to reference `err` and perform error joining rather than assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/exrpd/cmd/in-place-testnet.go`:
- Around line 361-365: The call to
math.NewIntFromString("10000000000000000000000") ignores its boolean success
return so a parsing failure would silently yield zero; update the code around
the amount variable (the math.NewIntFromString call that builds defaultCoins) to
check the boolean result, and if parsing failed handle it explicitly (e.g.,
return an error, log.Fatalf/process exit, or panic) so tests fail fast instead
of funding accounts with zero; locate the math.NewIntFromString invocation that
creates amount and add the boolean check and an appropriate fatal/logging/error
return.
- Line 88: Fix the typo in the example command where the flag is written as "--
replaced-operator-address" (with a space) — change it to the correct
"--replaced-operator-address" so the CLI will parse the flag; while editing,
replace the real-looking values for "--validator-privkey", "--validator-pubkey",
"--validator-operator", "--accounts-to-fund", and "--replaced-consensus-address"
with clearly marked placeholders to avoid leaking sensitive/sample secret
material and keep the other flags unchanged (e.g., references in the line:
"validator-operator", "validator-pubkey", "validator-privkey",
"accounts-to-fund", "replace-validator", "replaced-operator-address",
"replaced-consensus-address").
- Around line 584-588: The DeleteLatestBlock call is currently ignoring errors
which can leave the block store inconsistent with app/consensus heights; update
the blockStore.DeleteLatestBlock invocation (in the same conditional that checks
store.LoadBlockStoreState result and compares blockStoreState.Height to
state.LastBlockHeight and appHeight) to capture its returned error, handle it by
logging the failure with context (including blockStoreState.Height,
state.LastBlockHeight, appHeight) and/or return the error up the call stack so
the caller can abort; ensure you remove the nolint:errcheck and propagate or log
the error from DeleteLatestBlock instead of discarding it.
---
Nitpick comments:
In `@cmd/exrpd/cmd/in-place-testnet.go`:
- Around line 204-207: Rename the leftover variable gaiaApp to xrplevmApp to
better reflect the concrete type xrplevm.App: locate the assignment "gaiaApp, ok
:= app.(*xrplevm.App)" and change the identifier to "xrplevmApp" (and update any
subsequent uses of gaiaApp in this scope) so the variable name matches the type
and improves readability.
- Around line 396-403: The ScheduleUpgrade failure is currently panicking inside
the BeginBlock flow; replace the panic with consistent error propagation by
returning a wrapped error instead. In the block that calls
app.UpgradeKeeper.ScheduleUpgrade(appCtx, upgradePlan) (using variables appCtx,
upgradePlan, upgradeHeight), capture the error and return fmt.Errorf("failed to
schedule upgrade %s during BeginBlock at height %d: %w", upgradePlan.Name,
upgradeHeight, err) rather than calling panic, so the surrounding function
returns an error consistent with other code paths.
- Around line 308-320: The validator setup calls suppress errors for
SetValidator, SetValidatorByPowerIndex, and SetLastValidatorPower; remove the
//nolint:errcheck suppressions and propagate any returned errors like you do for
SetValidatorByConsAddr: call app.StakingKeeper.SetValidator(appCtx, newVal),
capture its error into err and return it if non-nil; do the same for
app.StakingKeeper.SetValidatorByPowerIndex(appCtx, newVal) and
app.StakingKeeper.SetLastValidatorPower(appCtx, newValAddr, valVotingPower),
ensuring each call checks err and returns on failure before proceeding to
Hooks().AfterValidatorCreated.
- Around line 438-443: The deferred close handler for stateStore currently
replaces any existing error with the close error (`stateStore.Close()` sets `err
= derr`), which loses earlier errors (e.g., from `stateStore.Save(state)`);
change the defer to preserve both errors by joining or wrapping them instead of
overwriting — capture the close error from `stateStore.Close()` as `derr`, and
if the named return error `err` is non-nil combine them (for example with
`errors.Join(err, derr)` or by wrapping: `err = fmt.Errorf("%w; close error:
%v", err, derr)`), and keep the `logger.Error("Failed to close consensus state
db", "err", derr)` logging; update the defer surrounding `stateStore.Close()` to
reference `err` and perform error joining rather than assignment.
| # Wipes the entire validator set and replaces it with a single validator | ||
| simd testnet unsafe-start-local-validator --validator-operator="cosmosvaloper17fjdcqy7g80pn0seexcch5pg0dtvs45p57t97r" --validator-pubkey="SLpHEfzQHuuNO9J1BB/hXyiH6c1NmpoIVQ2pMWmyctE=" --validator-privkey="AiayvI2px5CZVl/uOGmacfFjcIBoyk3Oa2JPBO6zEcdIukcR/NAe64070nUEH+FfKIfpzU2amghVDakxabJy0Q==" --accounts-to-fund="cosmos1ju6tlfclulxumtt2kglvnxduj5d93a64r5czge,cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl" [other_server_start_flags] | ||
| # Replaces the specified validator with the new keys | ||
| simd testnet unsafe-start-local-validator --validator-operator="cosmosvaloper17fjdcqy7g80pn0seexcch5pg0dtvs45p57t97r" --validator-pubkey="SLpHEfzQHuuNO9J1BB/hXyiH6c1NmpoIVQ2pMWmyctE=" --validator-privkey="AiayvI2px5CZVl/uOGmacfFjcIBoyk3Oa2JPBO6zEcdIukcR/NAe64070nUEH+FfKIfpzU2amghVDakxabJy0Q==" --accounts-to-fund="cosmos1ju6tlfclulxumtt2kglvnxduj5d93a64r5czge,cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl" --replace-validator -- replaced-operator-address="cosmosvaloper1r5v5srda7xfth3hn2s26txvrcrntldju7lnwmv" --replaced-consensus-address="04B333F6E43751948C2D56B273DC41C3E13E5932" [other_server_start_flags] |
There was a problem hiding this comment.
Typo in example command: -- replaced-operator-address should be --replaced-operator-address.
The example has an errant space after -- which would cause the flag to be misinterpreted.
Proposed fix
- simd testnet unsafe-start-local-validator --validator-operator="cosmosvaloper17fjdcqy7g80pn0seexcch5pg0dtvs45p57t97r" --validator-pubkey="SLpHEfzQHuuNO9J1BB/hXyiH6c1NmpoIVQ2pMWmyctE=" --validator-privkey="AiayvI2px5CZVl/uOGmacfFjcIBoyk3Oa2JPBO6zEcdIukcR/NAe64070nUEH+FfKIfpzU2amghVDakxabJy0Q==" --accounts-to-fund="cosmos1ju6tlfclulxumtt2kglvnxduj5d93a64r5czge,cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl" --replace-validator -- replaced-operator-address="cosmosvaloper1r5v5srda7xfth3hn2s26txvrcrntldju7lnwmv" --replaced-consensus-address="04B333F6E43751948C2D56B273DC41C3E13E5932" [other_server_start_flags]
+ simd testnet unsafe-start-local-validator --validator-operator="cosmosvaloper17fjdcqy7g80pn0seexcch5pg0dtvs45p57t97r" --validator-pubkey="<BASE64_ED25519_PUBKEY>" --validator-privkey="<BASE64_ED25519_PRIVKEY>" --accounts-to-fund="cosmos1ju6tlfclulxumtt2kglvnxduj5d93a64r5czge,cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl" --replace-validator --replaced-operator-address="cosmosvaloper1r5v5srda7xfth3hn2s26txvrcrntldju7lnwmv" --replaced-consensus-address="04B333F6E43751948C2D56B273DC41C3E13E5932" [other_server_start_flags]Note: This also addresses the previous review comment about replacing real-looking base64 private key material with placeholders.
📝 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
| simd testnet unsafe-start-local-validator --validator-operator="cosmosvaloper17fjdcqy7g80pn0seexcch5pg0dtvs45p57t97r" --validator-pubkey="SLpHEfzQHuuNO9J1BB/hXyiH6c1NmpoIVQ2pMWmyctE=" --validator-privkey="AiayvI2px5CZVl/uOGmacfFjcIBoyk3Oa2JPBO6zEcdIukcR/NAe64070nUEH+FfKIfpzU2amghVDakxabJy0Q==" --accounts-to-fund="cosmos1ju6tlfclulxumtt2kglvnxduj5d93a64r5czge,cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl" --replace-validator -- replaced-operator-address="cosmosvaloper1r5v5srda7xfth3hn2s26txvrcrntldju7lnwmv" --replaced-consensus-address="04B333F6E43751948C2D56B273DC41C3E13E5932" [other_server_start_flags] | |
| simd testnet unsafe-start-local-validator --validator-operator="cosmosvaloper17fjdcqy7g80pn0seexcch5pg0dtvs45p57t97r" --validator-pubkey="<BASE64_ED25519_PUBKEY>" --validator-privkey="<BASE64_ED25519_PRIVKEY>" --accounts-to-fund="cosmos1ju6tlfclulxumtt2kglvnxduj5d93a64r5czge,cosmos1r5v5srda7xfth3hn2s26txvrcrntldjumt8mhl" --replace-validator --replaced-operator-address="cosmosvaloper1r5v5srda7xfth3hn2s26txvrcrntldju7lnwmv" --replaced-consensus-address="04B333F6E43751948C2D56B273DC41C3E13E5932" [other_server_start_flags] |
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 88-88: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/exrpd/cmd/in-place-testnet.go` at line 88, Fix the typo in the example
command where the flag is written as "-- replaced-operator-address" (with a
space) — change it to the correct "--replaced-operator-address" so the CLI will
parse the flag; while editing, replace the real-looking values for
"--validator-privkey", "--validator-pubkey", "--validator-operator",
"--accounts-to-fund", and "--replaced-consensus-address" with clearly marked
placeholders to avoid leaking sensitive/sample secret material and keep the
other flags unchanged (e.g., references in the line: "validator-operator",
"validator-pubkey", "validator-privkey", "accounts-to-fund",
"replace-validator", "replaced-operator-address", "replaced-consensus-address").
Comment on lines
+361
to
+365
| amount, _ := math.NewIntFromString("10000000000000000000000") | ||
| // Fund accounts with both the bond denom and axrp (needed for gov deposits and gas) | ||
| defaultCoins := sdk.NewCoins( | ||
| sdk.NewCoin(xrplevm.BaseDenom, amount), | ||
| ) |
There was a problem hiding this comment.
Ignored success indicator from math.NewIntFromString could mask parsing failures.
math.NewIntFromString returns a boolean indicating whether parsing succeeded. If the string is malformed, amount defaults to zero, causing test accounts to receive no funds without any error indication.
Proposed fix
- amount, _ := math.NewIntFromString("10000000000000000000000")
+ amount, ok := math.NewIntFromString("10000000000000000000000")
+ if !ok {
+ return errors.New("failed to parse funding amount")
+ }📝 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
| amount, _ := math.NewIntFromString("10000000000000000000000") | |
| // Fund accounts with both the bond denom and axrp (needed for gov deposits and gas) | |
| defaultCoins := sdk.NewCoins( | |
| sdk.NewCoin(xrplevm.BaseDenom, amount), | |
| ) | |
| amount, ok := math.NewIntFromString("10000000000000000000000") | |
| if !ok { | |
| return errors.New("failed to parse funding amount") | |
| } | |
| // Fund accounts with both the bond denom and axrp (needed for gov deposits and gas) | |
| defaultCoins := sdk.NewCoins( | |
| sdk.NewCoin(xrplevm.BaseDenom, amount), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/exrpd/cmd/in-place-testnet.go` around lines 361 - 365, The call to
math.NewIntFromString("10000000000000000000000") ignores its boolean success
return so a parsing failure would silently yield zero; update the code around
the amount variable (the math.NewIntFromString call that builds defaultCoins) to
check the boolean result, and if parsing failed handle it explicitly (e.g.,
return an error, log.Fatalf/process exit, or panic) so tests fail fast instead
of funding accounts with zero; locate the math.NewIntFromString invocation that
creates amount and add the boolean check and an appropriate fatal/logging/error
return.
Comment on lines
+584
to
+588
| blockStoreState := store.LoadBlockStoreState(blockStoreDB) | ||
| if blockStoreState.Height > state.LastBlockHeight && blockStoreState.Height > appHeight { | ||
| //nolint:errcheck | ||
| blockStore.DeleteLatestBlock() | ||
| } |
There was a problem hiding this comment.
DeleteLatestBlock error is silently ignored.
If block deletion fails, the store may be left in an inconsistent state relative to app/consensus heights.
Proposed fix
if blockStoreState.Height > state.LastBlockHeight && blockStoreState.Height > appHeight {
- //nolint:errcheck
- blockStore.DeleteLatestBlock()
+ if err := blockStore.DeleteLatestBlock(); err != nil {
+ logger.Error("Failed to delete latest block", "err", err)
+ return 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
| blockStoreState := store.LoadBlockStoreState(blockStoreDB) | |
| if blockStoreState.Height > state.LastBlockHeight && blockStoreState.Height > appHeight { | |
| //nolint:errcheck | |
| blockStore.DeleteLatestBlock() | |
| } | |
| blockStoreState := store.LoadBlockStoreState(blockStoreDB) | |
| if blockStoreState.Height > state.LastBlockHeight && blockStoreState.Height > appHeight { | |
| if err := blockStore.DeleteLatestBlock(); err != nil { | |
| logger.Error("Failed to delete latest block", "err", err) | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/exrpd/cmd/in-place-testnet.go` around lines 584 - 588, The
DeleteLatestBlock call is currently ignoring errors which can leave the block
store inconsistent with app/consensus heights; update the
blockStore.DeleteLatestBlock invocation (in the same conditional that checks
store.LoadBlockStoreState result and compares blockStoreState.Height to
state.LastBlockHeight and appHeight) to capture its returned error, handle it by
logging the failure with context (including blockStoreState.Height,
state.LastBlockHeight, appHeight) and/or return the error up the call stack so
the caller can abort; ensure you remove the nolint:errcheck and propagate or log
the error from DeleteLatestBlock instead of discarding it.
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
3 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.
Summary by CodeRabbit
New Features
Chores