Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add constants to ChainHash for each Network #1283

Merged
merged 1 commit into from Sep 16, 2022

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Sep 16, 2022

ChainHash::using_genesis_block can't be made const because it uses a match expression, which is only valid in Rust 1.46. Add individual constants as a workaround so that ChainHash can be used in const contexts.

`ChainHash::using_genesis_block` can't be made `const` because it uses a
`match` expression, which is only valid in Rust 1.46. Add individual
constants as a workaround so that `ChainHash` can be used in `const`
contexts.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b1d8516

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b1d8516

@tcharding
Copy link
Member

Thanks for the contribution man!

@apoelstra apoelstra merged commit ba642da into rust-bitcoin:master Sep 16, 2022
@Kixunil
Copy link
Collaborator

Kixunil commented Sep 16, 2022

Oh, I didn't see this soon enough

can't be made const because it uses a match expression

Actually, it can!

let hashes = [GENESIS_BLOCK_HASH_BITCOIN, GENESIS_BLOCK_HASH_TESTNET, GENESIS_BLOCK_HASH_SIGNET, GENESIS_BLOCK_HASH_REGTEST];
ChainHash(hashes[network as usize])

I don't mind making them public, it's useful anywas, but having const fn too would be nice. :)

@jkczyz
Copy link
Contributor Author

jkczyz commented Sep 16, 2022

Yeah, I could put that together. Is there any concern that adding a new Network type won't cause a compilation error? Maybe there is a way to cause a test breakage? Looks like core::mem::variant_count could be useful but is unfortunately unstable.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 16, 2022

We could write a dummy test that matches all variants returning () and put a comment in it saying to remember adding the value into the fn. However we don't have #[non_exhaustive] on Network now which means we can't extend it. It seems bad but maybe there's a reason?

@apoelstra
Copy link
Member

No reason, just that Network long predates #[non_exhaustive].

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 16, 2022

I'm surprised we missed it before. I guess I will go through all of enums at some point.

@apoelstra
Copy link
Member

Yeah, we should -- but I think so far we only went through error enums because those were easy targets for non_exhaustie.

apoelstra added a commit that referenced this pull request Sep 16, 2022
92ef41b Make `ChainHash::using_genesis_block` constant (Jeffrey Czyz)

Pull request description:

  ChainHash::using_genesis_block can't be `const` if it uses a `match` expression prior to Rust 1.46. Use an array mapping to work around this limitation.

  Follow-up suggested in [#1283](#1283 (comment)).

ACKs for top commit:
  apoelstra:
    ACK 92ef41b
  Kixunil:
    ACK 92ef41b

Tree-SHA512: 71f95877c8e5335012ad0339e1f8691e3b33344fa02ecc24c3d4d728232cb7b0de62aec20eb1855b23eeccfbc2eeab920b21ee2243d95c6c89fa8ad5bc846975
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants