core: Add PrecomputedTransactionData

[sedited]

No description provided.

[joshdoman]

Nice! Do you think it makes sense to make PrecomputedTransactionData an Option in the verify API? That way users verifying a single non-taproot input can just pass in None.

Something like:

pub fn verify(
    script_pubkey: &impl ScriptPubkeyExt,
    amount: Option<i64>,
    tx_to: &impl TransactionExt,
    input_index: usize,
    flags: Option<u32>,
    precomputed_txdata: Option<&PrecomputedTransactionData>,
) -> Result<(), KernelError>

And then:

let ret = unsafe {
    btck_script_pubkey_verify(
        script_pubkey.as_ptr(),
        kernel_amount,
        tx_to.as_ptr(),
        precomputed_txdata.map_or(std::ptr::null(), |data| data.as_ptr()),
        input_index as u32,
        kernel_flags,
        &mut status,
    )
};

[sedited]

I implemented that at first, but now I'm not sure anymore. We should guide the user towards doing the correct thing, which in most situations is computing the hashes ahead of time. Also taking an option of a borrow seems a bit icky. They can always choose to not supply the outputs to the precomputed data.

[joshdoman]

That's reasonable. It is a cleaner API the way you have it, and I like the idea of guiding users toward best practices.

[joshdoman]

Successfully ran tests locally. LGTM.

[alexanderwiederin]

Looks good! Thanks for the patience.

[alexanderwiederin]

An empty slice should be passed for non-taproot transactions.

[sedited]

I think may is the correct verb here. The cost of doing so nevertheless is marginal (and something we do every time during validation in Bitcoin Core's internal usage of it).

[sedited]

Thanks for the review @alexanderwiederin, pushed to address your comments, safe the doc change I commented on.

[alexanderwiederin]

Thanks!

[alexanderwiederin]

From what I understand, this assumes that TxOut (i.e. the rust wrapper) has the same layout as *const btck_TransactionOutput (the raw pointer), which happens to be true because TxOut holds only one field. I think it would be safer to explicitly build a vector of pointers like we did for kernel_spent_outputs in the previous verify function.

Alternatively, we could add #[repr(transparent)] to the TxOut type for this to be truly safe.

Any thoughts?

[sedited]

Nice catch! I agree, we should always be collecting these explicitly instead of relying on somewhat coincidental alignment. We should also really expand the number of script test cases. Would be good to have a passing and failing example for each of the common transaction types, as well checking for multiple inputs.

[alexanderwiederin]

ACK

nit: move doc changes and null check into 9accb6c