search

Blockstream / Jade

Jade hardware wallet
MIT License
290 stars 40 forks source link

Do not display change output as a payment #143

Closed pythcoiner closed 1 hour ago

pythcoiner commented 3 weeks ago

When signing a transaction, the change ouput should be at least labelled as a change (or not displayed at all), w/ the current implementation both ouputs (external & change) are displyed in the same way, so the user cannot be sure if the change go to itself or someone else.

JamieDriver commented 3 weeks ago

My understanding was that change outputs were internally validated and not shown to the user at all (for singlesig and for registered multisigs and descriptors).
There certainly appears to be code trying to handle that case. I'll try to look into it.

pythcoiner commented 3 weeks ago

My understanding was that change outputs were internally validated and not shown to the user at all (for singlesig and for registered multisigs and descriptors).

i think its better yes, in order to reduce amount of infornation to be validated by the user

JamieDriver commented 3 weeks ago

I assume you're registering a descriptor, then using 'sign_psbt' ? [ just so I can try the same thing here ... ]

pythcoiner commented 3 weeks ago

I assume you're registering a descriptor, then using 'sign_psbt' ? [ just so I can try the same thing here ... ]

yes

JamieDriver commented 3 weeks ago

Ok, I'm not seeing the change output, as I'd expect... I register a (pretty simple 2of2 multisig) descriptor, then sign a psbt which has a change output. On Jade hw I only need to confirm the spend output (and the fee). I don't see the change, which Jade has verified internally (by rebuilding the script/address from the descriptor).

Possible causes:

pythcoiner commented 3 weeks ago

i'm testing w/ the most basic descriptor liana can produce:

wsh(or_d(pk(<alice_xpub>/<0;1>/*),and_v(v:pkh(<bob_xpub>/<0;1>/*),older(65535))))#<checksum>

i can prepare some test cases if you need or reproductible steps w/ async-hwi cli

JamieDriver commented 3 weeks ago

The attached works for me - it uses the seed phrase public in the jade tests - you'll need to load that into the jade with a 'testnet' network. eg. ensure the jade is currently uninitialised/has no PIN-saved wallet, run the script and then setup jade using 'restore' and 'scan qr', and paste the words into eg. https://www.qrcode-monkey.com, and scan the qr it provides. As I say, this isn't a 'sensitive' mnemonic phrase.

jade_test_psbt_change.py.txt

I am hoping this will then work for you!

If you can adjust it to not work when you think it should, eg. with your descriptor and psbt (but still using this wallet phrase so I can run exactly the same thing!) I'll take a look asap.

btw I am using fw 1.0.30

pythcoiner commented 3 weeks ago

thx will look at this!!

pythcoiner commented 3 weeks ago

Not yet test your way but i just do this:

will dig more this tmr, maybe i'm hitting an edge case

JamieDriver commented 3 weeks ago

btw I think that's the quick acid test of whether it should work ... look at the 'cli decode' of the psbt, and see the address of the change output in the tx details (at the top) - eg. for my case above:

"vout": [ { "value": 0.00000842, "n": 0, "scriptPubKey": { "asm": "0 209de60270426474752d1553da556442e3cd8e9ba3f0b6787f341caaf3a18e2c", "hex": "0020209de60270426474752d1553da556442e3cd8e9ba3f0b6787f341caaf3a18e2c", "address": "bcrt1qyzw7vqnsgfj8gafdz4fa54tygt3umr5m50ctv7rlxsw24uap3ckqd3g4py", "type": "witness_v0_scripthash" } }, { "value": 0.00001000, "n": 1, "scriptPubKey": { "asm": "OP_HASH160 b7c56de7fde5a631cbb4fc284587c09d9efc6c4f OP_EQUAL", "hex": "a914b7c56de7fde5a631cbb4fc284587c09d9efc6c4f87", "address": "2N9zvA22xVyCxivNJ1BfY4pf1tJuxHHmBMu", "type": "scripthash" } } ] }, < The change is the vout: 0 842 sats output - bcrt1qyzw7vqnsgfj8gafdz4fa54tygt3umr5m50ctv7rlxsw24uap3ckqd3g4py.

From the outputs section (at the bottom), we can get the key paths:

"outputs": [
    {
      "witness_script": {
        "asm": "2 02c99198821d69e326df0399c7698b8cd20fa25f32cdcc799bf076ec4fbdc0810c 032332c651a545260b82b1c9f2dbd65b4c15a73ba61ada165ad468e4411ada4538 2 OP_CHECKMULTISIG",
        "hex": "522102c99198821d69e326df0399c7698b8cd20fa25f32cdcc799bf076ec4fbdc0810c21032332c651a545260b82b1c9f2dbd65b4c15a73ba61ada165ad468e4411ada453852ae",
        "type": "multisig"
      },
      "bip32_derivs": [
        {
          "pubkey": "02c99198821d69e326df0399c7698b8cd20fa25f32cdcc799bf076ec4fbdc0810c",
          "master_fingerprint": "e3ebcc79",
          "path": "m/48'/1'/0'/2'/1/8"
        },
        {
          "pubkey": "032332c651a545260b82b1c9f2dbd65b4c15a73ba61ada165ad468e4411ada4538",
          "master_fingerprint": "0a5b42f6",
          "path": "m/48'/1'/0'/2'/1/8"
        }
      ]
    },
    {
    }
  ],
  "fee": 0.00000700

We can then see whether jade+descriptor will generate that address, using that path suffix:

    addr = jade.get_receive_address('localtest', 1, 8, descriptor_name=DESC_NAME)
    assert addr == 'bcrt1qyzw7vqnsgfj8gafdz4fa54tygt3umr5m50ctv7rlxsw24uap3ckqd3g4py'

If addresses not same, then there's either something wrong with the descriptor registration, or something wrong with the psbt - in that they are not consistent.

If they are the same, but Jade still isn't auto-verifying the change, there's def something for me to look into.

JamieDriver commented 3 weeks ago

Ahh, it's a send-to-self / redeposit ? That may be confusing matters as it might be thinking it can automatically verify all outputs, leaving nothing to show the user at all - which it will refuse to do and fail-over to showing them everything.
I'm not 100% but will dig further / try to run your case above, but I seem to recall there may be some issues in this area.

[ The 'fail safe' default behaviour with Jade is that if there is any uncertainty it shows the output to the user to confirm. ]

JamieDriver commented 3 weeks ago

Yes, I've run your case (thanks v much for the phrase, policy and psbt!) and your 'not working' tx has only one output, back to the registered descriptor - eg. utxo consolidation.
In this case while Jade could verify and hide that output, it still shows the output to the user to agree - otherwise there's nothing to show at all and the user wouldn't know what they were signing. ;-) - just that the fee was 139 sats.

As I say above, in any case like this, we prefer to show the user 'everything', rather than 'nothing'.

I'm hoping it should work as expected for 'external' spend (shown) and some change (internally verified, not shown).

Cheers.

pythcoiner commented 3 weeks ago

In this case while Jade could verify and hide that output, it still shows the output to the user to agree - otherwise there's nothing to show at all and the user wouldn't know what they were signing. ;-) - just that the fee was 139 sats.

I think you can display it if you notify user it'a a send-to-self. As user can only trust the signing device screen, he does not have a way do be sure he send money to a owned address if you don't display it...

pythcoiner commented 3 weeks ago

otherwise there's nothing to show at all and the user wouldn't know what they were signing.

The way ledger do is not bad i think, they just notify you you'll spend and show you the wallet name + the fees

JamieDriver commented 3 weeks ago

Right, so hiding normal 'change' output on an external spend is fine ... but when it's a re-org/consolidation and we show the outputs to self to the user to confirm, highlight it in some way that it's clear it's the wallet's own address.

There is a 'banner' area under the output details, should be easy enough to poke a message in there.

edouardparis commented 3 weeks ago

From my experience: change output address is detected for simple miniscript wallet like a 1 to 1 after timelock but not detected when there is a multisig combined with timelock for example a 2 of 2 with the jade device and an other key that becomes a 1 of 2 after some blocks.

Ex: 

wsh(or_d(multi(2,[f714c228/48'/1'/0'/2']tpubDEwJnTwfKoMvu8AXXBPydBVWDpzNP5tatjjZ56q4TQioGL7iL9xzTbMoCCQ3tfGihtff7vtR4xsjcRuhZ7HWARVAkGZ1HZcpBhVdou76k7j/<0;1>/*,[2522f23c/48'/1'/0'/2']tpubDEoTU4bDW1EXN1rnLXnRfue1a7DeqjJcs39PkEeLcVXhVKzCnFo9yQX2EeeXJ6kh4hgbz5o9v7YAc1EE97AEJpJbKNmDxE3ZQo4msGPSp2J/<0;1>/*),and_v(v:thresh(1,pkh([f714c228/48'/1'/0'/2']tpubDEwJnTwfKoMvu8AXXBPydBVWDpzNP5tatjjZ56q4TQioGL7iL9xzTbMoCCQ3tfGihtff7vtR4xsjcRuhZ7HWARVAkGZ1HZcpBhVdou76k7j/<2;3>/*),a:pkh([2522f23c/48'/1'/0'/2']tpubDEoTU4bDW1EXN1rnLXnRfue1a7DeqjJcs39PkEeLcVXhVKzCnFo9yQX2EeeXJ6kh4hgbz5o9v7YAc1EE97AEJpJbKNmDxE3ZQo4msGPSp2J/<2;3>/*)),older(65535))))#9s8ekrce
JamieDriver commented 3 weeks ago

Jade detects 'own wallet address' by trying to re-create the script/address of the output from the info given in the descriptor registration plus the key origins/paths given in the psbt.

In the simple case it detects 'change' by a) it is a verified 'own wallet address' as above, and b) uses the internal chain .../1/n.

For registered descriptor wallets it's that (a) is satisfied ofc, and that all inputs signed were for that registered policy, and this output is to that policy. Then it should consider it a 'change' output and hide it from the user.

Going forward if an output is verified as 'own wallet' as above, but is still shown to the user, we will place a message on the output screen indicating it has been verified as being for the spending wallet.

pythcoiner commented 2 weeks ago

Here steps to reproduce what @edouardparis describing:

Comment: both output are didplayed pyth13

JamieDriver commented 2 weeks ago

Thanks, I can reproduce the issue.
It looks like it isn't recognising that the descriptor fits the input being signed, and therefore doesn't have a 'spending descriptor' with which to test the outputs to see if they're going back to the spending wallet. (Possibly due to the same xpub being used twice with different derivation paths...)
Leave it with me, I'll look into it.

JamieDriver commented 2 weeks ago

Many thanks @edouardparis and @pythcoiner - fixed locally, will be included in next fw release. Was indeed limited to the case where the number of keys is not equal to the number of xpubs registered with the descriptor - ie. when the same signer xpub is reused in the policy with different paths.

IMG_20240701_094926968

Cheers.

JamieDriver commented 12 hours ago

FYI: Jade fw '1.0.31-beta1' is available, and should contain the above fix.

pythcoiner commented 12 hours ago

nice, will test it tmr!

pythcoiner commented 1 hour ago

It looks all good on 1.0.31-beta ! thanks!