rpc: In utxoupdatepsbt also look for the tx in the txindex #25939

1. 
   ishaanam commented at 7:07 pm on August 26, 2022: contributor

   Previously the utxoupdatepsbt RPC, added in #13932, only updated the inputs spending segwit outputs with the witness_utxo, because the full transaction is needed for legacy inputs. Before this RPC looked for just the utxos in the utxo set and the mempool. This PR makes it so that if the user has txindex enabled, then the full transaction is looked for there for all inputs. If it is not found in the txindex or txindex isn’t enabled, then the mempool is checked for the full transaction. If the transaction an input is spending from is still not found at that point, then the utxo set is searched and the inputs spending segwit outputs are updated with just the utxo.
2. DrahtBot added the label RPC/REST/ZMQ on Aug 26, 2022
3. ishaanam force-pushed on Aug 26, 2022
4. ishaanam force-pushed on Aug 26, 2022
5. ishaanam force-pushed on Aug 27, 2022
6. 
   DrahtBot commented at 2:26 pm on August 27, 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, Xekyo
   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:

   • #25796 (rpc: add descriptorprocesspsbt rpc by ishaanam)
   • #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.

7. 
   w0xlt commented at 8:02 pm on August 29, 2022: contributor

   Approach ACK. If the txindex is enabled and updated, it should be used.
8. ishaanam force-pushed on Nov 6, 2022
9. in src/rpc/rawtransaction.cpp:216 in 687d6c20a4 outdated

   234+        FindCoins(node, coins);
   235+        for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
   236+            PSBTInput& input = psbtx.inputs.at(i);
   237+
   238+            // If there are still missing utxos, add them if they were found in the utxo set
   239+            if (!coins.empty() && !input.non_witness_utxo) {
   

   ---

   ---

   

   achow101 commented at 5:47 pm on November 10, 2022:

   In 687d6c20a4d6fc99cf4ae5794dd4203ea446a8f2 “rpc, tests: in utxoupdatepsbt also look for the transaction in the txindex”

   Checking !coins.empty() again is redundant. Also, we may still want to add the witness_utxo even if a non_witness_utxo exists.

   We add the different utxo types depending on the script type. For legacy scripts, only non_witness_utxo should be added. For Segwit v0, both non_witness_utxo and witness_utxo should be added, For Segwit v1+, only witness_utxo should be added. This RPC should (try) to adhere to these guidelines.

   ---

   

   ishaanam commented at 5:03 pm on November 12, 2022:

   Also, we may still want to add the witness_utxo even if a non_witness_utxo exists.

   If the non_witness_utxo exists and a segwit output is being spent, SignPSBTInput will add the witness_utxo.

   For Segwit v1+, only witness_utxo should be added.

   I have updated this commit to adhere to this, but this comment indicates something a bit different:

   We can remove the non_witness_utxo if and only if there are no non-segwit or segwit v0 inputs in this transaction.

   So does the non_witness_utxo need to be present for a Segwit v1 input if there is another non-segwit input int he transaction?

   ---

   

   sipa commented at 3:05 pm on November 13, 2022:

   I think if there is at least one witv0 or legacy input in a transaction, the non_witness_utxo record of all inputs should be kept.

   ---

   

   ishaanam commented at 11:25 pm on November 15, 2022:

   I think if there is at least one witv0 or legacy input in a transaction, the non_witness_utxo record of all inputs should be kept.

   This is also what walletprocesspsbt does, so I have update this PR to do that as well.

10. ishaanam force-pushed on Nov 12, 2022
11. ishaanam force-pushed on Nov 15, 2022
12. 
    achow101 commented at 9:56 pm on November 18, 2022: member

    ACK f26d600d55d86099206456f394a422436d5f3ec3
13. DrahtBot added the label Needs rebase on Jan 17, 2023
14. rpc: extract psbt updating logic into ProcessPSBT

    This function is called from utxoupdatepsbt and will be modified
    in a following commit to allow for updating inputs with the
    `non_witness_utxo` as well.

    a5b4883fb4
15. ishaanam force-pushed on Jan 19, 2023
16. DrahtBot removed the label Needs rebase on Jan 19, 2023
17. 
    ishaanam commented at 4:08 am on January 19, 2023: contributor

    Rebased
18. 
    achow101 commented at 9:02 pm on January 25, 2023: member

    ACK 2ee94290bdbb66732f420dc9d74493436fb2a59f
19. in src/psbt.h:1234 in 2ee94290bd outdated

    1230@@ -1231,6 +1231,9 @@ bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned
    1231  **/
    1232 bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash = SIGHASH_ALL, SignatureData* out_sigdata = nullptr, bool finalize = true);
    1233 
    1234+/** Removes the unnecessary non_witness_utxos from a psbt. */
    

    ---

    ---

    

    murchandamus commented at 6:07 pm on March 17, 2023:

    I see that you’re just moving this code, but could you add a little more explanation here? When I was reviewing this, I was first trying to find out what non_witness_utxos and witness_utxos were, when we needed them, and finally learned per the previous PR when we could drop them. My understanding is that we can only drop non_witness_utxos when all inputs are segwit v1.

    I would suspect that we might also be able to drop witness_utxos on non-segwit inputs when we do have the corresponding non_witness_utxos, and vice versa, drop the non_witness_utxos on segwit inputs, if we do have the witness_utxos for them. Is that accurate?

    0/** Reduces the size of the PSBT by dropping unnecessary `non_witness_utxos` (i.e. previous transactions) from a psbt when all inputs are segwit v1. */
    

    ---

    

    ishaanam commented at 1:15 am on March 19, 2023:

    I see that you’re just moving this code, but could you add a little more explanation here?

    Done as per your suggestion above.

    I would suspect that we might also be able to drop witness_utxos on non-segwit inputs when we do have the corresponding non_witness_utxos,

    I don’t think witness_utxos should ever get added to non-segwit inputs.

    and vice versa, drop the non_witness_utxos on segwit inputs, if we do have the witness_utxos for them.

    Currently I’m not sure, though in #25939 (review) sipa said that in this situation all non_witness_utxos should be kept. Even if this is not the case, changing this behavior would be out of the scope of this PR.

