feat: Add support for Blake2b/2s/3 hash functions #212

larry0x · 2023-10-23T17:48:04Z

This is a result of this Twitter discussion: https://twitter.com/hdevalence/status/1715765138547212689

larry0x · 2023-10-23T17:48:24Z

README.md

@@ -6,7 +6,7 @@
 | ------------------ | ------------------------------------------------- | ---------------------------------------------- |
 | [Go](./go)         | [![Go Test][go-test-badge]][go-test-link]         | [![Go Cov][go-cov-badge]][go-cov-link]         |
 | [Rust](./rust)     | [![Rust Test][rust-test-badge]][rust-test-link]   | [![Rust Cov][rust-cov-badge]][rust-cov-link]   |
-| [TypeScript](./ts) | [![TypeScript Test][ts-test-badge]][ts-test-link] | [![TypeScript Cov][ts-cov-badge]][ts-cov-link] |
+| [TypeScript](./js) | [![TypeScript Test][ts-test-badge]][ts-test-link] | [![TypeScript Cov][ts-cov-badge]][ts-cov-link] |


larry0x · 2023-10-23T17:48:34Z

go/Makefile

-.PHONY: test 
+.PHONY: test


Remove a trailing whitespace

larry0x · 2023-10-23T17:49:24Z

testdata/TestDoHashData.json

