Ledger signing regressions

[zmanian]

Summary of Bug

Version

0.40.0-rc4 in gaia cosmoshub-test-stargate-e

Steps to Reproduce

Sending transactions from keys in the keyring on ledgers fails

❯ /Users/zakimanian/cosmos/gaia/build/gaiad --home /Users/zakimanian/cosmoshub-test-stargate --keyring-backend file tx bank send cosmos1lcsjy2d5s33h0sddd8lpuqvwyz5ruz7jsfj43h cosmos1lcsjy2d5s33h0sddd8lpuqvwyz5ruz7jsfj43h 100000uatom --chain-id cosmoshub-test-stargate-e
Enter keyring passphrase:
{"body":{"messages":[{"@type":"/cosmos.bank.v1beta1.MsgSend","from_address":"cosmos1lcsjy2d5s33h0sddd8lpuqvwyz5ruz7jsfj43h","to_address":"cosmos1lcsjy2d5s33h0sddd8lpuqvwyz5ruz7jsfj43h","amount":[{"denom":"uatom","amount":"100000"}]}],"memo":"","timeout_height":"0","extension_options":[],"non_critical_extension_options":[]},"auth_info":{"signer_infos":[],"fee":{"amount":[],"gas_limit":"200000","payer":"","granter":""}},"signatures":[]}

confirm transaction before signing and broadcasting [y/N]: y
Error: Unexpected characters
Usage:
  gaiad tx bank send [from_key_or_address] [to_address] [amount] [flags]

This appears to be because the system doesn't have a mechanism to switch to SIGN_MODE_AMINO when a keyring key is ledger based.

This flow works

/Users/zakimanian/cosmos/gaia/build/gaiad --home /Users/zakimanian/cosmoshub-test-stargate --keyring-backend file tx bank send cosmos1lcsjy2d5s33h0sddd8lpuqvwyz5ruz7jsfj43h cosmos1lcsjy2d5s33h0sddd8lpuqvwyz5ruz7jsfj43h 100000uatom --chain-id cosmoshub-test-stargate-e --generate-only > testtx.json

/Users/zakimanian/cosmos/gaia/build/gaiad --home /Users/zakimanian/cosmoshub-test-stargate --keyring-backend file tx sign testtx.json --from cosmos1lcsjy2d5s33h0sddd8lpuqvwyz5ruz7jsfj43h --chain-id cosmoshub-test-stargate-e

Also:
--output-document doesn't appear to work anymore. The signature is still sent to STDOUT even when this option is passed.

So two issues:

• Sending transactions from the command line fail
• fix: Signature only flag bug on tx sign command 7632 #8106 output-document doesn't work from the tx sign subcommand.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[jleni]

I just saw the link in Telegram :)
The cosmos app does not require amino, however, it is possible that some fields or tx structure is not expected. I am tempted to say (without testing) that we are not expecting @ and rejecting because of that...

[aaronc]

The ledger app does require amino JSON. We have not implemented SIGN_MODE_TEXTUAL yet for protobuf so the alternative is proto binary which won't work with the ledger.

[jleni]

yes, the app uses canonical JSON and has a few restrictions on the structure.. (described here https://github.com/cosmos/ledger-cosmos/blob/master/docs/TXSPEC.md#format). However, there is no direct connection to amino in the formar .. (maybe the field names?) although I understand the naming as this was done at the time amino was used.

proto binary would not work because the original design request was that the app had to be generic. So the tx needs to be self-documenting.. if we used protobuf, then we would need to restrict more the structure of the data. This would also affect other cosmos sdk based projects.

Due to memory restrictions in Nano S, it is not good to use code generation either.. even nanopb is a problem in Ledger devices.

Let me know if you need more info diagnosing why the error message. The easiest is to add this failing tx as a unit test. We can definitely adjust the app if the new format is well defined.

[tomtau]

This appears to be because the system doesn't have a mechanism to switch to SIGN_MODE_AMINO when a keyring key is ledger based.

https://github.com/cosmos/cosmos-sdk/blob/master/client/flags/flags.go#L62
--ledger --sign-mode=amino-json doesn't work?

[amaury1093]

Can anyone explain why Zaki's 2nd flow works? afaict testtx.json still holds a proto-encoded JSON, so shouldn't be signable by ledger either.

[zmanian]

@tomtau adding --sign-mode=amino-json works!

[robert-zaremba]

Seams like most of this issue is duplicated from #8106. I fixed tx sign testtx.json and --output-document.
Also, note that we change the flags and making it more clear. See this comment.

[robert-zaremba]

Amaury wrote a tests and look at the output-document independently here as well.

[amaury1093]

Zaki pointed out two bugs:

1. the --output-document bug, fixed by fix: Signature only flag bug on tx sign command 7632 #8106

2. The ledger signing error. @robert-zaremba, it that fixed by fix: Signature only flag bug on tx sign command 7632 #8106? If not, we could:

   • check the signer address, if it's a ledger key in the keyring, automatically add the --sign-mode=amino-json flag (see Ledger signing regressions #8109 (comment))
   • check the signer address, if it's a ledger key in the keyring, show a user-friendly error message to add --sign-mode=amino-json
   • something else that doesn't involve checking the signer in the keyring.

[robert-zaremba]

@amaurymartiny , I didn't inspect the ledger signing.
I think it's not possible to check a signer without any prior knowledge (keyring or external flag), right?

How about this:

1. Adding a flag to let user decide about the signer type
2. If user doesn't specify it, we check in keybase
3. if the signer is a ledger, then we check if --sign-mode=amino-json, if not, we print a warning and overwrite it (so something between first and second bullet point Amaury wrote above).

[clevinson]

• check the signer address, if it's a ledger key in the keyring, automatically add the --sign-mode=amino-json flag (see #8109 (comment))

@amaurymartiny I prefer this if we can make it happen.

[robert-zaremba]

@clevinson - don't you think we should print a warning if we change user setting?

[clevinson]

For ledger, it's not possible to sign with sign mode direct, it would only result in an error anyway. Maybe we provide a description in the command?

I would think that if we are adding amino-json flag ourselves that it overrides any settings from the user.

Also- we already have a flag to let the user decide the sign mode. This is just about us ignoring that flag and always using sign-mode=amino-json if the key being used comes from a ledger device (this is the only option that works anyway).

[amaury1093]

fix is here: #8136