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

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.

DrahtBot added the label RPC/REST/ZMQ on Aug 26, 2022

ishaanam force-pushed on Aug 26, 2022

ishaanam force-pushed on Aug 27, 2022

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.

w0xlt commented at 8:02 PM on August 29, 2022: contributor

Approach ACK. If the txindex is enabled and updated, it should be used.

ishaanam force-pushed on Nov 6, 2022

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.

ishaanam force-pushed on Nov 12, 2022

ishaanam force-pushed on Nov 15, 2022

achow101 commented at 9:56 PM on November 18, 2022: member

ACK f26d600d55d86099206456f394a422436d5f3ec3

DrahtBot added the label Needs rebase on Jan 17, 2023

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

ishaanam force-pushed on Jan 19, 2023

DrahtBot removed the label Needs rebase on Jan 19, 2023

ishaanam commented at 4:08 AM on January 19, 2023: contributor

Rebased

achow101 commented at 9:02 PM on January 25, 2023: member

ACK 2ee94290bdbb66732f420dc9d74493436fb2a59f

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?

/** 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.

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 12:59 AM on March 19, 2023:

Yes, I've moved it.

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:

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

ishaanam commented at 12:59 AM on March 19, 2023:

Done

murchandamus commented at 6:26 PM on March 17, 2023: contributor

ACK 2ee94290bdbb66732f420dc9d74493436fb2a59f

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

ishaanam force-pushed on Mar 19, 2023

achow101 commented at 4:51 PM on March 20, 2023: member

ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98

DrahtBot requested review from murchandamus on Mar 20, 2023

murchandamus commented at 8:32 PM on April 7, 2023: contributor

ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98

DrahtBot removed review request from murchandamus on Apr 7, 2023

achow101 merged this on Apr 21, 2023

achow101 closed this on Apr 21, 2023

sidhujag referenced this in commit 2bdc7af7b2 on Apr 21, 2023

ishaanam deleted the branch on Nov 30, 2023

bitcoin locked this on Nov 29, 2024