Skip to content

Commit

Permalink
Merge #351: Fix Ledger Bitcoin App 1.4.0 multple segwit input signing
Browse files Browse the repository at this point in the history 
d0de7b1 Update bitcoind patch (Andrew Chow)
55f9c7f Update speculos patch (Andrew Chow)
bd60b74 Update btchip library (Andrew Chow)

Pull request description:

  A workaround for the [multiple segwit input signing issue](LedgerHQ/app-bitcoin#154) was added to btchip-python. So we update our copy of btchip-python to include this fix.

  Also updates the speculos and bitcoind patches so that travis works.

  Note that speculos doesn't have a 1.4.0+ bitcoin app.

ACKs for top commit:
  instagibbs:
    tACK d0de7b1 tested multiple input segwit tx on ledger nano s with *non-trusted inputs*, bitcoin app 1.4.2, so at least nothing is broken for old flow.

Tree-SHA512: f0a7364b4342c1ca619e2119747a0a85062ac8cdab7bddbe78584a283cb62c21bf7c2e760a70b234a41fcc20fc776ca81b31c1ac9c03ca91ef9a89eaa71ca0a3
  • Loading branch information
@achow101
achow101 committed Jul 2, 2020
2 parents 5fc00fd + d0de7b1 commit bf7f575
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 38 deletions.
2 changes: 1 addition & 1 deletion hwilib/devices/btchip/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@
* limitations under the License.
********************************************************************************
"""

__version__ = "0.1.30"
10 changes: 6 additions & 4 deletions hwilib/devices/btchip/btchip.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def getTrustedInput(self, transaction, index):
result['value'] = response
return result

def startUntrustedTransaction(self, newTransaction, inputIndex, outputList, redeemScript, version=0x01, cashAddr=False):
def startUntrustedTransaction(self, newTransaction, inputIndex, outputList, redeemScript, version=0x01, cashAddr=False, continueSegwit=False):
# Start building a fake transaction with the passed inputs
segwit = False
if newTransaction:
Expand All @@ -186,7 +186,7 @@ def startUntrustedTransaction(self, newTransaction, inputIndex, outputList, rede
else:
p2 = 0x00
else:
p2 = 0x80
p2 = 0x10 if continueSegwit else 0x80
apdu = [ self.BTCHIP_CLA, self.BTCHIP_INS_HASH_INPUT_START, 0x00, p2 ]
params = bytearray([version, 0x00, 0x00, 0x00])
writeVarint(len(outputList), params)
Expand Down Expand Up @@ -215,8 +215,6 @@ def startUntrustedTransaction(self, newTransaction, inputIndex, outputList, rede
if currentIndex != inputIndex:
script = bytearray()
writeVarint(len(script), params)
if len(script) == 0:
params.extend(sequence)
apdu.append(len(params))
apdu.extend(params)
self.dongle.exchange(bytearray(apdu))
Expand All @@ -234,6 +232,10 @@ def startUntrustedTransaction(self, newTransaction, inputIndex, outputList, rede
apdu.extend(params)
self.dongle.exchange(bytearray(apdu))
offset += blockLength
if len(script) == 0:
apdu = [ self.BTCHIP_CLA, self.BTCHIP_INS_HASH_INPUT_START, 0x80, 0x00, len(sequence) ]
apdu.extend(sequence)
self.dongle.exchange(bytearray(apdu))
currentIndex += 1

def finalizeInput(self, outputAddress, amount, fees, changePath, rawTx=None):
Expand Down
52 changes: 26 additions & 26 deletions test/data/psbt_non_witness_utxo_segwit.patch
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
From 2789417a7ef31d4b58b2f80783e81a3518dd49af Mon Sep 17 00:00:00 2001
From eec69a3f42a654a62eacad78164e66b4ae8a68a1 Mon Sep 17 00:00:00 2001
From: Andrew Chow <[email protected]>
Date: Thu, 4 Jun 2020 23:43:25 -0400
Subject: [PATCH 1/4] rpc: show both UTXOs in decodepsbt
Expand All @@ -8,7 +8,7 @@ Subject: [PATCH 1/4] rpc: show both UTXOs in decodepsbt
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index e14217c307..45cf6be3a0 100644
index faec359d1c..5f8c02df65 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -1104,6 +1104,7 @@ UniValue decodepsbt(const JSONRPCRequest& request)
Expand Down Expand Up @@ -45,7 +45,7 @@ index e14217c307..45cf6be3a0 100644
2.27.0


From 8b7d74fd0e6d3180be0dfb315b4e190f0ad0057d Mon Sep 17 00:00:00 2001
From 6aa6d5495299a8e09f2e89a71fd5447351fbcf90 Mon Sep 17 00:00:00 2001
From: Andrew Chow <[email protected]>
Date: Thu, 4 Jun 2020 23:43:39 -0400
Subject: [PATCH 2/4] psbt: Allow both non_witness_utxo and witness_utxo
Expand All @@ -59,7 +59,7 @@ Subject: [PATCH 2/4] psbt: Allow both non_witness_utxo and witness_utxo
5 files changed, 53 deletions(-)

diff --git a/src/psbt.cpp b/src/psbt.cpp
index ef9781817a..4c8b40ca0b 100644
index 10260740f0..71a3e06708 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -35,14 +35,6 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
Expand Down Expand Up @@ -96,7 +96,7 @@ index ef9781817a..4c8b40ca0b 100644
void PSBTOutput::FillSignatureData(SignatureData& sigdata) const
{
if (!redeem_script.empty()) {
@@ -250,11 +230,6 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
@@ -261,11 +241,6 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
bool require_witness_sig = false;
CTxOut utxo;

Expand All @@ -108,7 +108,7 @@ index ef9781817a..4c8b40ca0b 100644
if (input.non_witness_utxo) {
// If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
COutPoint prevout = tx.vin[index].prevout;
@@ -345,10 +320,6 @@ TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector
@@ -356,10 +331,6 @@ TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector
return TransactionError::PSBT_MISMATCH;
}
}
Expand All @@ -120,7 +120,7 @@ index ef9781817a..4c8b40ca0b 100644
}

diff --git a/src/psbt.h b/src/psbt.h
index 888e0fd119..cbf4296bd2 100644
index 0a8ea2ea0b..401889e2fe 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -62,7 +62,6 @@ struct PSBTInput
Expand Down Expand Up @@ -179,10 +179,10 @@ index 64328fb66e..908e2b16f2 100644

for (const PSBTOutput& output : psbt.outputs) {
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 8a2a798644..9fae27975d 100644
index 3cc2611524..38d94335a3 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -595,11 +595,6 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb
@@ -597,11 +597,6 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb
continue;
}

Expand All @@ -194,7 +194,7 @@ index 8a2a798644..9fae27975d 100644
// Get the Sighash type
if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) {
return TransactionError::SIGHASH_MISMATCH;
@@ -2074,11 +2069,6 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
@@ -2086,11 +2081,6 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
continue;
}

Expand All @@ -207,10 +207,10 @@ index 8a2a798644..9fae27975d 100644
if (sign && input.sighash_type > 0 && input.sighash_type != sighash_type) {
return TransactionError::SIGHASH_MISMATCH;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 89737ca7b5..054b312cd7 100644
index 19acfa3322..974985acbf 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2490,11 +2490,6 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp
@@ -2507,11 +2507,6 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp
continue;
}

Expand All @@ -226,7 +226,7 @@ index 89737ca7b5..054b312cd7 100644
2.27.0


From a656eb32d35d2fdc1df5b8d55eba6a0c3efd1c2f Mon Sep 17 00:00:00 2001
From 889f7b3ce8ba2fccd58ab352ca622e00648147fe Mon Sep 17 00:00:00 2001
From: Andrew Chow <[email protected]>
Date: Thu, 4 Jun 2020 23:43:43 -0400
Subject: [PATCH 3/4] psbt: always put a non_witness_utxo and don't remove it
Expand All @@ -242,7 +242,7 @@ there.
5 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/psbt.cpp b/src/psbt.cpp
index 4c8b40ca0b..51f829d533 100644
index 71a3e06708..3fb743e5db 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -136,8 +136,8 @@ void PSBTInput::Merge(const PSBTInput& input)
Expand All @@ -255,7 +255,7 @@ index 4c8b40ca0b..51f829d533 100644
}

partial_sigs.insert(input.partial_sigs.begin(), input.partial_sigs.end());
@@ -263,10 +263,11 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
@@ -274,10 +274,11 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
if (require_witness_sig && !sigdata.witness) return false;
input.FromSignatureData(sigdata);

Expand All @@ -270,7 +270,7 @@ index 4c8b40ca0b..51f829d533 100644

// Fill in the missing info
diff --git a/src/psbt.h b/src/psbt.h
index cbf4296bd2..275fb03cd8 100644
index 401889e2fe..0951b76f83 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -67,12 +67,12 @@ struct PSBTInput
Expand All @@ -289,23 +289,23 @@ index cbf4296bd2..275fb03cd8 100644
SerializeToVector(s, witness_utxo);
}
diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp
index b4c65a8665..119457aadf 100644
index 3f85a48ff3..ce7e661b67 100644
--- a/src/wallet/test/psbt_wallet_tests.cpp
+++ b/src/wallet/test/psbt_wallet_tests.cpp
@@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
ssTx << psbtx;
std::string final_hex = HexStr(ssTx.begin(), ssTx.end());
std::string final_hex = HexStr(ssTx);
- BOOST_CHECK_EQUAL(final_hex, "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f00000000000100bb0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f6187650000000104475221029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f2102dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d752ae2206029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f10d90c6a4f000000800000008000000080220602dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d710d90c6a4f0000008000000080010000800001012000c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88701042200208c2353173743b595dfb4a07b72ba8e42e3797da74e87fe7d9d7497e3b2028903010547522103089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc21023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7352ae2206023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7310d90c6a4f000000800000008003000080220603089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc10d90c6a4f00000080000000800200008000220203a9a4c37f5996d3aa25dbac6b570af0650394492942460b354753ed9eeca5877110d90c6a4f000000800000008004000080002202027f6399757d2eff55a136ad02c684b1838b6556e5f1b6b34282a94b6b5005109610d90c6a4f00000080000000800500008000");
+ BOOST_CHECK_EQUAL(final_hex, "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f00000000000100bb0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f6187650000000104475221029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f2102dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d752ae2206029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f10d90c6a4f000000800000008000000080220602dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d710d90c6a4f0000008000000080010000800001008a020000000158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e8876500000001012000c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88701042200208c2353173743b595dfb4a07b72ba8e42e3797da74e87fe7d9d7497e3b2028903010547522103089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc21023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7352ae2206023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7310d90c6a4f000000800000008003000080220603089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc10d90c6a4f00000080000000800200008000220203a9a4c37f5996d3aa25dbac6b570af0650394492942460b354753ed9eeca5877110d90c6a4f000000800000008004000080002202027f6399757d2eff55a136ad02c684b1838b6556e5f1b6b34282a94b6b5005109610d90c6a4f00000080000000800500008000");

// Mutate the transaction so that one of the inputs is invalid
psbtx.tx->vin[0].prevout.n = 2;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 054b312cd7..7816a39ec1 100644
index 974985acbf..235b269805 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2491,7 +2491,7 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp
@@ -2508,7 +2508,7 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp
}

// If we have no utxo, grab it from the wallet.
Expand All @@ -315,18 +315,18 @@ index 054b312cd7..7816a39ec1 100644
const auto it = mapWallet.find(txhash);
if (it != mapWallet.end()) {
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 9b07c39606..2fe11ef116 100755
index 660953be9b..7703c4ecb1 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -37,6 +37,7 @@ class PSBTTest(BitcoinTestFramework):
@@ -38,6 +38,7 @@ class PSBTTest(BitcoinTestFramework):
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()

+ # TODO: Re-enable this test with segwit v1
def test_utxo_conversion(self):
mining_node = self.nodes[2]
offline_node = self.nodes[0]
@@ -344,7 +345,8 @@ class PSBTTest(BitcoinTestFramework):
@@ -352,7 +353,8 @@ class PSBTTest(BitcoinTestFramework):
for i, signer in enumerate(signers):
self.nodes[2].unloadwallet("wallet{}".format(i))

Expand All @@ -340,7 +340,7 @@ index 9b07c39606..2fe11ef116 100755
2.27.0


From 836d6fc375ae8709c6175d36f46df365776a497c Mon Sep 17 00:00:00 2001
From 85d143347a9ef8d1be712b7a699da0f1835dc837 Mon Sep 17 00:00:00 2001
From: Andrew Chow <[email protected]>
Date: Mon, 8 Jun 2020 19:27:16 -0400
Subject: [PATCH 4/4] tests: Check that segwit inputs in psbt have both UTXO
Expand All @@ -351,10 +351,10 @@ Subject: [PATCH 4/4] tests: Check that segwit inputs in psbt have both UTXO
1 file changed, 4 insertions(+)

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 2fe11ef116..df7d501e5b 100755
index 7703c4ecb1..e5e62fd646 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -149,6 +149,10 @@ class PSBTTest(BitcoinTestFramework):
@@ -157,6 +157,10 @@ class PSBTTest(BitcoinTestFramework):
# spend single key from node 1
rawtx = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99})['psbt']
walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(rawtx)
Expand Down
12 changes: 5 additions & 7 deletions test/data/speculos-auto-button.patch
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
From 80038049e33f6a708e1d1c2151ef48e1bce3ee70 Mon Sep 17 00:00:00 2001
From 73d6aaf4262b083bf4b8860914d9e0edcdff3ea6 Mon Sep 17 00:00:00 2001
From: Andrew Chow <[email protected]>
Date: Thu, 12 Mar 2020 17:25:09 -0400
Subject: [PATCH] Do button presses within seproxyhal
Expand All @@ -8,18 +8,16 @@ Subject: [PATCH] Do button presses within seproxyhal
1 file changed, 35 insertions(+)

diff --git a/mcu/seproxyhal.py b/mcu/seproxyhal.py
index b18f8a2..b0554ea 100644
index ffaaff5..eaf22c9 100644
--- a/mcu/seproxyhal.py
+++ b/mcu/seproxyhal.py
@@ -2,6 +2,7 @@ import binascii
@@ -1,4 +1,5 @@
import logging
import os
import select
+import struct
import sys
import time
import threading
@@ -146,6 +147,7 @@ class SeProxyHal:
@@ -141,6 +142,7 @@ class SeProxyHal:
daemon=True)
self.ticker_thread.start()
self.usb = usb.USB(self.packet_thread.queue_packet)
Expand Down Expand Up @@ -68,5 +66,5 @@ index b18f8a2..b0554ea 100644
elif tag == SephTag.SCREEN_DISPLAY_RAW_STATUS:
self.logger.debug("SephTag.SCREEN_DISPLAY_RAW_STATUS")
--
2.26.1
2.27.0

0 comments on commit bf7f575

Please sign in to comment.