Add chain hash type using consts #878

tcharding · 2022-03-16T01:51:26Z

The Lightning network defines a type called 'chain hash' that is used to uniquely represent the various Bitcoin networks as a 32 byte hash value. Chain hash is now being used by the DLC folks, as such it is useful to have it implemented in rust-bitcoin.

One method of calculating a chain hash is by hashing the genesis block for the respective network.

Add a ChainHash type that can be used to get the unique identifier of each of the 4 Bitcoin networks we support. Add a method that calculates the chain hash for a network using the double sha256 of the genesis block. Do so using hard coded consts and add unit tests (regression/sanity) that show these hard coded byte arrays match the hash of the data we return for the genesis block for the respective network.

The chain hash for the main Bitcoin network can be verified from LN docs (BOLT 0), add a link to this document.

Closes: #481

dr-orlovsky · 2022-03-16T14:39:45Z

Concept NACK. Nothing in LN requires chain hash to be a genesis block hash. For instance in RGB we use chain hash to be an client-side asset id.

tcharding · 2022-03-16T21:32:59Z

I'm not quite understanding why you ack'ed #872 and nack'ed this one, they both use genesis block as the basis for the chain hash? Am I missing something? Also, BOLT 0 specifies that the chain hash for Bitcoin mainnet must be the hash of the genesis block (https://github.com/lightningnetwork/lightning-rfc/blob/ffeece3dab1c52efdb9b53ae476539320fa44938/00-introduction.md#chain_hash).

Would you prefer if we added a call to impl_array_newtype! so that we get a From<[u8; 32] implementation for custom usage? We could also change the method name for_network -> using_genesis_block? But, as I mention above, I'm not sure why we would want to do that if I have understood the BOLT correctly.

tcharding · 2022-03-16T21:43:31Z

Implements Index instead of using impl_index_array as is done in #805

dr-orlovsky · 2022-03-18T11:19:38Z

@tcharding LN BOLTs authors had no idea that there might be use cases for assets which operate not on the level of blockchain. However, by the nature of LN to be a multi-asset network, chain_hash is indeed the asset id, and not blockchain id. When you use bitcoin mainnet as an asset, it is defined as a hash of genesis block, but if you will use some other (non-bitcoin) asset based on bitcoin blockchain, you will see that it cant' be a hash of bitcoin genesis network and must be something else.

#872 allows use of custom hash values for ChainHash type, this PR does not.

tcharding · 2022-03-18T22:11:51Z

Oh, I see, I left out the call to impl_array_newtype in this one. I'll push that change. Do you have a view on the for_network vs using_genesis_block naming?

dr-orlovsky · 2022-03-19T09:15:38Z

I think genesis block here is more relevant, since multiple assets can operate the same network (like in Liquid), while genesis block hash is definitely a for of the native asset id for the network, which is the only one

dr-orlovsky · 2022-03-20T23:09:20Z

src/blockdata/constants.rs

+/// The uniquely identifying hash of the target blockchain.
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ChainHash([u8; 32]);
+impl_array_newtype!(ChainHash, u8, 32);


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

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.

dr-orlovsky

My personal preference will be to have genesis hashes taken from genesis blocks by the means of const functions, not hex values, which are hard to validate

tcharding · 2022-03-21T22:12:44Z

While conceptually feasible, converting genesis_block and bitcoin_genesis_tx to be const functions is non-trivial because of the usage of script::Builder. Do the unit tests not validate that the hex is correct?

dr-orlovsky

ACK 03d5dff

@tcharding has convinced me :)

RCasatta · 2022-03-22T15:15:14Z

src/blockdata/constants.rs

@@ -176,15 +176,47 @@ pub fn genesis_block(network: Network) -> Block {
    }
 }

+// Mainnet value can be verified at https://github.com/lightning/bolts/blob/master/00-introduction.md
+const CHAIN_HASH_BITCOIN: &str = "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000";


nit: these constants ideally should be byte arrays (while in the test module there should be the hex constant to verify)

Oh nice, that's heaps better than parsing the hex strings, thanks!

I changed the const names to GENESIS_BLOCK_HASH_BITCOIN also.

tcharding · 2022-03-23T02:29:53Z

Please re-review, force push makes considerable changes as suggested by @RCasatta (incl. change of const names).

sanket1729

I left a small nit that is non-merge blocking. For future reference concerning my reviews, whenever I nit something, it is always non-merge blocking.

Other maintainers are free to go ahead and the PR. The author can ignore or decide the comment based on a case of case basis. (length of PR, urgency, number of existing ACKs, available time etc).

sanket1729 · 2022-03-23T17:45:36Z

src/blockdata/constants.rs

@@ -176,6 +176,33 @@ pub fn genesis_block(network: Network) -> Block {
    }
 }

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


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

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

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

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?

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.

sanket1729 · 2022-03-23T17:51:10Z

src/blockdata/constants.rs

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


AFAIR, there were some complaints exposing these as public constants. #568. But I like the way you have done it here.

Unrelated to this PR:

Maybe we should use Network::genesis_hash(&self) API?

AFAIR, there were some complaints exposing these as public constants. #568. But I like the way you have done it here.

Thanks for the link. I think this PR is ok according to that discussion, specifically:

These consts are not public

They are defined in constants.rs

All networks are covered not just mainnet

Maybe we should use Network::genesis_hash(&self) API?

We did that in #872 but after review decided to optimize and use const byte arrays with unit tests to verify them.

Separate PRs because I open two alternate solutions in parallel.

dr-orlovsky · 2022-03-24T06:15:59Z

ACK 25f3ef4

Not merging yet to give @Kixunil and @RCasatta opportunity to comment

RCasatta · 2022-03-24T08:51:55Z

In the referenced document https://github.com/lightning/bolts/blob/master/00-introduction.md we have:

For the main chain Bitcoin blockchain, the chain_hash value MUST be (encoded in hex): 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000.

while we are using reversed hex encoding like BlockHash.

eg. the following test fails

#[test]
    fn chain_hash_test_vector() {
        let h = ChainHash::using_genesis_block(Network::Bitcoin);
        assert_eq!("6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000", format!("{:x}", h));
    }

tcharding · 2022-03-24T23:47:49Z

Legend for catching that @RCasatta! I spent so much time on the first versions making sure that was correct and then dropped the ball when adding the byte arrays. Thanks man.

tcharding · 2022-03-25T00:17:35Z

Legend for catching that @RCasatta! I spent much time on the first versions making sure that was correct and then dropped the ball when adding the byte arrays. Thanks man.

EDIT: PR now includes your unit test.

tcharding · 2022-03-25T00:32:12Z

I ended up [ab]using a sha256::Hash in the tests to get at a LowerHex impl that doesn't display backwards.

dr-orlovsky · 2022-03-27T11:24:40Z

I start thinking that this is very LN-specific and probably should not be a part of rust-bitcoin, otherwise we need to do other LN-specific types here... Probably the proper place for it should be repo like https://github.com/LNP-BP/ln-types

TheBlueMatt

LGTM. I agree with Dr Orlovsly that we don't want lightning-specific things here, but this isn't - this is a new way of accessing the Genesis block in rust-bitcoin, that works without allocating and without doing the hashing. Whether this needs a whole type to represent it, and whether that type needs to be in rust-bitcoin...I kinda don't think so, but I also don't really care/feel strongly about it.

TheBlueMatt · 2022-03-27T13:49:12Z

src/blockdata/constants.rs

+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.


Can we just say this is the Genesis hash?

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 ¯\_(ツ)_/¯

Maybe it should be AssetId then?

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?

Kixunil · 2022-04-08T17:21:58Z

src/blockdata/constants.rs

+    ///
+    /// 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 {


Would impl From be too unreadable?

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

Kixunil

ACK 76166ac

There are some nits but feel free to merge, I don't care.

Kixunil · 2022-04-08T17:22:42Z

src/blockdata/constants.rs

+    fn regtest_chain_hash_genesis_block() {
+        let (got, want) = chain_hash_and_genesis_block(Network::Regtest);
+        assert_eq!(got, want);
+    }


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

Done, cheers!

Kixunil · 2022-04-08T17:24:25Z

src/blockdata/constants.rs

+
+    // 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) -> (String, String) {


A comment explaining why we stringly compare would be useful for future readers eager to clean it up.

As we do for the rest of the macros use the fully qualified path to `fmt` so users of the macro do not have to import it.

Import with wildcard is applicable in unit tests, use it.

The Lightning network defines a type called 'chain hash' that is used to uniquely represent the various Bitcoin networks as a 32 byte hash value. Chain hash is now being used by the DLC folks, as such it is useful to have it implemented in rust-bitcoin. One method of calculating a chain hash is by hashing the genesis block for the respective network. Add a `ChainHash` type that can be used to get the unique identifier of each of the 4 Bitcoin networks we support. Add a method that returns the chain hash for a network using the double sha256 of the genesis block. Do so using hard coded consts and add unit tests (regression/sanity) that show these hard code byte arrays match the hash of the data we return for the genesis block for the respective network. The chain hash for the main Bitcoin network can be verified from LN docs (BOLT 0), add a link to this document.

tcharding · 2022-05-19T05:08:28Z

Implemented @Kixunil's suggestions and rebased.

Kixunil

ACK 8e29f2b

sanket1729

ACK 8e29f2b.

LGTM, I still have a very slight preference to link to bitcoin hash values instead of lightning spec. But I can live it. 8e29f2b#r877663661

8e29f2b Add ChainHash type (Tobin Harding) cd8f511 blockdata: constants: Use wildcard import in unit tests (Tobin Harding) 71bf196 Use fully qualified path in macro (Tobin Harding) Pull request description: The Lightning network defines a type called 'chain hash' that is used to uniquely represent the various Bitcoin networks as a 32 byte hash value. Chain hash is now being used by the DLC folks, as such it is useful to have it implemented in rust-bitcoin. One method of calculating a chain hash is by hashing the genesis block for the respective network. Add a `ChainHash` type that can be used to get the unique identifier of each of the 4 Bitcoin networks we support. Add a method that calculates the chain hash for a network using the double sha256 of the genesis block. Do so using hard coded consts and add unit tests (regression/sanity) that show these hard coded byte arrays match the hash of the data we return for the genesis block for the respective network. The chain hash for the main Bitcoin network can be verified from LN docs (BOLT 0), add a link to this document. Closes: #481 ACKs for top commit: Kixunil: ACK 8e29f2b sanket1729: ACK 8e29f2b. Tree-SHA512: 8156bb55838b73694ddf77a606cbe403f53a31d363aa0dee11b97dc31aa9b62609d7d84b8f0f92c08e90372a3e8c7b416fb07989d6da9633763373b41339b1f5

tcharding mentioned this pull request Mar 16, 2022

Add chain hash type #872

Closed

tcharding force-pushed the add-chain-hash-type-using-consts branch from 1b40081 to ef802f1 Compare March 16, 2022 21:42

tcharding force-pushed the add-chain-hash-type-using-consts branch 2 times, most recently from f7a451e to 182fe2e Compare March 18, 2022 22:29

tcharding force-pushed the add-chain-hash-type-using-consts branch 4 times, most recently from 7e59d6c to 03d5dff Compare March 20, 2022 21:45

tcharding marked this pull request as ready for review March 20, 2022 22:01

dr-orlovsky reviewed Mar 20, 2022

View reviewed changes

dr-orlovsky reviewed Mar 21, 2022

View reviewed changes

dr-orlovsky previously approved these changes Mar 22, 2022

View reviewed changes

RCasatta reviewed Mar 22, 2022

View reviewed changes

tcharding dismissed dr-orlovsky’s stale review via 13e7608 March 23, 2022 02:26

tcharding force-pushed the add-chain-hash-type-using-consts branch 2 times, most recently from 13e7608 to 25f3ef4 Compare March 23, 2022 02:28

sanket1729 previously approved these changes Mar 23, 2022

View reviewed changes

tcharding dismissed sanket1729’s stale review via 76166ac March 25, 2022 00:31

tcharding force-pushed the add-chain-hash-type-using-consts branch from 25f3ef4 to 76166ac Compare March 25, 2022 00:31

TheBlueMatt reviewed Mar 27, 2022

View reviewed changes

Kixunil reviewed Apr 8, 2022

View reviewed changes

Kixunil previously approved these changes Apr 8, 2022

View reviewed changes

Kixunil mentioned this pull request Apr 8, 2022

Consider adding constants module LNP-WG/ln-types#17

Open

tcharding marked this pull request as draft April 17, 2022 22:19

tcharding added 3 commits May 19, 2022 14:14

Use fully qualified path in macro

71bf196

As we do for the rest of the macros use the fully qualified path to `fmt` so users of the macro do not have to import it.

blockdata: constants: Use wildcard import in unit tests

cd8f511

Import with wildcard is applicable in unit tests, use it.

tcharding dismissed Kixunil’s stale review via 8e29f2b May 19, 2022 05:07

tcharding force-pushed the add-chain-hash-type-using-consts branch from 76166ac to 8e29f2b Compare May 19, 2022 05:07

tcharding marked this pull request as ready for review May 19, 2022 05:36

Kixunil approved these changes May 25, 2022

View reviewed changes

sanket1729 approved these changes Jun 1, 2022

View reviewed changes

sanket1729 merged commit e9a3379 into rust-bitcoin:master Jun 1, 2022

tcharding deleted the add-chain-hash-type-using-consts branch July 11, 2022 01:35

Add chain hash type using consts #878

Add chain hash type using consts #878

Conversation

tcharding commented Mar 16, 2022 • edited

dr-orlovsky commented Mar 16, 2022

tcharding commented Mar 16, 2022

tcharding commented Mar 16, 2022

dr-orlovsky commented Mar 18, 2022 • edited

tcharding commented Mar 18, 2022

dr-orlovsky commented Mar 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

tcharding commented Mar 21, 2022

dr-orlovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 23, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 May 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Mar 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky commented Mar 24, 2022

RCasatta commented Mar 24, 2022

tcharding commented Mar 24, 2022

tcharding commented Mar 25, 2022 • edited

tcharding commented Mar 25, 2022

dr-orlovsky commented Mar 27, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented May 19, 2022

Kixunil left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding commented Mar 16, 2022 •

edited

dr-orlovsky commented Mar 18, 2022 •

edited

sanket1729 May 20, 2022 •

edited

tcharding Mar 24, 2022 •

edited

tcharding commented Mar 25, 2022 •

edited