Avoid duplicated sections in batches (backport #3882)

.changelog/unreleased/improvements/3882-better-batch-construction.md

@@ -0,0 +1,2 @@
+- Improved batch construction to reduce the size of the resulting tx.
+ ([\#3882](https://github.com/anoma/namada/pull/3882))

crates/sdk/src/signing.rs

@@ -61,7 +61,7 @@ use crate::wallet::{Wallet, WalletIo};
 use crate::{args, rpc, Namada};
 
 /// A structure holding the signing data to craft a transaction
-#[derive(Clone, PartialEq)]
+#[derive(Clone)]
 pub struct SigningTxData {
  /// The address owning the transaction
  pub owner: Option<Address>,
@@ -75,6 +75,27 @@ pub struct SigningTxData {
  pub fee_payer: common::PublicKey,
 }
 
+impl PartialEq for SigningTxData {
+ fn eq(&self, other: &Self) -> bool {
+ if !(self.owner == other.owner
+ && self.threshold == other.threshold
+ && self.account_public_keys_map == other.account_public_keys_map
+ && self.fee_payer == other.fee_payer)
+ {
+ return false;
+ }
+
+ // Check equivalence of the public keys ignoring the specific ordering
+ if self.public_keys.len() != other.public_keys.len() {
+ return false;
+ }
+
+ self.public_keys
+ .iter()
+ .all(|pubkey| other.public_keys.contains(pubkey))
+ }
+}
+
 /// Find the public key for the given address and try to load the keypair
 /// for it from the wallet.
 ///
@@ -290,7 +311,7 @@ where
  Ok(fee_payer_keypair) => {
  tx.sign_wrapper(fee_payer_keypair);
  }
- // The case where tge fee payer also signs the inner transaction
+ // The case where the fee payer also signs the inner transaction
  Err(_)
  if signing_data.public_keys.contains(&signing_data.fee_payer) =>
  {

crates/tx/src/types.rs

@@ -145,16 +145,49 @@ impl Tx {
 
  /// Add a new inner tx to the transaction. Returns `false` if the
  /// commitments already existed in the collection. This function expects a
- /// transaction carrying a single inner tx as input
- pub fn add_inner_tx(&mut self, other: Tx, cmt: TxCommitments) -> bool {
- if !self.header.batch.insert(cmt) {
+ /// transaction carrying a single inner tx as input and the provided
+ /// commitment is assumed to be present in the transaction without further
+ /// validation
+ pub fn add_inner_tx(&mut self, other: Tx, mut cmt: TxCommitments) -> bool {
+ if self.header.batch.contains(&cmt) {
  return false;
  }
 
- // TODO: avoid duplicated sections to reduce the size of the message
- self.sections.extend(other.sections);
+ for section in other.sections {
+ // PartialEq implementation of Section relies on an implementation
+ // on the inner types that doesn't account for the possible salt
+ // which is the correct behavior for this logic
+ if let Some(duplicate) =
+ self.sections.iter().find(|&sec| sec == &section)
+ {
+ // Avoid pushing a duplicated section. Adjust the commitment of
+ // this inner tx for the different section's salt if needed
+ match duplicate {
+ Section::Code(_) => {
+ if cmt.code_hash == section.get_hash() {
+ cmt.code_hash = duplicate.get_hash();
+ }
+ }
+ Section::Data(_) => {
+ if cmt.data_hash == section.get_hash() {
+ cmt.data_hash = duplicate.get_hash();
+ }
+ }
+ Section::ExtraData(_) => {
+ if cmt.memo_hash == section.get_hash() {
+ cmt.memo_hash = duplicate.get_hash();
+ }
+ }
+ // Other sections don't have a direct commitment in the
+ // header
+ _ => (),
+ }
+ } else {
+ self.sections.push(section);
+ }
+ }
 
- true
+ self.header.batch.insert(cmt)
  }
 
  /// Get the transaction header
@@ -1324,6 +1357,11 @@ mod test {
  let data_bytes2 = "WASD".as_bytes();
  let memo_bytes2 = "hjkl".as_bytes();
 
+ // Some duplicated sections
+ let code_bytes3 = code_bytes1;
+ let data_bytes3 = data_bytes2;
+ let memo_bytes3 = memo_bytes1;
+
  let inner_tx1 = {
  let mut tx = Tx::default();
 
@@ -1352,10 +1390,25 @@ mod test {
  tx
  };
 
+ let inner_tx3 = {
+ let mut tx = Tx::default();
+
+ let code = Code::new(code_bytes3.to_owned(), None);
+ tx.set_code(code);
+
+ let data = Data::new(data_bytes3.to_owned());
+ tx.set_data(data);
+
+ tx.add_memo(memo_bytes3);
+
+ tx
+ };
+
  let cmt1 = inner_tx1.first_commitments().unwrap().to_owned();
- let cmt2 = inner_tx2.first_commitments().unwrap().to_owned();
+ let mut cmt2 = inner_tx2.first_commitments().unwrap().to_owned();
+ let mut cmt3 = inner_tx3.first_commitments().unwrap().to_owned();
 
- // Batch `inner_tx1` and `inner_tx1` into `tx`
+ // Batch `inner_tx1`, `inner_tx2` and `inner_tx3` into `tx`
  let tx = {
  let mut tx = Tx::default();
 
@@ -1364,10 +1417,22 @@ mod test {
  assert_eq!(tx.header.batch.len(), 1);
 
  tx.add_inner_tx(inner_tx2, cmt2.clone());
+ // Update cmt2 with the hash of cmt1 code section
+ cmt2.code_hash = cmt1.code_hash;
  assert_eq!(tx.first_commitments().unwrap(), &cmt1);
  assert_eq!(tx.header.batch.len(), 2);
  assert_eq!(tx.header.batch.get_index(1).unwrap(), &cmt2);
 
+ tx.add_inner_tx(inner_tx3, cmt3.clone());
+ // Update cmt3 with the hash of cmt1 code and memo sections and the
+ // hash of cmt2 data section
+ cmt3.code_hash = cmt1.code_hash;
+ cmt3.data_hash = cmt2.data_hash;
+ cmt3.memo_hash = cmt1.memo_hash;
+ assert_eq!(tx.first_commitments().unwrap(), &cmt1);
+ assert_eq!(tx.header.batch.len(), 3);
+ assert_eq!(tx.header.batch.get_index(2).unwrap(), &cmt3);
+
  tx
  };
 
@@ -1390,5 +1455,40 @@ mod test {
 
  assert!(tx.memo(&cmt2).is_some());
  assert_eq!(tx.memo(&cmt2).unwrap(), memo_bytes2);
+
+ // Check sections of `inner_tx3`
+ assert!(tx.code(&cmt3).is_some());
+ assert_eq!(tx.code(&cmt3).unwrap(), code_bytes3);
+
+ assert!(tx.data(&cmt3).is_some());
+ assert_eq!(tx.data(&cmt3).unwrap(), data_bytes3);
+
+ assert!(tx.memo(&cmt3).is_some());
+ assert_eq!(tx.memo(&cmt3).unwrap(), memo_bytes3);
+
+ // Check that the redundant sections have been included only once in the
+ // batch
+ assert_eq!(tx.sections.len(), 5);
+ assert_eq!(
+ tx.sections
+ .iter()
+ .filter(|section| section.code_sec().is_some())
+ .count(),
+ 1
+ );
+ assert_eq!(
+ tx.sections
+ .iter()
+ .filter(|section| section.data().is_some())
+ .count(),
+ 2
+ );
+ assert_eq!(
+ tx.sections
+ .iter()
+ .filter(|section| section.extra_data_sec().is_some())
+ .count(),
+ 2
+ );
  }
 }