rpc: add descriptorprocesspsbt rpc #25796

pull ishaanam wants to merge 2 commits into bitcoin:master from ishaanam:descriptorprocesspsbt changing 6 files +142 −9
  1. ishaanam commented at 10:27 pm on August 6, 2022: contributor
    This PR implements an RPC called descriptorprocesspsbt. This RPC is based off of walletprocesspsbt, but instead of interacting with the wallet to update, sign and finalize a psbt, it instead accepts an array of output descriptors and uses that information along with information from the mempool, txindex, and the utxo set to do so. utxoupdatepsbt also updates a psbt in this manner, but doesn’t sign or finalize it. Because of this overlap, a helper function that is added in this PR is called by both utxoupdatepsbt and descriptorprocesspsbt. Whether or not the helper function signs a psbt is dictated by if the HidingSigningProvider passed to it contains any private information. There is also a test added in this PR for this new RPC that uses p2wsh, p2wpkh, and legacy outputs. Edit: see #25796 (comment)
  2. fanquake added the label RPC/REST/ZMQ on Aug 6, 2022
  3. ishaanam force-pushed on Aug 7, 2022
  4. DrahtBot commented at 9:35 am on August 7, 2022: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK achow101, instagibbs
    Concept ACK jonatack, Sjors
    Approach ACK w0xlt

    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:

    • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

  5. ishaanam commented at 2:40 pm on August 7, 2022: contributor
    This PR implements the last suggestion in #14739 (with a slightly modified name)
  6. in src/rpc/rawtransaction.cpp:204 in 0061b6d8bf outdated 
    201+        }
202+
203+        const Coin& coin = view.AccessCoin(psbtx.tx->vin[i].prevout);
204+
205+        if (coin.IsSpent()) {
206+            throw JSONRPCError(RPC_VERIFY_ERROR, "Input not found or already spent");
    ishaanam commented at 2:52 pm on August 7, 2022:
    This changes the behavior of utxoupdatepsbt since previously it didn’t throw an error if an input has already been spent or doesn’t exist. I’m not sure if the lack of an error here when utxoupdatepsbt was initially implemented was intentional. If it was intentional, then I will remove this.
    Sjors commented at 1:21 pm on August 8, 2022:

    b7c6179db597d00b465b4e83047af56292e372a6 Not sure what the original intention was, @achow101? Perhaps it’s useful to be able to process a PSBT on a node that’s behind (knows about its own cold storage UTXO, but not about others)?

    In any case it should probably be its own commit (along with the test you added in the next commit)

    achow101 commented at 6:30 pm on August 8, 2022:
    I don’t remember if it was intentional, however a PSBT could spend UTXOs that do not exist yet (e.g. not-yet-broadcast transactions) so I think it would still be correct to allow UTXOs that do not exist.
    ishaanam commented at 7:45 pm on August 8, 2022:
    Makes sense, in that case I’ll remove this.
  7. jonatack commented at 4:36 pm on August 7, 2022: contributor

    Concept ACK

     0diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
 1index b6715f7f20..1ccc267ec1 100644
 2--- a/src/rpc/rawtransaction.cpp
 3+++ b/src/rpc/rawtransaction.cpp
 4@@ -164,7 +164,7 @@ static std::vector<RPCArg> CreateTxDoc()
 5 
 6 // Update PSBT with information from the mempool, the UTXO set, and the provided descriptors.
 7 // Optionally sign the inputs which we can using information from the descriptors.
 8-PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const HidingSigningProvider provider, int sighash_type = 1, bool finalize = false)
 9+PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const HidingSigningProvider& provider, int sighash_type, bool finalize)
