Skip to content

Commit

Permalink
keys: 🥰 Address defers computation of diversified base (#4285)
Browse files Browse the repository at this point in the history
### 💬 describe your changes

fixes #1957.

the last remaining item mentioned in #1957 is that currently, while
`Address`
_caches_ the diversified base point, it eagerly computes it when
`Address::from_components()` is called.

we can defer this action until the base point is needed, using
`OnceLock<T>`.
in order to do that, we must revoke the `Copy` derivation. essentially,
this
branch makes the following change(s) to `Address`:

```diff
-#[derive(Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(try_from = "pb::Address", into = "pb::Address")]
 pub struct Address {
     d: Diversifier,
     /// cached copy of the diversified base
-    g_d: decaf377::Element,
+    g_d: OnceLock<decaf377::Element>,
 
     /// extra invariant: the bytes in pk_d should be the canonical encoding of an
     /// s value (whether or not it is a valid decaf377 encoding)
     /// this ensures we can use a PaymentAddress to form a note commitment,
     /// which involves hashing s as a field element.
     pk_d: ka::Public,
     /// transmission key s value
     transmission_key_s: Fq,
 
     ck_d: fmd::ClueKey,
 }
```
out of that, a few other types also lose their `Copy` property:
* AddressView
* FundingStream
* Recipient

..which leads to the addition of explicit `.clone()` calls in various
places.

largely, that tends to affect test code, but it is a more invasive
change than
expected, and i want to draw attention to that. it should _not_ be a
change in
behavior, however. these are the same copies as previously made, but are
now
performed explicitly.

for more, see the `Copy` documentation:

https://doc.rust-lang.org/stable/std/marker/trait.Copy.html#whats-the-difference-between-copy-and-clone

some extra documentation, a small compile-time check to enshrine
thread-safety
properties (`Send` and `Sync`), and an extra `From<T>` implementation
are added
too.

#### ➕ changes

to facilitate review and to make this easier to reason about, this
branch is
divided into distinct steps:

* keys: 🔖 add more `Address` documentation
* keys: 🙂 add compile-time assertions about addresses
* keys: 📫 `pb::Address: From<&Address>`
* keys: 👯 `Address` is not `Copy`
* keys: 🥰 defer diversified base using `OnceLock<T>`

...reviewers are encouraged to review this step-by-step. :mag: 

**NB:** using `OnceLock<T>` is the core change at play here.

### issue ticket number and link

* #1957

### checklist before requesting a review

- [x] if this code contains consensus-breaking changes, i have added the
"consensus-breaking" label. otherwise, i declare my belief that there
are not consensus-breaking changes, for the following reason:

  > this does not change the behavior of addresses, it only defers the
  > computation of the diversified base point.
  • Loading branch information
cratelyn authored Apr 29, 2024
1 parent 8d3f4f9 commit 673446e
Show file tree
Hide file tree
Showing 29 changed files with 252 additions and 122 deletions.
2 changes: 1 addition & 1 deletion crates/bin/pcli/src/command/ceremony.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl CeremonyCmd {
bid, address
);

handle_bid(app, *coordinator_address, index, bid).await?;
handle_bid(app, coordinator_address.clone(), index, bid).await?;

println!("connecting to coordinator...");
// After we bid, we need to wait a couple of seconds just for the transaction to be
Expand Down
6 changes: 3 additions & 3 deletions crates/bin/pcli/src/command/tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use penumbra_asset::{asset, asset::Metadata, Value, STAKING_TOKEN_ASSET_ID};
use penumbra_dex::{lp::position, swap_claim::SwapClaimPlan};
use penumbra_fee::Fee;
use penumbra_governance::{proposal::ProposalToml, proposal_state::State as ProposalState, Vote};
use penumbra_keys::keys::AddressIndex;
use penumbra_keys::{keys::AddressIndex, Address};
use penumbra_num::Amount;
use penumbra_proto::{
core::component::{
Expand Down Expand Up @@ -346,7 +346,7 @@ impl TxCmd {
.map(|v| v.parse())
.collect::<Result<Vec<Value>, _>>()?;
let to = to
.parse()
.parse::<Address>()
.map_err(|_| anyhow::anyhow!("address is invalid"))?;

let return_address = app
Expand All @@ -364,7 +364,7 @@ impl TxCmd {
.set_gas_prices(gas_prices)
.set_fee_tier((*fee_tier).into());
for value in values.iter().cloned() {
planner.output(value, to);
planner.output(value, to.clone());
}
let plan = planner
.memo(memo_plaintext)?
Expand Down
6 changes: 3 additions & 3 deletions crates/bin/pclientd/tests/network_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
//! where no tokens have been delegated, and the address with index 0
//! was distributedp 1cube.
use std::process::Command as StdCommand;
use std::{ops::Deref, process::Command as StdCommand};

use anyhow::Context;
use assert_cmd::cargo::CommandCargoExt;
Expand Down Expand Up @@ -120,7 +120,7 @@ async fn transaction_send_flow() -> anyhow::Result<()> {
let plan = view_client
.transaction_planner(TransactionPlannerRequest {
outputs: vec![tpr::Output {
address: Some((*test_keys::ADDRESS_1).into()),
address: Some(test_keys::ADDRESS_1.deref().clone().into()),
value: Some(
Value {
amount: 1_000_000u64.into(),
Expand Down Expand Up @@ -304,7 +304,7 @@ async fn swap_claim_flow() -> anyhow::Result<()> {
amount: Some(num::Amount { lo: 0, hi: 0 }),
asset_id: None,
}),
claim_address: Some((*test_keys::ADDRESS_1).into()),
claim_address: Some(test_keys::ADDRESS_1.deref().clone().into()),
}],
..Default::default()
})
Expand Down
12 changes: 9 additions & 3 deletions crates/core/app/src/action_handler/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ impl AppActionHandler for Transaction {

#[cfg(test)]
mod tests {
use std::ops::Deref;

use anyhow::Result;
use penumbra_asset::{Value, STAKING_TOKEN_ASSET_ID};
use penumbra_fee::Fee;
Expand Down Expand Up @@ -163,10 +165,14 @@ mod tests {
actions: vec![
SpendPlan::new(&mut OsRng, note, auth_path.position()).into(),
SpendPlan::new(&mut OsRng, note2, auth_path2.position()).into(),
OutputPlan::new(&mut OsRng, value, *test_keys::ADDRESS_1).into(),
OutputPlan::new(&mut OsRng, value, test_keys::ADDRESS_1.deref().clone()).into(),
],
detection_data: Some(DetectionDataPlan {
clue_plans: vec![CluePlan::new(&mut OsRng, *test_keys::ADDRESS_1, 1)],
clue_plans: vec![CluePlan::new(
&mut OsRng,
test_keys::ADDRESS_1.deref().clone(),
1,
)],
}),
memo: None,
};
Expand Down Expand Up @@ -228,7 +234,7 @@ mod tests {
},
actions: vec![
SpendPlan::new(&mut OsRng, note, auth_path.position()).into(),
OutputPlan::new(&mut OsRng, value, *test_keys::ADDRESS_1).into(),
OutputPlan::new(&mut OsRng, value, test_keys::ADDRESS_1.deref().clone()).into(),
],
detection_data: None,
memo: None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use {
GovernanceKey, IdentityKey,
},
rand_core::OsRng,
std::ops::Deref,
tap::Tap,
tracing::{error_span, info, Instrument},
};
Expand Down Expand Up @@ -251,13 +252,16 @@ async fn app_can_define_and_delegate_to_a_validator() -> anyhow::Result<()> {
let output = OutputPlan::new(
&mut rand_core::OsRng,
delegate.delegation_value(),
*test_keys::ADDRESS_1,
test_keys::ADDRESS_1.deref().clone(),
);
let mut plan = TransactionPlan {
actions: vec![spend.into(), output.into(), delegate.into()],
// Now fill out the remaining parts of the transaction needed for verification:
memo: MemoPlan::new(&mut OsRng, MemoPlaintext::blank_memo(*test_keys::ADDRESS_0))
.map(Some)?,
memo: MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)
.map(Some)?,
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
chain_id: TestNode::<()>::CHAIN_ID.to_string(),
Expand Down Expand Up @@ -410,14 +414,17 @@ async fn app_can_define_and_delegate_to_a_validator() -> anyhow::Result<()> {
let output = OutputPlan::new(
&mut rand_core::OsRng,
undelegate.unbonded_value(),
*test_keys::ADDRESS_1,
test_keys::ADDRESS_1.deref().clone(),
);

let mut plan = TransactionPlan {
actions: vec![spend.into(), output.into(), undelegate.into()],
// Now fill out the remaining parts of the transaction needed for verification:
memo: MemoPlan::new(&mut OsRng, MemoPlaintext::blank_memo(*test_keys::ADDRESS_0))
.map(Some)?,
memo: MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)
.map(Some)?,
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
chain_id: TestNode::<()>::CHAIN_ID.to_string(),
Expand Down
13 changes: 9 additions & 4 deletions crates/core/app/tests/app_can_disable_community_pool_spends.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use {
},
rand::Rng,
rand_core::OsRng,
std::collections::BTreeMap,
std::{collections::BTreeMap, ops::Deref},
tap::{Tap, TapFallible},
tracing::{error_span, info, Instrument},
};
Expand Down Expand Up @@ -204,7 +204,7 @@ async fn app_can_disable_community_pool_spends() -> anyhow::Result<()> {
CommunityPoolSpend { value }.into(),
CommunityPoolOutput {
value,
address: *test_keys::ADDRESS_0,
address: test_keys::ADDRESS_0.deref().clone(),
}
.into(),
],
Expand Down Expand Up @@ -233,12 +233,17 @@ async fn app_can_disable_community_pool_spends() -> anyhow::Result<()> {
actions: vec![
proposal,
// Next, create a new output of the exact same amount.
OutputPlan::new(&mut OsRng, proposal_nft_value, *test_keys::ADDRESS_0).into(),
OutputPlan::new(
&mut OsRng,
proposal_nft_value,
test_keys::ADDRESS_0.deref().clone(),
)
.into(),
],
// Now fill out the remaining parts of the transaction needed for verification:
memo: Some(MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(*test_keys::ADDRESS_0),
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)?),
detection_data: None,
transaction_parameters: TransactionParameters {
Expand Down
13 changes: 9 additions & 4 deletions crates/core/app/tests/app_can_propose_community_pool_spends.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use {
},
rand::Rng,
rand_core::OsRng,
std::collections::BTreeMap,
std::{collections::BTreeMap, ops::Deref},
tap::{Tap, TapFallible},
tracing::{error_span, info, Instrument},
};
Expand Down Expand Up @@ -198,7 +198,7 @@ async fn app_can_propose_community_pool_spends() -> anyhow::Result<()> {
CommunityPoolSpend { value }.into(),
CommunityPoolOutput {
value,
address: *test_keys::ADDRESS_0,
address: test_keys::ADDRESS_0.deref().clone(),
}
.into(),
],
Expand Down Expand Up @@ -227,12 +227,17 @@ async fn app_can_propose_community_pool_spends() -> anyhow::Result<()> {
actions: vec![
proposal,
// Next, create a new output of the exact same amount.
OutputPlan::new(&mut OsRng, proposal_nft_value, *test_keys::ADDRESS_0).into(),
OutputPlan::new(
&mut OsRng,
proposal_nft_value,
test_keys::ADDRESS_0.deref().clone(),
)
.into(),
],
// Now fill out the remaining parts of the transaction needed for verification:
memo: Some(MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(*test_keys::ADDRESS_0),
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)?),
detection_data: None,
transaction_parameters: TransactionParameters {
Expand Down
10 changes: 8 additions & 2 deletions crates/core/app/tests/app_can_spend_notes_and_detect_outputs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use {
memo::MemoPlaintext, plan::MemoPlan, TransactionParameters, TransactionPlan,
},
rand_core::OsRng,
std::ops::Deref,
tap::{Tap, TapFallible},
tracing::info,
};
Expand Down Expand Up @@ -63,12 +64,17 @@ async fn app_can_spend_notes_and_detect_outputs() -> anyhow::Result<()> {
)
.into(),
// Next, create a new output of the exact same amount.
OutputPlan::new(&mut OsRng, input_note.value(), *test_keys::ADDRESS_1).into(),
OutputPlan::new(
&mut OsRng,
input_note.value(),
test_keys::ADDRESS_1.deref().clone(),
)
.into(),
],
// Now fill out the remaining parts of the transaction needed for verification:
memo: Some(MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(*test_keys::ADDRESS_0),
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)?),
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
Expand Down
26 changes: 18 additions & 8 deletions crates/core/app/tests/app_can_undelegate_from_a_validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use {
memo::MemoPlaintext, plan::MemoPlan, TransactionParameters, TransactionPlan,
},
rand_core::OsRng,
std::ops::Deref,
tap::Tap,
tracing::{error_span, info, Instrument},
};
Expand Down Expand Up @@ -132,13 +133,16 @@ async fn app_can_undelegate_from_a_validator() -> anyhow::Result<()> {
let output = OutputPlan::new(
&mut rand_core::OsRng,
delegate.delegation_value(),
*test_keys::ADDRESS_1,
test_keys::ADDRESS_1.deref().clone(),
);
let mut plan = TransactionPlan {
actions: vec![spend.into(), output.into(), delegate.into()],
// Now fill out the remaining parts of the transaction needed for verification:
memo: MemoPlan::new(&mut OsRng, MemoPlaintext::blank_memo(*test_keys::ADDRESS_0))
.map(Some)?,
memo: MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)
.map(Some)?,
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
chain_id: TestNode::<()>::CHAIN_ID.to_string(),
Expand Down Expand Up @@ -230,13 +234,16 @@ async fn app_can_undelegate_from_a_validator() -> anyhow::Result<()> {
let output = OutputPlan::new(
&mut rand_core::OsRng,
undelegate.unbonded_value(),
*test_keys::ADDRESS_1,
test_keys::ADDRESS_1.deref().clone(),
);
let mut plan = TransactionPlan {
actions: vec![spend.into(), output.into(), undelegate.into()],
// Now fill out the remaining parts of the transaction needed for verification:
memo: MemoPlan::new(&mut OsRng, MemoPlaintext::blank_memo(*test_keys::ADDRESS_0))
.map(Some)?,
memo: MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)
.map(Some)?,
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
chain_id: TestNode::<()>::CHAIN_ID.to_string(),
Expand Down Expand Up @@ -317,8 +324,11 @@ async fn app_can_undelegate_from_a_validator() -> anyhow::Result<()> {
let mut plan = TransactionPlan {
actions: vec![claim.into()],
// Now fill out the remaining parts of the transaction needed for verification:
memo: MemoPlan::new(&mut OsRng, MemoPlaintext::blank_memo(*test_keys::ADDRESS_0))
.map(Some)?,
memo: MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)
.map(Some)?,
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
chain_id: TestNode::<()>::CHAIN_ID.to_string(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use {
FundingStreams, GovernanceKey, IdentityKey, Uptime,
},
rand_core::OsRng,
std::ops::Deref,
tap::Tap,
tracing::{error_span, Instrument},
};
Expand Down Expand Up @@ -191,13 +192,16 @@ async fn app_tracks_uptime_for_validators_only_once_active() -> anyhow::Result<(
let output = OutputPlan::new(
&mut rand_core::OsRng,
delegate.delegation_value(),
*test_keys::ADDRESS_1,
test_keys::ADDRESS_1.deref().clone(),
);
let mut plan = TransactionPlan {
actions: vec![spend.into(), output.into(), delegate.into()],
// Now fill out the remaining parts of the transaction needed for verification:
memo: MemoPlan::new(&mut OsRng, MemoPlaintext::blank_memo(*test_keys::ADDRESS_0))
.map(Some)?,
memo: MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)
.map(Some)?,
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
chain_id: TestNode::<()>::CHAIN_ID.to_string(),
Expand Down Expand Up @@ -312,14 +316,17 @@ async fn app_tracks_uptime_for_validators_only_once_active() -> anyhow::Result<(
let output = OutputPlan::new(
&mut rand_core::OsRng,
undelegate.unbonded_value(),
*test_keys::ADDRESS_1,
test_keys::ADDRESS_1.deref().clone(),
);

let mut plan = TransactionPlan {
actions: vec![spend.into(), output.into(), undelegate.into()],
// Now fill out the remaining parts of the transaction needed for verification:
memo: MemoPlan::new(&mut OsRng, MemoPlaintext::blank_memo(*test_keys::ADDRESS_0))
.map(Some)?,
memo: MemoPlan::new(
&mut OsRng,
MemoPlaintext::blank_memo(test_keys::ADDRESS_0.deref().clone()),
)
.map(Some)?,
detection_data: None, // We'll set this automatically below
transaction_parameters: TransactionParameters {
chain_id: TestNode::<()>::CHAIN_ID.to_string(),
Expand Down
4 changes: 2 additions & 2 deletions crates/core/app/tests/swap_and_swap_claim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ async fn swap_and_swap_claim() -> anyhow::Result<()> {
let delta_1 = Amount::from(100_000u64);
let delta_2 = Amount::from(0u64);
let fee = Fee::default();
let claim_address: Address = *test_keys::ADDRESS_0;
let claim_address: Address = test_keys::ADDRESS_0.deref().clone();

let plaintext =
SwapPlaintext::new(&mut rng, trading_pair, delta_1, delta_2, fee, claim_address);
Expand Down Expand Up @@ -295,7 +295,7 @@ async fn swap_with_nonzero_fee() -> anyhow::Result<()> {
let delta_1 = Amount::from(100_000u64);
let delta_2 = Amount::from(0u64);
let fee = Fee::from_staking_token_amount(Amount::from(1u64));
let claim_address: Address = *test_keys::ADDRESS_0;
let claim_address: Address = test_keys::ADDRESS_0.deref().clone();

let plaintext =
SwapPlaintext::new(&mut rng, trading_pair, delta_1, delta_2, fee, claim_address);
Expand Down
Loading

0 comments on commit 673446e

Please sign in to comment.