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

Efficient Fiat-Shamir challenges using hashing #15

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

Inphi
Copy link
Collaborator

@Inphi Inphi commented Aug 22, 2022

By avoiding ssz merklelization, we significantly reduce the latency of generating FS
challenges.

name                 old time/op    new time/op    delta
VerifyMultiple/8-6     73.9ms ± 0%    40.7ms ± 0%   ~     (p=1.000 n=1+1)
VerifyMultiple/16-6     138ms ± 0%      81ms ± 0%   ~     (p=1.000 n=1+1)

name                 old alloc/op   new alloc/op   delta
VerifyMultiple/8-6     25.2MB ± 0%    22.1MB ± 0%   ~     (p=1.000 n=1+1)
VerifyMultiple/16-6    50.4MB ± 0%    44.1MB ± 0%   ~     (p=1.000 n=1+1)

name                 old allocs/op  new allocs/op  delta
VerifyMultiple/8-6       400k ± 0%      301k ± 0%   ~     (p=1.000 n=1+1)
VerifyMultiple/16-6      800k ± 0%      603k ± 0%   ~     (p=1.000 n=1+1)

By avoiding ssz merklelization, we significantly reduce the latency of generating FS
challenges.

Before:
```
BenchmarkVerifyMultiple/8-6           16          69890960 ns/op        25221325 B/op     399904 allocs/op
BenchmarkVerifyMultiple/16-6                   8         138506286 ns/op        50439658 B/op     799792 allocs/op
```

After:
```
BenchmarkVerifyMultiple/8-6 	      27	  41349482 ns/op	22059083 B/op	  301451 allocs/op
BenchmarkVerifyMultiple/16-6         	      14	  80868200 ns/op	44114668 B/op	  602909 allocs/op
```
@roberto-bayardo
Copy link
Collaborator

This is great! I assume this is not a backwards compatible change since the hash values presumably change?

@Inphi
Copy link
Collaborator Author

Inphi commented Aug 22, 2022

yup. This isnt backwards compatible

@roberto-bayardo
Copy link
Collaborator

OK, will try to implement in prysm soon then.


// sszHash returns the hash ot the raw serialized ssz-container (i.e. without merkelization)
func sszHash(c codec.Serializable) ([32]byte, error) {
sha := sha256.New()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why sha256 and not keccak/sha3? I guess it might be faster but possibly less future proof?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No real reason other than to be consistent with how hashing is already done on the beacon chain. sha256 is the only hash being used on the beacon chain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, implementation consistency is certainly a good reason.

@Inphi
Copy link
Collaborator Author

Inphi commented Aug 23, 2022

Will merge once the spec has been updated for sha256 challenges.

@Inphi Inphi merged commit fce14ba into mdehoog:eip-4844 Aug 25, 2022
@Inphi Inphi deleted the inphi/fs-hash branch August 25, 2022 19:02
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