Skip to content

fix(rate-limiting): validate genesis state - #9021

Open
gjermundgaraba wants to merge 8 commits into
gg/ratelimit-key-migrationfrom
gg/ratelimit-genesis-validation-stacked
Open

gjermundgaraba wants to merge 8 commits into
gg/ratelimit-key-migrationfrom
gg/ratelimit-genesis-validation-stacked

Conversation

@gjermundgaraba

@gjermundgaraba gjermundgaraba commented Aug 2, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • validates all rate limits, blacklist entries, and whitelist pairs before import;
  • rejects exact duplicate rate-limit paths and whitelist pairs;
  • reuses the same quota and channel/client ID validation for messages and genesis;
  • makes InitGenesis validate before writing state;
  • preserves initialized epochs at midnight and accepts the height-zero epoch produced at InitChain.

The parent PR makes the underlying store-key encodings unambiguous, so this PR only needs to detect duplicate logical tuples.

Testing

  • make tidy-all
  • make build
  • make lint — exited 0; reported existing repository-wide goconst/govet findings outside this diff
  • make test-unit

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work. — No linked issue; this closes missing validation identified during review.
  • Include changelog entry when appropriate.
  • Wrote unit and integration tests if relevant.
  • Updated documentation (docs/) if anything is changed. — N/A: validation behavior is covered by the changelog and tests.
  • Added GoDoc comments if relevant.
  • Self-reviewed Files changed.
  • Provide a conventional commit message to follow the repository standards.

Proposed squash commit:

fix(rate-limiting): validate genesis state

Closes FOU-1198

@gjermundgaraba
gjermundgaraba requested a review from a team as a August 2, 2026 21:01
@gjermundgaraba
gjermundgaraba force-pushed the gg/ratelimit-genesis-validation-stacked branch from 1228251 to dad2f29 Compare August 2, 2026 21:01
@codecov

codecov Bot commented Aug 2, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.08%. Comparing base (6e0b0b5) to head (d0f29b3).

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     
Flag Coverage Δ
08-wasm 65.04% <ø> (ø)
ibc-go 67.15% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@greptile-apps

greptile-apps Bot commented Aug 2, 2026 •

Copy link
Copy Markdown

Greptile Summary

The PR validates rate-limiting genesis data before importing it and preserves valid height-zero epoch state.

  • Validates rate limits, quotas, flows, blacklist entries, whitelist pairs, and duplicate logical entries.
  • Reuses shared quota and channel/client identifier validation.
  • Updates epoch initialization and round-trip coverage.
  • Replaces the changelog placeholder with the correct PR fix(rate-limiting): validate genesis state #9021 reference.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
modules/apps/rate-limiting/types/genesis.go Adds comprehensive validation for imported rate-limiting state and permits valid height-zero epoch metadata.
modules/apps/rate-limiting/keeper/genesis.go Validates genesis state before writes and recognizes initialized epochs by their start time.
modules/apps/rate-limiting/types/msgs.go Consolidates channel/client identifier and quota validation shared by messages and genesis.
modules/apps/rate-limiting/types/quota.go Centralizes quota validation, including nil percentages, bounds, nonzero thresholds, and duration.
CHANGELOG.md Adds the release note with a consistent link to PR #9021.

Reviews (2): Last reviewed commit: "fix(rate-limiting): validate genesis sta..." | Re-trigger Greptile

Comment thread CHANGELOG.md Outdated
@gjermundgaraba
gjermundgaraba force-pushed the gg/ratelimit-genesis-validation-stacked branch 2 times, most recently from a89f67a to a2de463 Compare August 4, 2026 07:40
…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
gjermundgaraba force-pushed the gg/ratelimit-genesis-validation-stacked branch from a2de463 to d0f29b3 Compare August 4, 2026 07:41
@linear-code

linear-code Bot commented Aug 5, 2026

Copy link
Copy Markdown

FOU-1198

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant