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.

Sign up for GitHub

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

Jump to bottom

"PSBT Operations" dialog #18027

Merged
merged 5 commits into from Jun 21, 2020
Merged

"PSBT Operations" dialog #18027

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e7b23b
Improve TransactionErrorString messages.
gwillen Feb 1, 2020
5dd0c03
FillPSBT: report number of inputs signed (or would sign)
gwillen Feb 1, 2020
a6cb0b0
[gui] PSBT Operations Dialog (sign & broadcast)
gwillen May 29, 2020
11a0ffb
[gui] Load PSBT from clipboard
gwillen Jan 31, 2020
931dd47
Make lint-spelling.py happy
gwillen May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/interfaces/wallet.cpp
View file
Open in desktop
Expand Up @@ -335,9 +335,10 @@ class WalletImpl : public Wallet
bool sign,
bool bip32derivs,
PartiallySignedTransaction& psbtx,
bool& complete) override
bool& complete,
size_t* n_signed) override
{
return m_wallet->FillPSBT(psbtx, complete, sighash_type, sign, bip32derivs);
return m_wallet->FillPSBT(psbtx, complete, sighash_type, sign, bip32derivs, n_signed);
}
WalletBalances getBalances() override
{
Expand Down
3 changes: 2 additions & 1 deletion src/interfaces/wallet.h
View file
Open in desktop
Expand Up @@ -197,7 +197,8 @@ class Wallet
bool sign,
bool bip32derivs,
PartiallySignedTransaction& psbtx,
bool& complete) = 0;
bool& complete,
size_t* n_signed) = 0;

//! Get balances.
virtual WalletBalances getBalances() = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/qt/sendcoinsdialog.cpp
View file
Open in desktop
Expand Up @@ -392,7 +392,7 @@ void SendCoinsDialog::on_sendButton_clicked()
CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
PartiallySignedTransaction psbtx(mtx);
bool complete = false;
const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete);
const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete, nullptr);
assert(!complete);
assert(err == TransactionError::OK);
// Serialize the PSBT
Expand Down
2 changes: 1 addition & 1 deletion src/qt/walletmodel.cpp
View file
Open in desktop
Expand Up @@ -536,7 +536,7 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
if (create_psbt) {
PartiallySignedTransaction psbtx(mtx);
bool complete = false;
const TransactionError err = wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete);
const TransactionError err = wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, psbtx, complete, nullptr);
if (err != TransactionError::OK || complete) {
QMessageBox::critical(nullptr, tr("Fee bump error"), tr("Can't draft transaction."));
return false;
Expand Down
6 changes: 3 additions & 3 deletions src/util/error.cpp
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ bilingual_str TransactionErrorString(const TransactionError err)
case TransactionError::OK:
return Untranslated("No error");
case TransactionError::MISSING_INPUTS:
return Untranslated("Missing inputs");
return Untranslated("Inputs missing or spent");
case TransactionError::ALREADY_IN_CHAIN:
return Untranslated("Transaction already in block chain");
case TransactionError::P2P_DISABLED:
Expand All @@ -24,11 +24,11 @@ bilingual_str TransactionErrorString(const TransactionError err)
case TransactionError::MEMPOOL_ERROR:
return Untranslated("AcceptToMemoryPool failed");
case TransactionError::INVALID_PSBT:
return Untranslated("PSBT is not sane");
return Untranslated("PSBT is not well-formed");
case TransactionError::PSBT_MISMATCH:
return Untranslated("PSBTs not compatible (different transactions)");
case TransactionError::SIGHASH_MISMATCH:
return Untranslated("Specified sighash value does not match existing value");
return Untranslated("Specified sighash value does not match value stored in PSBT");
case TransactionError::MAX_FEE_EXCEEDED:
return Untranslated("Fee exceeds maximum configured by -maxtxfee");
// no default case, so the compiler can warn about missing cases
Expand Down
26 changes: 24 additions & 2 deletions src/wallet/scriptpubkeyman.cpp
View file
Open in desktop
Expand Up @@ -585,8 +585,11 @@ SigningResult LegacyScriptPubKeyMan::SignMessage(const std::string& message, con
return SigningResult::SIGNING_FAILED;
}

TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs) const
TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs, int* n_signed) const
{
if (n_signed) {
*n_signed = 0;
}
for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
const CTxIn& txin = psbtx.tx->vin[i];
PSBTInput& input = psbtx.inputs.at(i);
Expand Down Expand Up @@ -617,6 +620,14 @@ TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psb
SignatureData sigdata;
input.FillSignatureData(sigdata);
SignPSBTInput(HidingSigningProvider(this, !sign, !bip32derivs), psbtx, i, sighash_type);

bool signed_one = PSBTInputSigned(input);
if (n_signed && (signed_one || !sign)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent is to indicate whether signing happened, or whether we could sign. But this is really checking whether we could finalize, which may include signing. For instance, this wouldn't work if the input were multisig.

But that might be ok for this PR.

To reduce code duplication, maybe this should go inside SignPSBTInput?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very interested in a conversation about refactoring some of this stuff. Ok to save for another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I am interested both in (1) reducing code duplication and (2) working out more precisely whether we can sign something, which I found to be very difficult to actually compute, and I definitely recognize this as a conservative approximation.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to save for another PR?

Yep

// If sign is false, we assume that we _could_ sign if we get here. This
// will never have false negatives; it is hard to tell under what i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/what i/what/ here and at line 2136 below

// circumstances it could have false positives.
(*n_signed)++;
}
}

// Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change
Expand Down Expand Up @@ -2064,8 +2075,11 @@ SigningResult DescriptorScriptPubKeyMan::SignMessage(const std::string& message,
return SigningResult::OK;
}

TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs) const
TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, int sighash_type, bool sign, bool bip32derivs, int* n_signed) const
{
if (n_signed) {
*n_signed = 0;
}
for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
const CTxIn& txin = psbtx.tx->vin[i];
PSBTInput& input = psbtx.inputs.at(i);
Expand Down Expand Up @@ -2117,6 +2131,14 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
}

SignPSBTInput(HidingSigningProvider(keys.get(), !sign, !bip32derivs), psbtx, i, sighash_type);

bool signed_one = PSBTInputSigned(input);
if (n_signed && (signed_one || !sign)) {
// If sign is false, we assume that we _could_ sign if we get here. This
// will never have false negatives; it is hard to tell under what i
// circumstances it could have false positives.
(*n_signed)++;
}
}

// Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/scriptpubkeyman.h
View file
Open in desktop
Expand Up @@ -234,7 +234,7 @@ class ScriptPubKeyMan
/** Sign a message with the given script */
virtual SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const { return SigningResult::SIGNING_FAILED; };
/** Adds script and derivation path information to a PSBT, and optionally signs it. */
virtual TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false) const { return TransactionError::INVALID_PSBT; }
virtual TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const { return TransactionError::INVALID_PSBT; }

virtual uint256 GetID() const { return uint256(); }

Expand Down Expand Up @@ -393,7 +393,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv

bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const override;
SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override;
TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false) const override;
TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const override;

uint256 GetID() const override;

Expand Down Expand Up @@ -596,7 +596,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan

bool SignTransaction(CMutableTransaction& tx, const std::map<COutPoint, Coin>& coins, int sighash, std::map<int, std::string>& input_errors) const override;
SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override;
TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false) const override;
TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const override;

uint256 GetID() const override;

Expand Down
12 changes: 10 additions & 2 deletions src/wallet/wallet.cpp
View file
Open in desktop
Expand Up @@ -2471,8 +2471,11 @@ bool CWallet::SignTransaction(CMutableTransaction& tx, const std::map<COutPoint,
return false;
}

TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, int sighash_type, bool sign, bool bip32derivs) const
TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, int sighash_type, bool sign, bool bip32derivs, size_t * n_signed) const
{
if (n_signed) {
*n_signed = 0;
}
LOCK(cs_wallet);
// Get all of the previous transactions
for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
Expand Down Expand Up @@ -2503,10 +2506,15 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp

// Fill in information from ScriptPubKeyMans
for (ScriptPubKeyMan* spk_man : GetAllScriptPubKeyMans()) {
TransactionError res = spk_man->FillPSBT(psbtx, sighash_type, sign, bip32derivs);
int n_signed_this_spkm = 0;
TransactionError res = spk_man->FillPSBT(psbtx, sighash_type, sign, bip32derivs, &n_signed_this_spkm);
if (res != TransactionError::OK) {
return res;
}

if (n_signed) {
(*n_signed) += n_signed_this_spkm;
}
}

// Complete if every input is now signed
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/wallet.h
View file
Open in desktop
Expand Up @@ -964,7 +964,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
bool& complete,
int sighash_type = 1 /* SIGHASH_ALL */,
bool sign = true,
bool bip32derivs = true) const;
bool bip32derivs = true,
size_t* n_signed = nullptr) const;

/**
* Create a new transaction paying the recipients with a set of coins
Expand Down
2 changes: 1 addition & 1 deletion test/functional/rpc_psbt.py
View file
Open in desktop
Expand Up @@ -467,7 +467,7 @@ def test_psbt_input_keys(psbt_input, keys):
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Input 0 specifies invalid prevout')

assert_raises_rpc_error(-25, 'Missing inputs', self.nodes[0].walletprocesspsbt, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
assert_raises_rpc_error(-25, 'Inputs missing or spent', self.nodes[0].walletprocesspsbt, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')

if __name__ == '__main__':
PSBTTest().main()