From e516f246f6f35828b6e65705a8fd0c3d481e57d8 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 3 Oct 2024 11:49:34 +0200 Subject: [PATCH 1/5] Avoids duplicated sections when contructing a batch --- crates/tx/src/types.rs | 64 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 5 deletions(-) diff --git a/crates/tx/src/types.rs b/crates/tx/src/types.rs index be04f5946c..98ab87c8f3 100644 --- a/crates/tx/src/types.rs +++ b/crates/tx/src/types.rs @@ -143,18 +143,72 @@ impl Tx { self.header.batch.insert(TxCommitments::default()) } + // FIXME: update the comment on when we return false /// 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) { + // FIXME: wait if I change this to Result this becomes breaking! Maybe I can + // just return false in case of errors. Previously we were returnign false + // when the commitments were already present to signal that the tx had not + // been included in the tx. If I return false on errors I think it's ok as + // long as we guarantee that the tx has not been included in the batch + // FIXME: if we return a result should we aalso check that the tx indeed + // carries a single inner tx? Maybe yes. Probably this check should be done + // nayway FIXME: if returning a bollean is not enough/clen I caould keep + // this function as is and mark it as deprecated and then create a new one + // with the new API FIXME: do like this, remove the comment where I say + // that this exepct a Tx with a single inner and loop on all the + // commitments. Also check comments in teh caller. Keep the bool as returned + // type FIXME: can we also write a test for this? Should we? Maybe yes + // FIXME: I'm not sure if iterating on all the inner txs instead of passing + // a specific cmt is correct, the API here should take the commitments and + // the sections, without the rest of the header, so we'd probably need a + // completeley different API FIXME: also why d owe take a specific cmt? + // Can't we just take the first one of the tx? Why passing a specific one? + 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); + let Some(first_cmt_ref) = other.header.batch.first() else { + return false; + }; + + 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 == §ion) + { + // 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 first_cmt_ref.code_hash == section.get_hash() { + cmt.code_hash = duplicate.get_hash(); + } + } + Section::Data(_) => { + if first_cmt_ref.data_hash == section.get_hash() { + cmt.data_hash = duplicate.get_hash(); + } + } + Section::ExtraData(_) => { + if first_cmt_ref.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 From 3cce0a474d00bc3c7540edbf88b3b365d1a34dbe Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 3 Oct 2024 14:18:31 +0200 Subject: [PATCH 2/5] Fixes cmt reference in `add_inner_tx` --- crates/tx/src/types.rs | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/crates/tx/src/types.rs b/crates/tx/src/types.rs index 98ab87c8f3..ad0627fc3c 100644 --- a/crates/tx/src/types.rs +++ b/crates/tx/src/types.rs @@ -143,37 +143,17 @@ impl Tx { self.header.batch.insert(TxCommitments::default()) } - // FIXME: update the comment on when we return false /// 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 - // FIXME: wait if I change this to Result this becomes breaking! Maybe I can - // just return false in case of errors. Previously we were returnign false - // when the commitments were already present to signal that the tx had not - // been included in the tx. If I return false on errors I think it's ok as - // long as we guarantee that the tx has not been included in the batch - // FIXME: if we return a result should we aalso check that the tx indeed - // carries a single inner tx? Maybe yes. Probably this check should be done - // nayway FIXME: if returning a bollean is not enough/clen I caould keep - // this function as is and mark it as deprecated and then create a new one - // with the new API FIXME: do like this, remove the comment where I say - // that this exepct a Tx with a single inner and loop on all the - // commitments. Also check comments in teh caller. Keep the bool as returned - // type FIXME: can we also write a test for this? Should we? Maybe yes - // FIXME: I'm not sure if iterating on all the inner txs instead of passing - // a specific cmt is correct, the API here should take the commitments and - // the sections, without the rest of the header, so we'd probably need a - // completeley different API FIXME: also why d owe take a specific cmt? - // Can't we just take the first one of the tx? Why passing a specific one? + /// transaction carrying a single inner tx as input and the provided + /// commitment is assumed to be present in the transaction without further + /// validation + // FIXME: can we also write a test for this? Should we? Maybe yes pub fn add_inner_tx(&mut self, other: Tx, mut cmt: TxCommitments) -> bool { if self.header.batch.contains(&cmt) { return false; } - let Some(first_cmt_ref) = other.header.batch.first() else { - return false; - }; - 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 @@ -185,17 +165,17 @@ impl Tx { // this inner tx for the different section's salt if needed match duplicate { Section::Code(_) => { - if first_cmt_ref.code_hash == section.get_hash() { + if cmt.code_hash == section.get_hash() { cmt.code_hash = duplicate.get_hash(); } } Section::Data(_) => { - if first_cmt_ref.data_hash == section.get_hash() { + if cmt.data_hash == section.get_hash() { cmt.data_hash = duplicate.get_hash(); } } Section::ExtraData(_) => { - if first_cmt_ref.memo_hash == section.get_hash() { + if cmt.memo_hash == section.get_hash() { cmt.memo_hash = duplicate.get_hash(); } } @@ -1369,6 +1349,7 @@ mod test { } #[test] + // FIXME: ah maybe I can extend this test fn test_batched_tx_sections() { let code_bytes1 = "code brrr".as_bytes(); let data_bytes1 = "bingbong".as_bytes(); From ea859903ead00deb5a03ed012228e38652dffbf1 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 3 Oct 2024 14:19:44 +0200 Subject: [PATCH 3/5] Custom `PartialEq` impl for `SigningTxData` --- crates/sdk/src/signing.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index 81d9caed45..98f42d0dd7 100644 --- a/crates/sdk/src/signing.rs +++ b/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
, @@ -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) => { From a3ae115d783e2c99b1b74b294e199a7a97ca2404 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 3 Oct 2024 15:45:16 +0200 Subject: [PATCH 4/5] Extends batch sections test to check duplicate sections --- crates/tx/src/types.rs | 73 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/crates/tx/src/types.rs b/crates/tx/src/types.rs index ad0627fc3c..01de49e86a 100644 --- a/crates/tx/src/types.rs +++ b/crates/tx/src/types.rs @@ -148,7 +148,6 @@ impl Tx { /// transaction carrying a single inner tx as input and the provided /// commitment is assumed to be present in the transaction without further /// validation - // FIXME: can we also write a test for this? Should we? Maybe yes pub fn add_inner_tx(&mut self, other: Tx, mut cmt: TxCommitments) -> bool { if self.header.batch.contains(&cmt) { return false; @@ -1349,7 +1348,6 @@ mod test { } #[test] - // FIXME: ah maybe I can extend this test fn test_batched_tx_sections() { let code_bytes1 = "code brrr".as_bytes(); let data_bytes1 = "bingbong".as_bytes(); @@ -1359,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(); @@ -1387,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(); @@ -1399,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 }; @@ -1425,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 + ); } } From f7c195f7e73f48b3b25a14e1aa6154344fd43048 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 3 Oct 2024 15:57:12 +0200 Subject: [PATCH 5/5] Changelog #3882 --- .../unreleased/improvements/3882-better-batch-construction.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3882-better-batch-construction.md diff --git a/.changelog/unreleased/improvements/3882-better-batch-construction.md b/.changelog/unreleased/improvements/3882-better-batch-construction.md new file mode 100644 index 0000000000..d2949b06a5 --- /dev/null +++ b/.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)) \ No newline at end of file