Add custom query #114
Add custom query #114
Conversation
|
Yes, that is correct! |
|
|
||
| message QueryBtcCheckpointHeightRequest { | ||
| // Hex encoded raw checkpoint bytes | ||
| string raw_checkpoint = 1; |
There was a problem hiding this comment.
I think it might be a little difficult for the explorer or someone doing a query to have the BTC checkpoint bytes. I would suggest that we add the epoch number as a parameter here, since it would be much much easier (and intuitive) for the explorer and a user to query by the epoch number.
There was a problem hiding this comment.
I think @toliujiayi mentioned on slack that he already have a way of obtatining rawcheckpoint bytes. Is that correct ?
In general, I agree that epoch param seems nicer, and do not require additional step of asking checkpointing module for given epoch. What do you think @toliujiayi, is epoch param ok here for explorer ?
There was a problem hiding this comment.
epoch number would be much easier for me to use because 1. it doesn't require the extra step like you mention 2. i'm getting the rawcheckpoint content but not in bytes, so I would have to convert them to bytes if that's the case.
x/btccheckpoint/keeper/grpc_query.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| checkpointEpoch, err := k.GetCheckpointEpoch(ctx, rawCheckpointBytes) |
There was a problem hiding this comment.
If we have the epoch number as a parameter instead of the raw checkpoint bytes you can get this directly from the request.
| if headerNumber < lowestHeaderNumber { | ||
| lowestHeaderNumber = headerNumber | ||
| } |
There was a problem hiding this comment.
| if headerNumber < lowestHeaderNumber { | |
| lowestHeaderNumber = headerNumber | |
| } | |
| lowestHeaderNumber = math.Min(lowestHeaderNumber, headerNumber) |
There was a problem hiding this comment.
yea, I considered it but go library math.Min requires casting to float64, and having separate function for uint64 sounded a like a bit too much ceremony just for two uses.
There was a problem hiding this comment.
Oh yeah forgot about this go trickery 😮💨
KonradStaniec
force-pushed
the
query-lowest-block
branch
from
62884b7
to
12e1b9b
Compare
|
|
||
| // BtcCheckpointHeight returns earliest block height for given rawcheckpoint | ||
| rpc BtcCheckpointHeight(QueryBtcCheckpointHeightRequest) returns (QueryBtcCheckpointHeightResponse) { | ||
| option (google.api.http).get = "/babylon/btccheckpoint/v1/{epoch_num}"; |
There was a problem hiding this comment.
| option (google.api.http).get = "/babylon/btccheckpoint/v1/{epoch_num}"; | |
| option (google.api.http).get = "/babylon/btccheckpoint/v1/checkpoints/{epoch_num}/min-height"; |
Or /epochs/{epoch_num}/min-checkpoint-height.
Although if it was me I would probably want to make the query more general and return a composite response, not just the minimum height; that would just be a field in it. To make it extendable, like which bitcoin transactions was it, to let people look it up on BTC a bit easier. The Babylon explorer could even contain a link to a Bitcoin explorer to help.
There was a problem hiding this comment.
I totally agree with this sentiment.
Tbh I treated this query as extendable (or expendable) thing as imo just returning this minium height does not tell much.
For now I just treated this as requirement from frontend show something on demo. I preferred to have something working rather than starting discussion what this query should really contain. Imo when we reach first public testnet phase all queries will need to be revised.
| if !k.CheckHeaderIsOnMainChain(ctx, tk.Hash) { | ||
| return 0, errors.New("one of submission headers not on main chain") | ||
| } |
There was a problem hiding this comment.
Just note that eventually the light client will be garbage collected. It would be good to save the winning submission when the checkpoint is confirmed or finalized.
There was a problem hiding this comment.
hmm what do you mean by garbage collected ? Like keeping only n last headers of the main chain ?
Tbh from my point of view I am not sure it is worth the hassle. Each header is only 80bytes, so 1M headers is 80 MB, so we could as well hold whole btc header chain from genesis.
| return 0, errors.New("one of submission headers not on main chain") | ||
| } | ||
|
|
||
| headerNumber, err := k.GetBlockHeight(ctx, tk.Hash) |
There was a problem hiding this comment.
Same here, you can only rely on the BTC light client until it's garbage collected. The height could come from the submission, it has the header, doesn't it?
There was a problem hiding this comment.
so:
- submissions does not store headers at this moment
- btc headers themselves does not have height, so basically lightclient chain is source of truth for height of each header.
There was a problem hiding this comment.
Yeah I know, it would have to be lifted into the tombstone of the checkpoint 🤷
I'm also for keeping the BTC chain, it's fine. But this pruning was an argument we made for why we don't have to worry about garbage collecting orphaned forks.
| // CheckHeaderIsOnMainChain (which uses main chain depth) returned true which | ||
| // means header should be saved and we should know its heigh but GetBlockHeight | ||
| // returned error. Something is really bad, panic. | ||
| panic("Inconsistent data model in btc light client") |
There was a problem hiding this comment.
Just pruning in action. I don't think it's implemented but we discussed with from very early on with @vitsalis
I'm also not sure if it would never be implemented if it would cause some of those lightclient queries to loop all the way back to the root block all the time.
Also I would be very cautious of panicking in a gRPC query. It's better to return an error and let the explorer developer complain than to leave some assumption in query code (which should arguably not be in the node at all) and crash the node, then try to explain to the validators why someone took down the entire network where this port was exposed because of some change in the codebase and some query left over from demo times.
There was a problem hiding this comment.
hmm not sure I understand concern here.
In line 22 I am checking if the header is on the main chain ,and move here only if it is. So imo it would be weird if light client would be able to tell me this header is on main chain, but i do not know its height .
The one case I see this happening, is if between line 22 and 26, there would be some concurrent big reorg which would put the header suddenly on the fork, and deep enough to be instantly pruned. Not sure this is possible though.
In general I agree with being prudent with panics, although this case seems for me as generally broken data model things (unless of course case with big concurrent reorg is not theretical). Also I am still operating under assumption that all queries will need to be revised and reviewd either way. As mentioned in first comment, I am pretty sure this query will not live long enough in this form to even reach testnet phase.
There was a problem hiding this comment.
Ah okay, I didn't see these two as related. Fine then, but if pruning was happening as a result of block execution in action, it might go away indeed. This is something we never got confirmed, how queries behave during database commits.
It's fine, if as you say this query goes away then 🤷
Add custom query described in https://babylon-chain.atlassian.net/browse/BM-142
@toliujiayi just to clarify, by earliest you mean the block which is deepest in the chain i.e if there is submission on block 10 and 14 you woould like the one on block 10 ? (imo this the way it makes most sense)
@vitsalis does light client
BlockHeightcheck if header is on main chain or it just return height no matter on which chain given header resides ? From the code I see that is returs height from all chains, thats why I have added additional check forCheckHeaderIsOnMainChain(as imo this should only care about main chain headers) but I wanted to ask to be sure.