penumbra-zone / penumbra

Penumbra is a fully private proof-of-stake network and decentralized exchange for the Cosmos ecosystem.
https://penumbra.zone
Apache License 2.0
373 stars 293 forks source link

Test IBC relaying between Penumbra and any Cosmos testnet #2284

Closed conorsch closed 12 months ago

conorsch commented 1 year ago

Part of #465. Let's attempt to relay between the Penumbra Pasiphae 49 (#2255) and the Babylon testnet. Relevant docs:

We'll need to implement a relayer config for the Babylon testnet, similar to the approach in #2166. Then we'll need to confirm that rly transact link <path> works without errors.

conorsch commented 1 year ago

Some notes documenting the setup. At a high level, I created a keypair, documented its seed phrase in the usual place, tapped the Babylon testnet faucet for some test tokens, and confirmed the balance is reflected in my local wallet.

# create keypair
$ babylond keys add test
# view pubkey
$ babylond keys list
- address: bbn18vpy897s8m4wm8wfmcw6llh4zmzdvxvd865tw9
  name: test
  pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"A6yEPaXIyBK+TH3fT9Ez7jZMBypkVEDUE2lsBtzqIa6Z"}'
  type: local
# retrieve address
$ babylond keys list --output json | jq '.[0].address' -r
bbn18vpy897s8m4wm8wfmcw6llh4zmzdvxvd865tw9
# query balance
$ babylond query bank balances bbn18vpy897s8m4wm8wfmcw6llh4zmzdvxvd865tw9 --node http://rpc.testnet.babylonchain.io:26657
balances:
- amount: "100"
  denom: ubbn
pagination:
  next_key: null
  total: "0"

The --node argument is required because I'm not running a local node; the value defaults to http://127.0.0.1:26657. There's no corresponding env var, so it must be specified on the CLI for now.

conorsch commented 1 year ago

The relayer code we've been working with doesn't support Babylon. Instead, Babylon has its own version of the relayer: https://github.com/babylonchain/babylon-relayer Will try to get Penumbra support working in the latter, rather than Babylon support in the former.

hdevalence commented 1 year ago

Ah, hmm, that might be a good signal to pause, then — I think we might be overrunning our headlights if we try to combine features from two different forks of the Go relayer.

conorsch commented 1 year ago

We're optimistic we can get Babylon support working as a cosmos type asset. First attempt showed an error:

2023-03-31T16:07:59.845477Z info Failed sending transaction {"chain_id": "bbn-test1", "msg-0": {"msg_json": "{\"client_state\":{\"chain_id\":\"penumbra-testnet-pasiphae\",\"trust_level\":{\"numerator\":\"1\",\"denominator\":\"3\"},\"trusting_period\":\"7200000000000\",\"unbonding_period\":\"14400000000000\",\"max_clock_drift\":\"600000000000\",\"frozen_height\":{},\"latest_height\":{\"revision_height\":\"25373\"},\"proof_specs\":[{\"leaf_spec\":{\"hash\":1,\"prehash_key\":1,\"prehash_value\":1,\"prefix\":\"Sk1UOjpMZWFmTm9kZQ==\"},\"inner_spec\":{\"child_order\":[0,1],\"child_size\":32,\"min_prefix_length\":16,\"max_prefix_length\":48,\"hash\":1},\"max_depth\":64},{\"leaf_spec\":{\"hash\":1},\"inner_spec\":{\"child_order\":[0,1],\"child_size\":32,\"hash\":1},\"max_depth\":1}],\"upgrade_path\":[\"upgrade\",\"upgradedIBCState\"],\"allow_update_after_expiry\":true,\"allow_update_after_misbehaviour\":true},\"consensus_state\":{\"timestamp\":\"2023-03-31T16:05:36.216290502Z\",\"root\":{\"hash\":\"sGrsz63a/buhAV8vyiu96Ujhbvq5+Gj3uI19pgsf6c0=\"},\"next_validators_hash\":\"7B43CDC9DCEE9EA9C7B0AA60AAB5C74E39C9C4C0F4B1F2A0111D6A4294EBBDBA\"},\"signer\":\"bb18r9q7ynx9fnv8t7hfdc5p5qun2qvqdkg4y2xxg\"}"}, "error": "rpc error: code = InvalidArgument desc = invalid Bech32 prefix; expected bbn, got bb: invalid request"}

Edited the config bb -> bbn as suggested. Next error:

2023-03-31T16:16:21.494485Z info Failed sending transaction {"chain_id": "bbn-test1", "msg-0": {"msg_json": "{\"client_state\":{\"chain_id\":\"penumbra-testnet-pasiphae\",\"trust_level\":{\"numerator\":\"1\",\"denominator\":\"3\"},\"trusting_period\":\"7200000000000\",\"unbonding_period\":\"14400000000000\",\"max_clock_drift\":\"600000000000\",\"frozen_height\":{},\"latest_height\":{\"revision_height\":\"25446\"},\"proof_specs\":[{\"leaf_spec\":{\"hash\":1,\"prehash_key\":1,\"prehash_value\":1,\"prefix\":\"Sk1UOjpMZWFmTm9kZQ==\"},\"inner_spec\":{\"child_order\":[0,1],\"child_size\":32,\"min_prefix_length\":16,\"max_prefix_length\":48,\"hash\":1},\"max_depth\":64},{\"leaf_spec\":{\"hash\":1},\"inner_spec\":{\"child_order\":[0,1],\"child_size\":32,\"hash\":1},\"max_depth\":1}],\"upgrade_path\":[\"upgrade\",\"upgradedIBCState\"],\"allow_update_after_expiry\":true,\"allow_update_after_misbehaviour\":true},\"consensus_state\":{\"timestamp\":\"2023-03-31T16:12:32.571446554Z\",\"root\":{\"hash\":\"ru52/QBr/iKT4VuBOvCOfY79phoEU8WVSuECXYURjbg=\"},\"next_validators_hash\":\"7B43CDC9DCEE9EA9C7B0AA60AAB5C74E39C9C4C0F4B1F2A0111D6A4294EBBDBA\"},\"signer\":\"bbn18r9q7ynx9fnv8t7hfdc5p5qun2qvqdkgp8ny9q\"}"}, "error": "rpc error: code = NotFound desc = rpc error: code = NotFound desc = account bbn18r9q7ynx9fnv8t7hfdc5p5qun2qvqdkgp8ny9q not found: key not found"} Error: error creating clients: failed to create client on src chain{bbn-test1}: failed to send messages on chain{bbn-test1}: rpc error: code = NotFound desc = rpc error: code = NotFound desc = account bbn18r9q7ynx9fnv8t7hfdc5p5qun2qvqdkgp8ny9q not found: key not found

Haven't debugged further why that account key is unrecognized.

conorsch commented 1 year ago

Tried setting up links between some other, non-Babylon Cosmos testnets, and those messages failed with a similar error:

2023-04-04T18:50:27.747163Z     info    Error building or broadcasting transaction      {"provider_type": "cosmos", "chain_id": "osmo-test-4", "attempt": 5, "max_attempts": 5, "error": "rpc error: code = NotFound desc = rpc error: code = NotFound desc = account osmo1ufvdg6vlmq3l2gfxlm3vnaheyc95yyqk62qcat not found: key not found"}
2023-04-04T18:50:27.747196Z     info    Failed sending transaction      {"chain_id": "osmo-test-4", "msg-0": {"msg_json": "{\"client_state\":{\"chain_id\":\"blockspacerace-0\",\"trust_level\":{\"numerator\":\"1\",\"denominator\":\"3\"},\"trusting_period\":\"1540800000000000\",\"unbonding_period\":\"1814400000000000\",\"max_clock_drift\":\"600000000000\",\"frozen_height\":{},\"latest_height\":{\"revision_height\":\"169857\"},\"proof_specs\":[{\"leaf_spec\":{\"hash\":1,\"prehash_value\":1,\"length\":1,\"prefix\":\"AA==\"},\"inner_spec\":{\"child_order\":[0,1],\"child_size\":33,\"min_prefix_length\":4,\"max_prefix_length\":12,\"hash\":1}},{\"leaf_spec\":{\"hash\":1,\"prehash_value\":1,\"length\":1,\"prefix\":\"AA==\"},\"inner_spec\":{\"child_order\":[0,1],\"child_size\":32,\"min_prefix_length\":1,\"max_prefix_length\":1,\"hash\":1}}],\"upgrade_path\":[\"upgrade\",\"upgradedIBCState\"],\"allow_update_after_expiry\":true,\"allow_update_after_misbehaviour\":true},\"consensus_state\":{\"timestamp\":\"2023-04-04T18:46:26.258971443Z\",\"root\":{\"hash\":\"IpQXLT0dulmuCr4T8KJR5yNmlhI6TYqPtdvwrYftzxA=\"},\"next_validators_hash\":\"D7E5624101F0DDADC29A76482E1A28FBAB4522D92CED1DF7DBE8B10A3E8C69D2\"},\"signer\":\"osmo1ufvdg6vlmq3l2gfxlm3vnaheyc95yyqk62qcat\"}"}, "error": "rpc error: code = NotFound desc = rpc error: code = NotFound desc = account osmo1ufvdg6vlmq3l2gfxlm3vnaheyc95yyqk62qcat not found: key not found"}
Error: error creating clients: failed to create client on dst chain{blockspacerace-0}: failed to send messages on chain{blockspacerace-0}: rpc error: code = NotFound desc = rpc error: code = NotFound desc = account celestia19p7kxpdwzpachtlqlwwt4t9a6st62m6yk9n8rm not found: key not found

Note the "key not found" error, same as above for Babylon. Because the src and dest chains were not Penumbra and not Babylon, the error indicates that we're not configuring the relayer correctly. I'm going to pause on this spike for now to pivot and get a relayer integration from penumbra-preview <-> penumbra-testnet, as described in #465. When I pick this back up, I plan to pull in someone from SL to pair on the relayer configs, because I suspect I've misconfigured something.

conorsch commented 1 year ago

Updated to make this ticket more general: it doesn't need to be Babylon, it just needs to be a Cosmos testchain that we test against.

conorsch commented 1 year ago

I'm picking up this ticket for next sprint. Goal is to test, via a manual local deployment, relaying between a Penumbra test chain and a Cosmos testchain, then report results. If results positive, let's update the relayer deployment we run—else, open tickets to address blockers.

conorsch commented 1 year ago

Trying to IBC from Osmos testnet -> Penumbra preview fails with an unsupported address error:

❯ osmosisd tx ibc-transfer transfer --chain-id penumbra-testnet-tethys-6c54d048  --memo howdy  icahost channel-1010 penumbrav2t1twsngfwyjgzxgtezu33ssg787qh2kzkzktg79qa25s9ucdd20xhqrrtvc229t7gnh207s7qyzkta46ekvrd93wttp3w7a6deemlkfhel0e27m0t8r4a3zd2gtd5994jzj0ssg4 10uosmo --dry-run
Error: string could not be parsed as address: empty address string is not allowed: invalid address

The --dry-run flag means that we're not broadcasting any transactions, so the relayer never comes into play. The logic generating the error comes from ibc-go: https://github.com/cosmos/ibc-go/blob/ee95bcff4cebd639adee744ba7b847c3380a0763/modules/core/04-channel/types/msgs.go#L58-L61

