Cosmos-sdk: Impossible to initialize two or more IBC modules

Created on 21 Apr 2020 · 1Comment · Source: cosmos/cosmos-sdk

Summary of Bug

It is currently impossible to initialize two or more modules that are supposed to use the IBC due to a bug inside the BindPort method.

Version

793c6f17ffdf68541861992bb00fadf0bb992507

Steps to Reproduce

Code example inside app.go:

// Capability keepers
app.CapabilityKeeper = capability.NewKeeper(appCodec, keys[capability.StoreKey], memKeys[capability.MemStoreKey])
scopedIBCKeeper := app.CapabilityKeeper.ScopeToModule(ibc.ModuleName)
scopedTransferKeeper := app.CapabilityKeeper.ScopeToModule(transfer.ModuleName)
scopedPostsKeeper := app.CapabilityKeeper.ScopeToModule(ibcposts.ModuleName)

// Module initialization
app.TransferKeeper = transfer.NewKeeper(
    app.cdc, keys[transfer.StoreKey],
    app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
    app.AccountKeeper, app.BankKeeper, scopedTransferKeeper,
)
transferModule := transfer.NewAppModule(app.TransferKeeper)

app.IBCPostsKeeper = ibcposts.NewKeeper(
    app.cdc, keys[ibcposts.StoreKey], app.PostsKeeper,
    app.IBCKeeper.ChannelKeeper, app.IBCKeeper.PortKeeper, scopedPostsKeeper,
)
ibcPostsModule := ibcposts.NewAppModule(app.IBCPostsKeeper, app.PostsKeeper)

// IBC routing creation
ibcRouter := port.NewRouter()
ibcRouter.AddRoute(transfer.ModuleName, transferModule)
ibcRouter.AddRoute(ibcposts.ModuleName, ibcPostsModule)
app.IBCKeeper.SetRouter(ibcRouter)

// The rest is identical to the Gaia repo

The bug explained

I've spent almost all the morning trying to figure out why using this code the chain would return the following error:

I[2020-04-21|09:05:29.379] - [ibc0] -> sending transaction:             
[{"type":"ibc/channel/MsgChannelOpenInit","value":{"port_id":"transfer","channel_id":"ibconexfer","channel":{"state":"INIT","ordering":"ORDERED","counterparty":{"port_id":"transfer","channel_id":"ibczeroxfer"},"connection_hops":["ibconeconnection"],"version":"ics20-1"},"signer":"desmos1vlnt27rjwhhdu8km7rurxp4rrtmv678ckqe3jp"}}]
I[2020-04-21|09:05:29.379] ✘ [ibc0]@{22} - msg(0:channel_open_init) err(port: invalid port) 
{"height":"22","txhash":"A590F3B58F9F19C23FDDACF3C76409B7F7D838BDFEF3428E6C25627E4FB5A6E3","codespace":"port","code":3,"raw_log":"failed to execute message; message index: 0: could not retrieve module from portID: invalid port","gas_wanted":"200000","gas_used":"46226"}

After some hours putting a lot of fmt.Printf around the SDK code, I've found out what happens. Here's the log I got (they will be useful during the explanation of the bug):

// --- Transfer module initialization ---

===> isBound 
===> PortID: transfer 

===> GetCapability 
====> Key: ibc/rev/ports/transfer 
====> IndexBytes: [] 
====> Index: 0 
====> Stored capabilities: 
====> Found capability, but index bytes are empty 
====> Removing capability at index 0 
====> Stored capabilities: 

===> NewCapability 

===> GetCapability 
====> Key: ibc/rev/ports/transfer 
====> IndexBytes: [] 
====> Index: 0 
====> Stored capabilities: 
====> Found capability, but index bytes are empty 
====> Removing capability at index 0 
====> Stored capabilities: 

===> NewCapability - Resume 
====> Key: ibc/rev/ports/transfer 
====> Index: 0 
====> IndexBytes: [0 0 0 0 0 0 0 0] 
====> Stored capabilities: Capability{0xc0010b5380, 0}, 
====> Capability saved 

// ---- Posts module initialization ---

===> isBound 
===> PortID: posts 

===> GetCapability 
====> Key: ibc/rev/ports/posts 
====> IndexBytes: [] 
====> Index: 0 
====> Stored capabilities: Capability{0xc0010b5380, 0}, 
====> Found capability, but index bytes are empty 
====> Removing capability at index 0 
====> Stored capabilities: 

===> NewCapability 

===> GetCapability 
====> Key: ibc/rev/ports/posts 
====> IndexBytes: [] 
====> Index: 0 
====> Stored capabilities: 
====> Found capability, but index bytes are empty 
====> Removing capability at index 0 
====> Stored capabilities: 

===> NewCapability - Resume 
====> Key: ibc/rev/ports/posts 
====> Index: 1 
====> IndexBytes: [0 0 0 0 0 0 0 1] 
====> Stored capabilities: Capability{0xc0010b5c38, 1}, 
====> Capability saved

So, basically, this is what happens.

Inside the transfer module InitGenesis, the following call is made:
```
err := keeper.BindPort(ctx, state.PortID)
```

What this triggers is the following chain of calls:

portKeeper.BindPort
portKeeper.isBound
scopedKeeper.GetCapability

Inside the GetCapability method, the following lines will be executed:

key := types.RevCapabilityKey(sk.module, name)
indexBytes := memStore.Get(key)
index := sdk.BigEndianToUint64(indexBytes)

Since the indexBytes will be empty (as a Capability associated with the given port does not exist yet), we will also execute the following call:

if len(indexBytes) == 0 {
// If a tx failed and NewCapability got reverted, it is possible
// to still have the capability in the go map since changes to
// go map do not automatically get reverted on tx failure,
// so we delete here to remove unnecessary values in map
delete(sk.capMap, index)
return nil, false
}

After this, the capability will be saved correctly and so we will have a first capability which has index = 0 and is associated to the transfer module.
Now, the fun part. The second module (posts) will be initialized. We will do so by the same call inside the posts module InitGenesis:
```
err := keeper.BindPort(ctx, state.PortID)
```

This will trigger the same chain of calls:

portKeeper.BindPort
portKeeper.isBound
scopedKeeper.GetCapability

Inside the GetCapability method, the following lines will be executed:

key := types.RevCapabilityKey(sk.module, name)
indexBytes := memStore.Get(key)
index := sdk.BigEndianToUint64(indexBytes)

Now here's the catch.
Since a Capability for this key does not exist yet, the indexBytes here will be empty once again. The problem is that by using sdk.BigEndianToUint64 with an empty array, the returned index will be 0, which is the same of the first initialized Capability.

Since indexBytes is empty, we will enter the following if brach:

if len(indexBytes) == 0 {
// If a tx failed and NewCapability got reverted, it is possible
// to still have the capability in the go map since changes to
// go map do not automatically get reverted on tx failure,
// so we delete here to remove unnecessary values in map
delete(sk.capMap, index)
return nil, false
}

What this causes is the deletion of the capability at index 0 which was the one initialized through steps 1 - 5, thus making it completely useless.

This whole chain makes it impossible to use 2 or more modules at the same time due to the fact that the first initialized module will always be deleted during the initialization of the second one.

For Admin Use

[ ] Not duplicate issue
[ ] Appropriate labels applied
[ ] Appropriate contributors tagged
[ ] Contributor assigned/self-assigned

Source

RiccardoM

👍8 ❤2

Most helpful comment

Thank you for the very detailed bug report!! Was able to reproduce on local test case and then fix the issue by starting capability index at 1 so that an empty indexBytes doesn't get interpreted as a valid index