[NCC-E005955-WVM] zebra-consensus: Off-by-One Errors and Inconsistent Usage of PARAMETER_DOWNLOAD_MAX_RETRIES

[mpguerra]

Impact

The PARAMETER_DOWNLOAD_MAX_RETRIES may not behave as expected due to a potential off-by-one error.

Description

The parameter PARAMETER_DOWNLOAD_MAX_RETRIES is defined in zebra-consensus/src/
primitives/groth16/params.rs:

zebra/zebra-consensus/src/primitives/groth16/params.rs

Lines 18 to 22 in 5a88fe7

	/// The maximum number of times to retry download parameters.
	///
	/// Zebra will retry to download Sprout of Sapling parameters only if they
	/// failed for whatever reason.
	pub const PARAMETER_DOWNLOAD_MAX_RETRIES: usize = 3;

As noted, this parameter represents the number of times to retry the download of groth16 parameters. However, later in the same file the following description is given:

zebra/zebra-consensus/src/primitives/groth16/params.rs

Lines 63 to 70 in 5a88fe7

	impl Groth16Parameters {
	/// Download if needed, cache, check, and load the Sprout and Sapling Groth16 parameters.
	///
	/// # Panics
	///
	/// If the parameters were downloaded to the wrong path.
	/// After `PARAMETER_DOWNLOAD_MAX_RETRIES` failed download attempts.
	/// If the downloaded or pre-existing parameter files are invalid.

Here, the parameter is claimed to specify the number of download attempts, not the number of retry attempts. The implementation matches the described behavior and uses this parameter to cap the total number of download attempts; see retry_download_sapling_parameters() :

zebra/zebra-consensus/src/primitives/groth16/params.rs

Lines 132 to 144 in 5a88fe7

	let mut retries = 0;
	while let Err(error) = Groth16Parameters::download_sapling_parameters_once(
	sapling_spend_path,
	sapling_output_path,
	) {
	retries += 1;
	if retries >= PARAMETER_DOWNLOAD_MAX_RETRIES {
	panic!(
	"error downloading Sapling parameter files after {} retries. {:?} {}",
	PARAMETER_DOWNLOAD_MAX_RETRIES,
	error,
	Groth16Parameters::failure_hint(),
	);

The same behavior is implemented for retry_download_sprout_parameters() . Documentation for both of these functions is inconsistent:

zebra/zebra-consensus/src/primitives/groth16/params.rs

Lines 120 to 125 in 5a88fe7

	/// Download Sapling parameters and retry [`PARAMETER_DOWNLOAD_MAX_RETRIES`] if it fails.
	///
	/// # Panics
	///
	/// If the parameters were downloaded to the wrong path.
	/// After `PARAMETER_DOWNLOAD_MAX_RETRIES` failed download attempts.

For example, consider a case where PARAMETER_DOWNLOAD_MAX_RETRIES = 1 . Then this
comment specifies the following:

1. “Download Sapling parameters and retry 1 if it fails.” – This should likely read “1 time(s)” if it fails (currently missing the word “time”). This description matches the implied behavior based on the parameter name.
2. Panics “After 1 failed download attempts” – In other words, it will not retry 1 time. This description matches the implemented behavior.

For consistency, the parameter should be uniformly treated as the maximum number of download attempts, or the maximum number of retry attempts, and treated appropriately in all documentation and code.

Recommendation

Revise documentation, parameter names, and implemented behavior to correctly capture the behavior of PARAMETER_DOWNLOAD_MAX_RETRIES.

Location

zebra-consensus/src/primitives/groth16/params.rs

[mpguerra]

Hey team! Please add your planning poker estimate with Zenhub @arya2 @conradoplg @dconnolly @oxarbitrage @teor2345 @upbqdn

[teor2345]

This seems like a low priority, given the impact of the issue.