Improves safety & readability on transaction root derivation logic

[howardwu]

Motivation

This PR improves safety and readability upon PR 2577.

• Rename Deployment::len to Deployment::num_functions
  • Addresses @ljedrz's comment in the original PR
• Removes fee_index from Transaction::transaction_tree
  • This commit also adds a safety check to ensure fee_index does not exceed MAX_TRANSITIONS

Related PRs

• [Perf] Avoid tx root derivation during block generation #2577

[Copilot]

Pull Request Overview

This PR improves the safety and readability of transaction root derivation by refactoring how transaction trees are constructed and by renaming ambiguous function names. Key changes include:

• Removing the explicit fee index and updating the transaction tree construction logic to use an optional fee reference.
• Renaming Deployment::len to Deployment::num_functions for clarity.
• Updating comments in serialization and transaction-related modules to emphasize recomputation of ID values and prevent malleability.

Reviewed Changes

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

Show a summary per file

File	Description
ledger/block/src/transactions/rejected/mod.rs	Refactored transaction ID computation by removing the fee index and using fee.as_ref.
ledger/block/src/transaction/serialize.rs	Updated comments regarding exclusion of deployment/execution IDs from serialization.
ledger/block/src/transaction/mod.rs	Adjusted transaction tree derivation to pass an optional fee instead of an explicit fee index.
ledger/block/src/transaction/merkle.rs	Reimplemented transaction_tree to incorporate fee as an Option and updated error message checks.
ledger/block/src/transaction/deployment/mod.rs	Renamed Deployment::len to Deployment::num_functions to reflect function count.
ledger/block/src/transaction/bytes.rs	Updated comments to align with new transaction recomputation behavior.
console/program/src/state_path/configuration/mod.rs	Reordered type alias definitions for deployment and execution trees for clarity.

Comments suppressed due to low confidence (1)

ledger/block/src/transaction/merkle.rs:120

• The error message uses ambiguous formatting with '{fee_index}' and '{}' placeholders. Consider refactoring it (e.g., using format!()) to ensure that the fee index value is correctly inserted into the message.

ensure!(fee_index < Self::MAX_TRANSITIONS, "The fee index ('{fee_index}') in the transaction tree must be less than {}", Self::MAX_TRANSITIONS);

[raychu86]

If you want to bound it based on the transaction type, then MAX_TRANSITIONS is relevant only for executions. MAX_FUNCTIONS is the deployment related limit.

Right now we keep them all at 2^5, but if any are changed in the future, this check may not be correct.

[howardwu]

Good point.

As this method could contain either a deployment tree OR an execution tree, should I check both <= MAX_FUNCTIONS and < MAX_TRANSITIONS?

[raychu86]

It's probably a good idea to do both checks.

Alternatively, because we always know what type of tree we are passing into this function, we can do a conditional check (using a boolean or const generic)

[d0cd]

Do we want the check number_of_leaves < Self::MAX_{TRANSITIONS/FUNCTIONS} do also be run when there is no fee for added safety?

Do Self::deployment_tree and Self::execution_tree need similar checks?

[howardwu]

@raychu86 - added check here: 05f30a5

@d0cd - this is not necessary. The tree is correct by construction.

[howardwu]

After discussing offline with @d0cd, we've identified additional deployment size checks that should be added into the tree checks.

See bfa667f for the updates.

[vicsn]

LGTM, modulo Ray's and my open comments

[d0cd]

Good with the overall changes! Left a question on additional safety checks.

[howardwu]

This PR is ready for re-review and merging if LGTM'ed.