Return empty store tree on non-existing version

[RiccardoM]

Description

This PR tries to fix the different store versions loading problem. It simply returns an empty tree when a store version does not exist. Closes: #7385.

This has been tested using the following procedure:

1. Start a Desmos chain without the relationships module.
2. Create an upgrade proposal that switches to a new Desmos version with the new relationships module registered
3. Vote that upgrade proposal, and let it run.
4. Try querying the relationships.

Everything seems fine. The new module can be queried without any problem, as well as the old ones.

TODO

If this change is OK, the following things still have to be done:

• Change the store #GetImmutable documentation accordingly
• Fix the unit tests properly
• Add a CHANGELOG entry

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov]

Codecov Report

Merging #7415 into master will increase coverage by 0.01%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #7415      +/-   ##
==========================================
+ Coverage   55.96%   55.98%   +0.01%     
==========================================
  Files         594      594              
  Lines       37270    37281      +11     
==========================================
+ Hits        20859    20870      +11     
+ Misses      14300    14294       -6     
- Partials     2111     2117       +6     

[alexanderbez]

Nice! So how does this work under the following scenarios:

• Querying an existing store at some previous non-pruned height H
• Querying an existing store at some previous pruned height H_p
• Querying an existing store at current height H_c
• Querying a new store at some previous non-pruned height H
• Querying a new store at some previous pruned height H_p
• Querying a new store at current height H_c

/cc @erikgrinaker

[erikgrinaker]

Yeah, I don't really see how this can be right. If the app is at height 900 and the new store is at version 100 (such that version 1-100 in the new store map to heights 800-900 in the app) then surely querying height 850 for the new store will not work since there is no version 850 in the store.

Since you say that this works, then maybe the SDK ends up querying the latest version in this case (I'm not too familiar with the code), but the query requested height 850 not the latest height so it'll still be wrong.

[RiccardoM]

Yeah, I don't really see how this can be right. If the app is at height 900 and the new store is at version 100 (such that version 1-100 in the new store map to heights 800-900 in the app) then surely querying height 850 for the new store will not work since there is no version 850 in the store.

Since you say that this works, then maybe the SDK ends up querying the latest version in this case (I'm not too familiar with the code), but the query requested height 850 not the latest height so it'll still be wrong.

I think in this case, since we're returning an empty tree when the version does not exist, then all the queries will just be empty (which is correct since the store didn't exist). Am I missing something?

Anyway I'll try testing all the cases described by @alexanderbez. Do you have any idea how I can get Hp properly to make sure it's pruned?

[erikgrinaker]

I think in this case, since we're returning an empty tree when the version does not exist, then all the queries will just be empty (which is correct since the store didn't exist). Am I missing something?

I think so, because we're returning an empty store for the wrong heights.

In the example above, version 1-100 in the new store was created during heights 800-900. If I write "foo=850" at height 850, this will be stored at version 50 in the new store.

If I query foo as of height 850 it will return nothing, because the store has no version 850 - this is wrong, since the value at height 850 should be foo=850.

If I query foo as of height 50 it will return foo=850. This is wrong, since the store did not exist at all at height 50, and should return empty.

[RiccardoM]

In the example above, version 1-100 in the new store was created during heights 800-900. If I write "foo=850" at height 850, this will be stored at version 50 in the new store.

If I query foo as of height 850 it will return nothing, because the store has no version 850 - this is wrong, since the value at height 850 should be foo=850.

If I query foo as of height 50 it will return foo=850. This is wrong, since the store did not exist at all at height 50, and should return empty.

That makes sense. So, is there any way we can get the height at which the store was created? Or, maybe, we can set the new store height with the upgrade height and pad all the previous versions with empty stores? Could this be possible?

[erikgrinaker]

Yeah, so I don't want to prescribe a solution for the SDK here, since I'm not on that team - I'll defer to @alexanderbez. But in IAVL 0.15.0-rc2 we implemented an initial version API which allows setting the initial version for a new IAVL store, via Options.InitialVersion and/or MutableTree.SetInitialVersion(). I have backported this to 0.14 on this branch, but not cut a release yet: https://github.com/cosmos/iavl/tree/0.14.x

It's definitely a viable solution to create a new store with a specific initial version, and then ignore missing version errors when attempting to fetch any previous versions (although this can have the effect of silently ignoring actual bugs that cause versions to go missing). It's also possible to pad out the store with empty versions, corresponding to the versions that exist in the other stores (which will depend on pruning settings).

[alexanderbez]

