Electrum: trezor multisig does not compile change outputs

[ulrichard]

Tested with git#227f3973a631fbfa9a7332a03a1e83c616a7f62d

Signing a 2of3 with the old method still works.

partially signing 3of3 with trezor:
Traceback (most recent call last):
File "/home/richi/sourcecode/electrum/gui/qt/util.py", line 581, in run
result = task.task()
File "/home/richi/sourcecode/electrum/lib/wallet.py", line 1076, in sign_transaction
k.sign_transaction(tx, password)
File "/home/richi/sourcecode/electrum/plugins/trezor/plugin.py", line 65, in sign_transaction
self.plugin.sign_transaction(self, tx, prev_tx, xpub_path)
File "/home/richi/sourcecode/electrum/plugins/trezor/plugin.py", line 236, in sign_transaction
signed_tx = client.sign_tx('Bitcoin', inputs, outputs)[1]
File "/home/richi/sourcecode/electrum/plugins/trezor/clientbase.py", line 224, in wrapped
return func(self, _args, *_kwargs)
File "/usr/lib/python2.7/dist-packages/trezorlib/client.py", line 697, in sign_tx
res = self.call(proto.TxAck(tx=msg))
File "/usr/lib/python2.7/dist-packages/trezorlib/client.py", line 134, in call
msg = handler(resp)
File "/home/richi/sourcecode/electrum/plugins/trezor/clientbase.py", line 35, in callback_Failure
raise RuntimeError(msg.message)
RuntimeError: Failed to compile output

[ulrichard]

The problem is also present with:
git#8c603d63965c3b21471e8d5c12e709a70df4e49a <2.7.7>
git#e01e7d8562343e874141938a6f64ee2e8cf4f794 <2.7.6>
git#c4c2203caafe01906edff10b88920d9dab91bf2e <2.7.5>
git#cefae0d76a8a610cf68aca91d49559c8f034f883 <2.7.4>
git#bb0ddcecd027be04180e301a2cdf93fe06b3da21 <2.7.3>
git#3403db9b478e9dab3f7456f78180c030ddbd2916 <2.7.2>
git#2bb2c2bceefabeff4d74a6b0cfbdb9c05e96dda3 <2.7.1>
git#7bbfe4d9c7c44a3e9e9206e4a8ccc1755b126e63 <2.7.0>
git#c7ff3ba705e4ca3acf29217ddb5e2ee4ceedea9d
git#63a5e8f99b835e96925a3d268ef44ebe6309917b
git#4d4171fe53187e41bf5aab34d6e4e9d5bb9fdf8b

I'm sure it worked with one of these revisions before.
I don't think I upgraded the trezor/keepkey python libs since the last successful test.
But it's possible that I upgraded the firmware. Although I would think I tested the procedure also after the firmware upgrade.

[ecdsa]

so, the problem exists with 3of3 and not with 2of3?
what are the 3 cosigners in each case?

[ecdsa]

this seems to be related: https://www.reddit.com/r/Electrum/comments/57389i/failed_to_compile_output_cant_send_tx_from_trezor/d8p8olb

[ecdsa]

ack, I can reproduce it. (2of3 wallet)

[ecdsa]

it seems to be working when the transaction has only one output (no change)

[ecdsa]

retrospectively, all the transactions I did with this wallet were using a single output, so I don't think it is a regression.

[kyuupichan]

Maybe only the privacy chooser has the code? There was a limit imposed on
the change address list of one for hardware wallets.

On Sat, 15 Oct 2016, 20:11 ThomasV, [email protected] wrote:

retrospectively, all the transactions I did with this wallet were using a
single output, so I don't think it is a regression.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/spesmilo/electrum/issues/1975#issuecomment-253977896,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADKliALpGo-gJI2A22Sr517mdMoW4iC5ks5q0LT6gaJpZM4KXbd4
.

[ecdsa]

@kyuupichan this is not related to the coin chooser.
it's a message sent by trezor:
https://github.com/trezor/trezor-mcu/blob/master/firmware/signing.c#L410

[kyuupichan]

I'm well aware of that. The problem is that Electrum is generating more
than one change output. I'm saying I put code in so that doesn't happen.

grep for max_change_outputs =1

Pretty sure it worked fine before....

Neil

On Sat, 15 Oct 2016 at 20:24 ThomasV [email protected] wrote:

@kyuupichan https://github.com/kyuupichan this is not related to the
coin chooser.
it's a message sent by trezor:
https://github.com/trezor/trezor-mcu/blob/master/firmware/signing.c#L410

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/spesmilo/electrum/issues/1975#issuecomment-253978449,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADKliMfkFKzOijHllWQ0AFzHrXkYyENrks5q0LfggaJpZM4KXbd4
.

[ecdsa]

no, it fails too if there's only one change output.
it even fails if the change address is the only output.

why are you sure it worked fine before?
did you ever test trezor in a multisig wallet?

[kyuupichan]

Not multisig, no. Single sig, yes. Surprised they would be different as
the change output creation is all in one place, or so I believed.

Neil.

On Sat, 15 Oct 2016 at 20:32 ThomasV [email protected] wrote:

