Blockstream / Jade

Jade hardware wallet
MIT License
322 stars 50 forks source link

Bug: Wrong Fee Warning #162

Open andreasgriffin opened 1 month ago

andreasgriffin commented 1 month ago

The 0 fee testnet PSBT cHNidP8BAF4BAAAAAReS33YxPXaFQsnndIkSW6rXL5qjximUqUlRo5yq67YjAQAAAAD9////ASQTAAAAAAAAIgAge7kf9Bn5AjXxXIaUPgkHt81IDcInF3nGuawbAulxGDwXUywAAAEA/QkBAgAAAAABARbYhfU+6jNGNk9CSQd2ktWkXldHCatbTNoQQOyRhE1dAAAAAAD9////AyQTAAAAAAAAFgAUdROCdhbgqv+zJ9skc4y0oNWW+oAkEwAAAAAAACIAIDF0NDYGhla3sq1SnTHsqqPPqo3/0ntmq7KZSwSfo6XPbv+uAwAAAAAWABS2GpcEAY9JtMlJ3R7n168rMNjbMAJHMEQCIGgxCUnE3l3AKO4ow5IcYaAgMBkSKm3G1uPdsSUfe4NqAiBL4VdAVj3biWNdOmBsDJImWUCLUdQzn2c9JLGvTcTWIwEhAmWUwOILiCvT0aa8yCYbiM3bv6AqS9JNNpPyS9f0x7OwdlEsAAEBKyQTAAAAAAAAIgAgMXQ0NgaGVreyrVKdMeyqo8+qjf/Se2arsplLBJ+jpc8BBWlSIQLO4pyoXCFhzco+eHYJ2t5PeC6FQEFLW5vvpabuAH2ULyEC4KJjDZmFl1dWLGTp5NwL8h1QEfOCuWxCqw1BRsRAnIYhAwcehaea8oJ14IP1qj77K1bQjIwSGixfYUCYnqmlhOCQU64iBgLO4pyoXCFhzco+eHYJ2t5PeC6FQEFLW5vvpabuAH2ULxzVtDVAMAAAgAEAAIAAAACAAgAAgAAAAAAAAAAAIgYC4KJjDZmFl1dWLGTp5NwL8h1QEfOCuWxCqw1BRsRAnIYcFMlJtDAAAIABAACAAAAAgAIAAIAAAAAAAAAAACIGAwcehaea8oJ14IP1qj77K1bQjIwSGixfYUCYnqmlhOCQHNjPdHUwAACAAQAAgAAAAIACAACAAAAAAAAAAAAAAQFpUiECCPVuZMLXHQr6+8FmS+uulT6Mt9AlDSg9X2fAC1T8HlshAiqNC+U+vJncOwrmJWrzksn6yTT0EQPW48PTwkaghPlFIQMgxxF62RTvu7DKVnak5kI+UkyFFFNrSvZLbWiywaeoUlOuIgICCPVuZMLXHQr6+8FmS+uulT6Mt9AlDSg9X2fAC1T8Hlsc2M90dTAAAIABAACAAAAAgAIAAIAAAAAAAgAAACICAiqNC+U+vJncOwrmJWrzksn6yTT0EQPW48PTwkaghPlFHBTJSbQwAACAAQAAgAAAAIACAACAAAAAAAIAAAAiAgMgxxF62RTvu7DKVnak5kI+UkyFFFNrSvZLbWiywaeoUhzVtDVAMAAAgAEAAIAAAACAAgAAgAAAAAACAAAAAA==

results in a false warning message d

The false warning message also appears for non-zero low fees.

JamieDriver commented 1 month ago

If I send the above psbt to Jade I don't see the warning (fw 1.0.31, btw).

