-
Notifications
You must be signed in to change notification settings - Fork 446
test(proof): add failing case #4369
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
Closed
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
🛠 PR Checks Summary🔴 Pending initial approval by a review team member, or review from tech-staff Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
zivkovicmilos
added a commit
that referenced
this pull request
Sep 29, 2025
The IBC protocol is based on merkle proofs, and over the years a standard of merkle proof representation has emerged: [ICS23](https://github.com/cosmos/ics23). The simplest way for Gno to be compatible with this protocol is to migrate the iavl implementation to [`cosmos/iavl`](https://github.com/cosmos/iavl), which already supports ICS23. ```diff - Therefore, this PR deprecates `tm2/pkg/iavl` in favour of imported `cosmos/iavl`. + Therefore, this PR replaces the code of `tm2/pkg/iavl` with the code of `cosmos/iavl@v1.3.5`. ``` Note that `cosmos/iavl` is just a maintened version of `tm2/pkg/iavl`, which has received numerous improvements and bug fixes over the years. In addition to the benefit of making Gno IBC-ready, this change also eliminates some drawbacks of `tm2/pkg/iavl`: - `tm2/pkg/iavl` uses the legacy `range-proof` under the hood which is considered unsafe. It has an exploited vulnerability (see DragonFruit [[1](https://github.com/cosmos/iavl/issues/584)][[2](https://forum.cosmos.network/t/cosmos-sdk-security-advisory-dragonfruit/7614)]) - `tm2/pkg/iavl` proof generation is buggy: see #4369 This change only concerns the proof system. The Gno light client realm which is the other important piece of the IBCv2 connection between Gnoland and AtomOne is still under progress [here](#4655). > [!IMPORTANT] > This [change](https://github.com/gnolang/gno/pull/4656/files#diff-4f1d80300014eb607e44892f6ebdcc31de2c0815669acd54f805b83501311b7fL419) to the commit info hashing is causing a app hash mismatch when restarting a node with a binary built from this branch. > ``` > panic: state.AppHash does not match AppHash after replay. Got > 76BB396891B9F4415AC49FB72CF81933F407AE6A2C04551814719C9A908B2E8E, expected 8EDAB60FFD0B2C6BB5AC398417CF0AFBFE6E7D491C830F831F9395DB7FAC522A. > ``` > Unfortunately without it some tests are failing because ics23 does not expect to have hashed value. As a result, upgrading a mainnet launched using `tm2/pkg/iavl` to a new version with `cosmos/iavl` might be challenging. It's probably a good idea to merge this change for the initial mainnet. ## For reviewers - Most of the diff is related to the `DB` interface, because most of declared methods return an error in `comos/iavl` - When possible I added a comment for change related to the difference between `tm2/pkg/iavl` and `cosmos/iavl` implementation. - Lazy loading has been removed in `cosmos/iavl` so I had no other choice than removing the option from the Gno store. Anyway this feature was not used. --------- Co-authored-by: n0izn0iz <n0izn0iz@users.noreply.github.com> Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
MikaelVallenet
pushed a commit
to MikaelVallenet/gno
that referenced
this pull request
Oct 1, 2025
…ng#4656) The IBC protocol is based on merkle proofs, and over the years a standard of merkle proof representation has emerged: [ICS23](https://github.com/cosmos/ics23). The simplest way for Gno to be compatible with this protocol is to migrate the iavl implementation to [`cosmos/iavl`](https://github.com/cosmos/iavl), which already supports ICS23. ```diff - Therefore, this PR deprecates `tm2/pkg/iavl` in favour of imported `cosmos/iavl`. + Therefore, this PR replaces the code of `tm2/pkg/iavl` with the code of `cosmos/iavl@v1.3.5`. ``` Note that `cosmos/iavl` is just a maintened version of `tm2/pkg/iavl`, which has received numerous improvements and bug fixes over the years. In addition to the benefit of making Gno IBC-ready, this change also eliminates some drawbacks of `tm2/pkg/iavl`: - `tm2/pkg/iavl` uses the legacy `range-proof` under the hood which is considered unsafe. It has an exploited vulnerability (see DragonFruit [[1](https://github.com/cosmos/iavl/issues/584)][[2](https://forum.cosmos.network/t/cosmos-sdk-security-advisory-dragonfruit/7614)]) - `tm2/pkg/iavl` proof generation is buggy: see gnolang#4369 This change only concerns the proof system. The Gno light client realm which is the other important piece of the IBCv2 connection between Gnoland and AtomOne is still under progress [here](gnolang#4655). > [!IMPORTANT] > This [change](https://github.com/gnolang/gno/pull/4656/files#diff-4f1d80300014eb607e44892f6ebdcc31de2c0815669acd54f805b83501311b7fL419) to the commit info hashing is causing a app hash mismatch when restarting a node with a binary built from this branch. > ``` > panic: state.AppHash does not match AppHash after replay. Got > 76BB396891B9F4415AC49FB72CF81933F407AE6A2C04551814719C9A908B2E8E, expected 8EDAB60FFD0B2C6BB5AC398417CF0AFBFE6E7D491C830F831F9395DB7FAC522A. > ``` > Unfortunately without it some tests are failing because ics23 does not expect to have hashed value. As a result, upgrading a mainnet launched using `tm2/pkg/iavl` to a new version with `cosmos/iavl` might be challenging. It's probably a good idea to merge this change for the initial mainnet. ## For reviewers - Most of the diff is related to the `DB` interface, because most of declared methods return an error in `comos/iavl` - When possible I added a comment for change related to the difference between `tm2/pkg/iavl` and `cosmos/iavl` implementation. - Lazy loading has been removed in `cosmos/iavl` so I had no other choice than removing the option from the Gno store. Anyway this feature was not used. --------- Co-authored-by: n0izn0iz <n0izn0iz@users.noreply.github.com> Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
Contributor
Author
|
Fixed by #4656 |
gfanton
pushed a commit
to gfanton/gno
that referenced
this pull request
Oct 10, 2025
…ng#4656) The IBC protocol is based on merkle proofs, and over the years a standard of merkle proof representation has emerged: [ICS23](https://github.com/cosmos/ics23). The simplest way for Gno to be compatible with this protocol is to migrate the iavl implementation to [`cosmos/iavl`](https://github.com/cosmos/iavl), which already supports ICS23. ```diff - Therefore, this PR deprecates `tm2/pkg/iavl` in favour of imported `cosmos/iavl`. + Therefore, this PR replaces the code of `tm2/pkg/iavl` with the code of `cosmos/iavl@v1.3.5`. ``` Note that `cosmos/iavl` is just a maintened version of `tm2/pkg/iavl`, which has received numerous improvements and bug fixes over the years. In addition to the benefit of making Gno IBC-ready, this change also eliminates some drawbacks of `tm2/pkg/iavl`: - `tm2/pkg/iavl` uses the legacy `range-proof` under the hood which is considered unsafe. It has an exploited vulnerability (see DragonFruit [[1](https://github.com/cosmos/iavl/issues/584)][[2](https://forum.cosmos.network/t/cosmos-sdk-security-advisory-dragonfruit/7614)]) - `tm2/pkg/iavl` proof generation is buggy: see gnolang#4369 This change only concerns the proof system. The Gno light client realm which is the other important piece of the IBCv2 connection between Gnoland and AtomOne is still under progress [here](gnolang#4655). > [!IMPORTANT] > This [change](https://github.com/gnolang/gno/pull/4656/files#diff-4f1d80300014eb607e44892f6ebdcc31de2c0815669acd54f805b83501311b7fL419) to the commit info hashing is causing a app hash mismatch when restarting a node with a binary built from this branch. > ``` > panic: state.AppHash does not match AppHash after replay. Got > 76BB396891B9F4415AC49FB72CF81933F407AE6A2C04551814719C9A908B2E8E, expected 8EDAB60FFD0B2C6BB5AC398417CF0AFBFE6E7D491C830F831F9395DB7FAC522A. > ``` > Unfortunately without it some tests are failing because ics23 does not expect to have hashed value. As a result, upgrading a mainnet launched using `tm2/pkg/iavl` to a new version with `cosmos/iavl` might be challenging. It's probably a good idea to merge this change for the initial mainnet. ## For reviewers - Most of the diff is related to the `DB` interface, because most of declared methods return an error in `comos/iavl` - When possible I added a comment for change related to the difference between `tm2/pkg/iavl` and `cosmos/iavl` implementation. - Lazy loading has been removed in `cosmos/iavl` so I had no other choice than removing the option from the Gno store. Anyway this feature was not used. --------- Co-authored-by: n0izn0iz <n0izn0iz@users.noreply.github.com> Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following convo on Signal, I create this draft PR to demonstrate the bug and report all the informations.
Disclaimer
I didn't test all the different scenarios so there might be other cases of proof generation that are incorrect. The case I present here is the one I had a chance to reproduce, but I found an other scenario where the proof was also incorrect but differently than the case here.
Context
While I was asserting the behavior of tm2 proofs (for the IBC connection with A1), I found an issue with absence proofs.
The test
TestTreeGetWithProof()(modified in this PR) builds a merkle tree with keys0x11, 0x32, 0x50, 0x72, 0x99, and so looks like this:KEY / HASHAfter asserting a proof of the presence of the key
0x32, which is not relevant here, the test asserts a proof of the absence of key0x1. The returned proof is correct and asserts that0x1is absent of the tree. In term of leaves, the proof contains only the leaf 1, because0x1 < 0x11(key of 1).Now if that key is changed to
0x1100like in this PR, the returned proof is exactly the same as with0x1, but this is incorrect because this time0x1100 > 0x11. For that proof to be correct, the proof's leaves should contain both 1 and 2 because0x1100is between0x11and0x32.Hence the verification algorithm returns an error, given that absence of second leaf:
absence not proved by right leaf (need another leaf?)Important
The same change applied to the same test in
cosmos/iavlproduces a valid proof.