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

Feature/htr #8

Merged
merged 8 commits into from
Jul 28, 2020
Merged

Feature/htr #8

merged 8 commits into from
Jul 28, 2020

Conversation

ferranbt
Copy link
Owner

This PR includes support for two things:

  • Byte slices (i.e. [32]byte)
  • Hashing of the structs with the function HashTreeRoot.

spectests/structs_test.go Outdated Show resolved Hide resolved
spectests/structs_test.go Outdated Show resolved Hide resolved
sszgen/main.go Show resolved Hide resolved
sszgen/marshal.go Show resolved Hide resolved
@prestonvanloon
Copy link
Contributor

prestonvanloon commented Jul 24, 2020

Please add a HashTreeRoot interface so we can do checks like this:

if v, ok := object.(ssz.HashTreeRooter); ok {
  return v.HashTreeRoot()
}

Edit: nevermind, i see you already did that. Thanks!

Copy link
Contributor

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

I tried this locally and it appears to be missing imports that worked previously.

Screenshot from 2020-07-24 11-16-57

v1alpha1 is still used in this file, but import was removed when I used this branch.

@prestonvanloon
Copy link
Contributor

Would it be possible to change the return type to [32]byte, error?
This would be inline with go-ssz.HashTreeRoot and other custom implementations in Prysm.

@ferranbt
Copy link
Owner Author

Sure, no problem

@prestonvanloon
Copy link
Contributor

Here's some benchmarks I ran to compare this against go-ssz.HashTreeRoot. Please don't interpret the execution time to be how long HTR takes, this benchmark was reading files from disk, unmarshalling, marshalling, and some byte comparisions against spec tests.

test.log

@ferranbt
Copy link
Owner Author

The 'htr_off' benchmarks are done with go-ssz or with the custom functions in prysm? I am not getting the same results on my benchmarks. I used this Htr benchmark for both fastssz and go-ssz and got:

goos: linux
goarch: amd64
pkg: github.com/ferranbt/fastssz/spectests
BenchmarkHashTreeRootFast
BenchmarkHashTreeRootFast-8    	    8347	    138159 ns/op	       0 B/op	       0 allocs/op
BenchmarkHashTreeRootGoSSZ
BenchmarkHashTreeRootGoSSZ-8   	     724	   1440258 ns/op	  264128 B/op	    6638 allocs/op
PASS
ok  	github.com/ferranbt/fastssz/spectests	3.120s

where 'BenchmarkHashTreeRootGoSSZ' is

import (
	oldSSZ "github.com/prysmaticlabs/go-ssz"
)

func BenchmarkHashTreeRootGoSSZ(b *testing.B) {
	obj := new(BeaconBlock)
	readValidGenericSSZ(nil, benchmarkTestCase, obj)

	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		goSSZ.HashTreeRoot(obj)
	}
}

The numbers seem accurate for fastssz. The implementation reuses all the memory and the main factor that accounts for most of the time consumed is the sha hashing.

@prestonvanloon
Copy link
Contributor

The 'htr_off' benchmarks are done with go-ssz or with the custom functions in prysm?

They are done with go-ssz, except for the BeaconState which uses its custom HTR function. As I mentioned before, don't interpret these times as HTR only, it does a lot more than only HTR. Here's the benchmark on a working branch: https://github.com/prysmaticlabs/prysm/blob/d89a4af4637b5fe00e3ad242d55f70ba8de3be39/proto/testing/ssz_static_mainnet_test.go#L11

@ferranbt ferranbt merged commit 0b6e349 into master Jul 28, 2020
@ferranbt ferranbt deleted the feature/htr branch August 26, 2020 17:12
letonchanh pushed a commit to letonchanh/fastssz that referenced this pull request Mar 28, 2024
Use `MerkleizeVectorizedHTR` inside `HashTreeRootWith`
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.

2 participants