bitcoin-core / HWI

Bitcoin Hardware Wallet Interface
MIT License
496 stars 198 forks source link

Incorrect script type on SegWit input with non_segwit_utxo #338

Closed matejcik closed 4 years ago

matejcik commented 4 years ago

The following PSBT fails to sign correctly on Trezor: (Testnet, seed "all all all...")

cHNidP8BAHECAAAAAecEAgWR8szJlB6pdc1WWqPRPz7QtRaBvrL8N+uBEwYJAQAAAAD9////AlDDAAAAAAAAFgAUoba87EO4px+ZskFuuEgHIKIyLfvDwgAAAAAAABYAFBzJhv75wfAhWc0Vbs53Trx3gtM2AAAAAAABAIgCAAAAAUN0ZYfD6gHGgpUpLr4s2HZ/THP5NZQxl/ZEoutJhmNPAAAAABcWABTHzP6hfUod5XE9TOw2dG2zX+oKJ/7///8CgKkQAAAAAAAWABSN0irV2k8nVHXyPE7hG+ZhfoOH4aCGAQAAAAAAFgAUMubjebQZ7UyhjcnN4t1BBosX/E34phoAIgYCwdb62tbxWXu7isZaztYffSFQCzUPXEJQkLzJXtwIKsIYXJ4ijVQAAIABAACAAwAAgAAAAAADAAAAAAAiAgJXlDJA50X02ihEXTHJgjWpI9BcMt8z3XsLBkeJGcNtrRhcniKNVAAAgAEAAIADAACAAQAAAAQAAAAA

It is spending a SegWit output, but the PSBT provides non_segwit_utxo field with the full transaction (this is allowed by the spec).

HWI uses this to determine that the script-type should be SPENDADDRESS (per https://github.com/bitcoin-core/HWI/blob/master/hwilib/devices/trezor.py#L166-L168). It produces an incorrect signature and bitcoind won't finalize the transaction.

Manually changing SPENDADDRESS to SPENDWITNESS produces a correct signature that is accepted by network (https://tbtc1.trezor.io/tx/5d05e39c686e73e61490f196f3a6d6aa15beddffa5b5c1fc07f74f8c4d12c312)

achow101 commented 4 years ago

The BIP only allows for non-segwit utxos to be provided when it is not known whether the input is segwit. Since it is known that the utxo is segwit, a non-segwit utxo shouldn't be provided.

HWI follows the simple signer algorithm which doesn't do script checking and just assumes that the incoming psbt has set the fields correctly.

matejcik commented 4 years ago

The BIP only allows for non-segwit utxos to be provided when it is not known whether the input is segwit.

FWIW the BIP says "This should only be present for inputs which spend non-segwit outputs" which traditionally means a recommendation. I never understood the part about "not knowing" whether the input is or isn't segwit -- but ISTM i could read it as the Creator / Updater not understanding the distinction between SegWit and nonSegWit, and adding the full UTXOs anyway?

HWI follows the simple signer algorithm which doesn't do script checking and just assumes that the incoming psbt has set the fields correctly.

This is of course reasonable, the above nonwithstanding. But I'm wondering, if the Simple Signer already has access to IsP2WSH() and friends, is it really simpler to use the presence/absence of fields?

something like:

if non_witness_utxo:
    scriptPubkey = non_witnes_utxo.vout.scriptPubkey
elif witness_utxo:
    scriptPubkey = witness_utxo.scriptPubkey
else:
    error

if redeemScript:
    assert P2SH(redeemScript) == scriptPubkey
    script = redeemScript
else:
    script = scriptPubkey

if is_P2WPKH(script):
    sign_witness(P2PKH(script))
elif is_P2WSH(script):
    assert witnessScript
    assert script == P2WSH(witnessScript)
    sign_witness(witnessScript)
else:
    sign_non_witness(script)
lontivero commented 4 years ago

In Windows OS processes' argument maximum length 32,698 is not long enough to pass big PSBTs. Can HWI read from stdin or any other file?

achow101 commented 4 years ago

In Windows OS processes' argument maximum length 32,698 is not long enough to pass big PSBTs. Can HWI read from stdin or any other file?

./hwi.py --stdin takes each argument on a newline via stdin.

instagibbs commented 4 years ago

Check the help: --stdin should work. There's also an example on reading in a binary file too I'm the repo.

On Tue, Jun 9, 2020, 12:03 PM Lucas Ontivero notifications@github.com wrote:

In Windows OS processes' argument maximum length 32,698 is not long enough to pass big PSBTs. Can HWI read from stdin or any other file?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin-core/HWI/issues/338#issuecomment-641402045, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU2VIXKOYJ3RQTHCUCLRVZMONANCNFSM4NN3RGYA .