Return TxId when error in checking of transactions

[AurelienFT]

When we try to turn a Tx in a Checked<Tx>, errors can happens, but we always start by computing the TxId and this can never fails. If something else, fails afterwards it's super useful to have the TxId to link the error with the transaction associated.

Currently, in the existing code calling into_checked(), if we have an error we are computing the TxId ourself again to link it to the CheckError which creates a double computation.

Breaking change

The TxId is now returned with the error.

Before :

fn precompute(&mut self, chain_id: &ChainId) -> Result<(), ValidityError>;

Now :

fn precompute(&mut self, chain_id: &ChainId) -> Result<(), (TxId, ValidityError)>;

Breaking change notes

I didn't placed this behind a feature flag because I think it's always valuable, however if we want to avoid breaking for now and have this information under a special feature I'm ok with this. Inputs from reviewers would be super valuable on that.

Checklist

• Breaking changes are clearly marked as such in the PR description and changelog
• New behavior is reflected in tests
• If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
• The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

• I have reviewed the code myself
• I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

• Rust SDK : Might be impacted by the change on precompute : https://github.com/FuelLabs/fuels-rs/blob/66b3f0f28817b55b5ff375d5ded1a5b5b400b7ed/packages/fuels-core/src/types/wrappers/transaction.rs#L458

[Copilot]

Copilot reviewed 5 out of 11 changed files in this pull request and generated no comments.

Files not reviewed (6)

• fuel-tx/src/transaction/types/blob.rs: Evaluated as low risk
• fuel-tx/src/transaction/types/create.rs: Evaluated as low risk
• fuel-tx/src/transaction/types/mint.rs: Evaluated as low risk
• fuel-tx/src/transaction/types/script.rs: Evaluated as low risk
• fuel-tx/src/transaction/types/upload.rs: Evaluated as low risk
• fuel-vm/src/checked_transaction.rs: Evaluated as low risk

Comments suppressed due to low confidence (3)

fuel-tx/src/transaction/types/upgrade.rs:16

• The error should return (TxId, ValidityError) instead of just ValidityError to match the new error handling pattern.

Err(ValidityError::TransactionUpgradeConsensusParametersChecksumMismatch)?;

fuel-vm/src/tests/upload.rs:280

• The error message should be updated to reflect the new return type, ensuring clarity and consistency.

.expect_err("Should fail to generate checked tx")

fuel-vm/src/tests/upload.rs:307

• The error message should be updated to reflect the new return type, ensuring clarity and consistency.

.expect_err("Should fail to generate checked tx")

[xgreenx]

It would be nice to make this change non breaking. I tried to play with generics, but it still produces breaking code in some cases: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5f70213c55d0cf6e17abf2cd3e5f17fe

Alternative solution will be to add one more function like into_checked_basic_with_metadata and this one will return metadata in the case of error.

[AurelienFT]

I improved your playground code to be more close to reality and I have something that is breaking only because you need to specify the error type. I don't know if we can still consider this breaking I think so but I would like to have your thoughts. Link : https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d256580f356615cd7ff3979bcd2e3706

[xgreenx]

Yeah, it is why I'm saying it is not good solution because it is still breaking=D So because of that I'm more in favor of the into_checked_basic_with_metadata

[AurelienFT]

Yeah, it is why I'm saying it is not good solution because it is still breaking=D So because of that I'm more in favor of the into_checked_basic_with_metadata

Okey will do that but it will be into_checked_basic_with_id because it's only the id that we have the metadata creation is finished after some error could have been triggered
and precdompute will still breaks, but it's fine i think

[xgreenx]

I just got an idea of how we can do it better. Instead of adding new methods, we can add a new type Immutable<Tx>, where Immutable only calculates the metadata but doesn't have any validation logic. Any Immutable can be converted into checked without calculation of the metadata.

In this case, you can have an id before the validation, and for actual validation, you can leave for STF. In this case, splitting also will be faster

[AurelienFT]

I just got an idea of how we can do it better. Instead of adding new methods, we can add a new type Immutable<Tx>, where Immutable only calculates the metadata but doesn't have any validation logic. Any Immutable can be converted into checked without calculation of the metadata.

In this case, you can have an id before the validation, and for actual validation, you can leave for STF. In this case, splitting also will be faster

Okey but the problem is that creating metadata can throw error too so the signature to create a new Immutable would be something like into_immutable(self) -> Result<Self, (TxId, ValidityError). Or you want to split all the checks that can create errors to Checked ? Which means iterating two times on inputs/outputs for example.

I don't understand why it's faster because the computation of the id and into_basic_checked in the splitting is already parrallelize. It seems to be a bigger change for no real gains.

I'll try to implement that in an other PR but I don't really understand how is it better.

[AurelienFT]

I tried to implement something but in any case you will need to compute the CheckedMetadata and to pass to a check you need to have a Tx only not a Immutable.

I tried a PR : #906 but it's not compiling because of the Checked that wants a Immutable<Tx>. I think it's possible to do something but it will require a lot more boilerplate. And I want that we first discuss about the real motivations and is it really useful (cf previous message)

[Voxelot]

Do we still need this PR?

cc @AurelienFT

[AurelienFT]

Do we still need this PR?

cc @AurelienFT

Mhhh not sure, I need to be more sure about the current implementation of the parallel-executor to close this or not