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

[Bug]: Legacy amino-json ledger signing fails for ibc MsgTransfer (protos outside of sdk) #17975

Closed
1 task done
damiannolan opened this issue Oct 5, 2023 · 7 comments · Fixed by #17996
Closed
1 task done
Assignees
Labels
C:Ledger Issues and features related Ledger integration and functionality C: Proto Proto definition and proto release T:Bug Type: QA Quality Assurance

Comments

@damiannolan
Copy link
Member

damiannolan commented Oct 5, 2023

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

A bug happened!

Ledger tx signing using sign-mode=amino-json is failing for (seemingly) protos defined outside of the SDK. For example, and most notably ibc MsgTransfer.

Attempting to sign using a connected ledger with flags --ledger --sign-mode amino-json fails with:

 ERR failure when running app err="proto:\u00a0not found"

The NotFound error is being propagated all the way from protoregistry.

In the SDK, it surfaces here: https://github.com/cosmos/cosmos-sdk/blob/x/tx/v0.10.0/x/tx/decode/unknown.go#L113-L117
As the SignModeHandler created here(https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-rc.1/client/tx/aux_builder.go#L208) cannot find the proto descriptors for ibc messages.

I've tried editing the HybridResolver to protoregistry.GlobalFiles which yields the same result - which is expected considering HybridResolver godoc says its a merged registry of gogo and global.

The base of the problem is with proto generated code and how file/msg descriptors are populated within the global proto type registry. The resolver for file/msg descriptors is failing within the signing context and cannot use the protoreflect API to generate signing bytes to provide to the ledger.


I should also note I tried signing using sign-mode=textual and got:

 ERR failure when running app err="protoFiles does not have descriptor /ibc.applications.transfer.v1.MsgTransfer: proto:\u00a0not found"

Cosmos SDK Version

v0.50.0-rc.1

Trace of the signing code is:
https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-rc.1/client/tx/tx.go#L321
https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-rc.1/x/auth/signing/adapter.go#L60
https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-rc.1/x/tx/signing/handler_map.go#L53-L59
https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-rc.1/x/tx/signing/aminojson/aminojson.go#L61

How to reproduce?

Full steps to reproduce using a local test environment can be found here: https://github.com/damiannolan/simd-scripts

@kocubinski kocubinski self-assigned this Oct 5, 2023
@damiannolan damiannolan added C:Ledger Issues and features related Ledger integration and functionality Type: QA Quality Assurance C: Proto Proto definition and proto release R:Eden labels Oct 6, 2023
@kocubinski
Copy link
Member

kocubinski commented Oct 6, 2023

I am trying to reproduce this but simd is failing with

❯ simd keys add ledgerKey --ledger --home ~/src/simd-scripts/data/test-1
failed to retrieve device: ledger nano S: hidapi: failed to open device

The device is freshly reset ledger nano s.

@kocubinski
Copy link
Member

Can you share the precise simd command you are trying to run which is failing? This way I try to hack around using a ledger while still maintaining the same environment.

@kocubinski
Copy link
Member

^^ This PR fixes the issue from my machine. Can you check on your end @damiannolan? I was not able to set up a ledger, but was able to reproduce with the following command (which now produces an account sequence error):

❯ simd tx ibc-transfer transfer transfer channel-0 cosmos1mjk79fjjgpplak5wq838w0yd982gzkyfrk07am 100stake --from wallet1 --node tcp://localhost:16657 --home ./data/test-1 --keyring-backend test --sign-mode amino-json
auth_info:
  fee:
    amount: []
    gas_limit: "200000"
    granter: ""
    payer: ""
  signer_infos: []
  tip: null
body:
  extension_options: []
  memo: ""
  messages:
  - '@type': /ibc.applications.transfer.v1.MsgTransfer
    memo: ""
    receiver: cosmos1mjk79fjjgpplak5wq838w0yd982gzkyfrk07am
    sender: cosmos1m9l358xunhhwds0568za49mzhvuxx9uxre5tud
    source_channel: channel-0
    source_port: transfer
    timeout_height:
      revision_height: "1825"
      revision_number: "2"
    timeout_timestamp: "1696620125587177000"
    token:
      amount: "100"
      denom: stake
  non_critical_extension_options: []
  timeout_height: "0"
signatures: []
confirm transaction before signing and broadcasting [y/N]: y
code: 4
codespace: sdk
data: ""
events: []
gas_used: "0"
gas_wanted: "0"
height: "0"
info: ""
logs: []
raw_log: 'signature verification failed; please verify account number (1), sequence
  (4) and chain-id (test-1): unauthorized'
timestamp: ""
tx: null
txhash: 2C656921262CCBED784E77CB2FCFA39F7F41102279A821844F2482C324A8F21A

@damiannolan
Copy link
Member Author

Hey @kocubinski thanks a lot for looking into this! I replied on the PR. The fix is working with amino-json signing but seeing a new error with textual.

@tac0turtle
Copy link
Member

Hey @kocubinski thanks a lot for looking into this! I replied on the PR. The fix is working with amino-json signing but seeing a new error with textual.

this is a ledger issue we tagged zondax on it

@felipe-op
Copy link

@kocubinski I found this thread because I'm having the same issue trying to communicate with a Ledger

hidapi: failed to open device

Were you able to get past this issue?

@julienrbrt
Copy link
Member

@kocubinski I found this thread because I'm having the same issue trying to communicate with a Ledger


hidapi: failed to open device

Were you able to get past this issue?

I think this one fits best: #18423 (comment)
Basically update ledger cosmos go library to latest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Ledger Issues and features related Ledger integration and functionality C: Proto Proto definition and proto release T:Bug Type: QA Quality Assurance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants