Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using shorter port IDs #1600

Closed
webmaster128 opened this issue Sep 4, 2023 · 2 comments
Closed

Consider using shorter port IDs #1600

webmaster128 opened this issue Sep 4, 2023 · 2 comments

Comments

@webmaster128
Copy link
Member

Currently the IBC ports assigned to contracts are in the form wasm.<contract address as bech32>. This is pretty long, e.g. wasm.juno1qr84ktm57q5t02u04ddk5r8s79axdzglad6tfdd9g2xgt4hkh6jsgeq9x2 is 68 bytes long.

Now those ports are used as keys in IBC storage as defined in https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channel-and-packet-semantics#store-paths, e.g.

"channelEnds/ports/{portIdentifier}/channels/{channelIdentifier}"
"nextSequenceSend/ports/{portIdentifier}/channels/{channelIdentifier}"
"commitments/ports/{portIdentifier}/channels/{channelIdentifier}/sequences/{sequence}"
"acks/ports/{portIdentifier}/channels/{channelIdentifier}/sequences/{sequence}"
"receipts/ports/{portIdentifier}/channels/{channelIdentifier}/sequences/{sequence}"

In KV databases it seems to be important to use short keys in order to keep them efficient. This allows faster lookups in the file system and more elements in memory. This was discussed by people much more knowledgeable than me in the SDK repo but it is plausible when looking at the basics of those databases.

So right now we waste something like 50 bytes for every IBC related db entry key due to the way the ports are constructed. I wonder if we can and should use a more compact format, such as

  1. wasm.<hex of 16 byte hash> which is 37 bytes long
  2. wasm.<account number> (e.g. wasm.516737) which is 11 bytes long
  3. wasm.<countract address counter> (e.g. wasm.15165) which remains slightly shorter than 2. assuming most accounts on a chain are not contracts.
  4. other ideas?

Option 1. would work and is pre-image resistent but not collision-resistant. This might become an issue if an attacker can freely brute force addresses which Instantiate2 makes possible. So I'd not do this as-is.

Option 2. is nice I think but assumes we always have that number from a different module available. This might be risky to assume as account numbers are made for signing and nothing else.

Option 3. is probably nice.

The problem wil all of the above is that a ContractFromPortID would require a lookup instead of just stripping off the prefix.

@alpe
Copy link
Contributor

alpe commented Sep 5, 2023

Thanks Simon for bringing this up. Storage efficiency does make sense and should be a goal.
The port address though is something that is also used by humans. When a channel is setup, people need to define the correct port to make it work. Therefore anything else than the contract address here would require an extra step and adds complexity. IMHO we must must not aim for storage efficiency in ibc-go but focus on good UX.
The ibc-go module can decide on a different storage layout to make it more efficient if needed.
We are in good company to focus on UX. Ics-20 for example uses transfer to make it very verbose.

We do store the ibc-port with the contract-info to make it queryable.

@webmaster128
Copy link
Member Author

When a channel is setup, people need to define the correct port to make it work. Therefore anything else than the contract address here would require an extra step and adds complexity.

That's a good point I was not aware of

The ibc-go module can decide on a different storage layout to make it more efficient if needed.

The storage layout is in the spec and cannot be changed without breaking IBC light clients.

Ics-20 for example uses transfer to make it very verbose.

Yeah, but this is 60 bytes less for every single IBC related entry than what wasmd uses.

Overall, I share your sentiment. There are two strong arguments to keep it as is:

  • The manual checking/readability when setting up the channels
  • The ability to implement ContractFromPortID in memory

Closing for now but happy to re-open if someone finds this later and has additional thoughs on the topic.

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

No branches or pull requests

2 participants