ibc: delete packet commitments instead of writing empty values #4644
Conversation
This file contains 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
avahowell
added
the
consensus-breaking
erwanor
added
A-IBC
erwanor
approved these changes
Jun 19, 2024
There was a problem hiding this comment.
ACK on the change, and the migration looks correct too.
To test this, we can:
- Halt a full node running the current chain
- Perform the migration (using
--force) - Start the node again with peering disabled
- Run
pcli query key ibc-data/commitments/ports/transfer/channels/channel-7/sequences/7 - Observe
Not found
|
One thing that occurred to me is that perhaps we should make a design change to the storage API to prevent writing empty values, since any empty value (in the ibc substore) will cause this problem |
erwanor
requested changes
Jun 20, 2024
The IBC spec calls for deletion of packet commitments after a successful acknowledgement, in order to avoid double-acknowledgements. Previously, our implementation accomplished this deletion by writing an empty byte slice vec![] to the commitment path. However, this will cause nonexistence proofs by exclusion (the standard NX-proof type used in ics23) of adjacent keys to fail, since ics23 does not support existence proofs of empty values. This PR changes `delete_packet_commitment` to actually delete the commitment, and introduces a migration for testnet 78 that will delete all of the previously overwritten commitments.
erwanor
force-pushed
the
ibc-key-deletion-timeout-fix
branch
from
11f24b5
to
72d4d98
Compare
We got it, while testing I found a couple issues with prefix caching, I will open issues shortly. This PR works though, so approving and merging on green CI. |
erwanor
approved these changes
Jun 21, 2024
avahowell
added a commit
that referenced
this pull request
Jun 24, 2024
The IBC spec calls for deletion of packet commitments after a successful acknowledgement, in order to avoid double-acknowledgements. Previously, our implementation accomplished this deletion by writing an empty byte slice vec![] to the commitment path. However, this will cause nonexistence proofs by exclusion (the standard NX-proof type used in ics23) of adjacent keys to fail, since ics23 does not support existence proofs of empty values. This PR changes `delete_packet_commitment` to actually delete the commitment, and introduces a migration for testnet 78 that will delete all of the previously overwritten commitments.
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.
The IBC spec calls for deletion of packet commitments after a successful acknowledgement, in order to avoid double-acknowledgements.
Previously, our implementation accomplished this deletion by writing an empty byte slice vec![] to the commitment path.
However, this will cause nonexistence proofs by exclusion (the standard NX-proof type used in ics23) of adjacent keys to fail, since ics23 does not support existence proofs of empty values. We observed this when timeouts from osmosis testnet failed to verify the NX proofs from penumbra testnet.
This PR changes
delete_packet_commitmentto actually delete the commitment, and introduces a migration for testnet 78 that will delete all of the previously overwritten commitments.