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

Ledger : Remove warning on Segwit inputs and newer Bitcoin application, use ge… #6293

Merged
merged 3 commits into from
Jul 2, 2020

Conversation

btchip
Copy link
Contributor

@btchip btchip commented Jun 27, 2020

…neric signing for P2SH inputs

@btchip btchip changed the title Remove warning on Segwit inputs and newer Bitcoin application, use ge… Ledger : Remove warning on Segwit inputs and newer Bitcoin application, use ge… Jun 27, 2020
@SomberNight
Copy link
Member

SomberNight commented Jul 1, 2020

I think it's worth it to just bump the min supported library version (and enforce it, now that you provide __version__), instead of making the code here more complicated (the logic is already hard to follow in some cases in sign_transaction). I've pushed some commits accordingly.

I have tested this with a Ledger Nano S with bitcoin app version 1.3.7 and 1.4.2, and it ~works.
I have tested spending p2pkh, p2wpkh-p2sh, p2wpkh, p2sh-multisig-2of2, p2wsh-p2sh-multisig-2of2, p2wsh-multisig-2of2 input scripts.

What does not work, and I think only with bitcoin app 1.4.2, is signing a segwit tx with multiple inputs. The device produces incorrect signatures. This should be unrelated from this PR I believe, and is probably a firmware (bitcoin app) bug.
So e.g. with a standard bip84 p2wpkh wallet, creating a 2-input tx fails (as in it will have incorrect signatures):

bitcoin-cli testmempoolaccept \[\"02000000000102d3f38976650ca5a7957c2eefbdf9bcdd776f6ff17a4477eb31f249397a5e69060100000000fdffffff6c8e66ec080deb705c72a54f68905c7ed06afc3637dfd2fcb589c4fa08d089e80000000000fdffffff024c640900000000001600142cc701ab46749e55833ccb8062171266a2a1c05100e1f50500000000160014ec8f27fdb283bfb5f76937e3640c1e4631e9827c02483045022100d2a720288ace7cb2e8863558c36a8c9cb76d9abe257ee39189c5057b9ffe820a022034d433315cef12c1d2d3178abf9e45091592771d2b23e02dfe8bdead5b200a540121037852f562292e82f9c73f8db9f453472cd3b23f84f72917e6a70a5850fd16d99c02483045022100dbf281d8e24a9bb3102d32c7649c2182f16c21ba7a1f1b218ce135f726c1ac70022024df7cdc7f2a1b222b60832e1e369db0a57b5c012dd3562466544f19ce4a22f40121020c73e7a792608b8c645335b002924ed2edce7830862241e38cc9dca2e8ee9396a4141b00\"\]
[
  {
    "txid": "3125124cd02281661909dc306a88e3a18ec66c5316ebb138051dc0466e979d32",
    "allowed": false,
    "reject-reason": "64: non-mandatory-script-verify-flag (Signature must be zero for failed CHECK(MULTI)SIG operation)"
  }
]

(but a 1-input tx works)
AFAICT this has already been reported at LedgerHQ/app-bitcoin#154

@SomberNight
Copy link
Member

Besides that firmware bug, please take a look at the current state of the PR; and if you think it's ok, I will merge it.

@btchip
Copy link
Contributor Author

btchip commented Jul 2, 2020

Thanks for the updates and tests. All look fine to me, you can bump the library requirement to 0.1.30 which provides a workaround for the issue you mentioned and merge

and minor simplification
really slow to scan usb devices for e.g. every tx input...
if user disconnects mid-signing, we would fail anyway.
@SomberNight SomberNight merged commit 4aed1df into spesmilo:master Jul 2, 2020
@SomberNight
Copy link
Member

Thanks. I confirm it's fixed with 0.1.30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants