v0.22.0: - resolve v018-v021 issues #278
Conversation
📝 WalkthroughWalkthroughThis pull request updates several core components. The changes include updating package imports and header handling in the main application, and extensive modifications to the upgrade process with multiple deleted upgrade files and new implementations in v021 and v022. New export functionality and command-line utilities have been added along with server startup enhancements. Additionally, dependency versions and fee payer validations in the ante module are revised. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Cobra CLI
participant E as CustomExportCmd
participant A as Application Exporter
participant O as Output (Stdout/File)
U->>C: Execute "custom-export" command
C->>E: Invoke CustomExportCmd
E->>A: Call customAppExport to export state
A->>E: Return ExportedApp JSON
E->>O: Write JSON to file or stdout
sequenceDiagram
participant UH as Upgrade Handler (v022)
participant MM as Module Manager
participant K as Keepers
participant L as Logger
UH->>MM: Run module migrations
MM-->>UH: Return version map
UH->>K: Apply CustomV022PatchLogic (reward patching, delegation processing)
K-->>UH: Update store and reference counts
UH->>L: Log conditional JSON and process results
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (29)
💤 Files with no reviewable changes (17)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
merge antstalepresh patch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (27)
app/upgrades/v021/upgrades.go (2)
181-265: Consider maintaining consistent logging patterns.
Some logging statements use structured logs (sdkCtx.Logger().Info("message", "key", value)), while others rely on string construction withfmt.Sprintf. Adopting a unified mechanism (preferably the structured approach) would enhance clarity and consistency in logs.
368-371: Fix potential function name typo.
CustommTokensFromSharesTruncatedhas an extra "m" in its identifier. Consider renaming for readability and consistency.-func CustommTokensFromSharesTruncated(t math.Int, ds math.LegacyDec, shares math.LegacyDec) math.LegacyDec { +func CustomTokensFromSharesTruncated(t math.Int, ds math.LegacyDec, shares math.LegacyDec) math.LegacyDec {go.mod (1)
235-238: Consider removing or gating debug replacement lines.If these replacements are for debugging only, keep them out of production or wrap them in a condition to avoid confusion.
app/upgrades/v022/upgrades.go (5)
41-50: Streamline error handling in patch logic.The patch logic calls
panic(err). Evaluate if it’s safer to bubble up errors. Panics might disrupt the entire chain upgrade in production, so returning an error might be more graceful.
197-264: Extract magic number for marginOfErr into a constant.The code uses a 0.095 margin of error. Use a descriptive constant or configuration to avoid confusion and facilitate future changes.
266-383: Review the unconditional panic in patch check.If
patchedis false, the logic triggers a panic. Catching the error might be preferable to prevent a potential chain halt in production scenarios.
385-425: Replace debug printing with structured logging.These print statements can clutter production logs. Switch to a logger or remove them to keep logs consistent and maintainable.
442-454: Revisit the zero reference count panic.Panicking if
historical.ReferenceCountis already zero might be too severe. Evaluate whether returning an error can handle this scenario more gracefully.server/util.go (2)
18-27: Consider using stricter file permissions for trace logs.
Currently, theopenTraceWriterfunction uses0o666, allowing read/write for all. For production environments, consider restricting permissions (e.g.,0o644) to reduce security risk.- 0o666, + 0o644,
29-34: Add usage notes or help text.
AddTestnetCreatorCommandexposes a new CLI command but lacks any descriptive usage messages. Consider clarifying its functionality (e.g., flags and expected arguments) to improve the developer experience.app/upgrades/v022/upgrades_test.go (2)
68-72: Remove or utilize commented-out code.
Lines 68-72 are commented out without explanation. Consider removing them if they are no longer needed, or add a comment explaining why they are temporarily disabled.
88-91: Switch to test logs instead of println for more structured output.
Printing directly to standard output can bury information. Using test logs such ass.T().Logf(ort.Logf) keeps logs organized under the relevant test.- fmt.Println("~~~~~~~~~ POST UPGRADE DEBUG ~~~~~~~~~~~~") - fmt.Printf("delAddr: %v\n", delAddr) - fmt.Printf("valAddr: %v\n", valAddr) + s.T().Logf("~~~~~~~~~ POST UPGRADE DEBUG ~~~~~~~~~~~~") + s.T().Logf("delAddr: %v", delAddr) + s.T().Logf("valAddr: %v", valAddr)cmd/bitsongd/cmd/custom_export.go (5)
1-2: Establish package-level documentation.Consider adding package-level documentation to clearly convey the purpose of this new command package and how it integrates into the overall codebase.
3-20: Consider grouping related imports.Grouping imports by standard library, external libraries, and local packages can improve readability. However, it's a minor style preference.
22-27: Clarify flag naming and usage.The flags such as
FlagForZeroHeight,FlagJailAllowedAddrs, andFlagNewOperatorAddrare meaningful. Brief inline comments or usage clarifications can help maintainers quickly understand these parameters.
136-139: Ensure DB cleanup is handled.Opening the DB is straightforward. Evaluate whether the caller handles closing it in all code paths to prevent resource leakage.
141-150: Add trace file existence checks.If the trace writer file can fail to open or be invalid, consider more robust error and fallback handling to ensure minimal friction during debugging.
cmd/bitsongd/cmd/root.go (2)
270-270: Highlight new testnet command.Registering
AddTestnetCreatorCommandensures better scoping for testnet creation. Confirm that any previously invoked testnet commands or scripts are updated accordingly.
281-281: Add usage docs forcustom-export.Adding
CustomExportCmd(customAppExport, bitsong.DefaultNodeHome)is crucial for debugging. A short mention in the project’s README or command help describing the debugging process will be helpful.app/upgrades/v022/constants.go (5)
9-13: Consider clarifying the usage of patch validator addresses.
Currently,PatchVal1andPatchVal2are defined, but their usage is not immediately clear here. Adding a short comment about when and where they are applied will help future maintainers.
15-19: Add a note explaining emptyStoreUpgrades.
TheUpgradevariable has emptyAddedandDeletedslices forStoreUpgrades. If this is intentional (e.g., no store migrations are needed), it may be helpful to add a comment clarifying.
21-28: Enhance documentation forConditionalJSON.
This struct aggregates multiple upgrade-related data structures. Documenting each field's purpose and how they are used in the upgrade process will greatly improve maintainability.
30-42: Verify JSON tags for distribution slash events.
DistrSlashObject,DistrSlashEvent, andSlashdo not consistently define JSON tags except for a few fields. If these types are serialized and consumed externally, consider adding tags to all fields (or ensuring consistent usage) for clarity.
48-52: Confirm thePatchedDelegationfield naming convention.
The fieldPatchedDelegation string \json:"patch"`` differs from the type name “PatchedDelegation”. Consider renaming the JSON tag to match the field or adding a clarifying comment for consistency.server/start.go (3)
111-207: Evaluate backup mechanism before modifying state inInPlaceTestnetCreator.
This command irreversibly modifies the existing local state after user confirmation. Consider offering an auto-backup functionality (e.g., copying data folder contents before modification) to mitigate accidental data loss.
209-261: Maintain consistency in flag naming and documentation.
A large set of flags is introduced here. While they've been grouped logically, adding short doc comments for potentially confusing flags (e.g.,FlagMempoolMaxTxs,FlagDisableIAVLFastNode) within the code—or referencing relevant docs—would improve dev experience.
872-911: Check for encryption or secure storage of nodeKey.
p2p.LoadOrGenNodeKey(cfg.NodeKeyFile())loads a node key in plaintext form. If operator security requires protecting node identity keys, consider documenting encryption or secure storage at rest.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
app/app.go(6 hunks)app/export_custom.go(1 hunks)app/upgrades/v010/constants.go(0 hunks)app/upgrades/v010/upgrades.go(0 hunks)app/upgrades/v011/constants.go(0 hunks)app/upgrades/v011/upgrades.go(0 hunks)app/upgrades/v013/constants.go(0 hunks)app/upgrades/v013/upgrades.go(0 hunks)app/upgrades/v014/constants.go(0 hunks)app/upgrades/v014/upgrades.go(0 hunks)app/upgrades/v015/constants.go(0 hunks)app/upgrades/v015/upgrades.go(0 hunks)app/upgrades/v016/constants.go(0 hunks)app/upgrades/v016/upgrades.go(0 hunks)app/upgrades/v018/constants.go(0 hunks)app/upgrades/v018/upgrades.go(0 hunks)app/upgrades/v020/constants.go(0 hunks)app/upgrades/v020/upgrade_test.go(0 hunks)app/upgrades/v020/upgrades.go(0 hunks)app/upgrades/v021/upgrades.go(3 hunks)app/upgrades/v022/constants.go(1 hunks)app/upgrades/v022/upgrades.go(1 hunks)app/upgrades/v022/upgrades_test.go(1 hunks)cmd/bitsongd/cmd/custom_export.go(1 hunks)cmd/bitsongd/cmd/root.go(4 hunks)go.mod(4 hunks)server/start.go(1 hunks)server/util.go(1 hunks)x/smart-account/ante/ante.go(1 hunks)
💤 Files with no reviewable changes (17)
- app/upgrades/v013/constants.go
- app/upgrades/v016/upgrades.go
- app/upgrades/v011/constants.go
- app/upgrades/v018/constants.go
- app/upgrades/v015/upgrades.go
- app/upgrades/v020/constants.go
- app/upgrades/v014/constants.go
- app/upgrades/v015/constants.go
- app/upgrades/v020/upgrade_test.go
- app/upgrades/v016/constants.go
- app/upgrades/v011/upgrades.go
- app/upgrades/v014/upgrades.go
- app/upgrades/v010/constants.go
- app/upgrades/v018/upgrades.go
- app/upgrades/v020/upgrades.go
- app/upgrades/v010/upgrades.go
- app/upgrades/v013/upgrades.go
🔇 Additional comments (27)
app/upgrades/v021/upgrades.go (6)
17-18: No issues found.
These new imports from the distribution and staking modules appear correct and properly used.
75-75: Confirm usage.
The call toCustomCalculateDelegationRewardsis a new injection point for reward calculus. Please ensure all relevant invocation paths have been updated or verified to leverage this new function correctly.
83-83: Reapplication of reward-patch logic approved.
ReusingV018ManualDelegationRewardsPatchhere appears consistent with the approach of re-patching delegations as needed.
267-316: Verify the 5% margin assumption.
At line 305, a 5% margin of error is hardcoded for stake reconciliation. Please confirm that this domain-specific threshold is correct and consider exposing it as a configurable parameter if it may vary in different deployments.
318-341: Reward calculation logic looks solid.
Truncating the ratio difference prevents accidental overpayment or negative rewards. The approach seems consistent with Cosmos SDK standards.
343-356: Reference count logic validated.
The approach of decrementing reference counts and pruning historical rewards on zero references aligns with Cosmos distribution design. No issues found here.x/smart-account/ante/ante.go (1)
288-293: Remove or reintroduce the commented feegrant logic.Currently, the error message references an "active feegrant allowance," but the logic for retrieving and checking that allowance is commented out, effectively disallowing fee grants. This may break expected fee grant functionality. If you truly intend to remove fee grant support, consider removing the commented lines to avoid confusion. Otherwise, reintroduce the feegrant checks for correctness.
go.mod (6)
19-19: Bump CometBFT from v0.38.16 to v0.38.17 is consistent with the PR objective.No issues found. This aligns with addressing security vulnerabilities present in earlier CometBFT versions.
35-35: Confirm necessity of new dependency.Before finalizing, ensure that "github.com/hashicorp/go-metrics v0.5.4" is indeed required, as adding new dependencies can bloat the go.mod if it's not used.
37-37: Prometheus client update from v1.20.5 to v1.21.0.Looks fine. Minor version bump typically includes bug fixes and performance improvements.
40-40: Cobra upgrade to v1.9.1.This is also a minor version bump, usually containing new features & bug fixes.
45-45: Added 'golang.org/x/sync v0.11.0' dependency.It’s generally beneficial. Confirm that it is actually in use.
98-98: Secp256k1 upgrade from v4.3.0 to v4.4.0.This ensures you have the latest improvements in cryptographic libraries.
app/upgrades/v022/upgrades.go (2)
427-440: Validate reference count increment logic.The code panics if
ReferenceCountexceeds 2. Confirm this limit aligns with your design constraints, or consider a safe error return if it must remain below 2.
456-487: Initialization logic for new delegations.Ensure that the reference count increments do not create orphan states if the process is interrupted. Additional test coverage is recommended to verify atomicity.
app/app.go (6)
64-64: Check alias consistency forcmtproto.Renaming the Tendermint proto alias to
cmtprotoreflects the shift to CometBFT. Ensure all references totmprotothroughout the code are replaced to maintain consistency and avoid potential import conflicts.
73-75: Confirm older upgrades are no longer needed.Ensure you're not skipping critical upgrade logic from older versions. If the chain never ran versions v010-v020 or if these are fully deprecated, removing them is fine; otherwise, consider retaining them for node operators upgrading from earlier versions.
95-98: Validate removal of older upgrade references.Commenting out older upgrades may cause issues for node operators on versions preceding v021. Confirm that the chain will not require the older upgrade pathways.
394-394: ReviewNewUncachedContextusage.Using
NewUncachedContext(true, cmtproto.Header{})indicates a context with minimal overhead. Ensure that any essential block header fields (e.g., ChainID, Time) do not need setting in this scenario.
657-657: Establish block header details for testnet context.Similarly, when initializing the testnet context with
cmtproto.Header{}, ensure that necessary header info (e.g., block time) is properly handled or is intentionally omitted in a testing environment.
50-50:✅ Verification successful
Confirm usage of new crypto package.
Replacing
"cosmossdk.io/api/cosmos/crypto/ed25519"with"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"can help ensure consistency across modules. Verify that any downstream references to ed25519 are updated accordingly.Run the following script to confirm there are no stale references to the old
cosmossdk.io/api/cosmos/crypto/ed25519import:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'import "cosmossdk.io/api/cosmos/crypto/ed25519"'Length of output: 70
Crypto Package Update Verification Needed
- The file
app/app.gonow imports"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519".- Please verify that no lingering references to the old package (
"cosmossdk.io/api/cosmos/crypto/ed25519") exist anywhere in the codebase.- You can run the above command to search the repository. If it returns no matches, then the new crypto package is being used consistently and any downstream references to ed25519 have been updated accordingly.
🏁 Script executed:
#!/bin/bash rg 'cosmossdk.io/api/cosmos/crypto/ed25519' -n .Length of output: 170
Crypto Package Update Confirmed
- The updated import in
app/app.gonow correctly uses"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519".- A repository-wide search found no references to
"cosmossdk.io/api/cosmos/crypto/ed25519", confirming that all downstream references have been updated accordingly.cmd/bitsongd/cmd/custom_export.go (1)
29-135: Validate the custom export logic.The
CustomExportCmdfunction is well-structured for debugging local upgrades. Double-check that:
- All flags are validated before operating on them.
- No CLI usage collisions exist with existing commands.
cmd/bitsongd/cmd/root.go (2)
37-37: Confirm separate testnet server usage.Introducing
testnetserver.AddTestnetCreatorCommandclarifies the testnet creation path. Ensure that original server creation logic is unaffected by this addition.
461-507: Review extended export mechanism.The
customAppExportfunction introduces custom logic for exporting state. Verify that only pinned differences from the standardappExportlogic are introduced and no extraneous changes have been made.app/upgrades/v022/constants.go (1)
21-56:✅ Verification successful
Validate usage of newly defined structs.
These new types (ConditionalJSON,DistrSlashObject,DistrSlashEvent,Slash,ZeroSharesDelegation,PatchedDelegation, andNilDelegationCalculation) appear foundational for custom logic in the v022 upgrade, but no direct usage is shown here.Please run the following script to locate all usage references and confirm they are properly tested:
🏁 Script executed:
#!/bin/bash # Description: Search the codebase for references to newly introduced types in `v022/constants.go`. rg -A 4 "ConditionalJSON" rg -A 4 "DistrSlashObject" rg -A 4 "DistrSlashEvent" rg -A 4 "Slash" rg -A 4 "ZeroSharesDelegation" rg -A 4 "PatchedDelegation" rg -A 4 "NilDelegationCalculation"Length of output: 92292
Usage Verified – No Changes Required
The newly defined types inapp/upgrades/v022/constants.go(namely,ConditionalJSON,DistrSlashObject,DistrSlashEvent,Slash,ZeroSharesDelegation,PatchedDelegation, andNilDelegationCalculation) are actively utilized in the codebase. In particular:
ConditionalJSONis instantiated in bothapp/export_custom.goandapp/upgrades/v022/upgrades.go.DistrSlashObjectandDistrSlashEventare referenced in the same files, confirming their role in the custom logic.- The other types (
ZeroSharesDelegation,PatchedDelegation, andNilDelegationCalculation) are also employed as part of the data structures used byConditionalJSON.Given these references, the intended usage for the v022 upgrade has been validated. No further modifications to the new structs appear necessary, though please ensure that relevant tests are kept up-to-date.
server/start.go (2)
275-298: Validate fallback when CometBFT is disabled.
The code defaults tostartStandAloneifwithCmtis false. Ensure all downstream components (telemetry, in-process calls, etc.) gracefully handle this mode and that they are tested to avoid unexpected corner cases.
817-870: Confirm robust error handling when running in “gRPC only” mode.
InstartInProcess, ifgRPCOnlyis true, CometBFT is skipped, and an in-process node is never started. Verify that subsequent calls (like the ones that register node services) do not cause errors or no-ops in this scenario.
This PR contains the following:
1. Modified CosmosSDK: bumps cometbft@v0.38.17 to make it the same. version used by go-bitsong
This patch aims to resolve issues experienced with #274 , #276 & #277, by syncing the version cosmos-sdk uses with the one go-bitsong uses. The decision to bump cosmos-sdk in contrast downgrade is due to a security vulnerability patched in the transitive versions of cometbft from v0.38.12 to v0.38.17. No other modifications to the sdk were made in this release.
2. Inplace-Testnet Fix: Import Custom Server Functions From Cosmos-SDK
Merged the inclusion of the custom server logic that creates the necessary testente validator signature for vote extensions
3. Custom export CLI: custom export cli command helpful to debug v022 upgradeHandler
By installing & running
bitsongd custom-export, we run a modified version of the export for zero height logic, that runs the function used in the upcoming upgradeHandler, on the state a node has locally.Once the upgradehandler logic is run, we validate the state with the crisis invariant feature, as well as reiterating through delegations and querying their rewards with the normal function from the distribution keeper.
4. V022 UpgradeHandler: contains logic that resolves current issues being experienced as a result of v018-v021 impact.
Summary by CodeRabbit