perf: avoid unnecessary unmarshaling of BTC PK #441
Conversation
| @@ -61,6 +64,9 @@ func (m *MsgCreateBTCDelegation) ValidateBasic() error { | |||
| if m.BtcPk == nil { | |||
| return fmt.Errorf("empty delegator BTC public key") | |||
| } | |||
| if _, err := m.BtcPk.ToBTCPK(); err != nil { | |||
There was a problem hiding this comment.
shouldn't we also check m.FpBtcPkList i.e keys of finality providers ?
There was a problem hiding this comment.
This is not necessary as the message handler will check whether the given FP BTC PKs are known to DB. If the PKs are malformed then the message will be rejected anyway.
There was a problem hiding this comment.
ahh you are right.
Although before the change in this pr, those pk-s were validated before even they got to message handler and before the check in db. So this pr introduce small change to message processing in case of invalid fp public key.
Not big deal though probably, although we will need to think on it when doing proper fix later.
There was a problem hiding this comment.
Although before the change in this pr, those pk-s were validated before even they got to message handler and before the check in db.
Actually Cosmos Sdk v0.50 removed native support for ValidateBasic before handler and currently we invoke it manually at the beginning of msg handler 😅
There was a problem hiding this comment.
thats one thing, but from what I understand Unmarshal method in protobuf, when when we had this if _, err := newPK.ToBTCPK(); err != nil { validation in it, it meant when node received data from the network and was decoding custom type it called this validation.
It essentialy meant that everywhere we had BIP340PubkeyByte type, which we decoded from bytes we could be sure it was valid public key.
| @@ -61,6 +64,9 @@ func (m *MsgCreateBTCDelegation) ValidateBasic() error { | |||
| if m.BtcPk == nil { | |||
| return fmt.Errorf("empty delegator BTC public key") | |||
| } | |||
| if _, err := m.BtcPk.ToBTCPK(); err != nil { | |||
There was a problem hiding this comment.
ahh you are right.
Although before the change in this pr, those pk-s were validated before even they got to message handler and before the check in db. So this pr introduce small change to message processing in case of invalid fp public key.
Not big deal though probably, although we will need to think on it when doing proper fix later.
Improper fix for the sake of deadline. This fix is not proper because
BIP340PubKeyis used in both client side and KVStore thus is no longer assumed to always correspond to a valid BTC PK without performing verification upon unmarshal. Proper fix will be introduced in #438Based on the profiling result at https://github.com/babylonchain/babylon/files/14107742/profile001.pdf, the
BeginBlockof 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.