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.

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

rpc: add descriptorprocesspsbt rpc #25796

Merged
merged 2 commits into from May 22, 2023

Conversation

ishaanam
Copy link
Contributor

@ishaanam ishaanam commented Aug 6, 2022

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)

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 7, 2022

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.

@ishaanam
Copy link
Contributor Author

ishaanam commented Aug 7, 2022

This PR implements the last suggestion in #14739 (with a slightly modified name)

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
@jonatack
Copy link
Contributor

jonatack commented Aug 7, 2022

Concept ACK

A few suggestions for your consideration

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index b6715f7f20..1ccc267ec1 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -164,7 +164,7 @@ static std::vector<RPCArg> CreateTxDoc()
 
 // Update PSBT with information from the mempool, the UTXO set, and the provided descriptors.
 // Optionally sign the inputs which we can using information from the descriptors.
-PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const HidingSigningProvider provider, int sighash_type = 1, bool finalize = false)
+PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const HidingSigningProvider& provider, int sighash_type, bool finalize)
 {
     // Unserialize the transactions
     PartiallySignedTransaction psbtx;
@@ -179,11 +179,9 @@ PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const Hidi
     {
-        NodeContext& node = EnsureAnyNodeContext(request.context);
+        const NodeContext& node = EnsureAnyNodeContext(request.context);
         const CTxMemPool& mempool = EnsureMemPool(node);
-        ChainstateManager& chainman = EnsureChainman(node);
-        LOCK2(cs_main, mempool.cs);
-        CCoinsViewCache &viewChain = chainman.ActiveChainstate().CoinsTip();
-        CCoinsViewMemPool viewMempool(&viewChain, mempool);
-        view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view
+        LOCK2(::cs_main, mempool.cs);
+        CCoinsViewMemPool view_mempool{&EnsureChainman(node).ActiveChainstate().CoinsTip(), mempool};
+        view.SetBackend(view_mempool); // temporarily switch cache backend to db+mempool view
 
         for (const CTxIn& txin : psbtx.tx->vin) {
             view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail.
@@ -211,7 +209,7 @@ PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const Hidi
         }
     }
 
-    const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx);
+    const PrecomputedTransactionData& txdata = PrecomputePSBTData(psbtx);
 
     for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
         if (PSBTInputSigned(psbtx.inputs.at(i))) {
@@ -220,9 +218,9 @@ PartiallySignedTransaction ProcessPSBT(const JSONRPCRequest& request, const Hidi
 
         // Update script/keypath information using descriptor data.
         // Note that SignPSBTInput does a lot more than just constructing ECDSA signatures.
-        // We only actually care about those if our signing provider doesn't hide private 
+        // We only actually care about those if our signing provider doesn't hide private
         // information, as is the case with `descriptorprocesspsbt`
-        SignPSBTInput(provider, psbtx, i, &txdata, sighash_type, nullptr, finalize);
+        SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, sighash_type, /*out_sigdata=*/nullptr, finalize);
     }
 
     // Update script/keypath information using descriptor data.
@@ -1649,7 +1647,11 @@ static RPCHelpMan utxoupdatepsbt()
     }
 
     // We don't actually need private keys further on; hide them as a precaution.
-    PartiallySignedTransaction psbtx = ProcessPSBT(request, HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false));
+    const PartiallySignedTransaction& psbtx = ProcessPSBT(
+        request,
+        HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false),
+        /*sighash_type=*/1,
+        /*finalize=*/false);
 
     CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
     ssTx << psbtx;
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index d5693dc555..86311adfed 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -1087,7 +1087,7 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys: '%s'", desc_str));
         }
         if (expand_priv) {
-            desc->ExpandPrivate(i, provider, provider);
+            desc->ExpandPrivate(/*pos=*/i, provider, /*out=*/provider);
         }
         std::move(scripts.begin(), scripts.end(), std::back_inserter(ret));
     }
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index d6db6fbfc6..b7a2a4628f 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -581,7 +581,7 @@ class PSBTTest(BitcoinTestFramework):
                 break
         assert shuffled
 
-        # Test that we can update and sign a psbt with descriptors
+        self.log.info("Test descriptorprocesspsbt updates and signs a psbt with descriptors")
         key_info = get_generate_key()
         key1 = key_info[0]
         wpkh_addr = key_info[5]

@ishaanam
Copy link
Contributor Author

ishaanam commented Aug 7, 2022

Thanks for the review @jonatack, I've implemented you're suggestions

Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

Concept ACK

The first commit is a useful refactor in any case.

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
@ishaanam
Copy link
Contributor Author

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.

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

Approach ACK

@ishaanam
Copy link
Contributor Author

Rebased

@ishaanam ishaanam marked this pull request as ready for review April 22, 2023 02:24
@ishaanam
Copy link
Contributor Author

Now that #25939 has been merged, this is now ready for review.

@achow101
Copy link
Member

achow101 commented May 3, 2023

ACK 9bf2d30

@fanquake fanquake requested review from instagibbs and w0xlt May 4, 2023 09:14
Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

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

concept ACK, would appreciate test tighten-up for maintainability

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

// Check whether or not all of the inputs are now signed
bool complete = true;
for (const auto& input : psbtx.inputs) {
Copy link
Member

Choose a reason for hiding this comment

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

future work: someone make this a subroutine since this shows up in many places

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
@DrahtBot DrahtBot removed the request for review from w0xlt May 4, 2023 14:19
This RPC can be the Updater, Signer, and optionally the Input
Finalizer for a psbt, and has no interaction with the Bitcoin
Core wallet.
@instagibbs
Copy link
Member

ACK 80f8bdf

@DrahtBot DrahtBot requested a review from achow101 May 5, 2023 21:14
@achow101
Copy link
Member

re-ACK 1bce12a

@DrahtBot DrahtBot requested review from instagibbs and removed request for achow101 May 20, 2023 00:41
@instagibbs
Copy link
Member

reACK 1bce12a

@DrahtBot DrahtBot removed the request for review from instagibbs May 20, 2023 10:49
@achow101 achow101 merged commit 22139f6 into bitcoin:master May 22, 2023
16 checks passed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 23, 2023
1bce12a test: add test for `descriptorprocesspsbt` RPC (ishaanam)
fb2a3a7 rpc: add descriptorprocesspsbt rpc (ishaanam)

Pull request description:

  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 bitcoin#25796 (comment)

ACKs for top commit:
  achow101:
    re-ACK 1bce12a
  instagibbs:
    reACK bitcoin@1bce12a

Tree-SHA512: e1d0334739943e71f2ee68b4db7637ebe725da62e7aa4be071f71c7196d2a5970a31ece96d91e372d34454cde8509e95ab0eebd2c8edb94f7d5a781a84f8fc5d
@ishaanam ishaanam deleted the descriptorprocesspsbt branch November 30, 2023 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants