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

Send change path to Ledger so user isn't prompted for change output #3455

Closed
wants to merge 3 commits into from

Conversation

mzhou
Copy link
Contributor

@mzhou mzhou commented Dec 6, 2017

This way the user can trust based on the Ledger confirmation that change is going to their own address without trusting Electrum on their computer. Currently the change address and amount is prompted as an output.
Only implemented for segwit, since input amounts are signed, guaranteeing that the Ledger prompt represents the total non-change outputs, including fees.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 42.84% when pulling 179a038 on mzhou:ledger_trust_change into 84239e1 on spesmilo:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 42.84% when pulling 179a038 on mzhou:ledger_trust_change into 84239e1 on spesmilo:master.

@SomberNight
Copy link
Member

SomberNight commented Dec 12, 2017

Tested ACK.

Tested with a p2wpkh-p2sh wallet,

  • sending to a single output (besides change) of different script types
  • with complex transactions sending to ~5 outputs of mixed script types

Seems I was too hasty after just a few tests...
If you cancel the signing, you will get

Traceback (most recent call last):
  File "/home/user/wspace/electrum/plugins/ledger/ledger.py", line 377, in sign_transaction
    format_satoshis_plain(tx.get_fee()), changePath, bfh(rawTx))
  File "/home/user/.local/lib/python3.5/site-packages/btchip/btchip.py", line 305, in finalizeInput
    params.extend(bytearray(outputAddress))
TypeError: string argument without an encoding

So output should be bytes instead of str. Maybe it could be changed where it gets its value in line 339. Not sure about the other lines where it's used...

With that change made, cancellation will give

Traceback (most recent call last):
  File "/home/user/wspace/electrum/plugins/ledger/ledger.py", line 376, in sign_transaction
    format_satoshis_plain(tx.get_fee()), changePath, bfh(rawTx))
  File "/home/user/.local/lib/python3.5/site-packages/btchip/btchip.py", line 311, in finalizeInput
    response = self.dongle.exchange(bytearray(apdu))
  File "/home/user/.local/lib/python3.5/site-packages/btchip/btchipComm.py", line 127, in exchange
    raise BTChipException("Invalid status %04x" % sw, sw)
btchip.btchipException.BTChipException: Exception : Invalid status 6d00
Exception : Invalid status 6d00

Which is weird as it normally gives 0x6985. (Note: cancellation UX in general should be improved but this is out of scope for the PR)
This status seems to be BTCHIP_SW_INS_NOT_SUPPORTED. Not sure what that stands for.

Anyway, this exception is caught and displayed as a pop-up, and is just as descriptive as 0x6985, so might as well leave it.

@@ -371,7 +371,11 @@ def sign_transaction(self, tx, password):
if segwitTransaction:
self.get_client().startUntrustedTransaction(True, inputIndex,
chipInputs, redeemScripts[inputIndex])
outputData = self.get_client().finalizeInputFull(txOutput)
if changePath:
outputData = self.get_client().finalizeInput(output, format_satoshis_plain(outputAmount),
Copy link
Member

Choose a reason for hiding this comment

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

output is str here but btchip-python expects bytes. You can import to_bytes from electrum.util to convert it. Not sure if it's better to convert here or where its value gets set initially.

@coveralls
Copy link

coveralls commented Dec 17, 2017

Coverage Status

Coverage remained the same at 42.932% when pulling dcbf6ff on mzhou:ledger_trust_change into 3851e78 on spesmilo:master.

Copy link
Member

@SomberNight SomberNight left a comment

Choose a reason for hiding this comment

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

Looks good to me.

EDIT: see comment below

@SomberNight
Copy link
Member

SomberNight commented Jan 10, 2018

I've now had another look at finalizeInput in btchip-python, and while this PR works, I have doubts if that method should be used like this... It seems that that method might fall back to creating a tx with two outputs (one using outputAddress, the other changePath) -- though this does not seem to happen for some reason (I think the 0x6d00 status is directly related to this).

Also note that I'm testing with a Nano S, and the mentioned undesired fallback might happen on older Ledgers.

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.

3 participants