Skip to content
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

fix penumbra-specific timestamp reads in ibc module #4822

Merged
merged 8 commits into from
Aug 21, 2024

Conversation

noot
Copy link
Collaborator

@noot noot commented Aug 20, 2024

Describe your changes

fix penumbra-specific timestamp reads in ibc module by using HostInterface instead.

  • Ics20WithdrawalWithHandler was created analogously to IbcRelayWithHandler, which attaches a HostInterface to be used in check_and_execute to get the current block timestamp
  • the rpc method client_state was also updated to use HostInterface

Issue ticket number and link

closes #4812

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    the timestamp reads are unchanged for penumbra, as the PenumbraHost is used, which calls the same methods as before

@conorsch
Copy link
Contributor

This PR is failing CI because the penumbra-ibc crate no longer compiles to wasm. That's a regression we check for, because we don't want to break compatibility in web contexts, e.g. for use in the wallet web extension. My understanding of the current diff is that it's the use of tendermint::Time that breaks wasm compat.

To see the breakage on the wasm side, check out the web repo and hop into packages/wasm/crate/: https://github.com/penumbra-zone/web/tree/b732bc580e16d827e55d25a64b1d5d05d85b93a0/packages/wasm/crate Add a patch to the Cargo.toml for the wasm crate to use the local penumbra-ibc checkout:

--- a/packages/wasm/crate/Cargo.toml
+++ b/packages/wasm/crate/Cargo.toml
@@ -50,3 +50,7 @@ web-sys = { version = "0.3.69", features = ["console"] }
 [dev-dependencies]
 wasm-bindgen-test = "0.3.42"
 serde_json = "1.0.120"
+
+[patch."https://github.com/penumbra-zone/penumbra"]
+# assumes that the "penumbra" protocol repo is cloned adjacent to the web repo.
+penumbra-ibc = { path = "../../../../penumbra/crates/core/component/ibc" }

Then run cargo update && cargo check.

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The penumbra-ibc changes break wasm compatibility, which would break web code.

@turbocrime
Copy link
Contributor

turbocrime commented Aug 21, 2024

wasm crate seems to build fine with this alternate patch. what's the difference here?

aside: is there a more conventional way to specify a repeated specifier like tag/branch

diff --git a/packages/wasm/crate/Cargo.toml b/packages/wasm/crate/Cargo.toml
index c78cd16f7..a58548b8e 100644
--- a/packages/wasm/crate/Cargo.toml
+++ b/packages/wasm/crate/Cargo.toml
@@ -14,21 +14,21 @@ default = ["console_error_panic_hook"]
 mock-database = []
 
 [dependencies]
-penumbra-auction = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-auction", default-features = false }
-penumbra-asset = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-asset" }
-penumbra-compact-block = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-compact-block", default-features = false }
-penumbra-dex = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-dex", default-features = false }
-penumbra-fee = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-fee", default-features = false }
-penumbra-governance = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-governance", default-features = false }
-penumbra-keys = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-keys" }
-penumbra-num = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-num" }
-penumbra-proof-params = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-proof-params", default-features = false }
-penumbra-proto = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-proto", default-features = false }
-penumbra-sct = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-sct", default-features = false }
-penumbra-shielded-pool = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-shielded-pool", default-features = false }
-penumbra-stake = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-stake", default-features = false }
-penumbra-tct = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-tct" }
-penumbra-transaction = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.80.0", package = "penumbra-transaction", default-features = false }
+penumbra-auction = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-auction", default-features = false }
+penumbra-asset = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-asset" }
+penumbra-compact-block = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-compact-block", default-features = false }
+penumbra-dex = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-dex", default-features = false }
+penumbra-fee = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-fee", default-features = false }
+penumbra-governance = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-governance", default-features = false }
+penumbra-keys = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-keys" }
+penumbra-num = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-num" }
+penumbra-proof-params = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-proof-params", default-features = false }
+penumbra-proto = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-proto", default-features = false }
+penumbra-sct = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-sct", default-features = false }
+penumbra-shielded-pool = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-shielded-pool", default-features = false }
+penumbra-stake = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-stake", default-features = false }
+penumbra-tct = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-tct" }
+penumbra-transaction = { git = "https://github.com/penumbra-zone/penumbra.git", branch = "noot/host-interface-fix", package = "penumbra-transaction", default-features = false }
 
 anyhow = "1.0.86"
 ark-ff = { version = "0.4.2", features = ["std"] }

@turbocrime
Copy link
Contributor

notably wasm doesn't depend on the ibc crate at all, lol. maybe that is the difference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any problem with restoring the previous Cargo.toml configuration? it seems to build fine, and satisfies the compat checker

Copy link
Contributor

@turbocrime turbocrime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

built and manually tested prax with this branch, both before and after my commit. testnet ibc roundtrips successful. afaict it's fully functional so it's fine by me

@conorsch
Copy link
Contributor

Excellent, thanks for the assist, @turbocrime! I'm not sure why my toml patch gave inaccurate results, but I agree with your solution, and appreciate the manual testing via local builds. My concerns are address, I'm happy to merge.

@conorsch conorsch merged commit 87adc8d into main Aug 21, 2024
13 checks passed
@conorsch conorsch deleted the noot/host-interface-fix branch August 21, 2024 18:38
@conorsch
Copy link
Contributor

@noot for now, please use a git dep against the latest commit on main: 87adc8d6b15f6081c1adf169daed4ca8873bd9f6. We'll likely have a v0.80.3 tag up in a week or two.

github-merge-queue bot pushed a commit to astriaorg/astria that referenced this pull request Aug 21, 2024
## Summary
Bumps penumbra deps to no longer use penumbra-specific key.

## Summary
The
`penumbra_ibc::component::packet::IBCPacket<Unchecked>::send_packet_check`
attempts to read a timestamp stored under a penumbra specific key which
we never write. This was an oversight in penumbra and recently fixed.

## Testing
Unblocks e2e testing.

## Related Issues

penumbra-zone/penumbra#4812 
penumbra-zone/penumbra#4822
@conorsch conorsch mentioned this pull request Aug 22, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

penumbra-ibc library accesses penumbra specific rocksdb path
4 participants