10 {
11     // Unserialize the transactions
12     PartiallySignedTransaction psbtx;
13@@ -179,11 +179,9 @@ PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const Hidi
14     {
15-        NodeContext& node = EnsureAnyNodeContext(request.context);
16+        const NodeContext& node = EnsureAnyNodeContext(request.context);
17         const CTxMemPool& mempool = EnsureMemPool(node);
18-        ChainstateManager& chainman = EnsureChainman(node);
19-        LOCK2(cs_main, mempool.cs);
20-        CCoinsViewCache &viewChain = chainman.ActiveChainstate().CoinsTip();
21-        CCoinsViewMemPool viewMempool(&viewChain, mempool);
22-        view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view
23+        LOCK2(::cs_main, mempool.cs);
24+        CCoinsViewMemPool view_mempool{&EnsureChainman(node).ActiveChainstate().CoinsTip(), mempool};
25+        view.SetBackend(view_mempool); // temporarily switch cache backend to db+mempool view
26 
27         for (const CTxIn& txin : psbtx.tx->vin) {
28             view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail.
29@@ -211,7 +209,7 @@ PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const Hidi
30         }
31     }
32 
33-    const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx);
34+    const PrecomputedTransactionData& txdata = PrecomputePSBTData(psbtx);
35 
36     for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
37         if (PSBTInputSigned(psbtx.inputs.at(i))) {
38@@ -220,9 +218,9 @@ PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const Hidi
39 
40         // Update script/keypath information using descriptor data.
41         // Note that SignPSBTInput does a lot more than just constructing ECDSA signatures.
42-        // We only actually care about those if our signing provider doesn't hide private 
43+        // We only actually care about those if our signing provider doesn't hide private
44         // information, as is the case with `descriptorprocesspsbt`
45-        SignPSBTInput(provider, psbtx, i, &txdata, sighash_type, nullptr, finalize);
46+        SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, sighash_type, /*out_sigdata=*/nullptr, finalize);
47     }
48 
49     // Update script/keypath information using descriptor data.
50@@ -1649,7 +1647,11 @@ static RPCHelpMan utxoupdatepsbt()
51     }
52 
53     // We don't actually need private keys further on; hide them as a precaution.
54-    PartiallySignedTransaction psbtx = ProcessPSBT(request, HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false));
55+    const PartiallySignedTransaction& psbtx = ProcessPSBT(
56+        request,
57+        HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false),
58+        /*sighash_type=*/1,
59+        /*finalize=*/false);
60 
61     CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
62     ssTx << psbtx;
63diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
64index d5693dc555..86311adfed 100644
65--- a/src/rpc/util.cpp
66+++ b/src/rpc/util.cpp
67@@ -1087,7 +1087,7 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
68             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys: '%s'", desc_str));
69         }
70         if (expand_priv) {
71-            desc->ExpandPrivate(i, provider, provider);
72+            desc->ExpandPrivate(/*pos=*/i, provider, /*out=*/provider);
73         }
74         std::move(scripts.begin(), scripts.end(), std::back_inserter(ret));
75     }
76diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
77index d6db6fbfc6..b7a2a4628f 100755
78--- a/test/functional/rpc_psbt.py
79+++ b/test/functional/rpc_psbt.py
80@@ -581,7 +581,7 @@ class PSBTTest(BitcoinTestFramework):
81                 break
82         assert shuffled
83 
84-        # Test that we can update and sign a psbt with descriptors
85+        self.log.info("Test descriptorprocesspsbt updates and signs a psbt with descriptors")
86         key_info = get_generate_key()
87         key1 = key_info[0]
88         wpkh_addr = key_info[5]
  8. ishaanam force-pushed on Aug 7, 2022
  9. ishaanam commented at 11:39 pm on August 7, 2022: contributor
    Thanks for the review @jonatack, I’ve implemented you’re suggestions
  10. Sjors commented at 1:36 pm on August 8, 2022: member

    Concept ACK

    The first commit is a useful refactor in any case.

  11. in src/rpc/rawtransaction.cpp:166 in 58f265f4de outdated 
    161@@ -162,6 +162,61 @@ static std::vector<RPCArg> CreateTxDoc()
162     };
163 }
164 
165+// Update PSBT with information from the mempool, the UTXO set, and the provided descriptors
166+PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const HidingSigningProvider provider)
    achow101 commented at 6:26 pm on August 8, 2022:

    In 58f265f4de0051bbecb0f7e367a39258b24d5a40 “rpc: extract psbt updating logic into ProcessPSBT()”

    I think it would be better to give the UniValue for the PSBT and a NodeContext rather than passing in the entire JSONRPCRequest.

    ishaanam commented at 7:51 pm on August 8, 2022:
    Done
  12. in src/rpc/rawtransaction.cpp:1925 in b7c6179db5 outdated 
    1920+    auto descs = request.params[1].get_array();
