IBC Rate Limiting #556

sampocs · 2023-01-04T19:37:34Z

Context and purpose of the change

The ratelimit module is meant as a safety control in the event of a bug or attack, and prevents massive inflows or outflows of IBC tokens to/from Stride in a short time frame. See module docs here for an overview of the purpose and tech spec.

Brief Changelog

Added ratelimit module as IBC middleware around transfer module
Added governance transactions AddRateLimit, UpdateRateLimit, ResetRateLimit and RemoveRateLimit
Added rate-limit, list-rate-limits and rate-limits-by-chain queries
Added unit tests and bash integration tests
Added Juno <> Osmo relayer config for testing transfers between non-stride chains
Added TODO to add store key when upgrade handler is added
Added Hour epoch

Upgrade

The upgrade handler was excluded from the PR as the target upgrade has not been determined yet.
IN THE ASSOCIATED UPGRADE, WE MUST REMEMBER TO ADD THE STORE KEY AND HOUR EPOCH
WE MUST ALSO ADD RATE LIMITS FOR EXISTING DENOMS!

Testing

There's a full suite of integration tests that were meant to be a comprehensive sanity check before launching this module. They are NOT meant to run in CI and are intentionally pedantic (they take ~30 minutes to run). They're also redundant with the unit tests.
Run tests with

make start-docker-all && bash dockernet/scripts/ratelimit/run_all_tests.sh

Once the PR has been reviewed and approved, I'll run through the normal stride integration tests and the rate limit integration tests one last time before merging:

CI Integration Tests for all host zones
RateLimit Integration Tests

Updates needed for sdk 46 and ibc-go v5

Change sdk.Int to cosmossdk/math
Remove rest client and rest route registered in module.go
IBC go callback args
Refactor ICS4Wrapper
Implement GetAppVesrion

Before Merging

Do we need to bind PortID? I don't think so since the middleware starts from the transfer port
Revert dockernet config settings to default
Add blacklist
Add evens

Author's Checklist

I have...

Run and PASSED locally all GAIA integration tests
If the change is contentful, I either:
- Added a new unit test OR
- Added test cases to existing unit tests
OR this change is a trivial rework / code cleanup without any test coverage

If skipped any of the tests above, explain.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

reviewed state machine logic
reviewed API design and naming
manually tested (if applicable)
confirmed the author wrote unit tests for new logic
reviewed documentation exists and is accurate

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md?
This pull request updates existing proto field values (and require a backend and frontend migration)?
Does this pull request change existing proto field names (and require a frontend migration)?
How is the feature or change documented?
- not applicable
- jira ticket XXX
- specification (x/<module>/spec/)
- README.md
- not documented

Co-authored-by: sampocs <[email protected]>

jstr1121 · 2023-02-07T15:54:26Z

It would be nice if this PR supports blacklisting for specific token IBC transfer (receive/send) and export interface for registration of those tokens in keeper level so that it can be used on other modules.

womensrights · 2023-02-07T17:51:45Z

I was just passing by and saw this comment, you might be interested in this middleware celestia have implemented

riley-stride · 2023-01-31T23:07:56Z

app/upgrades.go

+	//     }
+	//
+	// 2. Add hour epoch to store
+	// 3. Add rate limits for existing denoms


Should we implement these here and just leave them commented out?

It'd require adding the whole upgrade handler, so I think I'd lean toward doing it in a separate PR. Wdyt?

Agreed, makes sense to put in a separate PR. We should just make sure that goes in the next upgrade.

dockernet/scripts/ratelimit/test_denoms.sh

x/ratelimit/types/quota.go

riley-stride · 2023-02-08T04:07:55Z

It would be nice if this PR supports blacklisting for specific token IBC transfer (receive/send) and export interface for registration of those tokens in keeper level so that it can be used on other modules.

Sam's suggestion to set the rate limit to 0 for the particular IBC denom in this case sounds like the cleanest solution to this question

riley-stride

Unit testing coverage is really strong. I have no comments and I think this is ready to merge if you feel good about it @sampocs 🎉

riley-stride

LGTM!

x/ratelimit/README.md

riley-stride · 2023-02-17T23:20:55Z

x/ratelimit/keeper/rate_limit.go

