Client update transactions are too large

[faddat]

Summary of Bug

Client update transactions are too large and can be easily made smaller. The size of client update transactions is a risk to chains using ibc.

Notes from @ValarDragon

• each client update lists all the validators + all signatures with both carrying way too much data
• Simplest things would be:
  • Delete:
    • Signature/block_id_flag, infer "BLOCK_ID_COMMIT"
    • Signature/Timestamp -> UnixNano timestamp

"Can't tell if this validator set hash is verified against root, if so nothing that reads as a trivial change then (because priority changes every block), you can make an ID system for val addrs at least, but ID system isn't a mini change"

Expected Behaviour

Client updates are significantly reduced in size, without the use of zero knowledge proofs.

Version

all

Steps to Reproduce

Contact Jacob Gadikian via slack to discuss how to reproduce the issues that can be caused by this approach.

implementation

I think that most of this could be done in the relayer. Versioning could let the relayer know which client update info to send, and there wouldn't be breaking changes.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[ValarDragon]

Would echo this, we are wasting tremendous amounts of blockspace throughout cosmos due to this.

In general I'd expect the tx size to be on the order of magnitude of 100 signatures * 64 bytes / signature = 6.4kb. (Lets say 10-20kb is short-term reasonable after some overheads) I forgot how we estimated the real size, but I remember it being 150-200kb.

At least the idea of inferring BLOCK_ID_FLAG_COMMIT if blank seems like a very light change, and something I concur that relayer software should be able to handle, so it can be done in a way thats backwards compatible.

Would be great to do a quick survey of where all the size is coming from. What seems like the big deal to me is that we are communicating all validators two times, in a very space inefficient manner. (Using full addresses, pubkeys, voting power, proposer priority). We should stop communicating all validators and be much more refined in whats communicated. Proposer priority should never be needed for instance. Removing that would on quick estimate be around Note that its not included in validator set hash. (source: https://github.com/cometbft/cometbft/blob/869833ee12c8c399fb0c487befcdac427236be2b/types/validator.go#L16 )

[crodriguezvega]

Thanks for opening the issue, @faddat and for your comment, @ValarDragon. Linking here #24, since it was reporting a similar concern. We will triage the issue and investigate what can be done.

[faddat]

@ValarDragon - yeah, if we had a clientv1 and a clientv2, I think the relayer could do the rest.

I am still battling through the thing amulet published against my will, notional and I will be a minute here on fixes.

https://forum.cosmos.network/t/amulet-security-advisory-for-cometbft-asa-2023-002/11604/13

[ValarDragon]

I guess one thing interesting is that for proposer priority, the relayer could just edit it to be 0. Then it won't get serialized in protobuf

[ValarDragon]

Played with this, here is a URL for format-free decoding of header that Carlos shared: <link not working, you'll have to try editing post to get it> the overhead is actually not as bad. (50-70kb, not the believed 150-250kb!)

The BLOCK_ID_FLAG is actually not included, but instead an enum option! We also see proposer priority isn't included, and that for absent validators further data isn't included.

It appears the signatures are currently implemented as one entry per validator. Since thats the case, we can/should omit the validator address along with the signature, as its implied by the index. I do not know where the IBC software currently finds the address (is it from the vote, or implied by the index)

The tendermint ValidateBasic code checks this address, but we could easily just copy the validate basic code into IBC-go and omit this check. (this code is called here).

Should also be able to make the transmission of the validator set at least not resend the pubkey for pubkey/address pairs already known. (storing an Addr->Pubkey map in state)

[womensrights]

Adding in some more context from slack so it is not lost -

Improvements to the IBC client update structure could be via:

• Moving the light client verification from TM to IBC
• Not verifying / including all validator signatures, just 2/3rds of them. (And including indication whether others voted)
• Other data size improvements by removing elements that are not needed for light client operation

Another feature besides bandwidth / gas, is that this is currently the cause of 10% of all CPU usage from tx execution on Osmosis, so will have a slight benefit to sync rate

[damiannolan]

#2951 is related also

[faddat]

Update on this one -- have a use case.

I'm with one of the people implementing IBC for solana for composable right now, and the size of the updates is a problem for them.

Have a feeling we might be able to make some progress here.