The penumbrav2 address format fails to parse via the cosmos-sdk AccAddressFromBech32 function, a problem we also encountered early with interchaintest. Over there, we opted to use the AltBech32m proto field (https://github.com/penumbra-zone/penumbra/pull/2688) to sidestep the address munging logic.

If we assume that Cosmos chains default to using the cosmos-sdk logic for address validation, then most golang cosmos ecosystem clients will fail when attempting to make IBC withdrawals to Penumbra. If that's the case, should we plan to add Penumbra address support directly to upstream ibc-go?

hdevalence commented 1 year ago

We shouldn't plan to add Penumbra address support, we need to ensure that counterparty chains can work with Penumbra addresses without changes.

hdevalence commented 1 year ago

Looking at the AccAddressFromBech32 method defined here: https://github.com/cosmos/cosmos-sdk/blob/main/types/address.go#L194 (which may not be the exact same code as running on osmosisd, but close enough to start) there's the following definition:

func AccAddressFromBech32(address string) (addr AccAddress, err error) {
    if len(strings.TrimSpace(address)) == 0 {
        return AccAddress{}, errors.New("empty address string is not allowed")
    }

    bech32PrefixAccAddr := GetConfig().GetBech32AccountAddrPrefix()

    bz, err := GetFromBech32(address, bech32PrefixAccAddr)
    if err != nil {
        return nil, err
    }

    err = VerifyAddressFormat(bz)
    if err != nil {
        return nil, err
    }

    return AccAddress(bz), nil
}

The error string empty address string is not allowed comes from the first branch, if len(strings.TrimSpace(address)) is zero. Why would this be the case for a Penumbra address?

Do we know what value is being passed in as address to that function? Is it possible to build osmosisd from source, so that we can insert debugging statements?

hdevalence commented 1 year ago

Oh, wait!

The --dry-run flag means that we're not broadcasting any transactions, so the relayer never comes into play. The logic generating the error comes from ibc-go: https://github.com/cosmos/ibc-go/blob/ee95bcff4cebd639adee744ba7b847c3380a0763/modules/core/04-channel/types/msgs.go#L58-L61

I was looking inside, at the definition of AccAddressFromBech32, but looking at the calling code, we see

    _, err := sdk.AccAddressFromBech32(msg.Signer)
    if err != nil {
        return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
    }

So what's being parsed isn't the destination address inside the packet, but the "signer" field on the relaying transaction. The only purpose of the signer field is to attribute the relaying operations. On the Penumbra chain, we allow the signer to be the empty string, and that's fine, because it's our chain and we make the rules. On Osmosis, maybe the rules are different. But it seems more likely to be a configuration issue.

avahowell commented 1 year ago

If penumbra addresses are valid bech32, they should parse correctly (from what I can tell looking at the Osmosis impl). So I think this is correct, we need to configure a signer address here. If I remember correctly we might be hardcoding that to the empty string since our impl doesn't care about this, which would cause this issue.

hdevalence commented 1 year ago

We should also double check where the error is actually coming from -- I think there's something inconsistent between

The --dry-run flag means that we're not broadcasting any transactions, so the relayer never comes into play.

and

The logic generating the error comes from ibc-go: https://github.com/cosmos/ibc-go/blob/ee95bcff4cebd639adee744ba7b847c3380a0763/modules/core/04-channel/types/msgs.go#L58-L61

since the linked code is for validation for the MsgChannelOpenInit, but that's a message sent by a relayer to coordinate an IBC channel. So I'd suspect that either (1) this isn't the actual source of the error, or (2) the relayer is coming into play here.

hdevalence commented 12 months ago

The penumbrav2 address format fails to parse via the cosmos-sdk AccAddressFromBech32 function, a problem we also encountered early with interchaintest. Over there, we opted to use the AltBech32m proto field (https://github.com/penumbra-zone/penumbra/pull/2688) to sidestep the address munging logic.

This isn't what's happening at all. To check, replace the Penumbra address with the same osmosis address, and see that the error reoccurs. The Osmosis command line tooling is just very bad at reporting errors and managing keys.

hdevalence commented 12 months ago

Current status is written up here: https://github.com/penumbra-zone/penumbra/pull/3043#issuecomment-1722554083

conorsch commented 12 months ago

https://github.com/penumbra-zone/penumbra/pull/3043 has landed, and will ship in Testnet 61. There are a number of follow-up tasks we'll track in the IBC tracking issue. Closing this issue.