refactor(zebra-network): rename testnet_parameters in network config

[syszery]

Motivation

Fixes #9729 — The origin of the network parameters in the Config struct is not obvious without inspecting the details of DConfig and its deserialization.

This PR refactors DConfig by replacing network and testnet_parameters with an enum that has variants for default and custom configurations, and updates the deserialization logic to use this new enum structure.

Solution

• Introduce DNetwork enum with variants for default networks and custom Testnet parameters.
• Update Deserialize implementation to handle the new enum structure.
• Preserve all fields from DTestnetParameters for Testnet.

Tests

• Regtest currently always uses default parameters; custom Regtest parameters from the old testnet_parameters are not supported.
• Some acceptance tests fail (zebrad --test acceptance). This is likely because some config files still define testnet_parameters, which the current design no longer supports, or due to the mismatch described above (but it could also be the result of a bug in the deserialization).

failures: config_tests
          regtest_block_templates_are_valid_block_submissions
          restores_non_finalized_state_and_commits_new_blocks
          stored_configs_parsed_correctly
test result: FAILED. 41 passed; 4 failed; 13 ignored; 0 measured; 0 filtered out; finished in 164.68s

Guidance is requested on whether custom Regtest parameters should be supported in this PR. Also, removing testnet_parameters makes old config files incompatible—should this field be kept temporarily?

#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct DConfig {
    ...
    pub network: DNetwork,

    #[serde(default)]
    #[serde(skip_serializing_if = "Option::is_none")]
    pub testnet_parameters: Option<serde_json::Value>, // <-- keep old test configs working

    // rest of fields...
}

PR Checklist

• The PR name is suitable for the release notes.
• The PR follows the contribution guidelines.
• The library crate changelogs are up to date.
• The solution is tested.
• The documentation is up to date.

[syszery]

After investigating the failing tests, I believe the core issues with the current approach are the following:

1. Removing testnet_parameters too early breaks existing configs.
2. Regtest cannot follow the same deserialization path as Testnet: The previous implementation handled them separately, and the refactor apparently needs to restore that distinction.

The latest commit reintroduces testnet_parameters to restore compatibility.
Next TODO: fix impl<'de> Deserialize<'de> for Config to properly separate the Regtest/Testnet logic.

Current failing tests:

failures:
    regtest_block_templates_are_valid_block_submissions
    restores_non_finalized_state_and_commits_new_blocks

test result: FAILED. 43 passed; 2 failed; 13 ignored; 0 measured; 0 filtered out; finished in 170.61s

[Copilot]

Pull request overview

This PR refactors the zebra-network config to improve clarity about where network parameters originate. It replaces separate network (NetworkKind) and testnet_parameters (DTestnetParameters) fields with a single DNetwork enum that can represent either default networks or custom Testnet configurations.

Changes:

• Introduces DNetwork enum with DefaultForKind and TestnetWithParams variants to unify network configuration
• Updates serialization/deserialization logic to use the new enum structure
• Attempts to maintain backwards compatibility by keeping the legacy testnet_parameters field

Comments suppressed due to low confidence (2)

zebra-network/src/config.rs:824

• Unnecessary clones of moved values. The network_name and target_difficulty_limit are cloned after being moved out of *params via destructuring. Since these values are already owned after the destructuring, the .clone() calls are unnecessary and add overhead.

Remove the .clone() calls on lines 800 and 818.

                if let Some(network_name) = network_name.clone() {
                    params_builder = params_builder.with_network_name(network_name)
                }

                if let Some(network_magic) = network_magic {
                    params_builder = params_builder.with_network_magic(Magic(network_magic));
                }

                if let Some(genesis_hash) = genesis_hash {
                    params_builder = params_builder.with_genesis_hash(genesis_hash);
                }

                if let Some(slow_start_interval) = slow_start_interval {
                    params_builder = params_builder.with_slow_start_interval(
                        slow_start_interval.try_into().map_err(de::Error::custom)?,
                    );
                }

                if let Some(target_difficulty_limit) = target_difficulty_limit.clone() {
                    params_builder = params_builder.with_target_difficulty_limit(
                        target_difficulty_limit
                            .parse::<U256>()
                            .map_err(de::Error::custom)?,
                    );
                }

zebra-network/src/config.rs:882

• The legacy testnet_parameters field is extracted from DConfig but never used in the deserialization logic. This breaks backwards compatibility with existing config files that use the old testnet_parameters format.

When an old config file specifies network = "Testnet" along with [network.testnet_parameters], the testnet_parameters will be silently ignored, and the default Testnet will be used instead. This contradicts the stated goal of maintaining backwards compatibility.

The deserialization logic needs to check if testnet_parameters is present and use it as a fallback when dnetwork is DefaultForKind(NetworkKind::Testnet). For example:

let network = match (dnetwork, testnet_parameters) {
    (DNetwork::DefaultForKind(NetworkKind::Mainnet), _) => Network::Mainnet,
    (DNetwork::DefaultForKind(NetworkKind::Testnet), Some(params)) => {
        // Use legacy testnet_parameters
        // ... build testnet from params ...
    }
    (DNetwork::DefaultForKind(NetworkKind::Testnet), None) => Network::new_default_testnet(),
    // ... rest of cases
};

        let network = match dnetwork {
            DNetwork::DefaultForKind(NetworkKind::Mainnet) => Network::Mainnet,
            DNetwork::DefaultForKind(NetworkKind::Testnet) => Network::new_default_testnet(),
            DNetwork::DefaultForKind(NetworkKind::Regtest) => {
                Network::new_regtest(Default::default())
            }
            DNetwork::TestnetWithParams(params) => {
                let DTestnetParameters {
                    network_name,
                    network_magic,
                    slow_start_interval,
                    target_difficulty_limit,
                    disable_pow,
                    genesis_hash,
                    activation_heights,
                    pre_nu6_funding_streams,
                    post_nu6_funding_streams,
                    funding_streams,
                    pre_blossom_halving_interval,
                    lockbox_disbursements,
                    checkpoints,
                    extend_funding_stream_addresses_as_required,
                } = *params;

                let mut params_builder = testnet::Parameters::build();

                if let Some(network_name) = network_name.clone() {
                    params_builder = params_builder.with_network_name(network_name)
                }

                if let Some(network_magic) = network_magic {
                    params_builder = params_builder.with_network_magic(Magic(network_magic));
                }

                if let Some(genesis_hash) = genesis_hash {
                    params_builder = params_builder.with_genesis_hash(genesis_hash);
                }

                if let Some(slow_start_interval) = slow_start_interval {
                    params_builder = params_builder.with_slow_start_interval(
                        slow_start_interval.try_into().map_err(de::Error::custom)?,
                    );
                }

                if let Some(target_difficulty_limit) = target_difficulty_limit.clone() {
                    params_builder = params_builder.with_target_difficulty_limit(
                        target_difficulty_limit
                            .parse::<U256>()
                            .map_err(de::Error::custom)?,
                    );
                }

                if let Some(disable_pow) = disable_pow {
                    params_builder = params_builder.with_disable_pow(disable_pow);
                }

                // Retain default Testnet activation heights unless there's an empty [testnet_parameters.activation_heights] section.
                if let Some(activation_heights) = activation_heights {
                    params_builder = params_builder.with_activation_heights(activation_heights)
                }

                if let Some(halving_interval) = pre_blossom_halving_interval {
                    params_builder = params_builder.with_halving_interval(halving_interval.into())
                }

                // Set configured funding streams after setting any parameters that affect the funding stream address period.
                let mut funding_streams_vec = funding_streams.unwrap_or_default();

                if let Some(funding_streams) = post_nu6_funding_streams {
                    funding_streams_vec.insert(0, funding_streams);
                }

                if let Some(funding_streams) = pre_nu6_funding_streams {
                    funding_streams_vec.insert(0, funding_streams);
                }

                if !funding_streams_vec.is_empty() {
                    params_builder = params_builder.with_funding_streams(funding_streams_vec);
                }

                if let Some(lockbox_disbursements) = lockbox_disbursements {
                    params_builder =
                        params_builder.with_lockbox_disbursements(lockbox_disbursements);
                }

                params_builder = params_builder.with_checkpoints(checkpoints);

                if let Some(true) = extend_funding_stream_addresses_as_required {
                    params_builder = params_builder.extend_funding_streams();
                }

                // Return an error if the initial testnet peers includes any of the default initial Mainnet or Testnet
                // peers and the configured network parameters are incompatible with the default public Testnet.
                if !params_builder.is_compatible_with_default_parameters()
                    && contains_default_initial_peers(&initial_testnet_peers)
                {
                    return Err(de::Error::custom(
                        "cannot use default initials peers with incompatible testnet",
                    ));
                };

                // Return the default Testnet if no network name was configured and all parameters match the default Testnet
                if network_name.is_none() && params_builder == testnet::Parameters::build() {
                    Network::new_default_testnet()
                } else {
                    params_builder.to_network()
                }
            }
        };

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The untagged enum deserialization may have ambiguity issues. Since DNetwork is untagged, serde will try to deserialize as NetworkKind first, then as DTestnetParameters.

