rpc: Add support to populate PSBT input utxos via rpc #30886

1. 
   instagibbs commented at 5:39 pm on September 12, 2024: member

   Resolves #30873

   The typical use-case would be when you have a too low-fee(perhaps presigned) or timelocked parent transaction where you want to sign the child transaction before broadcasting anything.

   You could imagine the parent transaction being a LN commitment transaction at 0 fee, and the child transaction being an anchor spend. Due to the below mempool minimum of the parent, the parent and child transaction must be sent as a pair via submitpackage, so the parent isn’t available in the mempool or utxo set yet.

2. 
   DrahtBot commented at 5:39 pm on September 12, 2024: contributor

   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	rkrux
   Concept ACK	1440000bytes, BrandonOdiwuor
   Stale ACK	pablomartin4btc

   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:

   • #21283 (Implement BIP 370 PSBTv2 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.

3. DrahtBot added the label RPC/REST/ZMQ on Sep 12, 2024
4. instagibbs force-pushed on Sep 12, 2024
5. DrahtBot added the label CI failed on Sep 12, 2024
6. DrahtBot removed the label CI failed on Sep 13, 2024
7. 
   pablomartin4btc commented at 12:30 pm on September 16, 2024: member

   ACK abc835282a66495f65f342cfc113e45aac4ebb48

   1. Was this functionality already exposed somewhere I missed?

   I couldn’t find anything but perhaps I missed some.

   2. Should “non-witness utxos” always be the type added when updating the PSBT? Looks like we fill in non-witness wherever we can, and witness whenever scraping from utxo set

   You mean that segwit txs would need to be converted into legacy ones to be passed in prev_txs?

   I’ve checked the test and I understand the use case, but could you please expand a bit on the reasons why prev txs “can not be entered into the mempool or UTXO set”, if possible?

8. 
   instagibbs commented at 1:21 pm on September 16, 2024: member

   @pablomartin4btc I expanded on the use-case a bit in the OP

   For (2) I was asking what was typically done for non_witness_utxo vs witness_utxo. Due to pre-taproot weaknesses in sighash format I think it’s typical to add the full transaction when possible in this codebase at least, and I conform to that in this PR.

9. 
   1440000bytes commented at 9:42 pm on October 3, 2024: none

   Concept ACK
10. DrahtBot added the label CI failed on Oct 11, 2024
11. 
    BrandonOdiwuor commented at 10:12 am on October 12, 2024: contributor

    Concept ACK
12. DrahtBot removed the label CI failed on Oct 13, 2024
13. in src/rpc/rawtransaction.cpp:1705 in abc835282a outdated

    1700@@ -1680,6 +1701,9 @@ static RPCHelpMan utxoupdatepsbt()
    1701                          {"range", RPCArg::Type::RANGE, RPCArg::Default{1000}, "Up to what index HD chains should be explored (either end or [begin,end])"},
    1702                     }},
    1703                 }},
    1704+                {"prevtxs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "An array of dependant serialized transactions as hex", {
    1705+                    {"", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A serialized previout transaction in hex"},
    

    ---

    ---

    

    rkrux commented at 9:57 am on October 20, 2024:

    0                    {"", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A serialized previous transaction in hex"},
    

    Assuming it’s supposed to say previous.

    ---

    

    instagibbs commented at 2:30 pm on October 21, 2024:

    fixed
14. in src/rpc/rawtransaction.cpp:1992 in abc835282a outdated

    1987@@ -1947,6 +1988,9 @@ RPCHelpMan descriptorprocesspsbt()
    1988             "       \"SINGLE|ANYONECANPAY\""},
    1989                     {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them"},
    1990                     {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible"},
    1991+                    {"prevtxs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "An array of dependant serialized transactions as hex", {
    1992+                        {"", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A serialized previout transaction in hex"},
    

    ---

    ---

    

    rkrux commented at 9:57 am on October 20, 2024:

    0                        {"", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A serialized previous transaction in hex"},
    

    ---

    

    instagibbs commented at 2:30 pm on October 21, 2024:

    changed thanks
15. in src/rpc/rawtransaction.cpp:2014 in abc835282a outdated

    2031+                                   "TX decode failed: " + rawtx.get_str() + " Make sure the prev tx has at least one input.");
    2032+            }
    2033+            prev_txns.emplace_back(MakeTransactionRef(std::move(mtx)));
    2034+        }
    2035+    }
    2036+
    

    ---

    ---

    

    rkrux commented at 10:03 am on October 20, 2024:

    Does it make sense to deduplicate this code piece by accepting prev_txs, request.params[i] as inputs to a function?

    ---

    

    instagibbs commented at 2:30 pm on October 21, 2024:

    good point, could be used elsewhere too in a follow-up e.g., testmempoolaccept, submitpackage, etc. done.
16. in test/functional/rpc_psbt.py:227 in abc835282a outdated

    222+
    223+        psbtx_child = self.nodes[0].createpsbt([{"txid": parent_txid, "vout": parent_vout}], {self.nodes[0].getnewaddress(): parent_txinfo["vout"][0]["value"] - Decimal("0.01")})
    224+
    225+        # Can not sign due to lack of utxo
    226+        res = self.nodes[0].walletprocesspsbt(psbtx_child)
    227+        assert not res["complete"]
    

    ---

    ---

    

    rkrux commented at 10:07 am on October 20, 2024:

    Outside the scope of this PR: While testing I realised that the output does not display the reason why it failed, would it be a good idea to display the error as well?

    0➜  src git:(2024-09-updateutxo_psbt) ✗ bitcoincli walletprocesspsbt cHNidP8BAFICAAAAAfDLyQnCSPG31wZz1rHABK8QVnQLIubUuakp3tA1b688AAAAAAD9////AQBlzR0AAAAAFgAU0NrK8DrQVPpN/ZzmgNtNu55HmNgAAAAAAAAA
    1{
    2  "psbt": "cHNidP8BAFICAAAAAfDLyQnCSPG31wZz1rHABK8QVnQLIubUuakp3tA1b688AAAAAAD9////AQBlzR0AAAAAFgAU0NrK8DrQVPpN/ZzmgNtNu55HmNgAAAAAAAAiAgNKBeuPVHjpCVZn1sLhqf9axYBqutVFf3QOBrh0J+qI4hi/EclnVAAAgAEAAIAAAACAAAAAAAEAAAAA",
    3  "complete": false
    4}
    

    ---

    

    instagibbs commented at 2:01 pm on October 21, 2024:

    lots of ways things can go wrong, unsure what would be good to report personally
17. rkrux approved
18. 
    rkrux commented at 10:11 am on October 20, 2024: none

    tACK https://github.com/bitcoin/bitcoin/commit/abc835282a66495f65f342cfc113e45aac4ebb48

    Successful make and functional tests. I manually tested the new argument in utxoupdatepsbt using my regtest environment by creating parent transaction and a subsequent child transaction. Processing the child PSBT without passing in the raw parent transaction hex led to incomplete processing and was successful later when the parent was passed. Also tested for duplicate parent transaction error.

19. DrahtBot requested review from BrandonOdiwuor on Oct 20, 2024
20. instagibbs force-pushed on Oct 21, 2024
21. 
    DrahtBot commented at 4:25 pm on October 21, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31832620127

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

22. DrahtBot added the label CI failed on Oct 21, 2024
23. instagibbs force-pushed on Oct 21, 2024
24. rpc: Add support to populate PSBT input utxos via rpc

    This feature is useful when construction a series of
    presigned transactions that can not be entered into the
    mempool or UTXO set before signing everything.

    87ceb610a7
25. instagibbs force-pushed on Oct 21, 2024
26. DrahtBot removed the label CI failed on Oct 21, 2024
27. in src/rpc/util.cpp:1373 in 87ceb610a7

    1369@@ -1370,6 +1370,23 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
    1370     return ret;
    1371 }
    1372 
    1373+std::vector<CTransactionRef> ParseTransactionVector(const UniValue txns_param)
    

    ---

    ---

    

    rkrux commented at 3:27 am on October 22, 2024:

    Thanks, this pure function implementation is nice!
28. rkrux approved
29. 
    rkrux commented at 3:33 am on October 22, 2024: none

    tACK 87ceb610a72fda00e02f98426c6ee66b34fd6f54

    Rebuilt and reran functional tests.

30. DrahtBot requested review from pablomartin4btc on Oct 22, 2024
31. 
    rkrux commented at 7:39 am on October 22, 2024: none

    This is far easier to use compared to the earlier way of signrawtransactionwithwallet where few details need to be passed manually such as txid, vout, scriptPubKey, witnessScript, amount!

    0➜  bcli28 signrawtransactionwithwallet 0300000001f70daae6a5f540e7ff0216e443f6422200f13c2d3c57fa10ab0cff3f1f2abf3f0000000000fdffffff0120e20f2401000000160014e2370a8c1504f239194d158eb0b1450c932b56a900000000 "[{\"txid\":\"3fbf2a1f3fff0cab10fa573c2d3cf1002242f643e41602ffe740f5a5e6aa0df7\", \"vout\": 0, \"scriptPubKey\": \"0014e2370a8c1504f239194d158eb0b1450c932b56a9\", \"witnessScript\": \"035d276773f6e233b41aba595b63e1fa31c3c21b68ca58c4383bb658a3ead4d296\", \"amount\": 48.99989}]"
    1{
    2  "hex": "03000000000101f70daae6a5f540e7ff0216e443f6422200f13c2d3c57fa10ab0cff3f1f2abf3f0000000000fdffffff0120e20f2401000000160014e2370a8c1504f239194d158eb0b1450c932b56a9024730440220562a76cf2d701f61bd34a7e741523b1925dfa96b303df94ae7d25c45e186ac3f02207e420ee8a9b9c792b3c67bad8d108c0fc4f0a8d8605f66d61282115a63982d8c0121037dd7593d1344e3b8a13b95ab8cc320ee93466f2b558d2869a6d03361b251d65800000000",
    3  "complete": true
    4}
    

Contributors
instagibbs DrahtBot pablomartin4btc 1440000bytes BrandonOdiwuor rkrux

Review Requested
BrandonOdiwuor pablomartin4btc

Labels
RPC/REST/ZMQ