feat(tm2): Add Specific Block Height query Feature to ABCI Queries #2722
Conversation
notJoon
requested review from
zivkovicmilos,
leohhhn,
gfanton,
jaekwon,
moul and
piux2
as code owners
github-actions
bot
added
📦 🌐 tendermint v2
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2722 +/- ##
==========================================
- Coverage 63.32% 63.09% -0.24%
==========================================
Files 548 548
Lines 78511 81354 +2843
==========================================
+ Hits 49719 51327 +1608
- Misses 25438 26591 +1153
- Partials 3354 3436 +82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Latest approach seems better than old one. However to fully support this functionality, I think we need |
I think this is probably related 🤔 https://github.com/gnolang/gno/blob/master/tm2/pkg/store/types/options.go#L10-L23 |
There was a problem hiding this comment.
Thank you for tackling this 🙏
I can't seem to make the querying work for custom heights. This simple post request:
{
"id": 1,
"jsonrpc": "2.0",
"method": "abci_query",
"params": [
"bank/balances/g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5",
"",
"10", // HEIGHT
false
]
}Will return:
{
"jsonrpc": "2.0",
"id": 1,
"result": {
"response": {
"ResponseBase": {
"Error": {
"@type": "/std.InternalError"
},
"Data": null,
"Events": null,
"Log": "",
"Info": ""
},
"Key": null,
"Value": null,
"Proof": null,
"Height": "0"
}
}
}The logs on the node would just have this:
ABCIQuery {"module": "rpc", "path": "bank/balances/g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5", "data": "", "result": {"Error":{},"Data":null,"Events":null,"Log":"","Info":"","Key":null,"Value":null,"Proof":null,"Height":0}}
tm2/pkg/bft/rpc/client/batch.go
Outdated
| "data": data, | ||
| "prove": opts.Prove, | ||
| } | ||
| if opts.Height != 0 { |
There was a problem hiding this comment.
Just curious: isn't this misleading? Specifying 0 will cause an error down the line if I'm not mistaken, but in this case it will just continue on
There was a problem hiding this comment.
hmm.. yes it would be clearer to only add the key if opts.Height is greater than 0
tm2/pkg/bft/rpc/core/abci.go
Outdated
| @@ -56,6 +60,15 @@ import ( | |||
| // | height | int64 | 0 | false | Height (0 means latest) | | |||
| // | prove | bool | false | false | Includes proof if true | | |||
| func ABCIQuery(ctx *rpctypes.Context, path string, data []byte, height int64, prove bool) (*ctypes.ResultABCIQuery, error) { | |||
| if height < 0 { | |||
| return nil, errors.New("height cannot be negative") | |||
There was a problem hiding this comment.
This should be a standard error
| @@ -56,6 +60,15 @@ import ( | |||
| // | height | int64 | 0 | false | Height (0 means latest) | | |||
| // | prove | bool | false | false | Includes proof if true | | |||
| func ABCIQuery(ctx *rpctypes.Context, path string, data []byte, height int64, prove bool) (*ctypes.ResultABCIQuery, error) { | |||
There was a problem hiding this comment.
Can you please add a small unit test that verifies the errors?
I know our RPC testing suite is super janky, but it shouldn't be too much of a hassle to write a few cases
There was a problem hiding this comment.
I promise refactoring this testing suite is on my bucket list
tm2/pkg/sdk/baseapp.go
Outdated
| @@ -389,10 +392,6 @@ func (app *BaseApp) Query(req abci.RequestQuery) (res abci.ResponseQuery) { | |||
| default: | |||
| return handleQueryCustom(app, path, req) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Why remove this error wrap?
There was a problem hiding this comment.
I removed it because it was unreacahble. I'll add the error message back in proper position
tm2/pkg/sdk/baseapp.go
Outdated
| @@ -49,6 +49,7 @@ type BaseApp struct { | |||
| // See methods setCheckState and setDeliverState. | |||
| checkState *state // for CheckTx | |||
| deliverState *state // for DeliverTx | |||
| currentState *state // current state, set after Commit | |||
There was a problem hiding this comment.
This is always nil, I think you can drop it
tm2/pkg/sdk/baseapp.go
Outdated
| @@ -487,7 +488,11 @@ func handleQueryCustom(app *BaseApp, path []string, req abci.RequestQuery) (res | |||
|
|
|||
| // cache wrap the commit-multistore for safety | |||
| // XXX RunTxModeQuery? | |||
| ctx := NewContext(RunTxModeCheck, cacheMS, app.checkState.ctx.BlockHeader(), app.logger).WithMinGasPrices(app.minGasPrices) | |||
| // ctx := NewContext(RunTxModeCheck, cacheMS, app.checkState.ctx.BlockHeader(), app.logger).WithMinGasPrices(app.minGasPrices) | |||
There was a problem hiding this comment.
Why not remove the comment?
Description
Added a new function that performs ABCI queries at a specific block height.
ABCIQueryWithOptionsin the RPC client to support querying at specific block heights.QueryHandlerandQueryCfgto include height and prove options.Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description