cHNidP8BAF4BAAAAAReS33YxPXaFQsnndIkSW6rXL5qjximUqUlRo5yq67YjAQAAAAD9////ASQTAAAAAAAAIgAge7kf9Bn5AjXxXIaUPgkHt81IDcInF3nGuawbAulxGDwXUywAAAEAnAIAAAABFtiF9T7qM0Y2T0JJB3aS1aReV0cJq1tM2hBA7JGETV0AAAAAAP3///8DJBMAAAAAAAAWABR1E4J2FuCq/7Mn2yRzjLSg1Zb6gCQTAAAAAAAAIgAgMXQ0NgaGVreyrVKdMeyqo8+qjf/Se2arsplLBJ+jpc9u/64DAAAAABYAFLYalwQBj0m0yUndHufXrysw2NswdlEsAAEBKyQTAAAAAAAAIgAgMXQ0NgaGVreyrVKdMeyqo8+qjf/Se2arsplLBJ+jpc8BBWlSIQLO4pyoXCFhzco+eHYJ2t5PeC6FQEFLW5vvpabuAH2ULyEC4KJjDZmFl1dWLGTp5NwL8h1QEfOCuWxCqw1BRsRAnIYhAwcehaea8oJ14IP1qj77K1bQjIwSGixfYUCYnqmlhOCQU64iBgLO4pyoXCFhzco+eHYJ2t5PeC6FQEFLW5vvpabuAH2ULxzVtDVAMAAAgAEAAIAAAACAAgAAgAAAAAAAAAAAIgYC4KJjDZmFl1dWLGTp5NwL8h1QEfOCuWxCqw1BRsRAnIYcFMlJtDAAAIABAACAAAAAgAIAAIAAAAAAAAAAACIGAwcehaea8oJ14IP1qj77K1bQjIwSGixfYUCYnqmlhOCQHNjPdHUwAACAAQAAgAAAAIACAACAAAAAAAAAAAAAAQFpUiECCPVuZMLXHQr6+8FmS+uulT6Mt9AlDSg9X2fAC1T8HlshAiqNC+U+vJncOwrmJWrzksn6yTT0EQPW48PTwkaghPlFIQMgxxF62RTvu7DKVnak5kI+UkyFFFNrSvZLbWiywaeoUlOuIgICCPVuZMLXHQr6+8FmS+uulT6Mt9AlDSg9X2fAC1T8Hlsc2M90dTAAAIABAACAAAAAgAIAAIAAAAAAAgAAACICAiqNC+U+vJncOwrmJWrzksn6yTT0EQPW48PTwkaghPlFHBTJSbQwAACAAQAAgAAAAIACAACAAAAAAAIAAAAiAgMgxxF62RTvu7DKVnak5kI+UkyFFFNrSvZLbWiywaeoUhzVtDVAMAAAgAEAAIAAAACAAgAAgAAAAAACAAAAAA==

ofc I don't have the same wallet/seed as you, so I do get a warning about there not being any inputs for me to sign, but that is as expected.

If you can reproduce on testnet, it'd be ideal if you could do so in a temporary wallet where you can share the seed phrase.

