fix(rate-limiting): validate genesis state - #9021
Open
gjermundgaraba wants to merge 8 commits into
Open
gjermundgaraba wants to merge 8 commits into
gjermundgaraba wants to merge 8 commits into
Conversation
gjermundgaraba
force-pushed
the
gg/ratelimit-genesis-validation-stacked
branch
from
August 2, 2026 21:01
1228251 to
dad2f29
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## gg/ratelimit-key-migration #9021 +/- ##
==============================================================
+ Coverage 66.99% 67.08% +0.08%
==============================================================
Files 330 330
Lines 17484 17512 +28
==============================================================
+ Hits 11714 11748 +34
+ Misses 5028 5026 -2
+ Partials 742 738 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThe PR validates rate-limiting genesis data before importing it and preserves valid height-zero epoch state.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(rate-limiting): validate genesis sta..." | Re-trigger Greptile |
gjermundgaraba
force-pushed
the
gg/ratelimit-genesis-validation-stacked
branch
2 times, most recently
from
August 4, 2026 07:40
a89f67a to
a2de463
Compare
…h v3 migration The legacy rate limit key is a separator-less denom||channelID concat: distinct (denom, channel) pairs can collide to identical bytes, and the denom-first order makes per-channel range scans impossible. The new key is uvarint(len(channelOrClientID)) || channelOrClientID || denom. The uvarint is self-delimiting, so the encoding is unambiguous at any length with no ceiling to enforce; identifiers cap at 64 chars by 24-host anyway, and every length below 128 encodes to the same single byte a fixed-width prefix would. Chains already on v11.2.0 carry old-layout keys, and legacy cosmos/ibc-apps rate-limit chains migrate onto this module in-place with their keys carried over, so a v3 store migration re-keys every entry, deriving new keys from the stored Path values (the old key is ambiguous and unparseable). The migration deletes all legacy keys before writing any new ones because a new key can equal a not-yet-migrated legacy key; the test suite pins this with a mutation-tested collision fixture and a full-snapshot idempotence check. Actually exploiting the new prefix in RateLimitsByChannelOrClientID, which still scans every rate limit, is deferred to the querier rework in #8996.
IsAddressPairWhitelisted decided membership with len(value) != 0, but a
zero-value WhitelistedAddressPair marshals to zero bytes. Under the old
concatenated layout this was unreachable: AddressWhitelistKey("", "")
produced an empty key, which store.Set rejects. The new length-prefixed
layout makes it the valid one-byte key 0x00, so InitGenesis can now
store such a pair (GenesisState.Validate does not inspect
WhitelistedAddressPairs) and no reader can ever find it again. Decide
membership with store.Has instead.
GetAllRateLimits logged the failing entry's key as a string. Keys now
begin with a raw uvarint length byte, so the module's only diagnostic
for a corrupt rate limit rendered a control character. Log it as hex.
The changelog entry omitted the module consensus version bump, which is the fact that tells an integrator their upgrade handler has to run module migrations for the re-key to apply. TestMigrate2to3's comment claimed the test proves Migrate2to3 reaches v3 through the keeper's own store service and codec, but the test constructs both itself, so it cannot fail for that reason. Describe what it does check.
CodeQL go/allocation-size-overflow (high) fired on both key builders: the make() capacity was computed as 1+len(a)+len(b) directly from caller-supplied string lengths, with no bound checked first. Share one helper that checks each addition against math.MaxInt before computing the capacity. The emitted bytes are unchanged; the layout tests in keys_test.go pin that. Allocation stays exact at one alloc. Refs https://github.com/cosmos/ibc-go/security/code-scanning/2176
The bounds-checked version cleared go/allocation-size-overflow but introduced beginendblock-panic: BeginBlocker resets rate limits, so it reaches these builders, and the module deliberately swallows every error from that path to avoid failing block processing. A panic there defeats that guarantee no matter how unreachable the condition is. Growing the key by append removes both: no capacity expression for the overflow rule to flag, and no panic on a consensus path. Emitted bytes are unchanged; keys_test.go pins the layout.
gjermundgaraba
force-pushed
the
gg/ratelimit-genesis-validation-stacked
branch
from
August 4, 2026 07:41
a2de463 to
d0f29b3
Compare
This branch has not been deployed
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
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.
Description
Depends on #9020. This is the second PR in stack #9022.
The rate-limiting genesis validator currently checks pending packet IDs and epoch metadata, but not the rate limits and related state that InitGenesis writes. Malformed paths, quotas, flows, blacklist entries, whitelist pairs, and duplicate logical entries can therefore enter the store during chain initialization.
This PR:
The parent PR makes the underlying store-key encodings unambiguous, so this PR only needs to detect duplicate logical tuples.
Testing
Proposed squash commit:
Closes FOU-1198