It's definitely a viable solution to create a new store with a specific initial version, and then ignore missing version errors when attempting to fetch any previous versions (although this can have the effect of silently ignoring actual bugs that cause versions to go missing).

This is the route I'd like to attempt to go with.

1. Set initial versions for new stores on upgrades. Maybe we can automatically do this in the root multi-store during LoadVersion so app devs don't have to worry about this during app construction. And,
2. Ignore the store when the version is not available.

[RiccardoM]

This is the route I'd like to attempt to go with.

1. Set initial versions for new stores on upgrades. Maybe we can automatically do this in the root multi-store during LoadVersion so app devs don't have to worry about this during app construction. And,
2. Ignore the store when the version is not available.

Do you have any hint how this can be achieved? Even high level ones, I'll try to do this tomorrow

[alexanderbez]

Use the SetInitialVersion API. I think when loading stores, we can tell if a store is new (i.e. latest version is zero). If so, we can call SetInitialVersion on it. Would that work @erikgrinaker?

[erikgrinaker]

I think so, yeah.

[RiccardoM]

@alexanderbez @erikgrinaker Ok, I've tried implementing it as you said. I had to introduce a little hack to make it work properly: when using tree.LoadVersion, I ignore any error if the store has been added recently. This is to avoid the following error when using tree.SetInitialVersion:

initial version set to 2, but found earlier version 1

Apparently, when loading a specific version of the tree, the IAVL checks to make sure that the initial version of each store is not greater than any other previous versions of any store. If you have any other way of doing this without this magic trick that would be great. I think it might lead to some problems in the future to ignore all the errors.

I've added a test set name TestLoadStore inside store_test.go, but I was not able to find a way to prune a specific height to test such cases.

[erikgrinaker]

initial version set to 2, but found earlier version 1

Doesn't this indicate that the fix doesn't work? It's too late to set the initial version if the tree already has a version in it.

I don't think you can call Version() until the tree has been loaded, it always returns 0 in that case. You may have to call Load() first, which will load the latest available version, but that may not be optimal here since we're given a specific version to load.

[RiccardoM]

Doesn't this indicate that the fix doesn't work? It's too late to set the initial version if the tree already has a version in it.

I don't think you can call Version() until the tree has been loaded, it always returns 0 in that case. You may have to call Load() first, which will load the latest available version, but that may not be optimal here since we're given a specific version to load.

Ok, I've tried moving the SetInitialVersion after loading the store. I still have some doubts about the tests. Are they correct? Cause I don't think so to be honest

[alexanderbez]

I believe the version should be set before loading the store. Somewhere in the root multi-store when we load all the versions on bootstrapping, we need to detect that a store is new (i.e. its version is zero) and the last commit id version is higher, so we call SetInitialVersion on that store when creating the IAVL store.

[RiccardoM]

I believe the version should be set before loading the store. Somewhere in the root multi-store when we load all the versions on bootstrapping, we need to detect that a store is new (i.e. its version is zero) and the last commit id version is higher, so we call SetInitialVersion on that store when creating the IAVL store.

Can you please point me to where this should happen then? I'm not familiar with the multi store but I get dig into this having a starting place where to look at (method name).

[erikgrinaker]

It occurred to me that another option would be to simply set the initial version to H and then explicitly save an empty version at the upgrade height. It might be more intuitive that the store exists from the upgrade height onwards (inclusive), but don't know if this would complicate the implementation. Don't have any strong opinions on this though, just an idea.

[RiccardoM]

It occurred to me that another option would be to simply set the initial version to H and then explicitly save an empty version at the upgrade height. It might be more intuitive that the store exists from the upgrade height onwards (inclusive), but don't know if this would complicate the implementation. Don't have any strong opinions on this though, just an idea.

@alexanderbez Do you have any opinion on this? Should we leave it as it is setting the initial value to H + 1, or should we make it H and save an empty commit?

[alexanderbez]

I don't see any immediate benefit to having an empty commit. I think the H+1 approach should be fine. Glad we figured it out.

[erikgrinaker]

@RiccardoM IAVL 0.14.1 is out: https://github.com/cosmos/iavl/releases/tag/v0.14.1

[RiccardoM]

@erikgrinaker @alexanderbez @amaurymartiny I think this is now ready to be merged. I'm also working on another branch to port this to v0.39.1 as well

[alexanderbez]

one minor nit, otherwise LGTM 👍

[erikgrinaker]

I've released IAVL 0.15.0-rc4, which now errors when attempting to load a non-existing version from an empty tree. This change may affect this PR. See: cosmos/iavl#317