no, it fails too if there's only one change output.
it even fails if the change address is the only output.

why are you sure it worked fine before?
did you ever test trezor in a multisig wallet?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/spesmilo/electrum/issues/1975#issuecomment-253978840,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADKliAYA3FmsAJOmXLHZku2JZ9nIDBEWks5q0Lm7gaJpZM4KXbd4
.

[ecdsa]

this bug is about multisig. it is completely different.
trezor has to verify change addresses, and it's not clear to me how it does it in that case.

[ecdsa]

@slush0 any idea?

[kyuupichan]

I see. Honestly I find this check obnoxious and I have no idea why they do
it (I know they want to discover what is being paid outside your wallet for
confirmation but that isn't a reason to limit payment options; they don't
check non-change outputs).

I asked KeepKey and they had no idea and said they would consider removing
the check. I think Trezor should consider doing the same.

Neil.

On Sat, 15 Oct 2016 at 20:37 ThomasV [email protected] wrote:

this bug is about multisig. it is completely different.
trezor has to verify change addresses, and it's not clear how it does it
in that case.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/spesmilo/electrum/issues/1975#issuecomment-253979073,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADKliGBLIRLdlI4w_dX1fPtvGLgiDtyjks5q0LrugaJpZM4KXbd4
.

[ecdsa]

we can disable the check ourselves, from the plugin. (line 322, trezor/plugin.py)
if I do that, trezor will just ask confirmation for the change output, as if it was a normal output.

[ecdsa]

@slush0 consider that commit as a workaround. please let us know if there is a clean solution.

[slush0]

The reason behind change output check is pretty obvious; user don't have a clue if change address is correct or not. If TREZOR will always display some random address in every signing, people will just confirm it blindly.

That's why we want to enforce that coins are not leaving the device, limiting the attack vector dramatically. I think disabling it (=force users to confirm unknown output) goes against security.

I don't know about any bug related to checking multisig changes, pinging @prusnak .

[slush0]

Also, by not marking change output, TREZOR will display incorrect amount on last screen (how many BTC is leaving the device). I suppose most of people will get scared if they'll see all their funds are moving out when they just wanted to pay for the beer.

[ecdsa]

@slush0 how is it supposed to work with multisig?
is there an example somewhere?

[slush0]

Oh, now I see. You generate random address for change output. That's of course incorrect. The change must go to the same xpubs as from coins are spending.

[prusnak]

is there an example somewhere?

https://github.com/trezor/python-trezor/blob/master/tests/test_multisig.py

https://github.com/trezor/python-trezor/blob/master/tests/test_multisig_change.py

Address is considered as a change address when it was derived from the XPUBs.

I suggest everyone integrating TREZOR to go through the unit tests, it is quite beneficial in order to understand how code is supposed to be used correctly. (I agree that having a proper documentation would be better, but we are understaffed in that area, unfortunately).

[prusnak]

I asked KeepKey and they had no idea and said they would consider removing
the check.

Of course, they have no idea. They just copied the code without understanding most of it. The check is there for a reason, of course.

I think Trezor should consider doing the same.

Haha!

[ecdsa]

@slush0 I don't undestand what you mean by "You generate random address for change output."
Are you referring to something Electrum does?

[ecdsa]

thanks @prusnak for the test link

[slush0]

From brief check of Electrum code I suppose you don't generate change output which belongs to the exact combination of used xpubs from multisig. That's what @prusnak pointed out.

[ecdsa]

@slush0 sorry but I don't understand. what makes you suppose that?

[slush0]

Address is considered as a change address when it was derived from the XPUBs.

Are you doing that?

[slush0]

Considering that TREZOR tests can pass thru multisig transaction with change output without problem and there's no record of Electrum doing the same successfully, I suppose it is Electrum issue:-).

[ecdsa]

Of course we are deriving change addresses from the same set of xpubs. I was asking because I was under the impression that you would know which part of Electrum code caused this issue. Which part of the Electrum code did you actually review?

[slush0]

I am not an expect in Electrum signing internals. I read just a code around the commit in this issue and I understood that like the change is generated incorrectly. Maybe I was just mistaken and I am sorry for that.

[ecdsa]

I believe the issue is in the trezor plugin. from the test page, I can see that TxOutputType() accepts a 'multisig' parameter, that is not passed in the current plugin code.

[slush0]

Thats possible. I never touched multisig code in Electrum so Im not a right person to ask about it :).

[ulrichard]

Yes, the issue I faced was only present with change. My previous tests only had one output.
With the current fix, I can sign and execute the tx I tried yesterday.
But I agree with the trezor guys. The current output on the trezor and keepkey makes it appear that the whole input is sent out. It would be much better if the change was treated as such.

[ulrichard]

Trezor works perfectly now, but the KeepKey still displays the change as if it would leave the wallet.

[slush0]

Maybe they already removed that check or they changed the logic earlier?

[ecdsa]

@ulrichard KeepKey works correcly for me; maybe you have an older firmware?

[ecdsa]

@ulrichard sorry I was wrong; both keepkey and trezor display the change if the transaction is already partially signed. they do not do that if they are the first to sign. I guess it's an issue with the plugin.