FillPSBT errantly returning complete=true
#30357
Fixes: #30077
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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:
…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 #30077.
86+ raw = wallet.createrawtransaction([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{substitute_addr: 0.9999}])
87+ signed_psbt_obj.g.map[PSBT_GLOBAL_UNSIGNED_TX] = bytes.fromhex(raw)
88+
89+ # Check that the walletprocesspsbt call succeeds but also recognizes that the transaction is not complete
90+ signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)
91+ 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.
82+
83+ # Modify the raw transaction by changing the output address, so the signature is no longer valid
84+ signed_psbt_obj = PSBT.from_base64(signed_psbt)
85+ substitute_addr = wallet.getnewaddress(address_type="bech32m")
86+ raw = wallet.createrawtransaction([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{substitute_addr: 0.9999}])
87+ signed_psbt_obj.g.map[PSBT_GLOBAL_UNSIGNED_TX] = bytes.fromhex(raw)
getnewaddress call and just hard-code something, or change one of the output amounts. Maybe only if you retouch.
ACK f57f68ba95fc8778fe2dc755da1e631fe60592c4
Reviewed code and confirmed test fails without patch. Approach is great, one question below about how much farther to take this approach…
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK f57f68ba95fc8778fe2dc755da1e631fe60592c4
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaECVgACgkQ5+KYS2KJ
7yTq5vw//QNiwjThHf65qtM+57G6YDPWQ3zVu2s278T0aZU5ND62Hu4A9/DHqDBSQ
836A4klRON1ddIfFX91tE3GrwT4PZ51vK3uhX0xJYkZ66xkhObVZz2lCifMs+A2ng
9ldEoEBT86d17fyuQLle8/VNolqXqWx9Dw5RZY3WclnO0Hex2l9xiWepel4sFzAM8
10P4YUgsw6ZaEbRUjh67tZ3kLlmz5l5yBNV5NaO7rbsPeBiIqizRbGl7CQlhlxgM4g
11ZF4PGUd9rk0sSnWWMHUcVA+Uskuw64bDH0keJBLUYeKpNLfRUZ5KTRSJilurayY5
12M2B7veORZ4MWjGHjycbpV+hdpjpQzX5z1cyCxGqYN4gC8P7kpFVBrB6sMUvOLoku
13lGTC+SKE+sxjRmgYcE1QpPMz7uYs8KTgkTHpzlbyj61oEL1SylXDtOlc7nRN2I0A
14XEnV8kK/2VKF3V45dTRPUwFm+2qX1NnCEyJ53x2srg9+U3px9UxfUMAuwTmZYyOV
15E/9XCDouqE63CqCzqM683lmdL+sCsfA5IHi9sqzVRP2cY8s1NOkj7QdXQO0Z8V07
16ycsGpj8nI8GAq/b647aYOU9O+Ni2PGq2wdkpP8ciD0Iafp7S9MkpCjHx9J+p4YPE
17TQVElxmGYgx5zIPFrdrBehZCJGX/YDqj8Zd9Ra0QYwVzYg9aqow=
18=KJef
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on keybase
Thanks @willcl-ark for picking this up! Confirmed this fixes the CHECK_NONFATAL abort when a witness signature is invalid.
tACK f57f68ba95fc8778fe2dc755da1e631fe60592c4
67@@ -68,6 +68,28 @@ def set_test_params(self):
68 def skip_test_if_missing_module(self):
69 self.skip_if_no_wallet()
70
71+ def test_psbt_incomplete_after_invalid_modification(self):
72+ self.log.info("Check that PSBT is correctly marked as incomplete after invalid modification")
73+ node = self.nodes[2]
74+ wallet = node.get_wallet_rpc(self.default_wallet_name)
75+ address = wallet.getnewaddress(address_type="bech32m")
In b6cea03b20bfe9486ff8f38a1c40c985d066e72d “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).
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`.
75+ address = wallet.getnewaddress()
76+ wallet.sendtoaddress(address=address, amount=1.0)
77+ self.generate(node, nblocks=1, sync_fun=lambda: self.sync_all(self.nodes[:2]))
78+
79+ utxos = wallet.listunspent(addresses=[address])
80+ psbt = wallet.createpsbt([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): 0.9999}])
nano nit:
0psbt = wallet.createpsbt(utxos, [{wallet.getnewaddress(): 0.9999}])
85+ substitute_addr = wallet.getnewaddress()
86+ raw = wallet.createrawtransaction([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{substitute_addr: 0.9999}])
87+ signed_psbt_obj.g.map[PSBT_GLOBAL_UNSIGNED_TX] = bytes.fromhex(raw)
88+
89+ # Check that the walletprocesspsbt call succeeds but also recognizes that the transaction is not complete
90+ signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)
walletprocesspsbt appends the new information without removing/updating the previous one. See https://github.com/furszy/bitcoin-core/commit/698e090ead3d0603146c98bd2ec06e44389f34cc.
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: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:walletprocesspsbt-no-finalize-fixups
I took inspiration from https://github.com/furszy/bitcoin-core/commit/698e090ead3d0603146c98bd2ec06e44389f34cc (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: https://github.com/bitcoin/bitcoin/compare/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).
finalize=false? the test still passes without it.
86+ raw = wallet.createrawtransaction([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{substitute_addr: 0.9999}])
87+ signed_psbt_obj.g.map[PSBT_GLOBAL_UNSIGNED_TX] = bytes.fromhex(raw)
88+
89+ # Check that the walletprocesspsbt call succeeds but also recognizes that the transaction is not complete
90+ signed_psbt_incomplete = wallet.walletprocesspsbt(signed_psbt_obj.to_base64(), finalize=False)
91+ assert signed_psbt_incomplete["complete"] is False
nit
0 assert_equal(signed_psbt_incomplete["complete"], False)
Tested ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902
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.
re-ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902
Changes since last ACK are minimal, slight simplification in tests to enable more legacy compatability
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK 7e36dca657c66bc70b04d5b850e5a335aecfb902
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaRgqoACgkQ5+KYS2KJ
7yTq5NA/+NL1fOaCfl+P9yDIsXebxam/8Adf6iH5WUBt2TvUju06+305KL1dwvO6O
8n0j8wmAeaJQe9tevuSTFYopxRaH+7uVQmrMDxKucURgBZ5I/VdBWk8v3+CPUsnKQ
9v/hyt4RPMsL12rcti936UqKi77nfRc6E+7zsDOtRU0Rf6ME222lCaWUpqAjm4vPm
103nN18eXw6Az9rKHwG3KFzXxMy/bthlI6bsRtvbUqtFc6PkZ7xR7f79EauawCkJWS
11vCr4RXVcrC+1FMB8MHMcbLiNTmNjGApg48PAYnaoJsolpGQoRaHRNwKeRo2w0Uvw
12tk3uXeVNfwxRbYwzBI6pZ/JYTSa3u31qsVGqx3mVVxm8y1F/sA+fwsRB+AqToDaj
13Kzf4P8FLkyYT1t83i1n5yRKawHrQTVcsMVhxsN/+4nqBbvdGba6E5SpXG3fBJ3sI
14L/fJocldQmO9maY+I9CgjJqmM9GauMvhjSq+g0PnWE9pRUnt1n0bFy7DwdonzRFK
15bXtFx2xcWxtrHSAvLZt6oxTXTqwfh5XiFMHwG93ULySpzOJTHB+lTED1TKf4gwhk
16QcaVW4Oi0lQ8QE6rtq+o0nx0FFz+lA9RoT2MFWQU2L+HFGqokwI9PQTVWxzm9Vjd
17ZWOF04PI0QvCz0DITFHV0HTX1YsyQobWWWXRxwttqy4GDUi0PPw=
18=DAZD
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on keybase
