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

Proof integration fix store #1505

Open
wants to merge 2 commits into
base: proof-integration
Choose a base branch
from

Conversation

eshelB
Copy link
Contributor

@eshelB eshelB commented Aug 8, 2023

uses a version of the cosmos sdk that passes a multistore to queries which contains height-1 as well, not only the last height

@@ -5,7 +5,7 @@ go 1.19
replace (
// dragonberry
github.com/confio/ics23/go => github.com/cosmos/cosmos-sdk/ics23/go v0.8.0
github.com/cosmos/cosmos-sdk => github.com/scrtlabs/cosmos-sdk v0.45.13-0.20230726163133-dc2b5eab1db5
github.com/cosmos/cosmos-sdk => github.com/scrtlabs/cosmos-sdk v0.45.13-0.20230808154448-1e7ad22811a4
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this scrtlabs/cosmos-sdk@1e7ad22811a4, is there a way to pass height-1 when we call this? I'm not sure what are the performance implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we pass height-1 it breaks all other queries other than wasm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasm doesn't care as it has the cache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I told Itzik about this, we thought maybe we should see what happens and if there are performance implications, we know what to look for. Also memory implications, as now queries have the whole db. But probably none of these implications exist as these are all pointers to subsections of the iavl store?

Copy link
Member

@toml01 toml01 Aug 15, 2023

Choose a reason for hiding this comment

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

If we see performance issues, we could probably even create a context with both height and height - 1 instead of the whole store.

Copy link
Contributor Author

@eshelB eshelB Sep 6, 2023

Choose a reason for hiding this comment

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

I tried to find a way to do this but it seems that the multiStore there didn't support more than one specific version, only all of them

Copy link
Member

@toml01 toml01 left a comment

Choose a reason for hiding this comment

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

This is a buggy cosmos-sdk version, causing x-cosmos-block-height (i.e. query at height) to stop working. See failing test.
We shouldn't merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants