Not increasing the version when the tree is identical

[b00f]

When the tree is same as previous version, saving version won't change the version. Fixing #128

[ethanfrey]

Uh, I think we need to update the version on every save. So it is in line with the block height. Otherwise it will have no meaning.

If you want to have no-empty-blocks, the issue is in cosmos-sdk, not in the iavl store: cosmos/cosmos-sdk#1351

The iavl store does not change app hash when the version increases.

[ethanfrey]

Also, please link to an issue (not self link to this PR) when saying "fixing".
I would like the related issue in cosmos-sdk to be fixed... since a year

[b00f]

Uh, I think we need to update the version on every save. So it is in line with the block height. Otherwise it will have no meaning.

In this case you can ignore this PR. But I think changing version when nothing has changed is not a good idea.

Also, please link to an issue (not self link to this PR) when saying "fixing".

Sorry. My very bad mistake. I linked the correct issue.

[b00f]

I do believe that this patch can fix this issue on tendermint. tendermint/tendermint#1909

Do a try please.

[ethanfrey]

@b00f when you do /abci_query with a specified height parameter, this requires a clear mapping from block height to iavl version to retrieve the information.

In cosmos-sdk (as well as weave), we rely on this mapping to provide the last executed block in Info. This is essential for syncing tendermint and abci on a restart.

Breaking this 1:1 mapping between block height and iavl version will break many things. You may also note that this "empty block" issue only affects the sdk (I have no empty blocks working happily for over a year in weave). There is a one line fix that doesn't break anything else in cosmos/cosmos-sdk#1351 (don't include the version in the hash the cosmos-sdk multistore creates).

I do not deny that it can fix one issue, but it creates many more. And I still don't know why the proper solution is not being merged

[ethanfrey]

Proper fix is here cosmos/cosmos-sdk#4613 if cosmos-sdk team will merge it

[b00f]

Maybe we need to add new method (like ForceSaveVersion) to update the version even the content is identical.
If you ask my opinion I prefer to not update the version when the content has not changed.

[ethanfrey]

If you ask my opinion I prefer to not update the version when the content has not changed.

Why? Cosmos-sdk code relies on this fact. Can you provide a clear argument why it is wrong. As changing this will require changing much code downstream

If you are writing your own app framework (not cosmos-sdk), you can just avoid saving if there were no transactions and handle the height <-> version mapping manually elsewhere in your code.

[b00f]

If you are writing your own app framework (not cosmos-sdk), you can just avoid saving if there were no transactions and handle the height <-> version mapping manually elsewhere in your code.

I have done it before when I was debugging empty_block_creation issue on Tendermint and I found out that this library always changes the app hash because version always changes. So there are two ways to fix:
1- App can ignore version and always rely on block height.
2- Not changing version for empty blocks

Why? Cosmos-sdk code relies on this fact. Can you provide a clear argument why it is wrong. As changing this will require changing much code downstream

Maybe different taste. But consider another application (not necessarily a blockchain one) is going to use this library. It's more nicer if a snapshot is created only when the tree is changed. That might give better performance when looking for a specific version.

My big concern is resolving the empty_block_creation issue. Unfortunately Tendermint blocks are big and in long term it's needed to spend more money to store the blockchain history. For small and middle companies this is a big concern.

[melekes]

@b00f

1. are you using cosmos SDK or your own framework? ABCI app?
2. how hard it is to detect if there were any changes to the IAVL tree externally and only call SaveVersion once there are some changes?

[ethanfrey]

cosmos/cosmos-sdk#4613 was merged. It will be in the next cosmos-sdk release.

Then create_empty_blocks=false will work without any disruptive changes.

My point was that much of the code depends on this behavior, and doing it is a bigger architecture issue, and not related to empty blocks at all actually

[b00f]

are you using cosmos SDK or your own framework? ABCI app?

We are using cosmos-sdk for a new blockchain project. And we prefer to not change the core.

how hard it is to detect if there were any changes to the IAVL tree externally and only call SaveVersion once there are some changes?

It's almost impossible. Since Cosmos-sdk is handling everything. But cosmos/cosmos-sdk#4624 resolved this issue. Many thanks.

@ethanfrey That will be very helpful. I think we can close this PR now. Also please close issue #128 .

[melekes]

Okay, great! Many thanks @ethanfrey 🙇