Skip to content

Commit

Permalink
fix(CLI-args): Drop static variable in caching accessor
Browse files Browse the repository at this point in the history
Accessor function `proving_capability` in `cli_args::Args` uses a
`OnceLock` to avoid estimating the current machine's proving
capability twice. Previously, the `OnceLock` object was stored in
a static variable defined locally in the function. As a result, all
instances of struct `Args` gave the same result when this function is
called -- even when the `Args` objects were created with different
values for `tx_proving_capability`.

The solution is to store the `OnceLock` on the `Args` struct properly.
The new field, `tx_proving_capability_cache` is armed with the
`#[clap(skip)]` annotation so that clap does not confuse it for a real
CLI argument.

This commit also adds a test case that verifies the diagnosis (by
failing) and the solution (by passing).
  • Loading branch information
aszepieniec committed Nov 20, 2024
1 parent 5c7e1cd commit ed38cf5
Showing 1 changed file with 22 additions and 3 deletions.
25 changes: 22 additions & 3 deletions src/config_models/cli_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,13 @@ pub struct Args {
#[clap(long)]
pub tx_proving_capability: Option<TxProvingCapability>,

/// Cache for the proving capability. If the above parameter is not set, we
/// want to estimate proving capability and afterwards reuse the result from
/// previous estimations. This argument cannot be set from CLI, so clap
/// ignores it.
#[clap(skip)]
pub tx_proving_capability_cache: OnceLock<TxProvingCapability>,

/// The number of seconds between each attempt to upgrade transactions in
/// the mempool to proofs of a higher quality. Will only run if the machine
/// on which the client runs is powerful enough to produce `SingleProof`s.
Expand Down Expand Up @@ -253,10 +260,9 @@ impl Args {
}

/// Get the proving capability CLI argument or estimate it if it is not set.
/// Cache the result so we don't estimate more than once.
pub(crate) fn proving_capability(&self) -> TxProvingCapability {
static CAPABILITY: OnceLock<TxProvingCapability> = OnceLock::new();

*CAPABILITY.get_or_init(|| {
*self.tx_proving_capability_cache.get_or_init(|| {
if let Some(proving_capability) = self.tx_proving_capability {
proving_capability
} else if self.compose {
Expand Down Expand Up @@ -339,4 +345,17 @@ mod cli_args_tests {
// doubles as a no-crash test
println!("{}", Args::estimate_proving_capability());
}

#[test]
fn cli_args_can_differ_about_proving_capability() {
let a = Args {
tx_proving_capability: Some(TxProvingCapability::ProofCollection),
..Default::default()
};
let b = Args {
tx_proving_capability: Some(TxProvingCapability::SingleProof),
..Default::default()
};
assert_ne!(a.proving_capability(), b.proving_capability());
}
}

0 comments on commit ed38cf5

Please sign in to comment.