When a user provides a config with network = { network_name = "MyTestnet", ... }, serde will first attempt to deserialize this as a NetworkKind string, which will fail. Only then will it try DTestnetParameters, which should succeed.

However, there's a potential issue: if someone tries to use network = "Testnet" in a config, it will be deserialized as DefaultForKind(NetworkKind::Testnet) rather than looking for custom parameters. The untagged approach means there's no way to explicitly request "parse this as custom testnet params" vs "parse this as a network kind".

This should work correctly for the intended use cases, but consider adding a comment explaining the deserialization order and behavior to aid future maintainers.

[syszery]

This seems to be the issue, why the tests fail. After adding a variant for Regtest, I've tested all possible orders and all lead to different kinds of bugs or test failures. I've commented on this in the message below.

[oxarbitrage]

Hi @syszery , I think the approach here is right, i will say keep going with it. A few things to fix:

• Use testnet_parameters in Deserialize
  Match on both fields, like the original code did:

   let network = match (dnetwork, testnet_parameters) {
       (DNetwork::ConfiguredTestnet(params), _)                          => build_configured_testnet(*params)?,
       (DNetwork::DefaultForKind(NetworkKind::Mainnet), _)               => Network::Mainnet,
       (DNetwork::DefaultForKind(NetworkKind::Testnet), Some(params))    => build_configured_testnet(params)?,
       (DNetwork::DefaultForKind(NetworkKind::Testnet), None)            => Network::new_default_testnet(),
       (DNetwork::DefaultForKind(NetworkKind::Regtest), Some(params))    => Network::new_regtest(build_regtest_params(params)),
       (DNetwork::DefaultForKind(NetworkKind::Regtest), None)            => Network::new_regtest(Default::default()),
   };

• Yes, we need to keep custom Regtest support
  The original (NetworkKind::Regtest, Some(params)) arm was removed. Add a ConfiguredRegtest(Box<DTestnetParameters>) variant for the new format, and restore the legacy path.

• Yes, keep testnet_parameters as a deprecated alias
  Leave it as #[serde(default, skip_serializing_if = "Option::is_none")] so old configs keep working.

• Small things

  • Rename TestnetWithParams as ConfiguredTestnet
  • Add a doc comment on DNetwork explaining untagged variant ordering
  • Add a unit test covering both old and new formats if possible

Let me know if you still want to work on those or i can give them a try.

[syszery]

Hi @oxarbitrage, thanks for the feedback. I‘ll try to fix this.

[syszery]

Apologies for the delay. I'm still working on this but need a bit more time than expected.

[oxarbitrage]

Hi @syszery, just checking in. Are you still working on the changes from the review? No rush, but if you're blocked or don't have bandwidth, happy to pick this up. Let me know!

[syszery]

Hi @oxarbitrage, apologies for the delay. I’ve been a bit tied up recently. I should be able to wrap it up over the next week.

[syszery]

Hi @oxarbitrage, I’ve implemented most of the suggested changes. Thanks again for the clear guidance. This is much appreciated.

However, there are still 4 failing acceptance tests:

failures:
    invalidate_and_reconsider_block
    regtest_block_templates_are_valid_block_submissions
    restores_non_finalized_state_and_commits_new_blocks
    trusted_chain_sync_handles_forks_correctly

All fail with:

cannot use reserved network name 'Regtest' as configured Testnet name

I believe the root cause is deserialization: both ConfiguredTestnet and ConfiguredRegtest currently deserialize from the same structure (DTestnetParameters). Reordering the variants only shifts the bug (network tests fail or acceptance tests hang).

I’m not sure of the best way to distinguish Regtest from Testnet. Is untagged my problem (and just the wrong approach)? Do I miss a simpler solution?