-
Notifications
You must be signed in to change notification settings - Fork 201
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
base: proof-integration
Are you sure you want to change the base?
Conversation
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
uses a version of the cosmos sdk that passes a multistore to queries which contains height-1 as well, not only the last height