From 97ad9c21ccc96a95a93d1ec17fdf4a2adc5e2925 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 28 Aug 2024 18:14:28 +0200 Subject: [PATCH 1/4] Batches reveal pk with the actual tx in client when only one reveal is needed --- crates/apps_lib/src/client/tx.rs | 170 +++++++++++++++++++------------ 1 file changed, 106 insertions(+), 64 deletions(-) diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index 211f347942..0a738b835e 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -195,11 +195,11 @@ pub async fn sign( // Build a transaction to reveal the signer of the given transaction. pub async fn submit_reveal_aux( context: &impl Namada, - args: args::Tx, + args: &args::Tx, address: &Address, -) -> Result<(), error::Error> { +) -> Result, error::Error> { if args.dump_tx { - return Ok(()); + return Ok(None); } if let Address::Implicit(ImplicitAddress(pkh)) = address { @@ -210,20 +210,42 @@ pub async fn submit_reveal_aux( .map_err(|e| error::Error::Other(e.to_string()))?; if tx::is_reveal_pk_needed(context.client(), address).await? { + // FIXME: rework this message? yes slightly display_line!( context.io(), "Submitting a tx to reveal the public key for address \ {address}..." ); - let (mut tx, signing_data) = - tx::build_reveal_pk(context, &args, &public_key).await?; + return Ok(Some( + tx::build_reveal_pk(context, args, &public_key).await?, + )); + } + } - sign(context, &mut tx, &args, signing_data).await?; + Ok(None) +} - context.submit(tx, &args).await?; - } +async fn batch_opt_reveal_pk_and_submit( + namada: &N, + args: &args::Tx, + owner: &Address, + tx_data: (Tx, SigningTxData), +) -> Result<(), error::Error> +where + ::Error: std::fmt::Display, +{ + let submit_pk_tx_data = submit_reveal_aux(namada, args, owner).await?; + let mut batched_tx_data = + submit_pk_tx_data.map_or_else(Default::default, |data| vec![data]); + batched_tx_data.push(tx_data); + let (mut batched_tx, batched_signing_data) = + namada_sdk::tx::build_batch(batched_tx_data)?; + + for sig_data in batched_signing_data { + sign(namada, &mut batched_tx, args, sig_data).await?; } + namada.submit(batched_tx, args).await?; Ok(()) } @@ -231,17 +253,18 @@ pub async fn submit_bridge_pool_tx( namada: &N, args: args::EthereumBridgePool, ) -> Result<(), error::Error> { - let tx_args = args.tx.clone(); - let (mut tx, signing_data) = args.clone().build(namada).await?; + let bridge_pool_tx_data = args.clone().build(namada).await?; if args.tx.dump_tx { - tx::dump_tx(namada.io(), &args.tx, tx); + tx::dump_tx(namada.io(), &args.tx, bridge_pool_tx_data.0); } else { - submit_reveal_aux(namada, tx_args.clone(), &args.sender).await?; - - sign(namada, &mut tx, &tx_args, signing_data).await?; - - namada.submit(tx, &tx_args).await?; + batch_opt_reveal_pk_and_submit( + namada, + &args.tx, + &args.sender, + bridge_pool_tx_data, + ) + .await?; } Ok(()) @@ -254,16 +277,18 @@ pub async fn submit_custom( where ::Error: std::fmt::Display, { - submit_reveal_aux(namada, args.tx.clone(), &args.owner).await?; - - let (mut tx, signing_data) = args.build(namada).await?; + let custom_tx_data = args.build(namada).await?; if args.tx.dump_tx { - tx::dump_tx(namada.io(), &args.tx, tx); + tx::dump_tx(namada.io(), &args.tx, custom_tx_data.0); } else { - sign(namada, &mut tx, &args.tx, signing_data).await?; - - namada.submit(tx, &args.tx).await?; + batch_opt_reveal_pk_and_submit( + namada, + &args.tx, + &args.owner, + custom_tx_data, + ) + .await?; } Ok(()) @@ -768,15 +793,17 @@ pub async fn submit_transparent_transfer( )); } - for datum in args.data.iter() { - submit_reveal_aux(namada, args.tx.clone(), &datum.source).await?; - } - let (mut tx, signing_data) = args.clone().build(namada).await?; if args.tx.dump_tx { tx::dump_tx(namada.io(), &args.tx, tx); } else { + // FIXME: need to extend batch_opt_revel for this to support multiple + // reveal pkls + for datum in args.data.iter() { + submit_reveal_aux(namada, &args.tx, &datum.source).await?; + } + sign(namada, &mut tx, &args.tx, signing_data).await?; namada.submit(tx, &args.tx).await?; } @@ -803,8 +830,9 @@ pub async fn submit_shielding_transfer( namada: &impl Namada, args: args::TxShieldingTransfer, ) -> Result<(), error::Error> { + // FIXME: same here, extend for datum in args.data.iter() { - submit_reveal_aux(namada, args.tx.clone(), &datum.source).await?; + submit_reveal_aux(namada, &args.tx, &datum.source).await?; } // Repeat once if the tx fails on a crossover of an epoch @@ -873,20 +901,18 @@ pub async fn submit_ibc_transfer( where ::Error: std::fmt::Display, { - submit_reveal_aux( - namada, - args.tx.clone(), - &args.source.effective_address(), - ) - .await?; - let (mut tx, signing_data, _) = args.build(namada).await?; + let (tx, signing_data, _) = args.build(namada).await?; if args.tx.dump_tx { tx::dump_tx(namada.io(), &args.tx, tx); } else { - sign(namada, &mut tx, &args.tx, signing_data).await?; - - namada.submit(tx, &args.tx).await?; + batch_opt_reveal_pk_and_submit( + namada, + &args.tx, + &args.source.effective_address(), + (tx, signing_data), + ) + .await?; } // NOTE that the tx could fail when its submission epoch doesn't match // construction epoch @@ -904,7 +930,7 @@ where let current_epoch = rpc::query_and_print_epoch(namada).await; let governance_parameters = rpc::query_governance_parameters(namada.client()).await; - let (mut tx_builder, signing_data) = if args.is_pgf_funding { + let (proposal_tx_data, proposal_author) = if args.is_pgf_funding { let proposal = PgfFundingProposal::try_from(args.proposal_data.as_ref()) .map_err(|e| { @@ -916,11 +942,12 @@ where .map_err(|e| { error::TxSubmitError::InvalidProposal(e.to_string()) })?; + let proposal_author = proposal.proposal.author.clone(); - submit_reveal_aux(namada, args.tx.clone(), &proposal.proposal.author) - .await?; - - tx::build_pgf_funding_proposal(namada, &args, proposal).await? + ( + tx::build_pgf_funding_proposal(namada, &args, proposal).await?, + proposal_author, + ) } else if args.is_pgf_stewards { let proposal = PgfStewardProposal::try_from( args.proposal_data.as_ref(), @@ -947,11 +974,12 @@ where .map_err(|e| { error::TxSubmitError::InvalidProposal(e.to_string()) })?; + let proposal_author = proposal.proposal.author.clone(); - submit_reveal_aux(namada, args.tx.clone(), &proposal.proposal.author) - .await?; - - tx::build_pgf_stewards_proposal(namada, &args, proposal).await? + ( + tx::build_pgf_stewards_proposal(namada, &args, proposal).await?, + proposal_author, + ) } else { let proposal = DefaultProposal::try_from(args.proposal_data.as_ref()) .map_err(|e| { @@ -976,19 +1004,24 @@ where .map_err(|e| { error::TxSubmitError::InvalidProposal(e.to_string()) })?; + let proposal_author = proposal.proposal.author.clone(); - submit_reveal_aux(namada, args.tx.clone(), &proposal.proposal.author) - .await?; - - tx::build_default_proposal(namada, &args, proposal).await? + ( + tx::build_default_proposal(namada, &args, proposal).await?, + proposal_author, + ) }; if args.tx.dump_tx { - tx::dump_tx(namada.io(), &args.tx, tx_builder); + tx::dump_tx(namada.io(), &args.tx, proposal_tx_data.0); } else { - sign(namada, &mut tx_builder, &args.tx, signing_data).await?; - - namada.submit(tx_builder, &args.tx).await?; + batch_opt_reveal_pk_and_submit( + namada, + &args.tx, + &proposal_author, + proposal_tx_data, + ) + .await?; } Ok(()) @@ -1096,7 +1129,13 @@ pub async fn submit_reveal_pk( where ::Error: std::fmt::Display, { - submit_reveal_aux(namada, args.tx, &(&args.public_key).into()).await?; + let tx_data = + submit_reveal_aux(namada, &args.tx, &(&args.public_key).into()).await?; + + if let Some((mut tx, signing_data)) = tx_data { + sign(namada, &mut tx, &args.tx, signing_data).await?; + namada.submit(tx, &args.tx).await?; + } Ok(()) } @@ -1108,17 +1147,20 @@ pub async fn submit_bond( where ::Error: std::fmt::Display, { - let default_address = args.source.clone().unwrap_or(args.validator.clone()); - submit_reveal_aux(namada, args.tx.clone(), &default_address).await?; - - let (mut tx, signing_data) = args.build(namada).await?; + let submit_bond_tx_data = args.build(namada).await?; if args.tx.dump_tx { - tx::dump_tx(namada.io(), &args.tx, tx); + tx::dump_tx(namada.io(), &args.tx, submit_bond_tx_data.0); } else { - sign(namada, &mut tx, &args.tx, signing_data).await?; - - namada.submit(tx, &args.tx).await?; + let default_address = + args.source.clone().unwrap_or(args.validator.clone()); + batch_opt_reveal_pk_and_submit( + namada, + &args.tx, + &default_address, + submit_bond_tx_data, + ) + .await?; } Ok(()) From dff222821792eb47c1dfae1595cf3c1514a4bf62 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 28 Aug 2024 18:54:54 +0200 Subject: [PATCH 2/4] Extends pk reveal batching to transfers --- crates/apps_lib/src/client/tx.rs | 89 +++++++++++++++++++------------- 1 file changed, 52 insertions(+), 37 deletions(-) diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index 0a738b835e..d9c5e414fb 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -228,25 +228,32 @@ pub async fn submit_reveal_aux( async fn batch_opt_reveal_pk_and_submit( namada: &N, args: &args::Tx, - owner: &Address, + // FIXME: references here + owners: &[Address], tx_data: (Tx, SigningTxData), -) -> Result<(), error::Error> +) -> Result where ::Error: std::fmt::Display, { - let submit_pk_tx_data = submit_reveal_aux(namada, args, owner).await?; - let mut batched_tx_data = - submit_pk_tx_data.map_or_else(Default::default, |data| vec![data]); + let mut batched_tx_data = vec![]; + + // FIXME: can improve this for loop? + for owner in owners { + if let Some(reveal_pk_tx_data) = + submit_reveal_aux(namada, args, owner).await? + { + batched_tx_data.push(reveal_pk_tx_data); + } + } batched_tx_data.push(tx_data); + let (mut batched_tx, batched_signing_data) = namada_sdk::tx::build_batch(batched_tx_data)?; - for sig_data in batched_signing_data { sign(namada, &mut batched_tx, args, sig_data).await?; } - namada.submit(batched_tx, args).await?; - Ok(()) + namada.submit(batched_tx, args).await } pub async fn submit_bridge_pool_tx( @@ -261,7 +268,7 @@ pub async fn submit_bridge_pool_tx( batch_opt_reveal_pk_and_submit( namada, &args.tx, - &args.sender, + &[args.sender], bridge_pool_tx_data, ) .await?; @@ -285,7 +292,7 @@ where batch_opt_reveal_pk_and_submit( namada, &args.tx, - &args.owner, + &[args.owner], custom_tx_data, ) .await?; @@ -793,19 +800,20 @@ pub async fn submit_transparent_transfer( )); } - let (mut tx, signing_data) = args.clone().build(namada).await?; + let transfer_data = args.clone().build(namada).await?; if args.tx.dump_tx { - tx::dump_tx(namada.io(), &args.tx, tx); + tx::dump_tx(namada.io(), &args.tx, transfer_data.0); } else { - // FIXME: need to extend batch_opt_revel for this to support multiple - // reveal pkls - for datum in args.data.iter() { - submit_reveal_aux(namada, &args.tx, &datum.source).await?; - } - - sign(namada, &mut tx, &args.tx, signing_data).await?; - namada.submit(tx, &args.tx).await?; + let reveal_pks: Vec<_> = + args.data.into_iter().map(|datum| datum.source).collect(); + batch_opt_reveal_pk_and_submit( + namada, + &args.tx, + &reveal_pks, + transfer_data, + ) + .await?; } Ok(()) @@ -830,25 +838,33 @@ pub async fn submit_shielding_transfer( namada: &impl Namada, args: args::TxShieldingTransfer, ) -> Result<(), error::Error> { - // FIXME: same here, extend - for datum in args.data.iter() { - submit_reveal_aux(namada, &args.tx, &datum.source).await?; - } - // Repeat once if the tx fails on a crossover of an epoch for _ in 0..2 { - let (mut tx, signing_data, tx_epoch) = - args.clone().build(namada).await?; + let (tx, signing_data, tx_epoch) = args.clone().build(namada).await?; if args.tx.dump_tx { tx::dump_tx(namada.io(), &args.tx, tx); break; - } else { - sign(namada, &mut tx, &args.tx, signing_data).await?; - let cmt_hash = tx.first_commitments().unwrap().get_hash(); - let wrapper_hash = tx.wrapper_hash(); - let result = namada.submit(tx, &args.tx).await?; - match result { + } + + let cmt_hash = tx.commitments().last().unwrap().get_hash(); + let wrapper_hash = tx.wrapper_hash(); + + let reveal_pks: Vec<_> = args + .data + .clone() + .into_iter() + .map(|datum| datum.source) + .collect(); + let result = batch_opt_reveal_pk_and_submit( + namada, + &args.tx, + &reveal_pks, + (tx, signing_data), + ) + .await?; + + match result { ProcessTxResponse::Applied(resp) if // If a transaction is rejected by a VP matches!( @@ -874,7 +890,6 @@ pub async fn submit_shielding_transfer( // benefit from resubmission _ => break, } - } } Ok(()) } @@ -909,7 +924,7 @@ where batch_opt_reveal_pk_and_submit( namada, &args.tx, - &args.source.effective_address(), + &[args.source.effective_address()], (tx, signing_data), ) .await?; @@ -1018,7 +1033,7 @@ where batch_opt_reveal_pk_and_submit( namada, &args.tx, - &proposal_author, + &[proposal_author], proposal_tx_data, ) .await?; @@ -1157,7 +1172,7 @@ where batch_opt_reveal_pk_and_submit( namada, &args.tx, - &default_address, + &[default_address], submit_bond_tx_data, ) .await?; From 50cbeb65303659ceda39414f5f30d8b55f25d5c8 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 28 Aug 2024 19:03:12 +0200 Subject: [PATCH 3/4] Address as reference in reveal pk --- crates/apps_lib/src/client/tx.rs | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index d9c5e414fb..7716773737 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -210,11 +210,10 @@ pub async fn submit_reveal_aux( .map_err(|e| error::Error::Other(e.to_string()))?; if tx::is_reveal_pk_needed(context.client(), address).await? { - // FIXME: rework this message? yes slightly display_line!( context.io(), "Submitting a tx to reveal the public key for address \ - {address}..." + {address}" ); return Ok(Some( tx::build_reveal_pk(context, args, &public_key).await?, @@ -228,8 +227,7 @@ pub async fn submit_reveal_aux( async fn batch_opt_reveal_pk_and_submit( namada: &N, args: &args::Tx, - // FIXME: references here - owners: &[Address], + owners: &[&Address], tx_data: (Tx, SigningTxData), ) -> Result where @@ -237,7 +235,6 @@ where { let mut batched_tx_data = vec![]; - // FIXME: can improve this for loop? for owner in owners { if let Some(reveal_pk_tx_data) = submit_reveal_aux(namada, args, owner).await? @@ -268,7 +265,7 @@ pub async fn submit_bridge_pool_tx( batch_opt_reveal_pk_and_submit( namada, &args.tx, - &[args.sender], + &[&args.sender], bridge_pool_tx_data, ) .await?; @@ -292,7 +289,7 @@ where batch_opt_reveal_pk_and_submit( namada, &args.tx, - &[args.owner], + &[&args.owner], custom_tx_data, ) .await?; @@ -806,7 +803,7 @@ pub async fn submit_transparent_transfer( tx::dump_tx(namada.io(), &args.tx, transfer_data.0); } else { let reveal_pks: Vec<_> = - args.data.into_iter().map(|datum| datum.source).collect(); + args.data.iter().map(|datum| &datum.source).collect(); batch_opt_reveal_pk_and_submit( namada, &args.tx, @@ -850,12 +847,8 @@ pub async fn submit_shielding_transfer( let cmt_hash = tx.commitments().last().unwrap().get_hash(); let wrapper_hash = tx.wrapper_hash(); - let reveal_pks: Vec<_> = args - .data - .clone() - .into_iter() - .map(|datum| datum.source) - .collect(); + let reveal_pks: Vec<_> = + args.data.iter().map(|datum| &datum.source).collect(); let result = batch_opt_reveal_pk_and_submit( namada, &args.tx, @@ -924,7 +917,7 @@ where batch_opt_reveal_pk_and_submit( namada, &args.tx, - &[args.source.effective_address()], + &[&args.source.effective_address()], (tx, signing_data), ) .await?; @@ -1033,7 +1026,7 @@ where batch_opt_reveal_pk_and_submit( namada, &args.tx, - &[proposal_author], + &[&proposal_author], proposal_tx_data, ) .await?; @@ -1167,8 +1160,7 @@ where if args.tx.dump_tx { tx::dump_tx(namada.io(), &args.tx, submit_bond_tx_data.0); } else { - let default_address = - args.source.clone().unwrap_or(args.validator.clone()); + let default_address = args.source.as_ref().unwrap_or(&args.validator); batch_opt_reveal_pk_and_submit( namada, &args.tx, From 3f8c058ea9d3ec9f17745061036f5b982765f430 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 29 Aug 2024 11:24:25 +0200 Subject: [PATCH 4/4] Changelog #3720 --- .changelog/unreleased/improvements/3720-batch-reveal-pk.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/improvements/3720-batch-reveal-pk.md diff --git a/.changelog/unreleased/improvements/3720-batch-reveal-pk.md b/.changelog/unreleased/improvements/3720-batch-reveal-pk.md new file mode 100644 index 0000000000..46466a48df --- /dev/null +++ b/.changelog/unreleased/improvements/3720-batch-reveal-pk.md @@ -0,0 +1,3 @@ +- If an additional `reveal_pk` transaction is required, the client now groups + it with the actual transaction into a single batch instead of submitting it + separately. ([\#3720](https://github.com/anoma/namada/pull/3720)) \ No newline at end of file