20. in src/rpc/rawtransaction.cpp:183 in 2ee94290bd outdated

    193-
    194-        for (const CTxIn& txin : psbtx.tx->vin) {
    195-            view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail.
    196-        }
    197+    // Fetch previous transactions:
    198+    // First, look in the txindex and the mempool
    

    ---

    ---

    

    murchandamus commented at 6:13 pm on March 17, 2023:

    It seems to me that this comment might fit better before line 192.

    ---

    

    ishaanam commented at 0:59 am on March 19, 2023:

    Yes, I’ve moved it.
21. in src/rpc/rawtransaction.cpp:197 in 2ee94290bd outdated

    213+        PSBTInput& psbt_input = psbtx.inputs.at(i);
    214+        const CTxIn& tx_in = psbtx.tx->vin.at(i);
    215 
    216-        if (input.non_witness_utxo || !input.witness_utxo.IsNull()) {
    217-            continue;
    218+        if (psbt_input.non_witness_utxo) continue;
    

    ---

    ---

    

    murchandamus commented at 6:16 pm on March 17, 2023:

    0        // The `non_witness_utxo` is the whole previous transaction
    1        if (psbt_input.non_witness_utxo) continue;
    

    ---

    

    ishaanam commented at 0:59 am on March 19, 2023:

    Done
22. 
    murchandamus commented at 6:26 pm on March 17, 2023: contributor

    ACK 2ee94290bdbb66732f420dc9d74493436fb2a59f
23. rpc, tests: in `utxoupdatepsbt` also look for the transaction in the txindex

    Previously only the segwit utxos being spent by the psbt were looked for and
    added to the psbt. Now, the full transaction corresponding to each of these
    utxos (legacy and segwit) is looked for in the txindex and mempool and added
    to the psbt. If txindex is disabled and the transaction is not in the mempool,
    then we fall back to getting just the utxo (if segwit) from the utxo set.

    6e9f8bb050
24. ishaanam force-pushed on Mar 19, 2023
25. 
    achow101 commented at 4:51 pm on March 20, 2023: member

    ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98
26. DrahtBot requested review from murchandamus on Mar 20, 2023
27. 
    murchandamus commented at 8:32 pm on April 7, 2023: contributor

    ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98
28. DrahtBot removed review request from murchandamus on Apr 7, 2023
29. achow101 merged this on Apr 21, 2023
30. achow101 closed this on Apr 21, 2023

    ---

31. sidhujag referenced this in commit 2bdc7af7b2 on Apr 21, 2023
32. ishaanam deleted the branch on Nov 30, 2023
33. bitcoin locked this on Nov 29, 2024

Contributors
ishaanam DrahtBot w0xlt achow101 sipa murchandamus

Labels
RPC/REST/ZMQ