1921+    for (size_t i = 0; i < descs.size(); ++i) {
1922+        EvalDescriptorStringOrObject(descs[i], provider, /*expand_priv=*/true);
1923+    }
1924+
1925+    int nHashType = ParseSighashString(request.params[2]);
    achow101 commented at 6:27 pm on August 8, 2022:

    In b7c6179db597d00b465b4e83047af56292e372a6 “rpc: add descriptorprocesspsbt rpc”

    nit: snake_case

    ishaanam commented at 7:50 pm on August 8, 2022:
    Done
  13. ishaanam force-pushed on Aug 8, 2022
  14. ishaanam force-pushed on Aug 26, 2022
  15. ishaanam marked this as a draft on Aug 26, 2022
  16. ishaanam commented at 7:14 pm on August 26, 2022: contributor
    While working on this PR, I noticed that it might be useful for utxoupdatepbst to also look in the txindex (if enabled) for the full tx that an input is spending from, which would allow legacy inputs to be updated as well. I have opened a separate PR for this, #25939, and this PR builds on top of that one so that descriptorprocesspsbt can do this as well.
  17. ishaanam force-pushed on Aug 26, 2022
  18. ishaanam force-pushed on Aug 26, 2022
  19. in src/rpc/rawtransaction.cpp:238 in 2b67f9b731 outdated 
    223+                }
224+            }
225+        }
226+    }
227+
228+    const PrecomputedTransactionData& txdata = PrecomputePSBTData(psbtx);
    achow101 commented at 7:33 pm on October 31, 2022:

    In 2b67f9b73143e07a30add5f372395a612762da46 “rpc: add descriptorprocesspsbt rpc”

    Seems like this should be in the previous commit.

    ishaanam commented at 5:05 pm on November 6, 2022:
    I’ve moved this to the previous commit.
  20. w0xlt commented at 7:46 pm on October 31, 2022: contributor
    Approach ACK
  21. ishaanam force-pushed on Nov 6, 2022
  22. ishaanam force-pushed on Nov 6, 2022
  23. ishaanam force-pushed on Dec 4, 2022
  24. DrahtBot added the label Needs rebase on Jan 17, 2023
  25. ishaanam force-pushed on Jan 19, 2023
  26. ishaanam commented at 4:09 am on January 19, 2023: contributor
    Rebased
  27. DrahtBot removed the label Needs rebase on Jan 19, 2023
  28. ishaanam force-pushed on Jan 23, 2023
  29. DrahtBot added the label Needs rebase on Apr 21, 2023
  30. ishaanam force-pushed on Apr 22, 2023
  31. ishaanam marked this as ready for review on Apr 22, 2023
  32. ishaanam commented at 2:25 am on April 22, 2023: contributor
    Now that #25939 has been merged, this is now ready for review.
  33. DrahtBot removed the label Needs rebase on Apr 22, 2023
  34. achow101 commented at 9:30 pm on May 3, 2023: member
    ACK 9bf2d303295594b8327ce0d67b9c1de98701f80a
  35. fanquake requested review from instagibbs on May 4, 2023
  36. fanquake requested review from w0xlt on May 4, 2023
  37. in src/rpc/rawtransaction.cpp:1703 in efc62d61e6 outdated 
    1698@@ -1697,7 +1699,9 @@ static RPCHelpMan utxoupdatepsbt()
1699     const PartiallySignedTransaction& psbtx = ProcessPSBT(
1700         request.params[0].get_str(),
1701         request.context,
1702-        HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false));
1703+        HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false),
1704+        /*sighash_type=*/1,
    instagibbs commented at 1:43 pm on May 4, 2023:
    use SIGHASH_ALL for clarity
    ishaanam commented at 3:24 pm on May 5, 2023:
    Done
  38. in src/rpc/rawtransaction.cpp:1982 in efc62d61e6 outdated 
    1977+        sighash_type,