-  "bitcoin": {
-    "HashOp": 5,
+  "sha256": {
+    "HashOp": 1,
    "Preimage": "food",
-    "ExpectedHash": "0bcb587dfb4fc10b36d57f2bba1878f139b75d24"
+    "ExpectedHash": "c1f026582fe6e8cb620d0c85a72fe421ddded756662a8ec00ed4c297ad10676b"
+  },
+  "sha512": {
+    "HashOp": 2,
+    "PreImage": "food",
+    "ExpectedHash": "c235548cfe84fc87678ff04c9134e060cdcd7512d09ed726192151a995541ed8db9fda5204e72e7ac268214c322c17787c70530513c59faede52b7dd9ce64331"
  },
  "ripemd160": {
    "HashOp": 4,
    "Preimage": "food",
    "ExpectedHash": "b1ab9988c7c7c5ec4b2b291adfeeee10e77cdd46"
  },
-  "sha256": {
-    "HashOp": 1,
+  "bitcoin": {
+    "HashOp": 5,
    "Preimage": "food",
-    "ExpectedHash": "c1f026582fe6e8cb620d0c85a72fe421ddded756662a8ec00ed4c297ad10676b"
+    "ExpectedHash": "0bcb587dfb4fc10b36d57f2bba1878f139b75d24"
  },


Prettifying this file by ordering test cases by the HashOp indexes. Also adding a test case for SHA2-512

larry0x · 2023-10-24T13:53:57Z

rust/src/ops.rs

+        LengthOp::Fixed32Big => {
+            let mut len = (data.len() as u32).to_be_bytes().to_vec();
+            len.extend(data);
+            return Ok(len);
+        }
        LengthOp::Fixed32Little => {
            let mut len = (data.len() as u32).to_le_bytes().to_vec();
            len.extend(data);
            return Ok(len);
        }
+        LengthOp::Fixed64Big => {
+            let mut len = (data.len() as u64).to_be_bytes().to_vec();
+            len.extend(data);
+            return Ok(len);
+        }
+        LengthOp::Fixed64Little => {
+            let mut len = (data.len() as u64).to_le_bytes().to_vec();
+            len.extend(data);
+            return Ok(len);
+        }


Implementations for these LengthOps are missing, so I add them here because why not

I am also going to add tests for these and push to your branch. I hope that's ok with you.

larry0x · 2023-10-25T21:52:59Z

go/ops.go

+	case LengthOp_FIXED32_BIG:
+		res := make([]byte, 4, 4+len(data))
+		binary.BigEndian.PutUint32(res[:4], uint32(len(data)))
+		res = append(res, data...)
+		return res, nil
 	case LengthOp_FIXED32_LITTLE:
 		res := make([]byte, 4, 4+len(data))
 		binary.LittleEndian.PutUint32(res[:4], uint32(len(data)))
 		res = append(res, data...)
 		return res, nil
+	case LengthOp_FIXED64_BIG:
+		res := make([]byte, 8, 8+len(data))
+		binary.BigEndian.PutUint64(res[:8], uint64(len(data)))
+		res = append(res, data...)
+		return res, nil
+	case LengthOp_FIXED64_LITTLE:
+		res := make([]byte, 8, 8+len(data))
+		binary.LittleEndian.PutUint64(res[:8], uint64(len(data)))
+		res = append(res, data...)
+		return res, nil
 		// TODO
 		// case LengthOp_VAR_RLP:
-		// case LengthOp_FIXED32_BIG:
-		// case LengthOp_FIXED64_BIG:
-		// case LengthOp_FIXED64_LITTLE:


Add a few missing LengthOp impls

Thanks for implementing these. I am going to add tests for them in TestDoHashData.json. However, the expected value that I will use in the test is the one I got from running the test and getting the value from the code. Not ideal, I know, but didn't know of a quick way of doing it otherwise.

codecov · 2023-10-26T11:26:35Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (9b23800) 48.66% compared to head (fd969cb) 49.02%.

Files	Patch %	Lines
rust/src/ops.rs	76.92%	9 Missing ⚠️
go/ops.go	79.31%	4 Missing and 2 partials ⚠️
rust/src/cosmos.ics23.v1.rs	0.00%	6 Missing ⚠️
rust/src/cosmos.ics23.v1.serde.rs	66.66%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
+ Coverage   48.66%   49.02%   +0.35%     
==========================================
  Files          23       23              
  Lines        9908    10024     +116     
  Branches       86       86              
==========================================
+ Hits         4822     4914      +92     
- Misses       4725     4747      +22     
- Partials      361      363       +2

Flag	Coverage Δ
go	`38.38% <79.31%> (+0.28%)`	⬆️
rust	`65.41% <73.91%> (+0.16%)`	⬆️
typescript	`42.52% <100.00%> (+0.48%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AdityaSripal

Thanks for this @larry0x! I think the proto names could be more specific.

Could you also add some more context to the PR description for future readers?

Specifically what is your use case for adding blake3? As you say, there's no official support in golang right now. This would mean that any chain using blake3 for its tree will not be able to connect to a cosmos-sdk chain, since we will use the go package of ICS23 that doesn't support this hash function. Seems strange to add a hash function that is incompatible with the vast majority of deployed ibc chains

AdityaSripal · 2023-10-26T13:36:10Z

testdata/TestDoHashData.json

  },
  "sha512_256": {
    "HashOp": 6,
    "Preimage": "food",
    "ExpectedHash": "5b3a452a6acbf1fc1e553a40c501585d5bd3cca176d562e0a0e19a3c43804e88"
+  },
+  "blakd2b": {


Suggested change

"blakd2b": {

"blake2b": {

ditto for typos below

AdityaSripal · 2023-10-26T13:36:42Z

rust/src/proto_descriptor.bin

The Rust crate actually uses this file: https://github.com/cosmos/ics23/blob/rust/v0.11.0/rust/src/lib.rs#L21

AdityaSripal · 2023-10-26T13:52:10Z

proto/cosmos/ics23/v1/proofs.proto

+  BLAKE2B    = 7;
+  BLAKE2S    = 8;


Should we append with _512 and _256 to specify which variant we're using?

larry0x · 2023-10-26T14:12:44Z

Specifically what is your use case for adding blake3? As you say, there's no official support in golang right now. This would mean that any chain using blake3 for its tree will not be able to connect to a cosmos-sdk chain, since we will use the go package of ICS23 that doesn't support this hash function. Seems strange to add a hash function that is incompatible with the vast majority of deployed ibc chains

@AdityaSripal This is true. There are two options: 1) use one of the unofficial Go implementations, or 2) create a Wasm light client based on the Rust ICS-23. In fact I'm now leaning towards a Wasm client that uses a custom proof format, so whether this PR gets accepted will not impact my project. Your call.

crodriguezvega

Thank you for these additions, @larry0x!

I am going to push now some tests for the extra length ops that you have implemented.

Regarding the support for blake3 in Go, there's someone we can contact for advice. Maybe one of the unofficial implementations is well positioned to eventually land in x/crypto and we can start already using it now.

crodriguezvega · 2023-10-26T12:25:52Z

go/ops.go

+	case LengthOp_FIXED32_BIG:
+		res := make([]byte, 4, 4+len(data))
+		binary.BigEndian.PutUint32(res[:4], uint32(len(data)))
+		res = append(res, data...)
+		return res, nil
 	case LengthOp_FIXED32_LITTLE:
 		res := make([]byte, 4, 4+len(data))
 		binary.LittleEndian.PutUint32(res[:4], uint32(len(data)))
 		res = append(res, data...)
 		return res, nil
+	case LengthOp_FIXED64_BIG:
+		res := make([]byte, 8, 8+len(data))
+		binary.BigEndian.PutUint64(res[:8], uint64(len(data)))
+		res = append(res, data...)
+		return res, nil
+	case LengthOp_FIXED64_LITTLE:
+		res := make([]byte, 8, 8+len(data))
+		binary.LittleEndian.PutUint64(res[:8], uint64(len(data)))
+		res = append(res, data...)
+		return res, nil
 		// TODO
 		// case LengthOp_VAR_RLP:
-		// case LengthOp_FIXED32_BIG:
-		// case LengthOp_FIXED64_BIG:
-		// case LengthOp_FIXED64_LITTLE:


Thanks for implementing these. I am going to add tests for them in TestDoHashData.json. However, the expected value that I will use in the test is the one I got from running the test and getting the value from the code. Not ideal, I know, but didn't know of a quick way of doing it otherwise.

crodriguezvega · 2023-10-26T17:35:37Z

testdata/TestDoHashData.json

+  "blakd3": {
+    "HashOp": 9,
+    "Preimage": "food",
+    "ExpectedHash": "f775a8ccf8cb78cd1c63ade4e9802de4ead836b36cea35242accf31d2c6a3697"


I will remove this because it's used in the go tests, but if there is no implementation for it yet, then the test will panic.

crodriguezvega · 2023-10-26T17:39:40Z

rust/src/ops.rs

+        LengthOp::Fixed32Big => {
+            let mut len = (data.len() as u32).to_be_bytes().to_vec();
+            len.extend(data);
+            return Ok(len);
+        }
        LengthOp::Fixed32Little => {
            let mut len = (data.len() as u32).to_le_bytes().to_vec();
            len.extend(data);
            return Ok(len);
        }
+        LengthOp::Fixed64Big => {
+            let mut len = (data.len() as u64).to_be_bytes().to_vec();
+            len.extend(data);
+            return Ok(len);
+        }
+        LengthOp::Fixed64Little => {
+            let mut len = (data.len() as u64).to_le_bytes().to_vec();
+            len.extend(data);
+            return Ok(len);
+        }


I am also going to add tests for these and push to your branch. I hope that's ok with you.

) Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.13.0 to 0.14.0. - [Commits](golang/crypto@v0.13.0...v0.14.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <[email protected]>

Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.26.1 to 1.27.1. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.26.1...v1.27.1) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <[email protected]>

Bumps [eslint](https://github.com/eslint/eslint) from 8.50.0 to 8.52.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) - [Commits](eslint/eslint@v8.50.0...v8.52.0) --- updated-dependencies: - dependency-name: eslint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <[email protected]>

crodriguezvega · 2023-10-31T09:05:28Z

@AdityaSripal This is true. There are two options: 1) use one of the unofficial Go implementations, or 2) create a Wasm light client based on the Rust ICS-23. In fact I'm now leaning towards a Wasm client that uses a custom proof format, so whether this PR gets accepted will not impact my project. Your call.

I messaged our Golang cryptography expert and this is what he says:

There's little appetite for adding BLAKE3 to the standard library (x/crypto or otherwise) as of now, because it's not needed by anything else in the stdlib, it's not trivial to maintain, it needs to be heavily optimized (because why use BLAKE3 otherwise), and there are good 3rd party modules for it. I have not reviewed it extensively, but lukechampine.com/blake3 is by a known author, looks well maintained, and its use of Avo, its tests, and its API are reassuring.

So we could use the implementation that he recommends. What do you think, @AdityaSripal?

larry0x · 2023-11-01T14:36:29Z

Hey guys, just want to clarify that I'm now leaning towards using my own custom proof format + Wasm light client. So, if you don't feel like supporting Blake in this implementation for any reason, it will not impact my project.

AdityaSripal · 2023-11-06T14:42:11Z

I think i'd prefer not supporting blake3 in go until its official. I think requiring golang chains to use wasm light clients if they need to connect to a state tree that uses blake3 is acceptable.

@larry0x is there a reason you want to go with a custom proof format over ICS23 beyond the fact this isn't released yet?

larry0x · 2023-11-06T23:15:46Z

is there a reason you want to go with a custom proof format over ICS23 beyond the fact this isn't released yet?

For my case a custom proof format can be more compact and probably cheaper to run, especially when it comes to non-existent proofs, because my tree is a radix tree, I only need to show that a node doesn't have a certain children, I don't need to show that two siblings both exist as is how ICS23 works. @AdityaSripal

romac · 2023-11-08T09:54:10Z

The Rust code looks good to me. Thanks for adding tests @crodriguezvega! 🙏

AdityaSripal

Nice work all!

AdityaSripal · 2023-11-21T14:18:27Z

testdata/TestDoHashData.json

+  "blake2s": {
+    "HashOp": 8,
+    "Preimage": "food",
+    "ExpectedHash": "5a1ec796f11f3dfc7e8ca5de13828edf2e910eb7dd41caaac356a4acbefb1758"
  }
 }


no blake3 test?

There's no Go impl, so if you add a Blake3 test case here it will throw an "unimplemented" error

Ahh right, thanks

larry0x added 7 commits October 23, 2023 17:19

update proto files

e46a354

update rust files

c4abb3e

update go files (blake3 unimplemented)

352b1e2

fix a typo in readme

b7bbb4f

update js files

f56bcd7

add tests

530f4cb

remove test case for keccak due to a confusion (cosmos#211)

b6c7a29

larry0x commented Oct 23, 2023

View reviewed changes

larry0x changed the title ~~Add support for Blake2b/2s/3 hash functions~~ feat: Add support for Blake2b/2s/3 hash functions Oct 23, 2023

add missing implementations for a few LengthOps

eddc1d1

larry0x commented Oct 24, 2023

View reviewed changes

crodriguezvega and others added 3 commits October 25, 2023 11:41

Merge branch 'master' into blake

993e3db

implement a few missing LengthOps in Go

bdf792c

fix Go linter warnings

2cae93f

larry0x commented Oct 25, 2023

View reviewed changes

fix Rust dev dependencies

164cc34

AdityaSripal requested changes Oct 26, 2023

View reviewed changes

crodriguezvega reviewed Oct 26, 2023

View reviewed changes

crodriguezvega and others added 4 commits October 26, 2023 19:42

add tests for new length ops implementations

4b60a5c

crodriguezvega force-pushed the blake branch from ae9c2d2 to ebf7efe Compare October 26, 2023 17:48

go mod tidy

11b2677

crodriguezvega force-pushed the blake branch from 6784b53 to 11b2677 Compare October 26, 2023 18:03

crodriguezvega added 2 commits October 26, 2023 20:03

remove print statement

2f78782

Merge branch 'master' into blake

819b6e6

crodriguezvega mentioned this pull request Nov 6, 2023

feat: support BLAKE3 cryptographic hash function #135

Closed

crodriguezvega added 2 commits November 7, 2023 16:36

Merge branch 'master' into blake

43ff4ac

review comment: rename blake2b -> blake2b_512 and blake2s -> blake2s_256

b5dd62e

crodriguezvega requested a review from AdityaSripal November 7, 2023 16:25

Merge branch 'master' into blake

88d9b7c

Merge branch 'master' into blake

03c7fe9

AdityaSripal approved these changes Nov 21, 2023

View reviewed changes

go mod tidy

fd969cb

crodriguezvega merged commit d583e12 into cosmos:master Nov 21, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for Blake2b/2s/3 hash functions #212

feat: Add support for Blake2b/2s/3 hash functions #212

larry0x commented Oct 23, 2023

larry0x Oct 23, 2023

larry0x Oct 23, 2023

larry0x Oct 23, 2023

larry0x Oct 24, 2023

crodriguezvega Oct 26, 2023

larry0x Oct 25, 2023

crodriguezvega Oct 26, 2023

codecov bot commented Oct 26, 2023 •

edited

Loading

AdityaSripal left a comment

AdityaSripal Oct 26, 2023

AdityaSripal Oct 26, 2023

larry0x Oct 26, 2023

AdityaSripal Oct 26, 2023

larry0x commented Oct 26, 2023

crodriguezvega left a comment

crodriguezvega Oct 26, 2023

crodriguezvega Oct 26, 2023

crodriguezvega Oct 26, 2023

crodriguezvega commented Oct 31, 2023 •

edited

Loading

larry0x commented Nov 1, 2023

AdityaSripal commented Nov 6, 2023

larry0x commented Nov 6, 2023

romac commented Nov 8, 2023

AdityaSripal left a comment

AdityaSripal Nov 21, 2023

larry0x Nov 21, 2023

AdityaSripal Nov 21, 2023

		.PHONY: test
		.PHONY: test

feat: Add support for Blake2b/2s/3 hash functions #212

feat: Add support for Blake2b/2s/3 hash functions #212

Conversation

larry0x commented Oct 23, 2023

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

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

AdityaSripal 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

larry0x commented Oct 26, 2023

crodriguezvega 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

crodriguezvega commented Oct 31, 2023 • edited Loading

larry0x commented Nov 1, 2023

AdityaSripal commented Nov 6, 2023

larry0x commented Nov 6, 2023

romac commented Nov 8, 2023

AdityaSripal 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

codecov bot commented Oct 26, 2023 •

edited

Loading

crodriguezvega commented Oct 31, 2023 •

edited

Loading