+}
+
+// If the rate limit is exceeded or the denom is blacklisted, we emit an event
+func EmitRateLimitExceededEvent(ctx sdk.Context, denom, channelId string, direction types.PacketDirection, amount sdkmath.Int, err error) {


Nit: might be nice to have separate events for attempts to transfer blacklisted assets vs attempts to transfer assets whose rate limits were exceeded. The former seems important to be able to identify at a moment's notice in potential emergency scenarios

I went back and forth on that a little. My thinking was you could easily determine whether it was blacklisted or throttled based on the error message and I thought it would be nice to have a common event type to listen to all blocked txs.

Another option is to change the event type to TransferBlocked and then add an attribute that's either RateLimitExceeded or BlacklistedDenom?
Also fine with just using different event types

Wdyt?

change the event type to TransferBlocked and then add an attribute that's either RateLimitExceeded or BlacklistedDenom?

I think this would be nice n clean. I know this is nit, but it could make a big difference in the moment if there's ever a blacklist event

riley-stride · 2023-02-17T23:26:45Z

Was wondering @sampocs : is there any reason we should prevent from-and-to-Stride transfers of blacklisted assets? (eg bank send from stride1...A to stride1....B. I can't think of any, but curious if you can!

Co-authored-by: riley-stride <[email protected]>

sampocs · 2023-02-17T23:51:07Z

Was wondering @sampocs : is there any reason we should prevent from-and-to-Stride transfers of blacklisted assets? (eg bank send from stride1...A to stride1....B. I can't think of any, but curious if you can!

Good question! Nothing comes to mind

…e-limiter

Co-authored-by: antstalepresh <[email protected]> Co-authored-by: antstalepresh <[email protected]> Co-authored-by: Aidan Salzmann <[email protected]> Co-authored-by: riley-stride <[email protected]>

antstalepresh and others added 30 commits January 4, 2023 13:36

scaffold ibc rate limit module (#473)

950a274

Co-authored-by: sampocs <[email protected]>

stride v3 -> v4 in rate limit module

21089a8

v3 -> v4 for rate limit protos

f991e57

formatted and regenerated protos

6e2a037

removed stale ignite comments

32c67d5

changed store key since module name was changed

8a608c5

added remove quota proto types

d5e4098

added remove quota message to msg_server

573b0e8

added quota query types

43b8bca

added quota queries to grpc query

3e7b583

Added Unit Test Framework to ratelimit module (#480)

acc565c

add msg types and routes, add creator field to tx msgs

bff325c

some minor fixes

fab8893

minor fixes

1e35a51

add newflow function

6b2cb18

[RATE-LIMIT] Path helper functions (#479)

8adfb9f

[RATE-LIMIT]: RateLimit Helper Functions and Queries (#490)

ebcae0b

implement quota functions

8b5a0b9

change addquota to setquota, add unit tests for quota msgs

2937b8d

convert CheckExceedsQuota function to member function of quota object

d3a4369

adjustments for GetQuota function

c2e26da

add unit tests for quota keeper functions

d3612ad

minor adjustments

2b25f18

implement cli & tests

10ee62a

fix feedbacks

f45ad70

Move Quota Attributes to RateLimit object (#502)

12a0dfe

address pr comments

0200f69

minor fix

c83d069

adjust quota_test

275cbdb

minor adjustments

334de7f

sampocs added 6 commits January 23, 2023 17:26

added boilerplate for get app version

1f3d75f

fixed bug in denom flag

eb795bd

fixed integration tests

5409792

renamed ibc_module to ibc_middleware

0d1c62e

refactored rate limit middleware wiring

4f46c2e

finished middleware refactor

e4c1e9c

github-actions bot added the C:records label Jan 31, 2023

removed question from comments

703d432

riley-stride reviewed Feb 8, 2023

View reviewed changes

riley-stride approved these changes Feb 14, 2023

View reviewed changes

sampocs added 5 commits February 13, 2023 23:17

added events when rate limit was exceeded

1a1bd4e

revert config.sh

d3064f6

added denom blacklist

aa0af49

Merge branch 'main' into rate-limiter

4fa3ad7

fixed lint errors

72b07ce

riley-stride approved these changes Feb 17, 2023

View reviewed changes

Update x/ratelimit/README.md

62d5857

Co-authored-by: riley-stride <[email protected]>

sampocs added 6 commits February 17, 2023 18:34

refactored event to have differentiated attributes

fa505fc

Merge branch 'rate-limiter' of github.com:Stride-Labs/stride into rat…

8b5b394

…e-limiter

Merge branch 'main' into rate-limiter

e866d8e

added error logs

dad4ec9

fixed lint errors

7bee669

created lighterweight suiteless test helper

90349c3

sampocs merged commit 94e3a91 into main Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBC Rate Limiting #556

IBC Rate Limiting #556

sampocs commented Jan 4, 2023 •

edited

Loading

jstr1121 commented Feb 7, 2023

womensrights commented Feb 7, 2023

riley-stride Jan 31, 2023

sampocs Feb 9, 2023

riley-stride Feb 14, 2023

riley-stride commented Feb 8, 2023

riley-stride left a comment

riley-stride left a comment

riley-stride Feb 17, 2023

sampocs Feb 17, 2023

riley-stride Feb 18, 2023

riley-stride commented Feb 17, 2023

sampocs commented Feb 17, 2023

IBC Rate Limiting #556

IBC Rate Limiting #556

Conversation

sampocs commented Jan 4, 2023 • edited Loading

Context and purpose of the change

Brief Changelog

Upgrade

Testing

Updates needed for sdk 46 and ibc-go v5

Before Merging

Author's Checklist

Reviewers Checklist

Documentation and Release Note

jstr1121 commented Feb 7, 2023

womensrights commented Feb 7, 2023

riley-stride Jan 31, 2023

Choose a reason for hiding this comment

sampocs Feb 9, 2023

Choose a reason for hiding this comment

riley-stride Feb 14, 2023

Choose a reason for hiding this comment

riley-stride commented Feb 8, 2023

riley-stride left a comment

Choose a reason for hiding this comment

riley-stride left a comment

Choose a reason for hiding this comment

riley-stride Feb 17, 2023

Choose a reason for hiding this comment

sampocs Feb 17, 2023

Choose a reason for hiding this comment

riley-stride Feb 18, 2023

Choose a reason for hiding this comment

riley-stride commented Feb 17, 2023

sampocs commented Feb 17, 2023

sampocs commented Jan 4, 2023 •

edited

Loading