Reuse witnesses for identical secrets when building transactions (#459)

* Reuse witness indexes for the same private keys, don't automatically append a new witness when adding an unsigned coin

* clippy + test case fix

* Update CHANGELOG.md

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

---------

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

CHANGELOG.md

@@ -43,6 +43,9 @@ Description of the upcoming release here.
 - [#456](https://github.com/FuelLabs/fuel-vm/pull/456): Added a new type - `ChainId` to represent the identifier of the chain. 
 It is a wrapper around the `u64`, so any `u64` can be converted into this type via `.into()` or `ChainId::new(...)`.
 
+- [#459](https://github.com/FuelLabs/fuel-vm/pull/459) Require witness index to be specified when adding an unsigned coin to a transaction.
+This allows for better reuse of witness data when using the transaction builder and helper methods to make transactions compact.
+
 ### Changed
 
 - [#458](https://github.com/FuelLabs/fuel-vm/pull/458): Automatically sort storage slots for creation transactions.

fuel-tx/src/builder.rs

@@ -9,6 +9,7 @@ use fuel_crypto::SecretKey;
 use fuel_types::{BlockHeight, Nonce, Salt, Word};
 
 use alloc::vec::Vec;
+use std::collections::HashMap;
 
 pub trait BuildableAloc
 where
@@ -92,7 +93,8 @@ pub struct TransactionBuilder<Tx> {
 
     // We take the key by reference so this lib won't have the responsibility to properly zeroize
     // the keys
-    sign_keys: Vec<SecretKey>,
+    // Maps signing keys -> witness indexes
+    sign_keys: HashMap<SecretKey, u8>,
 }
 
 impl TransactionBuilder<Script> {
@@ -161,7 +163,7 @@ impl<Tx> TransactionBuilder<Tx> {
     fn with_tx(tx: Tx) -> Self {
         let should_prepare_script = false;
         let should_prepare_predicate = false;
-        let sign_keys = Vec::new();
+        let sign_keys = HashMap::new();
 
         Self {
             tx,
@@ -193,8 +195,8 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
         self
     }
 
-    pub fn sign_keys(&self) -> &[SecretKey] {
-        self.sign_keys.as_slice()
+    pub fn sign_keys(&self) -> impl Iterator<Item = &SecretKey> {
+        self.sign_keys.keys()
     }
 
     pub fn gas_price(&mut self, gas_price: Word) -> &mut Self {
@@ -227,9 +229,10 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
     ) -> &mut Self {
         let pk = secret.public_key();
 
-        self.sign_keys.push(secret);
+        let witness_index = self.upsert_secret(secret);
+
         self.tx
-            .add_unsigned_coin_input(utxo_id, &pk, amount, asset_id, tx_pointer, maturity);
+            .add_unsigned_coin_input(utxo_id, &pk, amount, asset_id, tx_pointer, maturity, witness_index);
 
         self
     }
@@ -244,12 +247,12 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
         data: Vec<u8>,
     ) -> &mut Self {
         let pk = secret.public_key();
-        self.sign_keys.push(secret);
-
         let recipient = Input::owner(&pk);
 
+        let witness_index = self.upsert_secret(secret);
+
         self.tx
-            .add_unsigned_message_input(sender, recipient, nonce, amount, data);
+            .add_unsigned_message_input(sender, recipient, nonce, amount, data, witness_index);
 
         self
     }
@@ -278,6 +281,20 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
         self
     }
 
+    /// Adds a secret to the builder, and adds a corresponding witness if it's a new entry
+    #[cfg(feature = "std")]
+    fn upsert_secret(&mut self, secret_key: SecretKey) -> u8 {
+        let witness_len = self.witnesses().len() as u8;
+
+        let witness_index = self.sign_keys.entry(secret_key).or_insert_with(|| {
+            // if this private key hasn't been used before,
+            // add a new witness entry and return its index
+            self.tx.witnesses_mut().push(Witness::default());
+            witness_len
+        });
+        *witness_index
+    }
+
     #[cfg(feature = "std")]
     fn prepare_finalize(&mut self) {
         if self.should_prepare_predicate {
@@ -297,7 +314,7 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {
 
         self.sign_keys
             .iter()
-            .for_each(|k| tx.sign_inputs(k, &self.parameters.chain_id));
+            .for_each(|(k, _)| tx.sign_inputs(k, &self.parameters.chain_id));
 
         tx.precompute(&self.parameters.chain_id);
 

fuel-tx/src/tests/valid_cases/input.rs

@@ -1,6 +1,7 @@
 use super::PARAMS;
 
 use fuel_crypto::{PublicKey, SecretKey};
+use fuel_tx::field::Witnesses;
 use fuel_tx::*;
 use fuel_tx_test_helpers::{generate_bytes, generate_nonempty_padded_bytes, TransactionFactory};
 use rand::rngs::StdRng;
@@ -62,7 +63,17 @@ fn input_coin_message_signature() {
             let maturity = rng.gen();
 
             sign_and_validate(rng, txs.by_ref(), |tx, public| {
-                tx.add_unsigned_coin_input(utxo_id, public, amount, asset_id, tx_pointer, maturity)
+                let witness_index = <Tx as fuel_tx::field::Witnesses>::witnesses(tx).len();
+                <Tx as fuel_tx::field::Witnesses>::witnesses_mut(tx).push(fuel_tx::Witness::default());
+                tx.add_unsigned_coin_input(
+                    utxo_id,
+                    public,
+                    amount,
+                    asset_id,
+                    tx_pointer,
+                    maturity,
+                    witness_index as u8,
+                )
             })
             .expect("Failed to validate transaction");
         }
@@ -74,7 +85,16 @@ fn input_coin_message_signature() {
             let data = generate_bytes(rng);
 
             sign_and_validate(rng, txs.by_ref(), |tx, public| {
-                tx.add_unsigned_message_input(sender, Input::owner(public), nonce, amount, data.clone())
+                let witness_index = <Tx as fuel_tx::field::Witnesses>::witnesses(tx).len();
+                <Tx as fuel_tx::field::Witnesses>::witnesses_mut(tx).push(fuel_tx::Witness::default());
+                tx.add_unsigned_message_input(
+                    sender,
+                    Input::owner(public),
+                    nonce,
+                    amount,
+                    data.clone(),
+                    witness_index as u8,
+                )
             })
             .expect("Failed to validate transaction");
         }
@@ -102,6 +122,46 @@ fn coin_signed() {
     assert_eq!(CheckError::InputWitnessIndexBounds { index: 0 }, err);
 }
 
+#[test]
+fn duplicate_secrets_reuse_witness() {
+    let rng = &mut StdRng::seed_from_u64(10000);
+    let key = SecretKey::random(rng);
+
+    // verify witness reuse for script txs
+    let script = TransactionBuilder::script(vec![], vec![])
+        // coin 1
+        .add_unsigned_coin_input(key, rng.gen(), 100, Default::default(), Default::default(), 0.into())
+        // coin 2
+        .add_unsigned_coin_input(key, rng.gen(), 200, rng.gen(), Default::default(), 0.into())
+        // message 1
+        .add_unsigned_message_input(key, rng.gen(), rng.gen(), 100, vec![])
+        .add_unsigned_message_input(key, rng.gen(), rng.gen(), 100, vec![rng.gen()])
+        .finalize();
+
+    assert_eq!(
+        script.witnesses().len(),
+        1,
+        "Script should only have one witness as only one private key is used"
+    );
+
+    // verify witness reuse for creation txs
+    let create = TransactionBuilder::create(Witness::default(), rng.gen(), vec![])
+        // coin 1
+        .add_unsigned_coin_input(key, rng.gen(), 100, Default::default(), Default::default(), 0.into())
+        // coin 2
+        .add_unsigned_coin_input(key, rng.gen(), 200, rng.gen(), Default::default(), 0.into())
+        // message 1
+        .add_unsigned_message_input(key, rng.gen(), rng.gen(), 100, vec![])
+        .add_unsigned_message_input(key, rng.gen(), rng.gen(), 100, vec![rng.gen()])
+        .finalize();
+
+    assert_eq!(
+        create.witnesses().len(),
+        2,
+        "Create should only have two witnesses (bytecode + signature) as only one private key is used"
+    )
+}
+
 #[test]
 fn coin_predicate() {
     let rng = &mut StdRng::seed_from_u64(8586);

fuel-tx/src/transaction.rs

@@ -288,13 +288,11 @@ pub trait Executable: field::Inputs + field::Outputs + field::Witnesses {
         asset_id: AssetId,
         tx_pointer: TxPointer,
         maturity: BlockHeight,
+        witness_index: u8,
     ) {
         let owner = Input::owner(owner);
 
-        let witness_index = self.witnesses().len() as u8;
         let input = Input::coin_signed(utxo_id, owner, amount, asset_id, tx_pointer, witness_index, maturity);
-
-        self.witnesses_mut().push(Witness::default());
         self.inputs_mut().push(input);
     }
 
@@ -313,15 +311,14 @@ pub trait Executable: field::Inputs + field::Outputs + field::Witnesses {
         nonce: Nonce,
         amount: Word,
         data: Vec<u8>,
+        witness_index: u8,
     ) {
-        let witness_index = self.witnesses().len() as u8;
         let input = if data.is_empty() {
             Input::message_coin_signed(sender, recipient, amount, nonce, witness_index)
         } else {
             Input::message_data_signed(sender, recipient, amount, nonce, witness_index, data)
         };
 
-        self.witnesses_mut().push(Witness::default());
         self.inputs_mut().push(input);
     }
 

fuel-vm/src/tests/contract.rs

@@ -1,6 +1,7 @@
 use fuel_asm::op;
 use fuel_asm::RegId;
 use fuel_tx::field::ScriptData;
+use fuel_tx::Witness;
 use fuel_vm::prelude::*;
 use fuel_vm::script_with_data_offset;
 use fuel_vm::util::test_helpers::TestBuilder;
@@ -45,7 +46,7 @@ fn prevent_contract_id_redeployment() {
             Output::change(rng.gen(), 0, asset_id),
             Output::coin(rng.gen(), spend_amount, asset_id),
         ],
-        vec![program],
+        vec![program, Witness::default()],
     );
     create.add_unsigned_coin_input(
         rng.gen(),
@@ -54,6 +55,7 @@ fn prevent_contract_id_redeployment() {
         asset_id,
         rng.gen(),
         Default::default(),
+        1,
     );
     let create = create
         .into_checked_basic(1.into(), &ConsensusParameters::default())

fuel-vm/src/tests/outputs.rs

@@ -1,4 +1,5 @@
 use fuel_asm::{op, RegId};
+use fuel_tx::Witness;
 use fuel_vm::util::test_helpers::find_change;
 use fuel_vm::{
     prelude::{field::Outputs, *},
@@ -115,7 +116,7 @@ fn correct_change_is_provided_for_coin_outputs_create() {
             Output::change(rng.gen(), 0, asset_id),
             Output::coin(rng.gen(), spend_amount, asset_id),
         ],
-        vec![program],
+        vec![program, Witness::default()],
     );
     create.add_unsigned_coin_input(
         rng.gen(),
@@ -124,6 +125,7 @@ fn correct_change_is_provided_for_coin_outputs_create() {
         asset_id,
         rng.gen(),
         Default::default(),
+        1,
     );
     let create = create
         .into_checked_basic(context.get_block_height(), context.get_params())