Interchaintest should have Penumbra support (via `pclientd`)

[conorsch]

There's ongoing work to integrate Penumbra into interchaintest. This issue tracks outstanding obstacles, to coordinate changes between interchaintest, upstream penumbra, and potentially the relayer.

• [x] The CLI flag for pd, specifying the target Tendermint RPC address, --tendermint-addr, should have an http:// prefix. pd was rather silently failing when this option was not set correctly. Patched upstream via https://github.com/penumbra-zone/penumbra/pull/2719
• [x] Write example golang code that takes a Penumbra address as bytes and converts it to a string representation. See example here.
• [x] Write example golang code that takes a Penumbra address as a string and converts it to a byte slice, suitable for passing to proto RPC methods such as BalanceByAddressRequest. Helpful constraints (which should probably be added to Penumbra's proto docs):
  • The byte slice should have length of 80
  • Each element of the slice should be an integer 0-255
  • N.B. we now have more ergonomic protos that accept a bech32m string for an Address, but we still need to solve this problem.
• [x] Given that BalanceByAddressRequest returns a stream, we must handle it as a special case from the other RPC calls. Therefore:
  • [x] Write example golang impl for handling a streaming request from pclientd. See exmaple here.
  • [x] Update the interchaintest calls to handle a streaming request reasonably well (see above for a good enough pattern)
  • [x] Update the Penumbra proto docs to make return types more explicit, to reduce the likelihood of unpleasant surprises. Tracked in https://github.com/penumbra-zone/penumbra/issues/2734
• [x] Testnet liquid allocations for Penumbra within interchaintest are getting clobbered. We haven't figured out why yet, but we're confident that the upenumbra allocations are not making it into the genesis file served by Tendermint within interchaintest.
• [x] Update pcli keys import phrase to pass the seed phrase on stdin, rather than as a CLI arg. This will ship in 0.56.0, see related breaking change in https://github.com/penumbra-zone/penumbra/pull/2733
• [x] Write golang business logic to make sense of "balances":
  • [x] Convert AssetId to human-readable denom: achievable via the https://buf.build/penumbra-zone/penumbra/docs/main:penumbra.client.v1alpha1#penumbra.client.v1alpha1.SpecificQueryService.DenomMetadataById RPC method
  • [x] Update Penumbra protos to accept an optional denom string on the balance request: https://github.com/penumbra-zone/penumbra/issues/2768
  • [x] Update interchaintest go code to consume new optional denom strings for AssetIds on the balance request, as per https://github.com/penumbra-zone/penumbra/issues/2768
  • [x] Update GetBalance methods to accept a denom string for filtering
  • [x] Handle amount translation between 64-bit and 128-bit integers
    • [x] Add golang function for translation Hi/Lo 64-bit values from RPC into combined 128-bit BigInt representation
    • [x] Support passing of 128-bit Amounts within interchaintest, being respectful of interfaces used by other chains - https://github.com/strangelove-ventures/interchaintest/pull/679; then:
    • [x] Add golang function for translating combined 128-bit BigInt representation and split it into 64-bit Hi/lo values, for compatibility with Penumbra RPCs
• [ ] Implement smoke test support
  • [x] Create transaction
  • [x] Build, authorize, broadcast
  • [x] Confirm successful tx by checking balance
  • [ ] Ensure that test allocations include greater-than-64bit-values, to exercise hi/lo amount code
• [x] Figure out how to target pd service url via an ephemeral container. This is technically a side quest, but would be massively helpful in unblocking interactively pcli testing to validate assumptions about what data should be returned from the rpc method calls.
• [x] Resolve https://github.com/strangelove-ventures/interchaintest/issues/658
• [ ] Update Penumbra protos used in integration to latest version available
  • [x] v0.55.0
  • [x] v0.57.0 - Balances doesn't work, we need a fix: https://github.com/penumbra-zone/penumbra/issues/2894
  • [x] v0.58.0
  • [x] v0.59.0
  • [x] v0.60.0
• [x] Figure out support for optional proto3 fields: https://github.com/penumbra-zone/penumbra/issues/2933
• [x] Resolve oomkill on pclientd high memory usage: https://github.com/penumbra-zone/penumbra/issues/2977
• [ ] https://github.com/strangelove-ventures/interchaintest/issues/313
• [x] Fix timeout handling error https://github.com/penumbra-zone/penumbra/pull/3607
• [x] Fix client state encoding error in Penumbra IBC impl https://github.com/penumbra-zone/penumbra/pull/3608

We'll continue to update this tracking ticket as we make progress.

[conorsch]

Testnet liquid allocations for Penumbra within interchaintest are getting clobbered.

For context, here's a request made from the host context to a Tendermint RPC container:

❯ curl -s http://localhost:39107/genesis | jq " .result.genesis.app_state.allocations | map(.denom)"
[
  "udelegation_penumbravalid1f67tthllqwf0u0xst653wqn7y82ql6z79l6vsap4e0mpn36x05xsf33px9",
  "udelegation_penumbravalid1xmvpjz9gazr0lsu9sfgtl2e7xwlqpqdynvunv0dahe56muns2yyqs5ms3a",
  "udelegation_penumbravalid1neu6aq87a0sllc8xjgjnlkgxkf8kwaa9gh9z9pzempr5dvszf5pqpptmqt",
  "udelegation_penumbravalid1tnfjrud5522kcjwvftz3ngr9upf6ypjmv2v9gpqxp58fmk0eus9s55t235"
]

The validator delegations are expected, but we also expect some upenumbra liquid allocations, which are getting dropped somewhere between generation and chain start. Let's figure out why.

[conorsch]

Figure out how to target pd service url via an ephemeral container.

Added in the last round of @jtieri's changes, see for example:

diff --git a/chain/penumbra/penumbra_app_node.go b/chain/penumbra/penumbra_app_node.go
index 792ff46..a54d63a 100644
--- a/chain/penumbra/penumbra_app_node.go
+++ b/chain/penumbra/penumbra_app_node.go
@@ -256,31 +256,18 @@ func (p *PenumbraAppNode) GetAddress(ctx context.Context, keyName string) ([]byt
 }
 
 func (p *PenumbraAppNode) GetBalance(ctx context.Context, keyName string) (int64, error) {
-       fmt.Println("Entering GetBalance function from app perspective...")
        keyPath := filepath.Join(p.HomeDir(), "keys", keyName)
-       // TODO Figure out the container's address, so we can use pcli to look up balance
-       // as a sanity check. "localhost" is not the right network context.
-       // pdUrl := fmt.Sprintf("http://%v", p.hostGRPCPort)
-       pdUrl := fmt.Sprintf("http://localhost:8080")
+       pdUrl := fmt.Sprintf("http://%s:8080", p.HostName())

[conorsch]

Add golang function for translation Hi/Lo 64-bit values from RPC into combined 128-bit BigInt representation

Check it out:

+// translateHiAndLo takes the high and low order bytes and decodes the two uint64 values into the single int128 value
+// they represent. Since Go does not support native uint128 we make use of the big.Int type.
+// see: https://github.com/penumbra-zone/penumbra/blob/4d175986f385e00638328c64d729091d45eb042a/crates/core/crypto/src/asset/amount.rs#L220-L240
+func translateHiAndLo(hi, lo uint64) *big.Int {
+       hiBig := big.NewInt(0).SetUint64(hi)
+       loBig := big.NewInt(0).SetUint64(lo)
+
+       // Shift hi 8 bytes to the left
+       hiBig.Lsh(hiBig, 64)
+
+       // Add the lower order bytes
+       return big.NewInt(0).Add(hiBig, loBig)
+}
+

We haven't actually confirmed this works, due to https://github.com/strangelove-ventures/interchaintest/issues/658, but we're optimistic.

[conorsch]

Support passing of 128-bit Amounts within interchaintest, being respectful of interfaces used by other chains, which are all to date limited at 64-bit amounts. Maybe change to 128-bit support everywhere? Definitely need to involve @agouin in interface changes.

Resolved: the plan is to use the Cosmos SDK BigInt type, and use that throughout Interchaintest. @jtieri is working on a PR to implement, which should be ready to merge in the next few days. On top of that, we'll rebase the existing Penumbra integration work, and update to the latest protos.

[akc2267]

Here's the PR for the bigint fix: https://github.com/strangelove-ventures/interchaintest/pull/679

[conorsch]

We've encountered a blocker in that the existing buf tooling to generate golang code from Penumbra protos does not support optional proto3 fields, due to the use of protoc-gen-gocosmos, from the cosmos/gogoproto repo. Lack of optional field support causes the TransactionPlannerRequest call to fail, because the AccountGroupId can't be encoded properly. Our next steps are in order of preference:

1. get optional tag support added to the cosmos fork of gogoproto
2. generate the code from the protos without it and get that to work
3. remove the optional field and just explicitly pass nil client side

[jtieri]

The first phase of pclientd support will be tackled in PR #480, this will get us basic integration with pclientd where we can perform basic tx execution for token transfers and querying balances local to an instance of Penumbra. That PR contains some context on a blocker with regards to our current design when it comes to performing node lookup before executing any sort of query or tx. A follow up PR will be necessary to iron out the design.

tl;dr is that the interfaces used in interchaintest were built with the assumption that each Chain is fully transparent and any node in the network can be used to observe the full chain state. we need a way to lookup the proper node and/or client before executing queries or broadcasting txs.

Edit: we have figured out a workaround within the existing design where we will just scope all test user accounts to one specific PenumbraNode instance so we can assume that all test user accounts exist at PenumbraChain.PenumbraNodes[0], which lets us use the existing getFullNode method whenever we need to retrieve an instance of pclientd for broadcasting a tx or performing a query.

[akc2267]

waiting for a new release from their team

[jonathanpberger]

Got changes back; there's a LOT there. @jtieri is wrapping up the changes. Merge is gonna suuuuuck. Stay tuned. @Reecepbcups generously volunteered to review it.

[jonathanpberger]

We're blocked on a timeout issue. Applied the latest update hoping that would solve it...but no. Waiting for Penumbra team to investigate. Once that's sorted, @Reecepbcups will take a look (as mentioned upthread).

[jonathanpberger]

Iceboxing.

[Reecepbcups]

Completed!