highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-03 01:33:20 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge #14588: Refactor PSBT signing logic to enforce invariant and fix signing bug

Browse Source 
e13fea975d Add regression test for PSBT signing bug #14473 (Glenn Willen)
565500508a Refactor PSBTInput signing to enforce invariant (Glenn Willen)
0f5bda2bd9 Simplify arguments to SignPSBTInput (Glenn Willen)
53e6fffb8f Add bool PSBTInputSigned (Glenn Willen)
65166d4cf8 New PartiallySignedTransaction constructor from CTransction (Glenn Willen)
4f3f5cb4b1 Remove redundant txConst parameter to FillPSBT (Glenn Willen)
fe5d22bc67 More concise conversion of CDataStream to string (Glenn Willen)

Pull request description:

  As discussed in the comments on #14473, I think that bug was caused primarily by failure to adhere to the invariant that a PSBTInput always has exactly one of the two utxo fields present -- an invariant that is already enforced by PSBTInput::IsSane, but which we were temporarily suspending during signing.

  This refactor repairs the invariant, also fixing the bug. It also simplifies some other code, and removes redundant parameters from some related functions.

  fixes #14473

Tree-SHA512: cbad3428175e30f9b7bac3f600668dd1a8f9acde16b915d27a940a2fa6d5149d4fbe236d5808fd590fb20a032274c99e8cac34bef17f79a53fdf69a5948c0fd0
This commit is contained in:
Pieter Wuille
2018-11-09 19:26:46 -08:00
parent cbf00939b5 e13fea975d
commit b30c62d4b9
7 changed files with 80 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
src/wallet/rpcwallet.cpp
View File

@@ -3772,24 +3772,34 @@ void AddKeypathToMap(const CWallet* pwallet, const CKeyID& keyID, std::map<CPubK
hd_keypaths.emplace(vchPubKey, std::move(info));
}
bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type, bool sign, bool bip32derivs)
bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs)
{
LOCK(pwallet->cs_wallet);
// Get all of the previous transactions
bool complete = true;
for (unsigned int i = 0; i < txConst->vin.size(); ++i) {
const CTxIn& txin = txConst->vin[i];
for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
const CTxIn& txin = psbtx.tx->vin[i];
PSBTInput& input = psbtx.inputs.at(i);
// If we don't know about this input, skip it and let someone else deal with it
const uint256& txhash = txin.prevout.hash;
const auto it = pwallet->mapWallet.find(txhash);
if (it != pwallet->mapWallet.end()) {
const CWalletTx& wtx = it->second;
CTxOut utxo = wtx.tx->vout[txin.prevout.n];
// Update both UTXOs from the wallet.
input.non_witness_utxo = wtx.tx;
input.witness_utxo = utxo;
if (PSBTInputSigned(input)) {
continue;
}
// Verify input looks sane. This will check that we have at most one uxto, witness or non-witness.
if (!input.IsSane()) {
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "PSBT input is not sane.");
}
// If we have no utxo, grab it from the wallet.
if (!input.non_witness_utxo && input.witness_utxo.IsNull()) {
const uint256& txhash = txin.prevout.hash;
const auto it = pwallet->mapWallet.find(txhash);
if (it != pwallet->mapWallet.end()) {
const CWalletTx& wtx = it->second;
// We only need the non_witness_utxo, which is a superset of the witness_utxo.
// The signing code will switch to the smaller witness_utxo if this is ok.
input.non_witness_utxo = wtx.tx;
}
}
// Get the Sighash type
@@ -3797,12 +3807,12 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Specified Sighash and sighash in PSBT do not match.");
}
complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), *psbtx.tx, input, i, sighash_type);
complete &= SignPSBTInput(HidingSigningProvider(pwallet, !sign, !bip32derivs), psbtx, i, sighash_type);
}
// Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change
for (unsigned int i = 0; i < txConst->vout.size(); ++i) {
const CTxOut& out = txConst->vout.at(i);
for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) {
const CTxOut& out = psbtx.tx->vout.at(i);
PSBTOutput& psbt_out = psbtx.outputs.at(i);
// Fill a SignatureData with output info
@@ -3867,19 +3877,15 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request)
// Get the sighash type
int nHashType = ParseSighashString(request.params[2]);
// Use CTransaction for the constant parts of the
// transaction to avoid rehashing.
const CTransaction txConst(*psbtx.tx);
// Fill transaction with our data and also sign
bool sign = request.params[1].isNull() ? true : request.params[1].get_bool();
bool bip32derivs = request.params[3].isNull() ? false : request.params[3].get_bool();
bool complete = FillPSBT(pwallet, psbtx, &txConst, nHashType, sign, bip32derivs);
bool complete = FillPSBT(pwallet, psbtx, nHashType, sign, bip32derivs);
UniValue result(UniValue::VOBJ);
CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
ssTx << psbtx;
result.pushKV("psbt", EncodeBase64((unsigned char*)ssTx.data(), ssTx.size()));
result.pushKV("psbt", EncodeBase64(ssTx.str()));
result.pushKV("complete", complete);
return result;
@@ -3971,29 +3977,18 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
FundTransaction(pwallet, rawTx, fee, change_position, request.params[3]);
// Make a blank psbt
PartiallySignedTransaction psbtx;
psbtx.tx = rawTx;
for (unsigned int i = 0; i < rawTx.vin.size(); ++i) {
psbtx.inputs.push_back(PSBTInput());
}
for (unsigned int i = 0; i < rawTx.vout.size(); ++i) {
psbtx.outputs.push_back(PSBTOutput());
}
// Use CTransaction for the constant parts of the
// transaction to avoid rehashing.
const CTransaction txConst(*psbtx.tx);
PartiallySignedTransaction psbtx(rawTx);
// Fill transaction with out data but don't sign
bool bip32derivs = request.params[4].isNull() ? false : request.params[4].get_bool();
FillPSBT(pwallet, psbtx, &txConst, 1, false, bip32derivs);
FillPSBT(pwallet, psbtx, 1, false, bip32derivs);
// Serialize the PSBT
CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
ssTx << psbtx;
UniValue result(UniValue::VOBJ);
result.pushKV("psbt", EncodeBase64((unsigned char*)ssTx.data(), ssTx.size()));
result.pushKV("psbt", EncodeBase64(ssTx.str()));
result.pushKV("fee", ValueFromAmount(fee));
result.pushKV("changepos", change_position);
return result;

2
src/wallet/rpcwallet.h
View File

@@ -30,5 +30,5 @@ bool EnsureWalletIsAvailable(CWallet *, bool avoidException);
UniValue getaddressinfo(const JSONRPCRequest& request);
UniValue signrawtransactionwithwallet(const JSONRPCRequest& request);
bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type = 1, bool sign = true, bool bip32derivs = false);
bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false);
#endif //BITCOIN_WALLET_RPCWALLET_H

6
src/wallet/test/psbt_wallet_tests.cpp
View File

@@ -59,12 +59,8 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
CDataStream ssData(ParseHex("70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f000000000000000000"), SER_NETWORK, PROTOCOL_VERSION);
ssData >> psbtx;
// Use CTransaction for the constant parts of the
// transaction to avoid rehashing.
const CTransaction txConst(*psbtx.tx);
// Fill transaction with our data
FillPSBT(&m_wallet, psbtx, &txConst, 1, false, true);
FillPSBT(&m_wallet, psbtx, SIGHASH_ALL, false, true);
// Get the final tx
CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);