IBC Rate Limit - integration and tests #2410

nicolaslara · 2022-08-15T07:48:53Z

Closes: #2305

What is the purpose of the change

Integrates IBC rate limiting with the chain.

Brief Changelog

An IBC middleware is now provided and registered with the IBC transfer module.
The level of rate limiting is controlled by a rate limiting contract that can be controlled by an address specified by governance (this could be the gov module itself)
The "governance address" of the rate limiting contract can reset the quota when a rate limit is hit (this could be the gov module itself)

Testing and Verifying

This change added tests and can be verified as follows:

Added integration tests for the middleware (IBC Message <-> IBC transfer module <-> RateLimitingMiddlware <-> Relayer <-> Counterparty chain) under x/ibc-rate-limit
Added tests to the contract (see IBC Rate limiting contract #2408)

Documentation and Release Note

This PR depends on @2408 and @2409. It also requires a change to the osmosis fork of the cosmos-sdk (#2307)

Does this pull request introduce a new feature or user-facing behavior changes? (yes)
- If the rate limit is reached, IBC transactions will be blocked for users.
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
- Should I add this?
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)
- Currently undocummented. Should I add docs for this? what is the right place for that?

…ithout GenesisSupplyOffsets

nicolaslara · 2022-10-04T10:25:29Z

Created #2941 to add the testing helpers needed by this branch. That one should be a quick merge and make this PR smaller

…ration-and-tests

…-integration-and-tests

ancazamfir · 2022-10-13T11:00:48Z

Hi, i was looking at the osmosis rate limiter code and have some questions. Maybe this PR is not the best to ask but will do anyway and please redirect. On

osmosis/x/ibc-rate-limit/ibc_module.go

Lines 117 to 121 in 4fed7db

    
           amount, denom, err := GetFundsFromPacket(packet) 
        
           if err != nil { 
        
           	return channeltypes.NewErrorAcknowledgement("bad packet") 
        
           } 
        
           channelValue := im.ics4Middleware.CalculateChannelValue(ctx, denom)

GetFundsFromPacket gets the denom as specified by the sender. On receive side (in the code above) shouldn't there be un-prefexing or prefexing performed depending on wether the receiving chain was the one originally sending the packet or not? Similar thing is done in the transfer module https://github.com/cosmos/ibc-go/blob/f693f6c11053192f369b5908c9186bd494c3a33d/modules/apps/transfer/keeper/relay.go#L165

The way it is the channel value is computed for a denom of the counterparty/ sender chain. Or maybe I am missing something important here.

ancazamfir · 2022-10-13T11:03:34Z

x/ibc-rate-limit/ibc_middleware_test.go

+		coins = transfertypes.GetTransferCoin(
+			suite.path.EndpointB.ChannelConfig.PortID,
+			suite.path.EndpointB.ChannelID,
+			sdk.DefaultBondDenom,
+			sdk.NewInt(1),
+		)
+		coins = sdk.NewCoin(sdk.DefaultBondDenom, amount)


why the overwrite and not:

Suggested change

coins = transfertypes.GetTransferCoin(

suite.path.EndpointB.ChannelConfig.PortID,

suite.path.EndpointB.ChannelID,

sdk.DefaultBondDenom,

sdk.NewInt(1),

)

coins = sdk.NewCoin(sdk.DefaultBondDenom, amount)

coins = transfertypes.GetTransferCoin(

suite.path.EndpointB.ChannelConfig.PortID,

suite.path.EndpointB.ChannelID,

sdk.DefaultBondDenom,

amount,

)

This got overlooked. Thanks for the catch!

This probably also means that you're other comment is also right. I'll review this and report back

The correct code here is coins = sdk.NewCoin(sdk.DefaultBondDenom, amount) because that message is sent from chainB to chainA. What you said about getting the funds still applies though: I was not wrapping the token before getting the value and this was only working because both chains in my tests were the same chain.

I've fixed that in my latest commit (032f33b)

The correct code here is coins = sdk.NewCoin(sdk.DefaultBondDenom, amount) because that message is sent from chainB to chainA.

I am not sure I understand TestRecvTransferWithRateLimiting() (which calls this with forward = false). The tokens sent back to A should be prefixed (something like "ibc/transfer/channel-0/stake"...can't remember the details). Will look at the test latest tomorrow (running out of time today).

What you said about getting the funds still applies though: I was not wrapping the token before getting the value and this was only working because both chains in my tests were the same chain.

I've fixed that in my latest commit (032f33b)

Looks good!

Yeah, this is testing native B tokens being sent to a. But maybe I should also add a test for native A tokens being sent "back" from B to A

nicolaslara · 2022-10-22T02:05:52Z

Modified this implementation to use the escrowed amount as the channel value for native tokens. This introduces a bootstrapping problem: channels need to have tokens on escrow before we initialize their quotas (otherwise the quota will always be lower than the sent amount). This can be seen in the tests.

I don't think this is a problem for our use case as we will only use rate limiting for existing channels, but it may become important if someone wants all channels to have rate limiting, or to auto-initialize rate-limiting for channels on creation

ancazamfir · 2022-10-24T15:00:24Z

Modified this implementation to use the escrowed amount as the channel value for native tokens. This introduces a bootstrapping problem: channels need to have tokens on escrow before we initialize their quotas (otherwise the quota will always be lower than the sent amount). This can be seen in the tests.

I don't think this is a problem for our use case as we will only use rate limiting for existing channels, but it may become important if someone wants all channels to have rate limiting, or to auto-initialize rate-limiting for channels on creation

Right. Another possibility is to set some threshold for which the ratelimiter activates/ deactivates.

ancazamfir · 2022-10-25T11:29:15Z

x/ibc-rate-limit/ics4_wrapper.go

+func (i *ICS4Wrapper) CalculateChannelValue(ctx sdk.Context, denom string, packet exported.PacketI) sdk.Int {
+	// If the source is the counterparty chain, this should be
+	if strings.HasPrefix(denom, "ibc/") {
+		return i.bankKeeper.GetSupplyWithOffset(ctx, denom).Amount
+	}
+
+	escrowAddress := transfertypes.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel())
+	return i.bankKeeper.GetBalance(ctx, escrowAddress, denom).Amount


At first look this doesn't seem correct. If we have something like:

A B C ch0 --- ch1 ch2 --- ch3

And a is chain A token. In the following cases:

B sends to C a packet with denom ch1/a over ch2. denom here is ch1/a.

B receives (back) from C a packet with denom ch3/ch1/a over ch2. denom here is ch1/a (unprefixed)

Here I think we get the ch1/a supply instead of the balance of escrow_address(ch2) for denom ch1/a.

Maybe something along these lines:

Suggested change

func (i *ICS4Wrapper) CalculateChannelValue(ctx sdk.Context, denom string, packet exported.PacketI) sdk.Int {

// If the source is the counterparty chain, this should be

if strings.HasPrefix(denom, "ibc/") {

return i.bankKeeper.GetSupplyWithOffset(ctx, denom).Amount

}

escrowAddress := transfertypes.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel())

return i.bankKeeper.GetBalance(ctx, escrowAddress, denom).Amount

func (i *ICS4Wrapper) CalculateChannelValue(ctx sdk.Context, denom string, packet exported.PacketI, send bool) sdk.Int {

prefix_for_source:= transfertypes.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel())

if send { // send packet

if !strings.HasPrefix(denom, prefix_for_source) {

// Sender chain is the source

escrowAddress := transfertypes.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel())

return i.bankKeeper.GetBalance(ctx, escrowAddress, denom).Amount

} else {

return i.bankKeeper.GetSupplyWithOffset(ctx, denom).Amount

}

} else { // receive packet

if strings.HasPrefix(denom, prefix_for_source) {

// Receiver chain is the source

escrowAddress := transfertypes.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel())

return i.bankKeeper.GetBalance(ctx, escrowAddress, denom).Amount

} else {

return i.bankKeeper.GetSupplyWithOffset(ctx, denom).Amount

}

}

It would be great to have tests to cover scenarios like above.

The current behaviour (what you described above) is what I was trying to achieve, where escrow value is only used for native tokens (osmo/ion/tokenfactory tokens). ch0/a, as an ibc token, should be rate limited based on the current amount of ch0/a on osmosis.

I'm open to changing it to what you suggest, but I think the volume of some token/channel combinations may be too low and make the limit to be hit too often

The current behaviour (what you described above) is what I was trying to achieve, where escrow value is only used for native tokens (osmo/ion/tokenfactory tokens). ch0/a, as an ibc token, should be rate limited based on the current amount of ch0/a on osmosis.

I see, thanks for the clarification.

I'm open to changing it to what you suggest, but I think the volume of some token/channel combinations may be too low and make the limit to be hit too often

But in this case ratelimiting should not be configured on those channels as they have a low value.
Are there osmosis uscases like the ones I mention above (the multi hop/channel transfers)?

nicolaslara added 30 commits August 3, 2022 13:16

added initial rate limiting contract

8fe23ab

added expiration

0ed673a

fixed integration test

e61ab68

added initial middleware

187b49c

simple test to debug weirdness in ibctesting

d53529e

using helpers

b6947a0

Merge branch 'main' into ibc-rate-limit-weird-tests

38cfe4d

