rpc: avoid using BIP340PubKey in messages

[SebastianElvis]

Based on the profiling result at https://github.com/babylonchain/babylon/files/14107742/profile001.pdf, the BeginBlock of BTC staking module has a performance issue -- 100000 BTC delegations will make its execution time to 2-3 seconds on a laptop, and most of the time is spent on unmarshaling BTC PK. The unmarshaling aims to ensure the BTC PK is legitimate when unmarshaling bytes into a BIP340 PK.

This PR proposes an optimisation that we perform this check only upon receiving relevant messages (create finality provider, create BTC delegation, submit covenant signatures). Once these stuff are included in DB the BTC PKs are guaranteed to be legitimate so we don't need to check BTC PK format again.

Tried benchmarking locally and this gives us 8x speedup, compared to results in #437.

➜  babylon git:(fix-btcstaking-opcost) ✗ go test -benchmem -run=^$ -bench=. github.com/babylonchain/babylon/x/btcstaking/keeper 
goos: linux
goarch: amd64
pkg: github.com/babylonchain/babylon/x/btcstaking/keeper
cpu: AMD Ryzen 7 7735HS with Radeon Graphics        
BenchmarkBeginBlock_100_10-16                 37          32560658 ns/op         9587270 B/op     210782 allocs/op
--- BENCH: BenchmarkBeginBlock_100_10-16
    store.go:206: <nil> DBG loadVersion ver=0
    store.go:244: <nil> DBG loadVersion commitID hash= key="KVStoreKey{0xc0013bc040, btcstaking}" ver=0
    store.go:62: <nil> INF Upgrading IAVL storage for faster queries + execution on live state. This may take a while commit=436F6D6D697449447B5B5D3A307D store_key="KVStoreKey{0xc0013bc040, btcstaking}" version=0
    store.go:76: <nil> DBG Finished loading IAVL tree
    store.go:206: <nil> DBG loadVersion ver=0
    store.go:244: <nil> DBG loadVersion commitID hash= key="KVStoreKey{0xc003088090, btcstaking}" ver=0
    store.go:62: <nil> INF Upgrading IAVL storage for faster queries + execution on live state. This may take a while commit=436F6D6D697449447B5B5D3A307D store_key="KVStoreKey{0xc003088090, btcstaking}" version=0
    store.go:76: <nil> DBG Finished loading IAVL tree
    store.go:206: <nil> DBG loadVersion ver=0
    store.go:244: <nil> DBG loadVersion commitID hash= key="KVStoreKey{0xc004c06040, btcstaking}" ver=0
        ... [output truncated]
PASS
ok      github.com/babylonchain/babylon/x/btcstaking/keeper     78.128s

[gitferry]

This is awesome!

[gitferry]

I was wondering if the performance gain will bring us lower gas costs. Can we also benchmark it? Maybe in another PR ofc

[SebastianElvis]

I was wondering if the performance gain will bring us lower gas costs.

I thought Cosmos SDK only charges gases for KVStore access, maybe I miss something?

[gitferry]

Aha I just learned that Cosmos-sdk does an automatic gas meter during read/write to the kv. It's up to the developer to do a manual gas meter in each module. ref

[SebastianElvis]

Aha I just learned that Cosmos-sdk does an automatic gas meter during read/write to the kv. It's up to the developer to do a manual gas meter in each module. ref

I see, then this PR might not affect the gas usage of existing txs, given that it does not affect access pattern to DB.

Would be good if we can take a look on optimising gas usage of certain msgs (e.g., commit randomness) in the coming days

[vitsalis]

Hmm generally I like this optimisation but it introduces a danger. Typically we have such checks in the Unmarshal method as we want the point of entry to be heavily fortified (i.e. make sure that it corresponds to the data type that we want), so that points of exit do not need extra checks. This means that when you have a particular type, you know that it corresponds to valid data.

[SebastianElvis]

Hmm generally I like this optimisation but it introduces a danger. Typically we have such checks in the Unmarshal method as we want the point of entry to be heavily fortified (i.e. make sure that it corresponds to the data type that we want), so that points of exit do not need extra checks. This means that when you have a particular type, you know that it corresponds to valid data.

I agree with your concern. This indeed changes the contract of BIP340PubKey a bit -- it now might correspond to a malformed PK, so that we need to perform extra checks on it in we wish to use the BTC PK corresponding to a BIP340PubKey object.

There is another way of arguing this property though -- everytime one needs to use the BTC PK corresponding to a BIP340PubKey object, it needs to call .ToBTCPK() function, which will return an error if the object is malformed. So the caller does not assume a BIP340PubKey object corresponds to a valid BTC PK anyway. If the caller has the chance to make such assumption it can use MustToBTCPK. Put it another way, with this PR, BIP340PubKey is no longer assumed to correspon to a valid BTC PK, but rather a wrapper structure for simplifying the encoding/decoding of BTC PK. I think this is worth it given the almost 10x improvement on marshalling/unmarshaling any struct that has a field in type BIP340PubKey. Wdyt?

[KonradStaniec]

Without digging much into benchmark, it seems we need to have two separate types (as I agree 10x improvement on the table is bad):

• the one we currently have, lets call it UnverifiedBIP340Pk which is super fortified, and unmarshaling from bytes have this kind of checks and this one should be used in the message.
• and the new one VerifiedBIP340 with which we will store in db, in grpc queries, which is guaranteed to be be correct i.e it was already validated before saving.

This way callers can differentiate between both types.

[SebastianElvis]

Without digging much into benchmark, it seems we need to have two separate types (as I agree 10x improvement on the table is bad):

• the one we currently have, lets call it UnverifiedBIP340Pk which is super fortified, and unmarshaling from bytes have this kind of checks and this one should be used in the message.
• and the new one VerifiedBIP340 with which we will store in db, in grpc queries, which is guaranteed to be be correct i.e it was already validated before saving.

This way callers can differentiate between both types.

Agreed, it's more clear to distinguish between unverified BTC PKs and verified ones. I changed the BTC PK to bytes in all messages of BTC staking and finality modules, given that we cannot ensure the BTC PKs from users are valid. I did not define a new type for unverified BTC PK since it does not make much sense for a user to wrap its BTC PK to an "unverfiied" struct -- if it's honest then it should use BIP340PubKey directly. Please take a look again to see if the approach makes sense

[SebastianElvis]

Closing this as it will be done in other subsequent PRs