Handle btc related wasm queries #353
Conversation
wasmbinding/bindings/query.go
Outdated
| HeaderInfo *BtcBlockHeaderInfo `json:"header_info,omitempty"` | ||
| } | ||
|
|
||
| type BtcHeaderByQueryResponse struct { |
There was a problem hiding this comment.
| type BtcHeaderByQueryResponse struct { | |
| type BtcHeaderQueryResponse struct { |
| // underlying data model for the whole btclightclient module. | ||
| // GetHeaderByHash returns header with given hash from main chain, returns nil such header is not found | ||
| // or is not on main chain | ||
| func (k Keeper) GetHeaderByHash(ctx sdk.Context, hash *bbn.BTCHeaderHashBytes) *types.BTCHeaderInfo { |
There was a problem hiding this comment.
Maybe it's better to move GetHeaderByHash and GetHeaderByHeight to state.go and implement them as a part of headersState?
Also, the existing HeaderExists can take advantage of GetHeaderByHash, and the existing HeadersByHeight can take advantage of GetHeaderByHeight
There was a problem hiding this comment.
I prefer to keep them separately as part of the keeper due to the fact that they build on top of methods exsisting in headersState. Currently both HeaderExists and HeadersByHeight do not care whether headers are on main chain or on fork, they just return whatever is in database currently.
On the other hand, both new functions, check whether returned headers are in fact parts of canonical chain. Otherwise smart contract calling them would not be able to verify wheter something is part of btc canonical chain.
There was a problem hiding this comment.
I'm fine with the current design, but from my understanding the design rationale behind headersState seems to be that any operation on the BTC headers has to go through headersState. The two functions here look a bit inconsistent with this rationale. On the other hand, it also makes sense to make keeper to expose certain frequently used operations like this. Your call.
There was a problem hiding this comment.
Typically I view keeper as the controller and headersState as the model to retrieve data. This means that for me the keeper contains the exposed methods while headersState the data retrieval ones. Due to that, I would typically keep this function in headersState with the name GetMainchainHeaderByHash.
There was a problem hiding this comment.
another problem I see, is that the best function I see for checking is something on main chain is actually MainChainDepth which is defined on keeper. And I would prefer not to duplicate it on headerState
wasmbinding/bindings/query.go
Outdated
| BtcTip *struct{} `json:"btc_tip,omitempty"` | ||
| BtcBaseHeader *struct{} `json:"btc_base_header,omitempty"` | ||
| BtcHeaderByHash *BtcHeaderByHash `json:"btc_header_by_hash,omitempty"` | ||
| BtcHeaderByNumber *BtcHeaderByNumber `json:"btc_header_by_number,omitempty"` |
There was a problem hiding this comment.
| BtcHeaderByNumber *BtcHeaderByNumber `json:"btc_header_by_number,omitempty"` | |
| BtcHeaderByHeight *BtcHeaderByNumber `json:"btc_header_by_height,omitempty"` |
There was a problem hiding this comment.
this need to match the one one in rust binding - https://github.com/babylonchain/bindings/blob/main/packages/bindings/src/querier.rs#L42, otherwise it will fail deserialization. Do you prefer naming with height ? If yest it needs to be changed in both places. I do not have strong opinions on that, some blockchains call this block number, some block height so it is a bit arbitrary.
There was a problem hiding this comment.
I would prefer height as that's the terminology we use everywhere else, but if it's too much trouble we can change it at a later point 🤷
There was a problem hiding this comment.
no worries I will do quick update to binding library :)
| // underlying data model for the whole btclightclient module. | ||
| // GetHeaderByHash returns header with given hash from main chain, returns nil such header is not found | ||
| // or is not on main chain | ||
| func (k Keeper) GetHeaderByHash(ctx sdk.Context, hash *bbn.BTCHeaderHashBytes) *types.BTCHeaderInfo { |
There was a problem hiding this comment.
Typically I view keeper as the controller and headersState as the model to retrieve data. This means that for me the keeper contains the exposed methods while headersState the data retrieval ones. Due to that, I would typically keep this function in headersState with the name GetMainchainHeaderByHash.
| // GetHeaderByHash returns header with given hash from main chain, returns nil such header is not found | ||
| // or is not on main chain | ||
| func (k Keeper) GetHeaderByHash(ctx sdk.Context, hash *bbn.BTCHeaderHashBytes) *types.BTCHeaderInfo { | ||
| depth, err := k.MainChainDepth(ctx, hash) |
There was a problem hiding this comment.
Overall, I'm not that much in favor of the design choice of only returning headers if they're on the mainchain. Especially given that it introduces the inefficiency of traversing the entire main chain. If someone has the hash of the header, and we have that header, why not return it?
I suggest we either forgo this design choice or we introduce it when we have optimised our data model. However, even if forks are not that interesting, I don't see why we could complicate our logic when the user asks something we already have.
Given that we plan to purge fork headers that are finalized (i.e. w-deep) and that we don't consider something that's less than w-deep as finalized, I would argue that fork headers are still interesting (if they are less than w-deep).
There was a problem hiding this comment.
So main reason for not returning fork headers is that when you return fork headers and main chain headers you need to give callers a possibility to discern between the two. So either you need to add some bool flag to response i.e isOnfMainChain or you need to have another function isOnMainChain(hash): bool . Otherwise you cannont build any useful protocol on top of that.
Imagine, as smart contract I only want to save anything which is included in some bitcoin transaction, and I retrieve header to validate some data, but if I do not know whether this is canonical chain header or not I cannot make any decision.
In general I would argue that forks are internal implementation detail btc light client, and should be pruned as soon as reorg happen and btc light client should only hold canonical chain.
Overall, I'm not that much in favor of the design choice of only returning headers if they're on the mainchain. Especially given that it introduces the inefficiency of traversing the entire main chain.
This inefficiency is already there when btc checkpoint module query btc light client about status of each submitted checkpoint.
I would argue that fork headers are still interesting
Why ? You cannot do anything interesting with them. Only use case I can think of, is that when reorg happen, it maybe the case that header submitter won't need to re-submit some headers, but this is a bit to small advantage to justify it.
As I see it we have 3 options now:
- Go with approach in this pr and optimise later. This functions are inefficient but it should not be problem for short lived testnet. We can later fix the data model/implementation without changing the api, but we will have some btc functionality on testnet available.
- Return all headers from those functions, and add additional method
isOnMainChain(hash): bool. This will be only inefficient function. Imo this makes api clunky, and ultimately this function won't be needed, as I would not store fork headers in our btc light client. - Remove btc functionality all together. This will make our smart contract api a bit empty with only two function, but we can always add it later.
I would go with 1 and 3, as 2 is hacky from api perspective. (where 1 is hacky from implementation perspective which I can live with)
There was a problem hiding this comment.
I see, ok then let's go with 1. We should shift focus towards optimizing the storage model soon so we have a good amount of time to test stuff out before mainnet.
x/btclightclient/keeper/keeper.go
Outdated
| return info | ||
| } | ||
|
|
||
| // GetHeaderByHeight returns header with given height from main chain, returns nil such header is not found |
There was a problem hiding this comment.
| // GetHeaderByHeight returns header with given height from main chain, returns nil such header is not found | |
| // GetHeaderByHeight returns header with given height from main chain, returns nil if such header is not found |
Add handling of btc related wasm queries.
One note, queries for block by hash and block by number only return answers from current best chain. Imo forks are not that interesting and introduce a bit of complications (i.e we would need another
boolin answers indiciting wheter headers in on main chain or fork, which makes whole api messy).In general I think, our light client should store only main and remove forked headers as soon as reorg happens, but I am not sure if its worth changing it now as we are considering moving to btc light client as smart contract.