testing with test changes from PR##2274

773ea2b

fixed tests and updated requirement to match cosmos-sdk fork branch w…

7c23a31

…ithout GenesisSupplyOffsets

updated to latest commit

2c7dd18

using branch of the sdk fork that allows for ibc tests

a4b8dc2

Merge branch 'ibc-rate-limit-weird-tests' into ibc-rate-limit

cd950ce

Merge branch 'main' into supply-offset-changes-on-sdk-fork

8069131

using the latest sdk fork changes with a new constructor

bef2500

Merge branch 'supply-offset-changes-on-sdk-fork' into ibc-rate-limit

ef44407

initial ibctest setup

2b2ba93

propperly sending the package for testing

19d9c94

Initialize app before tests and add default genesis state

574b109

initial middleware with working testing framework

c423f12

improved testing framework

2791d6a

can test both send and recv for success and failure

d45a830

cleanner testing framework

64a5ef0

added contract instantiation

875214b

working wasm integration

23ad652

added params for contract config

c1d52d9

extracted param registration

3cbcafe

active rate limiting

6d79e71

calculating channel value

225dfcf

cleaner tests

863abef

fix issue with epochs

76c0678

comments

8c36532

nicolaslara added 8 commits October 4, 2022 12:26

Merge branch 'nicolas/test-helpers' into nicolas/ibc-rate-limit-integ…

d125dbd

…ration-and-tests

removed unnecessary txConfig

cc42cab

fixed typos

41b3cae

clearer comment

0ad0a90

using second helper function for ExportGenesis

7b50cb4

Merge branch 'nicolas/export_for_modules' into nicolas/ibc-rate-limit…

40f8cc6

…-integration-and-tests

added new wasm file

e7798a1

Merge branch 'main' into nicolas/ibc-rate-limit-integration-and-tests

88b1c4a

nicolaslara added the V:state/breaking State machine breaking PR label Oct 10, 2022

ancazamfir reviewed Oct 13, 2022

View reviewed changes

nicolaslara added 5 commits October 21, 2022 11:18

Fixed send with ibc assets. Better tests and error messages

032f33b

updated contract with x86 version

f107877

gofumpt

237f144

fixed clippy errors

caad1f2

using the escrowed value as the channel value for native tokens

13f2621

nicolaslara added 3 commits October 21, 2022 19:06

gofumpt

79109a2

update fail string

60811cf

Merge branch 'main' into nicolas/ibc-rate-limit-integration-and-tests

c50cf10

nicolaslara requested review from czarcas7ic and p0mvn October 23, 2022 23:07

ancazamfir reviewed Oct 25, 2022

View reviewed changes

nicolaslara mentioned this pull request Oct 28, 2022

IBC rate limiting: Updated the contract to cosmwasm 1.1 and Uint256 for amounts #2950

Merged

Merge branch 'main' into nicolas/ibc-rate-limit-integration-and-tests

952a2e6

nicolaslara mentioned this pull request Oct 31, 2022

Rate limit - Cleaner tests #3183

Merged

ValarDragon closed this Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBC Rate Limit - integration and tests #2410

IBC Rate Limit - integration and tests #2410

nicolaslara commented Aug 15, 2022

nicolaslara commented Oct 4, 2022

ancazamfir commented Oct 13, 2022

ancazamfir Oct 13, 2022

nicolaslara Oct 13, 2022

nicolaslara Oct 13, 2022

nicolaslara Oct 21, 2022

ancazamfir Oct 24, 2022

nicolaslara Oct 24, 2022

nicolaslara commented Oct 22, 2022

ancazamfir commented Oct 24, 2022

ancazamfir Oct 25, 2022

nicolaslara Oct 25, 2022

ancazamfir Oct 26, 2022

IBC Rate Limit - integration and tests #2410

IBC Rate Limit - integration and tests #2410

Conversation

nicolaslara commented Aug 15, 2022

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

nicolaslara commented Oct 4, 2022

ancazamfir commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaslara commented Oct 22, 2022

ancazamfir commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment