Zero malleable fields before execution (#678)

* Zero malleable fields before execution and more. * Don't update tx.receiptsRoot after pushing receipts * Remove some now-obsolete GTF getters * Add a test checking that malleable fields do not affect validity * Update rest of the test cases * Changelog * Fix some warnings * Remove debug print * Move tx preparation to vm startup, fix tests * Cleanup * Clippy fix * Clippy * Cleanup * Move prepare_init_execute into ExecutableTransaction, fix tests * Fix changelog --------- Co-authored-by: Green Baneling <[email protected]>
FuelLabs · Feb 26, 2024 · 54110f1 · 54110f1
1 parent 166db56
commit 54110f1
Show file tree

Hide file tree

Showing 28 changed files with 281 additions and 535 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 #### Breaking
 
 - [#683](https://github.com/FuelLabs/fuel-vm/pull/683): Simplify `InterpreterStorage` by removing dependency on `MerkleRootStorage` and removing `merkle_` prefix from method names.
+- [#678](https://github.com/FuelLabs/fuel-vm/pull/678): Zero malleable fields before execution. Remove some now-obsolete GTF getters. Don't update `tx.receiptsRoot` after pushing receipts, and do it after execution instead.
 - [#672](https://github.com/FuelLabs/fuel-vm/pull/672): Remove `GasPrice` policy
 - [#672](https://github.com/FuelLabs/fuel-vm/pull/672): Add `gas_price` field to transaction execution
 - [#684](https://github.com/FuelLabs/fuel-vm/pull/684): Remove `maturity` field from `Input` coin types. Also remove related `GTF` getter.

diff --git a/fuel-asm/src/args.rs b/fuel-asm/src/args.rs
@@ -164,15 +164,6 @@ crate::enum_try_from! {
         /// Set `$rA` to `tx.inputs[$rB].outputIndex`
         InputContractOutputIndex = 0x221,
 
-        /// Set `$rA` to `Memory address of tx.inputs[$rB].balanceRoot`
-        InputContractBalanceRoot = 0x222,
-
-        /// Set `$rA` to `Memory address of tx.inputs[$rB].stateRoot`
-        InputContractStateRoot = 0x223,
-
-        /// Set `$rA` to `Memory address of tx.inputs[$rB].txPointer`
-        InputContractTxPointer = 0x224,
-
         /// Set `$rA` to `Memory address of tx.inputs[$rB].contractID`
         InputContractId = 0x225,
 
@@ -227,12 +218,6 @@ crate::enum_try_from! {
         /// Set `$rA` to `tx.outputs[$rB].inputIndex`
         OutputContractInputIndex = 0x304,
 
-        /// Set `$rA` to `Memory address of tx.outputs[$rB].balanceRoot`
-        OutputContractBalanceRoot = 0x305,
-
-        /// Set `$rA` to `Memory address of tx.outputs[$rB].stateRoot`
-        OutputContractStateRoot = 0x306,
-
         /// Set `$rA` to `Memory address of tx.outputs[$rB].contractID`
         OutputContractCreatedContractId = 0x307,
 
@@ -328,11 +313,6 @@ fn encode_gtf_args() {
         GTFArgs::InputCoinPredicate,
         GTFArgs::InputCoinPredicateData,
         GTFArgs::InputCoinPredicateGasUsed,
-        GTFArgs::InputContractTxId,
-        GTFArgs::InputContractOutputIndex,
-        GTFArgs::InputContractBalanceRoot,
-        GTFArgs::InputContractStateRoot,
-        GTFArgs::InputContractTxPointer,
         GTFArgs::InputContractId,
         GTFArgs::InputMessageSender,
         GTFArgs::InputMessageRecipient,
@@ -351,8 +331,6 @@ fn encode_gtf_args() {
         GTFArgs::OutputCoinAmount,
         GTFArgs::OutputCoinAssetId,
         GTFArgs::OutputContractInputIndex,
-        GTFArgs::OutputContractBalanceRoot,
-        GTFArgs::OutputContractStateRoot,
         GTFArgs::OutputContractCreatedContractId,
         GTFArgs::OutputContractCreatedStateRoot,
         GTFArgs::WitnessDataLength,

diff --git a/fuel-tx/src/builder.rs b/fuel-tx/src/builder.rs
@@ -103,8 +103,6 @@ impl<T> Buildable for T where T: BuildableSet {}
 pub struct TransactionBuilder<Tx> {
     tx: Tx,
 
-    should_prepare_script: bool,
-    should_prepare_predicate: bool,
     params: ConsensusParameters,
 
     // We take the key by reference so this lib won't have the responsibility to properly
@@ -126,12 +124,7 @@ impl TransactionBuilder<Script> {
             receipts_root: Default::default(),
             metadata: None,
         };
-
-        let mut slf = Self::with_tx(tx);
-
-        slf.prepare_script(true);
-
-        slf
+        Self::with_tx(tx)
     }
 }
 
@@ -190,14 +183,10 @@ impl TransactionBuilder<Mint> {
 
 impl<Tx> TransactionBuilder<Tx> {
     fn with_tx(tx: Tx) -> Self {
-        let should_prepare_script = false;
-        let should_prepare_predicate = false;
         let sign_keys = BTreeMap::new();
 
         Self {
             tx,
-            should_prepare_script,
-            should_prepare_predicate,
             params: ConsensusParameters::standard(),
             sign_keys,
         }
@@ -279,16 +268,6 @@ impl<Tx> TransactionBuilder<Tx> {
 }
 
 impl<Tx: Buildable> TransactionBuilder<Tx> {
-    pub fn prepare_script(&mut self, should_prepare_script: bool) -> &mut Self {
-        self.should_prepare_script = should_prepare_script;
-        self
-    }
-
-    pub fn prepare_predicate(&mut self, should_prepare_predicate: bool) -> &mut Self {
-        self.should_prepare_predicate = should_prepare_predicate;
-        self
-    }
-
     pub fn sign_keys(&self) -> impl Iterator<Item = &SecretKey> {
         self.sign_keys.keys()
     }
@@ -433,19 +412,7 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
         *witness_index
     }
 
-    fn prepare_finalize(&mut self) {
-        if self.should_prepare_predicate {
-            self.tx.prepare_init_predicate();
-        }
-
-        if self.should_prepare_script {
-            self.tx.prepare_init_script();
-        }
-    }
-
     fn finalize_inner(&mut self) -> Tx {
-        self.prepare_finalize();
-
         let mut tx = core::mem::take(&mut self.tx);
 
         self.sign_keys
@@ -459,8 +426,6 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
     }
 
     pub fn finalize_without_signature_inner(&mut self) -> Tx {
-        self.prepare_finalize();
-
         let mut tx = core::mem::take(&mut self.tx);
 
         tx.precompute(&self.get_chain_id())

diff --git a/fuel-tx/src/tests/mod.rs b/fuel-tx/src/tests/mod.rs
@@ -1,5 +1,4 @@
 mod offset;
-mod prepared_init;
 mod valid_cases;
 
 #[cfg(feature = "serde")]

diff --git a/fuel-tx/src/tests/prepared_init.rs b/fuel-tx/src/tests/prepared_init.rs
diff --git a/fuel-tx/src/transaction.rs b/fuel-tx/src/transaction.rs
@@ -412,31 +412,6 @@ pub trait Executable: field::Inputs + field::Outputs + field::Witnesses {
 
         self.inputs_mut().push(input);
     }
-
-    /// Prepare the transaction for VM initialization for script execution
-    ///
-    /// note: Fields dependent on storage/state such as balance and state roots, or tx
-    /// pointers, should already set by the client beforehand.
-    fn prepare_init_script(&mut self) -> &mut Self {
-        self.outputs_mut()
-            .iter_mut()
-            .for_each(|o| o.prepare_init_script());
-
-        self
-    }
-
-    /// Prepare the transaction for VM initialization for predicate verification
-    fn prepare_init_predicate(&mut self) -> &mut Self {
-        self.inputs_mut()
-            .iter_mut()
-            .for_each(|i| i.prepare_init_predicate());
-
-        self.outputs_mut()
-            .iter_mut()
-            .for_each(|o| o.prepare_init_predicate());
-
-        self
-    }
 }
 
 impl<T: field::Inputs + field::Outputs + field::Witnesses> Executable for T {}

diff --git a/fuel-tx/src/transaction/types/input.rs b/fuel-tx/src/transaction/types/input.rs
@@ -753,9 +753,9 @@ impl Input {
         owner == &Self::predicate_owner(predicate)
     }
 
-    /// Prepare the output for VM predicate execution
-    pub fn prepare_init_predicate(&mut self) {
-        self.prepare_sign()
+    /// Prepare the output for script or predicate execution
+    pub fn prepare_init_execute(&mut self) {
+        self.prepare_sign(); // This currently does the same thing
     }
 }
 

diff --git a/fuel-tx/src/transaction/types/output.rs b/fuel-tx/src/transaction/types/output.rs
@@ -217,29 +217,9 @@ impl Output {
     }
 
     /// Prepare the output for VM initialization for script execution
-    pub fn prepare_init_script(&mut self) {
-        match self {
-            Output::Change { amount, .. } => {
-                mem::take(amount);
-            }
-
-            Output::Variable {
-                to,
-                amount,
-                asset_id,
-            } => {
-                mem::take(to);
-                mem::take(amount);
-                mem::take(asset_id);
-            }
-
-            _ => (),
-        }
-    }
-
-    /// Prepare the output for VM initialization for predicate verification
-    pub fn prepare_init_predicate(&mut self) {
-        self.prepare_sign()
+    /// or predicate verification
+    pub fn prepare_init_execute(&mut self) {
+        self.prepare_sign() // Currently does the same thing
     }
 }
 

diff --git a/fuel-tx/src/transaction/types/script.rs b/fuel-tx/src/transaction/types/script.rs
@@ -100,6 +100,19 @@ impl Default for Script {
     }
 }
 
+impl Script {
+    /// Prepare script for execution by clearing malleable fields.
+    pub fn prepare_init_execute(&mut self) {
+        *self.receipts_root_mut() = Default::default();
+        self.inputs_mut()
+            .iter_mut()
+            .for_each(Input::prepare_init_execute);
+        self.outputs_mut()
+            .iter_mut()
+            .for_each(Output::prepare_init_execute);
+    }
+}
+
 impl crate::UniqueIdentifier for Script {
     fn id(&self, chain_id: &ChainId) -> Bytes32 {
         if let Some(id) = self.cached_id() {

diff --git a/fuel-vm/src/checked_transaction.rs b/fuel-vm/src/checked_transaction.rs
@@ -47,6 +47,7 @@ bitflags::bitflags! {
     pub struct Checks: u32 {
         /// Basic checks defined in the specification for each transaction:
         /// https://github.com/FuelLabs/fuel-specs/blob/master/src/tx-format/transaction.md#transaction
+        /// Also ensures that malleable fields are zeroed.
         const Basic         = 0b00000001;
         /// Check that signature in the transactions are valid.
         const Signatures    = 0b00000010;

diff --git a/fuel-vm/src/interpreter.rs b/fuel-vm/src/interpreter.rs
@@ -20,14 +20,20 @@ use fuel_asm::{
     PanicReason,
 };
 use fuel_tx::{
-    field,
+    field::{
+        self,
+        Inputs,
+        Outputs,
+        ReceiptsRoot,
+    },
     output,
     Chargeable,
     ContractParameters,
     Create,
     Executable,
     FeeParameters,
     GasCosts,
+    Input,
     Output,
     PredicateParameters,
     Receipt,
@@ -40,6 +46,7 @@ use fuel_tx::{
 };
 use fuel_types::{
     AssetId,
+    Bytes32,
     ChainId,
     ContractId,
     Word,
@@ -286,6 +293,11 @@ impl<S, Tx, Ecal> Interpreter<S, Tx, Ecal> {
         self.receipts.as_ref().as_slice()
     }
 
+    /// Compute current receipts root
+    pub fn compute_receipts_root(&self) -> Bytes32 {
+        self.receipts.root()
+    }
+
     /// Mutable access to receipts for testing purposes.
     #[cfg(any(test, feature = "test-helpers"))]
     pub fn receipts_mut(&mut self) -> &mut ReceiptsCtx {
@@ -474,6 +486,9 @@ pub trait ExecutableTransaction:
             }) if *input_index as usize == input)
         })
     }
+
+    /// Prepares the transaction for execution.
+    fn prepare_init_execute(&mut self);
 }
 
 impl ExecutableTransaction for Create {
@@ -496,6 +511,8 @@ impl ExecutableTransaction for Create {
     fn transaction_type() -> Word {
         TransactionRepr::Create as Word
     }
+
+    fn prepare_init_execute(&mut self) {}
 }
 
 impl ExecutableTransaction for Script {
@@ -518,6 +535,16 @@ impl ExecutableTransaction for Script {
     fn transaction_type() -> Word {
         TransactionRepr::Script as Word
     }
+
+    fn prepare_init_execute(&mut self) {
+        *self.receipts_root_mut() = Default::default();
+        self.inputs_mut()
+            .iter_mut()
+            .for_each(Input::prepare_init_execute);
+        self.outputs_mut()
+            .iter_mut()
+            .for_each(Output::prepare_init_execute);
+    }
 }
 
 /// The initial balances of the transaction.