Is there a reason we keep passing the spent_outputs instead of removing it (to simplify the API) and requiring callers to initialize txdata before calling CheckInputScripts?

My read is that the call to txdata.Init is delayed to after checking the script execution cache for performance reasons. Calling Init does some hashing, that would not be required if it is already in the cache. Moving the spent_outputs vector construction is already a potential performance downside for potential cache hits, though I think that is more acceptable in the grand scheme of things.

Oh, it makes sense. Thanks for the insights.

I think you can use AccessCoins here?

nit: inconsistent naming between GetUnspentOutputs and spent_outputs.

We could also use AccessCoin, but I think accessing it directly is a bit more efficient, because we don't allocate another vector. Do you have a better suggestion for the naming there? It is all a bit confusing.

I hadn't considered the extra vector allocation, yeah I'm okay leaving this as-is then.

I did nerd-snipe myself into looking into having AccessCoins return a view would be quite elegant in some ways, allowing lazy evaluation, but seems like it would require quite a bit of overhaul and might have other drawbacks I've not considered yet.

    auto AccessCoins(const CTransaction& tx) const
    {
        return tx.vin | std::views::transform([this](const CTxIn& input) -> const Coin& {
            return this->AccessCoin(input.prevout);
        });
    }

Do you have a better suggestion for the naming there? It is all a bit confusing.

I agree the naming is confusing. I'd suggest GetUnspentOutputs -> GetPreviousOutputs and spent_outputs -> previous_outputs to avoid the spent/unspent terminology. What do you think?

In a way, it's a bit silly to add a parameter to the CheckInputScripts signature to then just move it into another parameter (txdata). An alternative approach could be to let the caller handle this, and assert txdata.m_spent_outputs_ready, but that could raise runtime assertion errors instead of catching them at compile time.

As @TheCharlatan noted here, delaying txdata.Init could be on purpose for the potential cache hit here. I wonder if there is a cleaner version for achieving the same goal though.

In "consensus: Use Coin span in GetP2SHSigOpCount" 5709d07

nit: here and other places, these new variables should be snake case?

In "consensus: Use Coin span in GetP2SHSigOpCount" 5709d07

Here and other commits.
Sorted in what order?

Since we are doing this, I think this should also be separated into two functions.

1. EnforceBIP30 — a method that performs BIP30 validation.
2. SpendBlock — for each transaction in the block, checks that the outputs it is spending exist, spends them, and populates the CBlockUndo data structure.

It would also make testing easier, and removing BIP30 after a consensus cleanup soft fork would be straightforward.

What I will want to see is we have just SpendBlock and non-contextual block validation using the block data and undo data in a series of consensus rules validation functions.

In "validation: Add SpendBlock function" c937b12
why duplicate this, I think we should just add a comment warning that we expect they are already executed before?

I'm not sure what you mean here. Afaict this is moved over from ConnectBlock.

I think I'd rather keep these duplicated for now. These are short-circuits to our logic, similar to how the coinbase checks work in various places. Based on that I'd rather keep them in place. This could also easily be improved in a follow-up.

In "validation: Add SpendBlock function" c937b12

The goal is to move logic requiring access to the
CCoinsViewCache out of ConnectBlock and to the new SpendBlock method.
SpendBlock will in future handle all UTXO set interactions that
previously took place in ConnectBlock.

This is counter to the objective of the PR; we are still accessing the UTXO set in this block validation function.
My hope was that we would have two UTXO-accessing validation functions that 1 populate the block undo data and 2. BIP30 separately, while the remaining validation functions would simply get passed the block undo data and operate as stateless validation functions.

It is important that the BIP30 validation is done before we spend the coins. The current split makes sense to me for this reason. Can you elaborate a bit more what you think might not be optimal here?

I'm still not sure if this is a good thing. I don't see why you would ever spend the coins in the utxo set without also doing the bip30 checks beforehand. To this end, I did implement it, but added a tag type that gets passed from BIP30Validate to SpendBlock: https://github.com/sedited/bitcoin/tree/spendblock_17 . Let me know what you think.

I’ve reviewed the branch, and it looks good overall. My only concern is the last commit that adds the tag. Initially, I thought it should be the caller’s responsibility to decide whether bip30validate is called, rather than mandating it here.

My motivation for separating SpendBlock from BIP30Validate is discussed in:
stratum-mining/sv2-apps#120

The branch without the last commit would make it easier to implement a variant of the idea described in stratum-mining/sv2-apps#120, where block validation for this JDS is as follows call SpendBlock and then ConnectBlock skip BIP30. However, the idea of skipping consensus checks is not acceptable.

Given that, I’m inclined to keep this as-is.
For stratum-mining/sv2-apps#120, we can instead expose IPC methods: SpendBlock, which takes a block and returns the coins, and ValidateBlock, which performs the full block validation checks. Since blocks from miners tend to share many transactions, they can still benefit from this approach, because the JDS needs to only call SpendBlock to validate newly seen transactions.

For Utreexo client, they provide their own undo data and cannot perform BIP30 validation anyway, so they benefit from this approach as well.

Overall, this is still an improvement over the current status quo.