1978+        finalize);
1979+
1980+    // Check whether or not all of the inputs are now signed
1981+    bool complete = true;
1982+    for (const auto& input : psbtx.inputs) {
    instagibbs commented at 1:45 pm on May 4, 2023:
    future work: someone make this a subroutine since this shows up in many places
  39. in test/functional/rpc_psbt.py:668 in 9bf2d30329 outdated 
    662@@ -660,6 +663,55 @@ def test_psbt_input_keys(psbt_input, keys):
663                 break
664         assert shuffled
665 
666+        self.log.info("Test descriptorprocesspsbt updates and signs a psbt with descriptors")
667+        key_info = get_generate_key()
668+        key1 = key_info[0]
    instagibbs commented at 2:02 pm on May 4, 2023:
    key_info.privkey?
    ishaanam commented at 3:24 pm on May 5, 2023:
    Done
  40. in test/functional/rpc_psbt.py:669 in 9bf2d30329 outdated 
    662@@ -660,6 +663,55 @@ def test_psbt_input_keys(psbt_input, keys):
663                 break
664         assert shuffled
665 
666+        self.log.info("Test descriptorprocesspsbt updates and signs a psbt with descriptors")
667+        key_info = get_generate_key()
668+        key1 = key_info[0]
669+        wpkh_addr = key_info[5]
    instagibbs commented at 2:02 pm on May 4, 2023:
    key_info.p2wpkh_addr?
    ishaanam commented at 3:24 pm on May 5, 2023:
    Done
  41. in test/functional/rpc_psbt.py:675 in 9bf2d30329 outdated 
    670+        descriptor1 = descsum_create(f"wpkh({key1})")
671+
672+        a_key_info = get_generate_key()
673+        b_key_info = get_generate_key()
674+        c_key_info = get_generate_key()
675+        a_key = a_key_info[0]
    instagibbs commented at 2:05 pm on May 4, 2023:
    use named tuple args where you can
  42. in test/functional/rpc_psbt.py:681 in 9bf2d30329 outdated 
    676+        a_pubkey = a_key_info[1]
677+        b_key = b_key_info[0]
678+        b_pubkey = b_key_info[1]
679+        c_pubkey = c_key_info[1]
680+
681+        descriptor_a = descsum_create(f"wsh(multi(2,{a_key},{b_pubkey},{c_pubkey}))")
    instagibbs commented at 2:06 pm on May 4, 2023:

    Can you leave comments on each step to make it clear what’s happening? It’s been a few minutes of staring and I’m not quite sure what’s exactly being tested here, making it difficult to update in the future.

    My basic test flow would probably follow the lines of:

    1. pick a node that doesn’t have a wallet
    2. make a single wpkh utxo
    3. make single input psbt with that utxo
    4. make sure it signs it when expected, and finalizes when expected (e.g., give it a descriptor unrelated to the utxo, partial_signature lying around when expected, vary sighash modes)
    5. make sure extracted tx submits to mempool ok

    imo we don’t need to re-test all of the descriptor logic that’s internal and hopefully already tested; we just want to check that the API here makes sense and pipes through. The 2-of-3 multisig, multi-input stuff just seems like a distraction.

    ishaanam commented at 3:28 pm on May 5, 2023:
    I’ve rewritten the test to reflect this. I’ve also added more comments.
    instagibbs commented at 3:35 pm on May 5, 2023:
    great, thanks!
  43. instagibbs commented at 2:19 pm on May 4, 2023: member
    concept ACK, would appreciate test tighten-up for maintainability
  44. DrahtBot removed review request from w0xlt on May 4, 2023
  45. rpc: add descriptorprocesspsbt rpc 
    This RPC can be the Updater, Signer, and optionally the Input
Finalizer for a psbt, and has no interaction with the Bitcoin
Core wallet.
    fb2a3a70e8
  46. ishaanam force-pushed on May 5, 2023
  48. DrahtBot requested review from achow101 on May 5, 2023
  49. fanquake assigned achow101 on May 5, 2023
  50. DrahtBot added the label CI failed on May 5, 2023
  51. in test/functional/rpc_psbt.py:966 in 80f8bdfeb3 outdated 
    959+        address = key_info.p2wpkh_addr
960+
961+        descriptor = descsum_create(f"wpkh({key})")
962+
963+        txid = self.nodes[0].sendtoaddress(address, 1)
964+        self.sync_all()
    achow101 commented at 9:14 pm on May 5, 2023:

    In 80f8bdfeb3d9301e0aec4b797689cbe569eff5a6 “test: add test for descriptorprocesspsbt RPC”

    This sync is failing due to an earlier unconfirmed transaction not persisting after the restart. A possible fix is to generate a block before restarting ndoe 2.

    ishaanam commented at 3:28 pm on May 6, 2023:
    Thanks, I’ve fixed it.
  52. DrahtBot requested review from achow101 on May 5, 2023
  53. test: add test for `descriptorprocesspsbt` RPC 1bce12acd3
  54. ishaanam force-pushed on May 6, 2023
  55. DrahtBot removed the label CI failed on May 6, 2023
  56. achow101 commented at 0:41 am on May 20, 2023: member
    re-ACK 1bce12acd3e271a7c88d9400b4e3a5645bc8a911
  57. DrahtBot removed review request from achow101 on May 20, 2023
  58. DrahtBot requested review from instagibbs on May 20, 2023
  60. DrahtBot removed review request from instagibbs on May 20, 2023
  61. achow101 merged this on May 22, 2023
  62. achow101 closed this on May 22, 2023
  63. sidhujag referenced this in commit a17baa132b on May 23, 2023
  64. ishaanam deleted the branch on Nov 30, 2023


ishaanam DrahtBot Sjors achow101 jonatack w0xlt instagibbs

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me