Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: auto-set block timestamp for historical queries #15448

Merged
merged 22 commits into from
Mar 28, 2023

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Mar 17, 2023

Description

Closes: #12226

This change automatically set's the block time on the Context that is provided to all queries. It does this by querying for the CommitInfo for that particular version/height from the RMS.

cc @ValarDragon @tac0turtle @julienrbrt


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the C:CLI label Mar 17, 2023
@ValarDragon
Copy link
Contributor

Should we in a subsequent PR maintain an in-memory cache for historic block times ? Otherwise the extra query seems fine to me!

@tac0turtle
Copy link
Member

i dont think we can reliably use blocks here as the pruning strategies between the two are out of sync fairly often

@ValarDragon
Copy link
Contributor

Ah I see, this implicitly assumed that the blocks pruning strategy would be more relaxed than the states pruning. If that doesn't hold, then a height-> timestamp map may have to be stored in state machine. (Which does make sense to do)

@alexanderbez
Copy link
Contributor Author

Gonna take another approach. Just hack into CommitInfo and store the timestamp there! No extra query needed, just one additional quick DB read.

@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the C:CLI label Mar 21, 2023
@alexanderbez alexanderbez marked this pull request as ready for review March 22, 2023 16:54
@alexanderbez alexanderbez requested a review from a team as a code owner March 22, 2023 16:54
@alexanderbez alexanderbez enabled auto-merge (squash) March 27, 2023 13:46
@alexanderbez alexanderbez added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Mar 27, 2023
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this consensus breaking or is metadata outside the app hash

@facundomedica
Copy link
Member

is this consensus breaking or is metadata outside the app hash

As far as I understand the metadata is not included in the app hash. CommitInfo.Hash() uses only the hashes of the StoreInfos inside of it while the CommitInfo itself is written directly to the kv store.

Please confirm that's how it works @alexanderbez

@alexanderbez
Copy link
Contributor Author

Correct @facundomedica, this is not consensus breaking, only the resulting hash is used.

@alexanderbez alexanderbez merged commit ee9774a into main Mar 28, 2023
@alexanderbez alexanderbez deleted the bez/12226-auto-timestamp-query branch March 28, 2023 15:10
mergify bot pushed a commit that referenced this pull request Mar 28, 2023
(cherry picked from commit ee9774a)

# Conflicts:
#	CHANGELOG.md
#	api/cosmos/base/store/v1beta1/commit_info.pulsar.go
#	baseapp/abci.go
#	go.sum
#	simapp/go.mod
#	simapp/go.sum
#	store/go.mod
#	store/rootmulti/store.go
#	store/types/commit_info.pb.go
#	tests/go.mod
#	tests/go.sum
#	types/errors/errors.go
facundomedica pushed a commit that referenced this pull request Mar 28, 2023
… (#15573)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
@ValarDragon
Copy link
Contributor

Awesome!!

p0mvn pushed a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
…440)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
mergify bot pushed a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
…440)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
(cherry picked from commit 8a66b9a)

# Conflicts:
#	store/types/commit_info.pb.go
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
…(backport #440) (#441)

* feat: auto-set block timestamp for historical queries (cosmos#15448) (backport #440)

* updates

* updates

* updates

---------

Co-authored-by: Roman <roman@osmosis.team>
mergify bot pushed a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
…440)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
(cherry picked from commit 8a66b9a)

# Conflicts:
#	store/types/commit_info.pb.go
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 9, 2023
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 9, 2023
…(backport #440) (#442)

* feat: auto-set block timestamp for historical queries (cosmos#15448) (backport #440)

* go mod tidy

---------

Co-authored-by: Roman <roman@osmosis.team>
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
roy-dydx pushed a commit to dydxprotocol/cosmos-sdk that referenced this pull request Jul 11, 2023
…s#15448) (cosmos#15573)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:Store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect timestamp is used when querying at previous block heights
4 participants