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

pull ishaanam wants to merge 2 commits into bitcoin:master from ishaanam:utxoupdatepsbt_with_txindex changing 5 files +133 −88
  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


ishaanam DrahtBot w0xlt achow101 sipa murchandamus

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: 2025-11-09 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me