Incorrect timestamp is used when querying at previous block heights

[apollo-sturdy]

Summary of Bug

When querying at a previous block height, the function createQueryContext is run in baseapp/abci.go. At the end of this function the context is returned as:

ctx := sdk.NewContext(
		cacheMS, app.checkState.ctx.BlockHeader(), true, app.logger,
	).WithMinGasPrices(app.minGasPrices).WithBlockHeight(height)

This uses the latest block's time, rather than the timestamp of the requested block. This causes problems for example when querying CosmWasm contracts with SmartQuery, as this context is passed to the CosmWasm querier and passed into the contract's SmartQuery inside the env.block.time. If the SmartQuery depends on time, an incorrect result will be returned.

Version

0.45.5

Steps to Reproduce

I have created a minimum viable example of this bug in this repo: https://github.com/apollodao/querier-bug-demo
It simply returns the time when queried with query {"get_time":{}}. I deployed this contract on Terra classic. As you can see if you query it with a previous block height, the timestamp returned is the current: https://fcd.terra.dev/terra/wasm/v1beta1/contracts/terra1pdtgxyfhpmx9jw32r28lw66983rxt824lqsmy9/store?query_msg=eyJnZXRfdGltZSI6e319&height=7983128

I also confirmed it by modifying createQueryContext in Cosmos SDK to print out the time of ctx, and it is indeed the latest time.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[ethanfrey]

I looked into this bug and it does seem to be an sdk thing.

A number of contracts check block height or block time when performing queries. For example, to see if a voting period is open, or the state of a vesting contract. Using this info to calculate the result.

Naturally, we pass in what we get from Context. However, this doesn't seem to be set for any historical query.

This should affect eg. historical x/gov voting queries as well and is not just a "CosmWasm thing" but an oversight in the sdk made more clear by the many developers using CosmWasm for more and more use cases.

I'd love some feedback here

[alexanderbez]

Am I correct in understanding that you want sdk.NewContext(...) to also have the time set, i.e. WithBlockTime? If so, I'm not sure how we would get that block time as that would require first getting the block to begin with...which might not be that trivial.

[ValarDragon]

I think it'd be that WithBlockHeight should set WithBlockTime if its present in history

[alexanderbez]

Yes, but we'd have to query for it I think @ValarDragon. Alternatively, we can keep them in memory (at Commit) lol.

[ethanfrey]

It would be good to have a proper historical environment for the historical queries.

I think setting WithBlockTime is the only item missing there. And since this is in queries ands requires that the node has not pruned that block, I guess we can assume the node also has the proper tendermint header (and fails if either the iavl data or tendermint header is missing for the requested height)

If this is not possible, we could just document this limitation of historical queries, but it seems a number of teams are assuming they work. If there were an alternative (indexing?) service that could return this info, not the chain, that could also work. We do not need proofs on this, as those are executing logic (it doesn't affect raw queries that can be proven)

[alexanderbez]

Given that there are query paths that completely bypass Tendermint all together, I just can't think of a clean way to handle this (see createQueryContext and all callers that use it).

The only relatively clean solution I can really think of, within the constraints of BaseApp, is to just cache height -> timestamp mappings directly in BaseApp.

[ValarDragon]

Hey wanted to check in on this. This is an important issue for clients / things to be resyncable!

I see its in the sprint label, just wanted to comment that if folks don't have capacity I can try to help with this. Also can we get the solution for this backported to the SDK 43 and 45 lines? I believe its needed back to 43 to ensure accuracy of queries for any indexer trying to rebuild query results.

[alexanderbez]

For you @ValarDragon, I'll do it.

[ValarDragon]

TYSM!! :)

[tac0turtle]

naive question, could someone share me a query where a block in the past has a timestamp in the present? The demo in the issue is a historical query from within the state machine. This isnt the best example, i would say, since historical queries from within the state machine aren't supported and should not be used by apps

[ValarDragon]

So the Osmosis TWAP query "GeometricTwapToNow" will return an error in the state machine if "StartTime" is after "ctx.BlockTime()". Source: https://github.com/osmosis-labs/osmosis/blob/main/x/twap/api.go#L122

This block https://www.mintscan.io/osmosis/blocks/8770000 occured yesterday, March 19th. So if I provide a start time of March 20th at that block height, we would have gotten the mentioned error.

Here is what happens if I actually do that:

curl --request GET  --url 'https://lcd.archive.osmosis.zone/osmosis/twap/v1beta1/GeometricTwapToNow?pool_id=1&base_asset=ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2&quote_asset=uosmo&start_time=2023-03-20T05:00:50.709574725Z' --header 'x-cosmos-block-height: 8770000'
{
  "geometric_twap": "14.439339990000000000"
}

What I should get if timestamp were set properly in the historical context (done by using a timestamp in future relative to time of query):

curl --request GET  --url 'https://lcd.archive.osmosis.zone/osmosis/twap/v1beta1/GeometricTwapToNow?pool_id=1&base_asset=ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2&quote_asset=uosmo&start_time=2023-03-22T05:00:50.709574725Z' --header 'x-cosmos-block-height: 8770000'
{
  "code": 3,
  "message": "called GetArithmeticTwap with a start time that is after the end time. (start time 2023-03-22 05:00:50.709574725 +0000 UTC, end time 2023-03-20 09:48:18.740257983 +0000 UTC): invalid request",
  "details": [
  ]
}

In the latter, you see that the end time is the time of query, even though I used a historic block whose time was on march 19th / march 18th depending on your timezone. (Depending on time you query, you may need to adjust the start time in the latter)

[tac0turtle]

i see, im worried this may create a mess with our pruning strategy. If i prune blocks but not app state then this wouldnt work either. I dont think the extra query we do now in the PR proposed is sufficient. Storing a sort of map in baseapp or somewhere of height: timestamp. Then the sdk pruning would influence the pruning of the map.

cc @alexanderbez

[alexanderbez]

A map will not work unless it's persisted to disk, which is redundant I think.

---

I have another solution which is to store the timestamp in CommitInfo which is saved to disk on Commit. When doing a historical query, we ask the RMS what the timestamp is, which is a single simple read.