(Note the definition of 'send amount' is 'outputs that are not change' - but from looking at the psbt the outputs are not to change bip32 paths, so that shouldn't be relevant [more fyi]).

andreasgriffin commented 1 month ago

ok. on testnet, funded with a few sats

The following descriptor:

wsh(sortedmulti(2,[45a9be94/48'/1'/0'/2']tpubDEpkV8PcqfxmyRTZHGxWgUJBfSuQvi47otVB7rjvFUJap6DF8QEy7q6uo5XDtL7rUemJvDeB1shGcAVmmZ9iMeNrziqx8nTyKjokPcoj272/<0;1>/*,[30b8300e/48'/1'/0'/2']tpubDEqwba9CCfjDF8iEtx6xV1FzgGW6qUWQx1PdXswoNrY4AeCbmkWfdxZYsDqNP6VU4LSBD6wxfYXDmJ7UK98W5UreEXNz1H7DzktPWVYmQww/<0;1>/*,[394c395b/48'/1'/0'/2']tpubDF9eJKXpDXKCSEg9hDgiQ4HhiBXbpc2k5VVeLpKR2UP61L4Km5xtsPRZZ46JRegGkxpmvDmPTMVSjRhxpxHiJFj2mRcBsLpHqXuVqoSzdpL/<0;1>/*))#pcjqddc7

the following jade seed:

garment donkey split oppose rent pet office april grow again hawk evidence

the psbt:

cHNidP8BAF4BAAAAAaDVmaz5RvhybHusbcwtj7BUMfIvOhuvAdBs0z/YaLj5AAAAAAD9////AWoPAAAAAAAAIgAgSrr1L48byGG8+dtj7aU5S+7XjiSbKs6SM4yTqFYlL3KSpSwATwEENYfPBHzUlz6AAAACQWUorVFrxMIsO+IwGReke63wK1oOQUalJ2RQ5xBxgaoCKTc2l6cfghjEFjl0Sv/MAWoOvQ1EGzmRK9zi9/uoaUIURam+lDAAAIABAACAAAAAgAIAAIBPAQQ1h88Ef6Aha4AAAAI5C4dia+KR+JVM0lNxz8OYkoUiPo+x4gUH6WIN6P42dQNp0fljymO5TRZdbwS/O1cc0q4FEnkFwdXwvg+RFJBDQRQwuDAOMAAAgAEAAIAAAACAAgAAgE8BBDWHzwShCT+FgAAAAqrgps8heyaTCzT7P/2sdOxa6e9zz+cx7kEahtFDvsrHAmVfDy3tpHqeaFib3eAnOgC5pA8qsoQYeG7da7NTlA0CFLc9iccwAACAAQAAgAAAAIACAACATwEENYfPBKkmEJqAAAAC+tZjgJ95Agd8s3rnc5wTdKCbbemRo2MGk2cLr+d2QqQDPd7eC9yuk+6+sFpYrWx6l1QM5xt5MXj0AgZf2rXhFOUUOUw5WzAAAIABAACAAAAAgAIAAIBPAQQ1h88E8fAhO4AAAALWK+1nSLbMBn+ty4ArGBmqXkgGAQ7XgWcRmGw+Cao4DQL3WvfeQ5an/If7nSgXQWA8Yq1maXVRZFThyTpn7ujhThT1WR7sMAAAgAEAAIAAAACAAgAAgE8BBDWHzwT+5qJ5gAAAAuGbAETMrOE7JvEF41iLtSZeuAkn3uDMJw0iopoXUMDDAggEdt17QSy8Rwc24IgY2nCBqxMo7KDpSFd84jLphAd9FNW0NUAwAACAAQAAgAAAAIACAACAAAEA/VwBAQAAAAABATNFpkr5LG00rn3EpngL6z7TadKcfjdRKpTkPXMKggA9AAAAAAD9////AWoPAAAAAAAAIgAgOyvNy6yTqgNDws4XHhM2Wf0TAjWjWq3uG1qjsuBLJusEAEcwRAIgBVFo3mYZsuAvs7xyNquyZHRC4kGWKbQpFtcLhoZoRx8CIAfwE8E7FGcz+DxU1dGGqzM23y3kZnEtEuzAWvwj3gcMAUcwRAIgLk44GNFvEuFQ56lQOksrViPknAgHDE6srLrlNqGYKxICIAa97qYClAw/G73QiyGg1myGBw0P4QqTyjAcbl/njYwyAWlSIQMNqe9Xx5q/BmV1osH8rsFbnpB+7JiIpDpUK1TrkVdw7iEDf1iIv+q18JyQfWSF18ruSwA6VmSyyCyZFCgwS7E5p8YhA/4hqlStD4GVlP/vJ3YvcKMyQtby/dXDa8UjI+GdVSXwU66SpSwAAQErag8AAAAAAAAiACA7K83LrJOqA0PCzhceEzZZ/RMCNaNare4bWqOy4Esm6wEFaVIhAy+J3qzzX4GV+TA1ELko/uQM1u0fpTIbd1Yumq97RQiwIQOKsDN5KAJew8IZMQdO0NBScG5CFlpfR7WQPjl93iME5CEDwh0sFDFqw1KGG8jHbslgykTtVrhvnxX2z2C4+BNq34JTriIGAy+J3qzzX4GV+TA1ELko/uQM1u0fpTIbd1Yumq97RQiwHEWpvpQwAACAAQAAgAAAAIACAACAAAAAAAAAAAAiBgOKsDN5KAJew8IZMQdO0NBScG5CFlpfR7WQPjl93iME5Bw5TDlbMAAAgAEAAIAAAACAAgAAgAAAAAAAAAAAIgYDwh0sFDFqw1KGG8jHbslgykTtVrhvnxX2z2C4+BNq34IcMLgwDjAAAIABAACAAAAAgAIAAIAAAAAAAAAAAAABAWlSIQJwWM5A5pRJhBeOVvrf4mGHUTdNEsFWuIYNcDXSABFMASEDluQKIse7rn5bNe2OXl48EWwHQRkgGugHl/zckrWLXw8hA8JztYLtsL1OpGPln0mf3U+Esq1MBhIS+YE0zrCed6iJU64iAgJwWM5A5pRJhBeOVvrf4mGHUTdNEsFWuIYNcDXSABFMARxFqb6UMAAAgAEAAIAAAACAAgAAgAAAAAABAAAAIgIDluQKIse7rn5bNe2OXl48EWwHQRkgGugHl/zckrWLXw8cOUw5WzAAAIABAACAAAAAgAIAAIAAAAAAAQAAACICA8JztYLtsL1OpGPln0mf3U+Esq1MBhIS+YE0zrCed6iJHDC4MA4wAACAAQAAgAAAAIACAACAAAAAAAEAAAAA

gives me the described warning

JamieDriver commented 1 month ago

Did you register that descriptor on Jade (if so, how ... using some companion app?)

JamieDriver commented 1 month ago

ok, I used the attached script:

psbt_fee_test.py.txt

When signing I see:

I think this is what I'd expect to see.

What fw version are you using ?

JamieDriver commented 1 month ago

NOTE: if the descriptor wallet is not registered on Jade, ie. 'Step 3.' is:

    # 3. Register descriptor wallet
    #rslt = jade.register_descriptor(NETWORK, DESC_NAME, DESC_SCRIPT, DESC_KEYS)
    #assert rslt

    # Check loaded descriptor
    registered_descriptors = jade.get_registered_descriptors()
    desc = registered_descriptors.get(DESC_NAME)
    assert not desc
    #assert desc
    #assert desc['descriptor_len'] == len(DESC_SCRIPT)
    #assert desc['num_datavalues'] == len(DESC_KEYS)

    #desc = jade.get_registered_descriptor(DESC_NAME)
    #assert desc
    #assert desc['descriptor'] == DESC_SCRIPT
    #assert desc['datavalues'] == DESC_KEYS

(and I use the 'Options->Wallet' menu to ensure that registration is deleted...)

Then I no longer see the "Verified wallet output" message (as Jade does not have sufficient information about the cosigners to be able to assert this) but I still do not see a 'high fees' warning.

andreasgriffin commented 1 month ago

What fw version are you using ?

1.0.29

I will update to 1.0.31 and see if the bug is there still.

andreasgriffin commented 1 month ago

In 1.0.31 the error message does not appear

andreasgriffin commented 1 month ago

However for another PSBT the warning is still there (also with 0 fee):

PSBT: cHNidP8BAFIBAAAAAZsCKtC6UTSN/+4e3jnWiYyv0C7WYNGsOSdnNBWMU7y4AAAAAAD9////AdAHAAAAAAAAFgAU43t9/p+a2e2F8hB9+o5qYf5E6B255S0ATwEENYfPA2VmPE2AAAAAVtGT3N/zqb7CxcYIvWym7G7fFKsHqxld11mIdb31+KsD9OsPX3Due4Kn7npJ9Mk00OLW6t+2IiBvGL9jQls65iUQMLgwDlQAAIABAACAAAAAgAABAP1yAQEAAAAAAQJjlQsOh7WHbaNwmiWWwiPdE2E6jZ5qTpUcxhI5H0sjCQAAAAAA/f///zfYJ3KeEmhHnlWjLMQuKF2Jp5MZ3x8xNTx9mj2xWdQwAQAAAAD9////AtAHAAAAAAAAFgAU+7C6uZ/CKy5T0B8t4uj4P7Hxv0qgDwAAAAAAABYAFDTCvNGvZ2dAKDg6OGiQnxg5nsCsAkcwRAIgDYy81/lDTm5jI/1hazC9BOuI9mdTiwOK3EO1YmszbToCIB3xSW4H9JaOz2xNFPid+mtTFKpHzy7TeAJO/HAF1ziWASECYcxwU8waQTtyRR+TuQ5NJ+EaYJ30iAoff+ZqwjMO27oCRzBEAiB4/sBbRNnqGn9r21kTKSRjCDyIpxZd+8e0rotOf3G41wIgL5PMb5LIW7YGXaE36Y0EimnfqH2WFpASvdTGXBC1zEsBIQL+q48kJi5W+bQNMIUrJbrXpnh847CjGJ5jVyuaINUTybflLQABAR/QBwAAAAAAABYAFPuwurmfwisuU9AfLeLo+D+x8b9KIgYDFr55KGENkOM4dmVR+33t/oVX+lT9zMxbnyJUy6a2bScYMLgwDlQAAIABAACAAAAAgAAAAAAAAAAAACICAn0T361AbL8Cckns3wSUkg8GN7Yd0/wZWYyZ4R35E5rUGDC4MA5UAACAAQAAgAAAAIAAAAAAFAAAAAA=

using the same seed as above.

image

JamieDriver commented 1 month ago

If I substitute the above psbt into the script given above, I still do not see a warning, so I don't know how you are triggering it. Somehow it must think you're not spending anything...

The comparison that triggers that warning is essentially: fees >= spend_amount.

I could change to ( for example ): fees > 0 && fees > spend_amount, which should make it slightly less sensitive, and mean it never pops up when fees are zero (which seems sensible).

JamieDriver commented 1 month ago

My suspicion is that jade is not being passed this psbt, but rather it is being passed to something like HWI/hwilib (or similar), which is translating it into Jade's legacy format (there is an open PR on HWI to pass psbt through as is). In any case I think that might be marking all outputs back to the spending wallet as 'change', and therefore Jade would think the 'spend' is zero. This case would be addressed by the 'fees > 0' check above.

andreasgriffin commented 1 month ago

Yes, I use HWI (see https://github.com/andreasgriffin/bitcoin-usb/ ) . I was unaware there is a Jade legacy format. fees > 0 will not be sufficient, because the warning also happens for low fee transactions.

JamieDriver commented 1 month ago

There's nothing I can do about that until HWI is fixed. When you say 'transactions', I assume you mean 'transactions where I send coins only to myself' ?

Where you send funds back to your own wallet they can be categorised as 'spend' or 'change' (coins to external addresses are only ever 'spend'). HWI is telling Jade that all the funds to your own wallet are "change" and therefore there is no "spend" amount. In this case any fee will be > 'spend' (since 'spend' is 0), so the warning will be shown. Either a) HWI should have a better idea of when coins to self are 'spend' and when they are 'change', or b) HWI can just use the PSBT format - in which case it doesn't have to tell Jade which outputs are spend and which are change, as Jade deduces that for itself based on the bip32 path. (The fix for 'a' would be easy enough, but the fix for 'b' is already open and is a 'better' approach anyway ...)

fees > 0 is still a reasonable fix to make - there's no point saying 'the fees look a bit high!' when in fact it's a zero fee! ;-)