feat(wallet): add check for network and genesis_hash consistency

[oleonardolima]

fixes #1716

Description

Adds a new check on both create_with_params and load_with_params to validate that the given genesis_hash parameter matches the genesis_hash for expected Network, returns a new DescriptorError::Mismatch or LoadError::LoadMismatch error variants, respectively.

Notes to the reviewers

I'm not sure if adding the check on load_with_params is needed, and if that branch will ever be reached, at least it seems not trivial to test it using the standard of tests we currently have, that its: 1) creating a wallet; 2) loading and asserting the expected checks.

Changelog notice

• Adds a new check for consistency of genesis_hash and network parameters.
• Adds a new error MismatchError enum and Descriptor::Mismatch variant.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[oleonardolima]

I'm still unsure of how to properly test this one (or if it's even needed), because I can't initially create a wallet with network and genesis_hash that are valid (consistent) on create_with_params, but it will fail with an inconsistent check_genesis_hash on load_with_params.

It'll always fail on the previous check.

[ValuedMammal]

I had previously understood that bdk allowed you to use a custom genesis hash, meaning one that doesn't match the chain hash of the specified network. I interpreted the issue as saying the network isn't checked to be among the valid networks of a descriptor, but as you said that logic exists in into_wallet_descriptor.

[evanlinjin]

Approach-NACK, Concept-NACK

It seems the intention of the PR is to enforce a 1-possible-genesis-hash-per-network-variant invariance. The correct way to do this is to change the datastructure, not enforce the invariance ad-hoc with errors.

So now the question is whether we want to be able have the caller provide a custom genesis hash. I think we still should, but maybe our approach needs to be a bit different.

It is true that the Network enum is intended to represent a single network per variant so the current code is hard to reason with. To fix this, maybe we can introduce a new enum:

pub enum BdkNetwork {
    Network(bitcoin::Network),
    Custom(BlockHash),
}

impl BdkNetwork {
    /// Handy to work with keys.
    ///
    /// The `Custom` variant will always be `Text`.
    pub fn kind(&self) -> bitcoin::NetworkKind {}
}

[evanlinjin]

As mentioned by @ValuedMammal, the intention here is so the caller can provide a custom genesis hash.

However, if the intention is to enforce a 1-possible-genesis-hash-per-network-variant invariance, changing the data structure to disallow the invariance would make a lot more sense. I.e. remove params.genesis_hash.

[oleonardolima]

Approach-NACK, Concept-NACK

It seems the intention of the PR is to enforce a 1-possible-genesis-hash-per-network-variant invariance. The correct way to do this is to change the datastructure, not enforce the invariance ad-hoc with errors.

Agree, if we opt-out from allowing custom genesis_hash/networks, we can just remove the parameter altogether.

So now the question is whether we want to be able have the caller provide a custom genesis hash. I think we still should, but maybe our approach needs to be a bit different.

Yes, I agree with also allowing a custom genesis_hash. I didn't have that in mind when working on the changes. However, by the initial issue description, I reasoned that the current implementation is too loose and would allow a user to use a custom Genesis hash on Mainnet and Testnet.

It is true that the Network enum is intended to represent a single network per variant so the current code is hard to reason with. To fix this, maybe we can introduce a new enum:

pub enum BdkNetwork {
    Network(bitcoin::Network),
    Custom(BlockHash),
}

impl BdkNetwork {
    /// Handy to work with keys.
    ///
    /// The `Custom` variant will always be `Text`.
    pub fn kind(&self) -> bitcoin::NetworkKind {}
}

Thanks, this seems like a better approach.