Fix cases of calls to `FillPSBT` errantly returning `complete=true` #30357

willcl-ark · 2024-06-28T12:08:55Z

Fix cases of calls to FillPSBT returning complete=true when it's not
the case.

This can happen when some inputs have been signed but the transaction is
subsequently modified, e.g. in the context of PayJoins.

Also fixes a related bug where a finalized hex string is attempted to be
added during walletprocesspsbt but a CHECK_NONFATAL causes an abort.

DrahtBot · 2024-06-28T12:08:58Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ismaelsadeeq, pinheadmz, achow101
Concept ACK	BrandonOdiwuor
Stale ACK	spacebear21

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

pinheadmz · 2024-07-01T19:12:32Z

concept ACK

Confirmed the bug on master, fixed by the patch in this PR.

I'm not sure responding to the user-provided "finalize" option in this way is entirely correct though. It seems to me that the "complete" value that we return could be inaccurate even before #28414 because we haven't called PSBTInputSignedAndVerified() yet. So that makes me wonder if we should be verifying in FillPSBT() here, instead of just checking that something is in the scriptsig / witness:

bitcoin/src/wallet/wallet.cpp

Lines 2226 to 2230 in fe70be5

    
           // Complete if every input is now signed 
        
           complete = true; 
        
           for (const auto& input : psbtx.inputs) { 
        
               complete &= PSBTInputSigned(input); 
        
           }

...because as the issue demonstrates, the "complete" value could be misleading.

In other words, I think "complete" should be false already, before the "hex" was added to the rpc output.

Fix cases of calls to `FillPSBT` returning `complete=true` when it's not the case. This can happen when some inputs have been signed but the transaction is subsequently modified, e.g. in the context of PayJoins. Also fixes a related bug where a finalized hex string is attempted to be added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort. Reported in bitcoin#30077.

willcl-ark · 2024-07-02T08:59:02Z

Rebased to fix CI

pinheadmz

ACK f57f68b

Reviewed code and confirmed test fails without patch. Approach is great, one question below about how much farther to take this approach...

Show Signature

pinheadmz's public key is on keybase

pinheadmz · 2024-07-02T13:57:26Z

test/functional/rpc_psbt.py

+        signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)
+        assert signed_psbt_incomplete["complete"] is False


This makes me wonder if finalize=True should result in "complete": True. It's another case of using PSBTInputSigned() instead of PSBTInputSignedAndVerified(). We could update this code below to automatically replace / update invalid signatures.

bitcoin/src/wallet/wallet.cpp

Lines 2181 to 2194 in d2c8d16

std::optional<PSBTError> CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, int sighash_type, bool sign, bool bip32derivs, size_t * n_signed, bool finalize) 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) {

const CTxIn& txin = psbtx.tx->vin[i];

PSBTInput& input = psbtx.inputs.at(i);

if (PSBTInputSigned(input)) {

continue;

}

pinheadmz · 2024-07-02T14:02:38Z

test/functional/rpc_psbt.py

+        signed_psbt_obj = PSBT.from_base64(signed_psbt)
+        substitute_addr = wallet.getnewaddress(address_type="bech32m")
+        raw = wallet.createrawtransaction([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{substitute_addr: 0.9999}])
+        signed_psbt_obj.g.map[PSBT_GLOBAL_UNSIGNED_TX] = bytes.fromhex(raw)


There might be a simpler way to malleate the PSBT. You could at least skip the getnewaddress call and just hard-code something, or change one of the output amounts. Maybe only if you retouch.

spacebear21 · 2024-07-02T15:51:46Z

Thanks @willcl-ark for picking this up! Confirmed this fixes the CHECK_NONFATAL abort when a witness signature is invalid.

tACK f57f68b

achow101 · 2024-07-02T20:14:11Z

test/functional/rpc_psbt.py

+        self.log.info("Check that PSBT is correctly marked as incomplete after invalid modification")
+        node = self.nodes[2]
+        wallet = node.get_wallet_rpc(self.default_wallet_name)
+        address = wallet.getnewaddress(address_type="bech32m")


In b6cea03 "test: add test for modififed walletprocesspsbt calls"

Removing address_type here and below would allow this test to run on legacy wallets so it does not need the if self.options.descriptors below (which would remove the conflict with #28710 and save me a rebase).

Done in 7e36dca

This test checks that we can successfully process PSBTs and opt out of finalization. Previously trying to call `walletprocesspsbt` would attempt to auto-finalize (as a convenience), and would not permit opt-out of finalization, instead aborting via `CHECK_NONFATAL`.

furszy · 2024-07-04T16:33:55Z

test/functional/rpc_psbt.py

+        utxos = wallet.listunspent(addresses=[address])
+        psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): 0.9999}])


nano nit:

Suggested change

utxos = wallet.listunspent(addresses=[address])

psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): 0.9999}])

psbt = wallet.createpsbt(utxos, [{wallet.getnewaddress(): 0.9999}])

furszy · 2024-07-04T20:27:16Z

test/functional/rpc_psbt.py

+        signed_psbt_obj = PSBT.from_base64(signed_psbt)
+        substitute_addr = wallet.getnewaddress()
+        raw = wallet.createrawtransaction([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{substitute_addr: 0.9999}])
+        signed_psbt_obj.g.map[PSBT_GLOBAL_UNSIGNED_TX] = bytes.fromhex(raw)
+
+        # Check that the walletprocesspsbt call succeeds but also recognizes that the transaction is not complete
+        signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)


Seems that the PSBT 'outputs' field retains the previous output derivation path after replacement. walletprocesspsbt appends the new information without removing/updating the previous one. See furszy@698e090.

Thanks @furszy, this indeed feels unclean to me.

I made a few changes in another test branch and I wonder if they make sense to you: master...willcl-ark:bitcoin:walletprocesspsbt-no-finalize-fixups

I took inspiration from furszy@698e090 (thanks) but it seems that I can probably just clear the entire output map as done in L94 and then update the PSBT_GLOBAL_UNSIGNED_TX in the global map before calling walletprocesspsbt again.

I made a few changes in another test branch and I wonder if they make sense to you: master...willcl-ark:bitcoin:walletprocesspsbt-no-finalize-fixups

The question here is whether walletprocesspsbt should update the existing PSBT 'outputs' information (similar to how it updates the transaction inputs) or should only append data to it as it is currently doing. In the current test form, the first entry of the PSBT 'outputs' field ends up with two BIP32 derivation paths that cannot coexist: they share the same master key fingerprint and are under the same derivation path: the first one is m/.../15 while the other one is m/.../16.

Clearing the map on the test side hides this topic under the carpet (which is also a possibility if you want to leave it for a follow-up too).

All of our PSBT updating operations should never remove any data from the PSBT. If the user decides to do something that invalidates a field in the PSBT, it's their duty to fix it themselves.

BrandonOdiwuor

Concept ACK

ismaelsadeeq

Tested ACK 7e36dca

I've verified that this PR fixes the issue #30077, complete=false is returned instead of throwing an error.

Just a single test question and a non-blocking nit.

ismaelsadeeq · 2024-07-09T13:55:49Z

test/functional/rpc_psbt.py

+        signed_psbt_obj.g.map[PSBT_GLOBAL_UNSIGNED_TX] = bytes.fromhex(raw)
+
+        # Check that the walletprocesspsbt call succeeds but also recognizes that the transaction is not complete
+        signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)


Why pass finalize=false? the test still passes without it.

ismaelsadeeq · 2024-07-09T13:57:53Z

test/functional/rpc_psbt.py

+
+        # Check that the walletprocesspsbt call succeeds but also recognizes that the transaction is not complete
+        signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)
+        assert signed_psbt_incomplete["complete"] is False


nit

Suggested change

assert signed_psbt_incomplete["complete"] is False

assert_equal(signed_psbt_incomplete["complete"], False)

pinheadmz

re-ACK 7e36dca

Changes since last ACK are minimal, slight simplification in tests to enable more legacy compatability

Show Signature

pinheadmz's public key is on keybase

achow101 · 2024-07-16T20:32:31Z

ACK 7e36dca

Fix cases of calls to `FillPSBT` returning `complete=true` when it's not the case. This can happen when some inputs have been signed but the transaction is subsequently modified, e.g. in the context of PayJoins. Also fixes a related bug where a finalized hex string is attempted to be added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort. Reported in bitcoin#30077. Github-Pull: bitcoin#30357 Rebased-From: 39cea21

This test checks that we can successfully process PSBTs and opt out of finalization. Previously trying to call `walletprocesspsbt` would attempt to auto-finalize (as a convenience), and would not permit opt-out of finalization, instead aborting via `CHECK_NONFATAL`. Github-Pull: bitcoin#30357 Rebased-From: 7e36dca

Fix cases of calls to `FillPSBT` returning `complete=true` when it's not the case. This can happen when some inputs have been signed but the transaction is subsequently modified, e.g. in the context of PayJoins. Also fixes a related bug where a finalized hex string is attempted to be added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort. Reported in bitcoin#30077. Github-Pull: bitcoin#30357 Rebased-From: 39cea21

This test checks that we can successfully process PSBTs and opt out of finalization. Previously trying to call `walletprocesspsbt` would attempt to auto-finalize (as a convenience), and would not permit opt-out of finalization, instead aborting via `CHECK_NONFATAL`. Github-Pull: bitcoin#30357 Rebased-From: 7e36dca

fanquake · 2024-07-17T10:33:11Z

Backported to 27.x in #30467.

4f23c86 [WIP] doc: update release notes for 27.x (fanquake) 54bb9b0 test: add test for modififed walletprocesspsbt calls (willcl-ark) f22b9ca wallet: fix FillPSBT errantly showing as complete (willcl-ark) 05192ba init: change shutdown order of load block thread and scheduler (Martin Zumsande) ab42206 Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner) 064f214 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner) 0933cf5 net: fix race condition in self-connect detection (Sebastian Falbesoner) fa90989 psbt: Check non witness utxo outpoint early (Ava Chow) Pull request description: Backports: * #29855 * #30357 * #30394 (modified test commit) * #30435 ACKs for top commit: stickies-v: ACK 4f23c86 willcl-ark: ACK 4f23c86 Tree-SHA512: 5c26445f0855f9d14890369ce19873b0686804eeb659e7d6da36a6f404f64d019436e1e6471579eaa60a96ebf8f64311883b4aef9d0ed528a95bd610c101c079

Fix cases of calls to `FillPSBT` returning `complete=true` when it's not the case. This can happen when some inputs have been signed but the transaction is subsequently modified, e.g. in the context of PayJoins. Also fixes a related bug where a finalized hex string is attempted to be added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort. Reported in bitcoin#30077. Github-Pull: bitcoin#30357 Rebased-From: 39cea21

This test checks that we can successfully process PSBTs and opt out of finalization. Previously trying to call `walletprocesspsbt` would attempt to auto-finalize (as a convenience), and would not permit opt-out of finalization, instead aborting via `CHECK_NONFATAL`. Github-Pull: bitcoin#30357 Rebased-From: 7e36dca

willcl-ark requested a review from pinheadmz June 28, 2024 13:29

DrahtBot mentioned this pull request Jun 29, 2024

Remove the legacy wallet and BDB dependency #28710

Draft

willcl-ark force-pushed the walletprocesspsbt-no-finalize branch 2 times, most recently from efa8138 to b6cea03 Compare July 1, 2024 19:47

willcl-ark changed the title ~~Permit opt-out of finalization during walletprocesspsbt~~ Fix cases of calls to FillPSBT errantly returning complete=true Jul 1, 2024

DrahtBot added the CI failed label Jul 2, 2024

willcl-ark force-pushed the walletprocesspsbt-no-finalize branch from b6cea03 to f57f68b Compare July 2, 2024 08:58

DrahtBot removed the CI failed label Jul 2, 2024

pinheadmz approved these changes Jul 2, 2024

View reviewed changes

achow101 reviewed Jul 2, 2024

View reviewed changes

willcl-ark force-pushed the walletprocesspsbt-no-finalize branch from f57f68b to 7e36dca Compare July 3, 2024 08:31

fanquake added this to the 28.0 milestone Jul 3, 2024

furszy reviewed Jul 4, 2024

View reviewed changes

BrandonOdiwuor reviewed Jul 5, 2024

View reviewed changes

ismaelsadeeq reviewed Jul 9, 2024

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and pinheadmz July 9, 2024 14:09

pinheadmz approved these changes Jul 12, 2024

View reviewed changes

achow101 merged commit 6f9db1e into bitcoin:master Jul 16, 2024
15 of 16 checks passed

fanquake mentioned this pull request Jul 17, 2024

[27.x] More backports #30467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cases of calls to `FillPSBT` errantly returning `complete=true` #30357

Fix cases of calls to `FillPSBT` errantly returning `complete=true` #30357

willcl-ark commented Jun 28, 2024 •

edited

Loading

DrahtBot commented Jun 28, 2024 •

edited

Loading

pinheadmz commented Jul 1, 2024

willcl-ark commented Jul 2, 2024

pinheadmz left a comment

pinheadmz Jul 2, 2024

pinheadmz Jul 2, 2024

spacebear21 commented Jul 2, 2024

achow101 Jul 2, 2024

willcl-ark Jul 3, 2024

furszy Jul 4, 2024

furszy Jul 4, 2024

willcl-ark Jul 5, 2024

furszy Jul 5, 2024

achow101 Jul 16, 2024

BrandonOdiwuor left a comment

ismaelsadeeq left a comment

ismaelsadeeq Jul 9, 2024

ismaelsadeeq Jul 9, 2024

pinheadmz left a comment

achow101 commented Jul 16, 2024 •

edited

Loading

fanquake commented Jul 17, 2024

		signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)
		assert signed_psbt_incomplete["complete"] is False

	std::optional<PSBTError> CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& complete, int sighash_type, bool sign, bool bip32derivs, size_t * n_signed, bool finalize) 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) {
	const CTxIn& txin = psbtx.tx->vin[i];
	PSBTInput& input = psbtx.inputs.at(i);

	if (PSBTInputSigned(input)) {
	continue;
	}

		utxos = wallet.listunspent(addresses=[address])
		psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): 0.9999}])

	utxos = wallet.listunspent(addresses=[address])
	psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): 0.9999}])
	psbt = wallet.createpsbt(utxos, [{wallet.getnewaddress(): 0.9999}])

	assert signed_psbt_incomplete["complete"] is False
	assert_equal(signed_psbt_incomplete["complete"], False)

Fix cases of calls to FillPSBT errantly returning complete=true #30357

Fix cases of calls to FillPSBT errantly returning complete=true #30357

Conversation

willcl-ark commented Jun 28, 2024 • edited Loading

DrahtBot commented Jun 28, 2024 • edited Loading

Code Coverage

Reviews

Conflicts

pinheadmz commented Jul 1, 2024

willcl-ark commented Jul 2, 2024

pinheadmz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacebear21 commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz left a comment

Choose a reason for hiding this comment

achow101 commented Jul 16, 2024 • edited Loading

fanquake commented Jul 17, 2024

Fix cases of calls to `FillPSBT` errantly returning `complete=true` #30357

Fix cases of calls to `FillPSBT` errantly returning `complete=true` #30357

willcl-ark commented Jun 28, 2024 •

edited

Loading

DrahtBot commented Jun 28, 2024 •

edited

Loading

achow101 commented Jul 16, 2024 •

edited

Loading