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 chain hash type using consts #878

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 72 additions & 3 deletions src/blockdata/constants.rs
Expand Up @@ -176,15 +176,40 @@ pub fn genesis_block(network: Network) -> Block {
}
}

// Mainnet value can be verified at https://github.com/lightning/bolts/blob/master/00-introduction.md
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would rather have this bitcoin core source than a lighting spec. But it's okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this even part of Bitcoin Core? From my understanding this is a LN and DLC thing.

Copy link
Member

Choose a reason for hiding this comment

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

The Genesis block? I think that's a Bitcoin thing :)

Copy link
Member Author

Choose a reason for hiding this comment

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

lol, I meant that chain hash is a LN/DLC thing and the LN spec says that the identifier for mainnet is the hash of the genesis block - hence I linked to the LN spec.

@sanket1729 would you prefer that this comment explained what the value used is but then linked to the bitcoin core source for the actual value?

Copy link
Member

Choose a reason for hiding this comment

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

I am okay without a comment here. But I have a slight preference to link to the bitcoin core source. When reading a constant GENESIS_BLOCK_HASH_BITCOIN in file src/blockdata/constants in rust-bitcoin, I would expect it to be a bitcoin thing. It would feel odd that I need to check a lightning spec to double-check that the bitcoin genesis block is correct.

ChainHash functions can link to lightning spec if needed.

const GENESIS_BLOCK_HASH_BITCOIN: [u8; 32] = [111, 226, 140, 10, 182, 241, 179, 114, 193, 166, 162, 70, 174, 99, 247, 79, 147, 30, 131, 101, 225, 90, 8, 156, 104, 214, 25, 0, 0, 0, 0, 0];
const GENESIS_BLOCK_HASH_TESTNET: [u8; 32] = [67, 73, 127, 215, 248, 38, 149, 113, 8, 244, 163, 15, 217, 206, 195, 174, 186, 121, 151, 32, 132, 233, 14, 173, 1, 234, 51, 9, 0, 0, 0, 0];
const GENESIS_BLOCK_HASH_SIGNET: [u8; 32] = [246, 30, 238, 59, 99, 163, 128, 164, 119, 160, 99, 175, 50, 178, 187, 201, 124, 159, 249, 240, 31, 44, 66, 37, 233, 115, 152, 129, 8, 0, 0, 0];
const GENESIS_BLOCK_HASH_REGTEST: [u8; 32] = [6, 34, 110, 70, 17, 26, 11, 89, 202, 175, 18, 96, 67, 235, 91, 191, 40, 195, 79, 58, 94, 51, 42, 31, 199, 178, 183, 60, 241, 136, 145, 15];

/// The uniquely identifying hash of the target blockchain.
Copy link
Member

Choose a reason for hiding this comment

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

Can we just say this is the Genesis hash?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not restricted to being the genesis block hash (according to #878 (comment)). Re-reading that comment made me think that the word 'hash' shouldn't even be in the comment but then its in the type name so ¯\_(ツ)_/¯

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it should be AssetId then?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use AssetId then this is no longer anything to do with the LN spec's chain hash. Perhaps this comment, along with @TheBlueMatt's observation that this PR is just a new way of getting the genesis block without hashing implies that we should not add the ChainHash type at all and just expose a way for folk to get the genesis block without hashing so they can use it if they wish to create a ChainHash type themselves?

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct ChainHash([u8; 32]);
impl_array_newtype!(ChainHash, u8, 32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can use the approach you proposed in #870 just to avoid forgetting to re-base that PR after this one got merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not fully happy with #870, that's why I ended up using impl_array_newtype so that this one is in line with the current code.

impl_bytes_newtype!(ChainHash, 32);

impl ChainHash {
/// Returns the hash of the `network` genesis block for use as a chain hash.
///
/// See [BOLT 0](https://github.com/lightning/bolts/blob/ffeece3dab1c52efdb9b53ae476539320fa44938/00-introduction.md#chain_hash)
/// for specification.
pub fn using_genesis_block(network: Network) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would impl From be too unreadable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think from discussion above we want to make it explicit that we are using the genesis block.

match network {
Network::Bitcoin => ChainHash(GENESIS_BLOCK_HASH_BITCOIN),
Network::Testnet => ChainHash(GENESIS_BLOCK_HASH_TESTNET),
Network::Signet => ChainHash(GENESIS_BLOCK_HASH_SIGNET),
Network::Regtest => ChainHash(GENESIS_BLOCK_HASH_REGTEST),
}
}
}

#[cfg(test)]
mod test {
use core::default::Default;
use super::*;
use crate::hashes::hex::FromHex;

use crate::network::constants::Network;
use crate::consensus::encode::serialize;
use crate::blockdata::constants::{genesis_block, bitcoin_genesis_tx};
use crate::blockdata::constants::{MAX_SEQUENCE, COIN_VALUE};

#[test]
fn bitcoin_genesis_first_transaction() {
Expand Down Expand Up @@ -250,5 +275,49 @@ mod test {
assert_eq!(format!("{:x}", gen.header.block_hash()),
"00000008819873e925422c1ff0f99f7cc9bbb232af63a077a480a3633bee1ef6".to_string());
}

// The *_chain_hash tests are sanity/regression tests, they verify that the const byte array
// representing the genesis block is the same as that created by hashing the genesis block.
fn chain_hash_and_genesis_block(network: Network) {
use hashes::{sha256, Hash};

// The genesis block hash is a double-sha256 and it is displayed backwards.
let genesis_hash = genesis_block(network).block_hash();
// We abuse the sha256 hash here so we get a LowerHex impl that does not print the hex backwards.
let hash = sha256::Hash::from_slice(&genesis_hash.into_inner()).unwrap();
let want = format!("{:02x}", hash);

let chain_hash = ChainHash::using_genesis_block(network);
let got = format!("{:02x}", chain_hash);

// Compare strings because the spec specifically states how the chain hash must encode to hex.
assert_eq!(got, want);
}

macro_rules! chain_hash_genesis_block {
($($test_name:ident, $network:expr);* $(;)*) => {
$(
#[test]
fn $test_name() {
chain_hash_and_genesis_block($network);
}
)*
}
}

chain_hash_genesis_block! {
mainnet_chain_hash_genesis_block, Network::Bitcoin;
testnet_chain_hash_genesis_block, Network::Testnet;
signet_chain_hash_genesis_block, Network::Signet;
regtest_chain_hash_genesis_block, Network::Regtest;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use a macro to deduplicate? And perhaps move assert_eq to the common function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, cheers!


// Test vector taken from: https://github.com/lightning/bolts/blob/master/00-introduction.md
#[test]
fn mainnet_chain_hash_test_vector() {
let got = format!("{:x}", ChainHash::using_genesis_block(Network::Bitcoin));
let want = "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000";
assert_eq!(got, want);
}
}

8 changes: 4 additions & 4 deletions src/internal_macros.rs
Expand Up @@ -84,7 +84,7 @@ macro_rules! impl_array_newtype {
pub fn into_bytes(self) -> [$ty; $len] { self.0 }
}

impl<'a> ::core::convert::From<&'a [$ty]> for $thing {
impl<'a> core::convert::From<&'a [$ty]> for $thing {
fn from(data: &'a [$ty]) -> $thing {
assert_eq!(data.len(), $len);
let mut ret = [0; $len];
Expand All @@ -109,7 +109,7 @@ macro_rules! impl_array_newtype {

macro_rules! display_from_debug {
($thing:ident) => {
impl ::core::fmt::Display for $thing {
impl core::fmt::Display for $thing {
fn fmt(&self, f: &mut ::core::fmt::Formatter) -> Result<(), ::core::fmt::Error> {
::core::fmt::Debug::fmt(self, f)
}
Expand Down Expand Up @@ -365,13 +365,13 @@ macro_rules! impl_bytes_newtype {

impl ::core::fmt::Display for $t {
fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
fmt::LowerHex::fmt(self, f)
::core::fmt::LowerHex::fmt(self, f)
}
}

impl ::core::fmt::Debug for $t {
fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
fmt::LowerHex::fmt(self, f)
::core::fmt::LowerHex::fmt(self, f)
}
}

Expand Down