Return tx validation errors, fix max script size

[JoseSK999]

Proposing these changes as the BlockRules PR was closed.

• Added a few ? when needed.
• Corrected the non-taproot max script size to 10,000 bytes. validate_script_size now takes a taproot_spend bool.

Then also fix some error return cases:

• Check if the first tx is coinbase and return FirstTxIsNotCoinbase if it is not. verify_coinbase was previously only called if transaction.is_coinbase() && n == 0 and the not coinbase case was not handled.
• Previous get_out_value returned error when output had 0 sats value, but this is valid under the consensus rules. Removed it.
• consume_utxos changed to get_utxo: the utxos are already consumed by the verify_with_flags method, since it takes a utxos.remove(outpoint) closure.

[jaoleal]

Nice, the code is much cleaner!

[Davidson-Souza]

concept ACK

I'll add some review later, but there's one thing that's boggling me:

We have two kinds of script size limits: scriptPubkey size and scriptSig/Witness Script/Tapscript sizes. The current version of validate_script_size only checks for the former, not the latter. scriptpubkeys should always be <10_000 bytes, or they are unspendable. Script Sigs and alike have more complicated rules:

• P2SH: The script (which is a stack push) should not be gt than 512 bytes (without the OP_PUSH)
• P2WSH: The script (the first witness element) should not be gt than 10k bytes
• P2TR: The script (also a witness element) have no limits w.r.t. size

I think (TODO: check) that libbitcoinconsensus will error if the script sig is too big, not sure about scriptpubkey. We can't use the some code to check both, tho

[JoseSK999]

I have updated it with @jaoleal suggestions

[jaoleal]

Surely this was my mistake when i did this code... is_taproot doesnt acutally check if its taproot at all... just checks if the transaction is using witness and doesnt validate taproot & witness transactions...

Since 10k bytes(for p2wsh) was something so rare to happen and so expensive to do that i believed that this would never really happen...

This version of the function should run fine

    /// Validates the script size and the number of sigops in a script.
    fn validate_script_size(script: &ScriptBuf) -> Result<(), BlockValidationErrors> {
        if script.count_sigops() > 80_000 {
            return Err(BlockValidationErrors::ScriptError);
        }
        if script.is_p2tr() {
            // We return early because Taproot scripts has unlimited sizes.
            return Ok(());
        }

        let scriptpubkeysize = script.len();
        let is_witness =
            script.witness_version() == Some(WitnessVersion::V1) && scriptpubkeysize == 32;
        
        if scriptpubkeysize > 520 || scriptpubkeysize < 2 && !is_witness {
            //the scriptsig size must be between 2 and 100 bytes unless is taproot
            return Err(BlockValidationErrors::ScriptError);
        }
        if scriptpubkeysize >= 10_000 && is_witness {
            return Err(BlockValidationErrors::ScriptError);
        }

        Ok(())
    }

EDIT

We have two kinds of script size limits: scriptPubkey size and scriptSig/Witness Script/Tapscript sizes.

Actually, if we want to do this type of things like bitcoin core, the function would be much simpler:

    const fn validate_script_size(script: &ScriptBuf) -> bool {
        !(script.len() > 10_000)
    }

The input being the whole script(e.g. scriptsig + scriptpubkey)

source: https://github.com/bitcoin/bitcoin/blob/110183746150428e6385880c79f8c5733b1361ba/src/script/script.h#L38C1-L39C42

https://github.com/bitcoin/bitcoin/blob/110183746150428e6385880c79f8c5733b1361ba/src/script/interpreter.cpp#L427

This is bitcoin 28

[JoseSK999]

is_p2tr will only check if this is a taproot output (scriptPubKey), the unbounded size applies to the script spending such output, which is why I use the taproot_spend bool.

I think (TODO: check) that libbitcoinconsensus will error if the script sig is too big, not sure about scriptpubkey.

@Davidson-Souza actually it makes total sense that we may not need to perform the size check manually if we use the script validation logic from rust-bitcoinconsensus (including the 520 bytes max pushable size for the P2SH redeem script you mentioned). And the same for the sigops check.

[Davidson-Souza]

@Davidson-Souza actually it makes total sense that we may not need to perform the size check manually if we use the script validation logic from rust-bitcoinconsensus (including the 520 bytes max pushable size for the P2SH redeem script you mentioned). And the same for the sigops check.

Yeah, it does perform most of the checks. One disadvantage of deferring everything to libbitcoinconsensus is that for assume-valid blocks we won't check this, so we'll check less stuff than core for those blocks.

[Davidson-Souza]

I'm creating some transactions we can use to test script sizes and limits (https://gist.github.com/Davidson-Souza/5c5cf707704ffdb20c65c0e230f4392b)

[JoseSK999]

Ok so the idea is to have the basic script resource and size limits implemented for assume-valid.

[Davidson-Souza]

Ok so the idea is to have the basic script resource and size limits implemented for assume-valid.

Yes, I think it's valuable to have those implemented here. Otherwise, we wouldn't check script constraints on assume-valid mode.

That can be another PR, tho.

[jaoleal]

IMO we should only check if its less than 10_000 like core does... thoughts about delegating this type of check made me think: "If the script has invalid size its because its invalid at all, so the script validation will fail anyway"

[JoseSK999]

Rebased

[Davidson-Souza]

Concept ACK. I'm just not sure about the script size thing, I think we should make one function for witness sizes (not here, tho) and remove this is_taproot flag, see my comment bellow for an explanation

[JoseSK999]

Removed the taproot_spend argument as suggested and added a TODO comment for the witness size function.

The future witness function may actually need the taproot_spend flag or similar as it needs to skip the limit for taproot witnesses but not for P2WPKH/P2WSH spends.

[Davidson-Souza]

ACK fad23c9061830197c18380e66bd0a63c0d46e11