Snapshot sync: use hasher for chunk hashes (#7215)

[robert-zaremba]

Description

Blake3 improves on security and speed compared to sha256.
https://blake3.io/

In this PR:

• use optimized blake3 hashes (with dedicated SIMD code)
  instead of sha256
• reuse resources on hashing chunks.
• cleaned few error return statements.
• fixed some old linter issue.

TODO:

• add small benchmark

closes: #7215

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov]

Codecov Report

Merging #7259 (4e277f0) into master (eb71698) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7259      +/-   ##
==========================================
+ Coverage   61.80%   61.83%   +0.02%     
==========================================
  Files         609      609              
  Lines       35144    35124      -20     
==========================================
- Hits        21722    21719       -3     
+ Misses      11146    11137       -9     
+ Partials     2276     2268       -8     

Impacted Files	Coverage Δ
snapshots/store.go	74.31% <100.00%> (+5.84%)	⬆️

[amaury1093]

Blake3 indeed does seem like an improvement, but I'm always a bit wary about new and shiny tech that's "better, faster, stronger".

Some questions I have:

• Has any project (blockchain or not) used blake3 in production for a longer period of time (say >6 months), and has some data/insights about it?
• Is this library audited? If not, are we okay to use this library from just another guy on the internet?

[alexanderbez]

I'm sure @ValarDragon would have excellent input here.

[aaronc]

Blake3 looks amazing at first glance. And I tend to agree with @amaurymartiny. Would love to have those questions answered and hear what others think. Thanks for working on this @robert-zaremba !

[alessio]

@amaurymartiny dixit:

Is this library audited? If not, are we okay to use this library from just another guy on the internet?

Exactly my thoughts.

@robert-zaremba dixit:

Blake3 improves on security and speed compared to sha256.

Speaking of performance, could you provide benchmarks please?

[robert-zaremba]

Speaking of performance, could you provide benchmarks please?

They are in the link in OP: https://blake3.io/
here are more benchmarks: https://github.com/zeebo/blake3/blob/master/README.md

[tac0turtle]

Has sha256 been shown to be a performance inhibitor in this area? I am hesitant to be trying to optimize this without testing and benchmarks for our use case and for the above mentioned reasons.

[alessio]

On Tue, Sep 8, 2020 at 2:42 PM Robert Zaremba ***@***.***> wrote: They are in the link in OP: https://blake3.io/ here are more benchmarks: https://github.com/zeebo/blake3/blob/master/README.md

Yes, I saw those :) I was wondering whether you could benchmark your changes, i.e. 1. write a benchmark for the functions affected by your patch 2. run it on the current implementation and collect results 3. apply your patch, run the test again and collect results 4. compare results How about that?

…

-- AT

[robert-zaremba]

Is this library audited? If not, are we okay to use this library from just another guy on the internet?

The library is a rewrite of a reference and audited implementation in Rust.

Moreover, this change impact the security of a blockchain. The hashes we are checking are used only to do a quick fallback if data doesn't match. For security we are still using standard Merkle Tree and compare the state of Merkle Tree with the state in the block Merkle Root.

[robert-zaremba]

The performance impact should be small, because most of the work in snapshot is IO based. If you want I can do the benchmark using the tests which are already there. (it's just time consuming).

Also, if the source of the code is questionable I can swap it to blake2b. Some blockchains are using blake2b for hashing.

[robert-zaremba]

Has any project (blockchain or not) used blake3 in production for a longer period of time (say >6 months), and has some data/insights about it?

BLAKE3 was designed by:

• @oconnor663 (Jack O'Connor)
• @sneves (Samuel Neves)
• @veorq (Jean-Philippe Aumasson)
• @zookozcash (Zooko)

The development of BLAKE3 was sponsored by Teserakt and Electric Coin Company.

So basically ZCash is on it.

Moreover Blake3 it's recommended by the blake2 team, and there is lot of research and use behind it.

[tac0turtle]

Has any project (blockchain or not) used blake3 in production for a longer period of time (say >6 months), and has some data/insights about it?

BLAKE3 was designed by:

* @oconnor663 (Jack O'Connor)

* @sneves (Samuel Neves)

* @veorq (Jean-Philippe Aumasson)

* @zookozcash (Zooko)

The development of BLAKE3 was sponsored by Teserakt and Electric Coin Company.

So basically ZCash is on it.

Okay, this is known. I think it would be beneficial to bring some benchmarks.

I agree there will be a performance increase, but the amount may not be worth it at this time. If performance is a bottleneck here then something should be done with the implementation not only the hashing function.

This could easily be a post stargate issue. There are issues & PRs that still need work, these will pay larger dividends immediately than this IMO.

[robert-zaremba]

This could easily be a post stargate issue. There are issues & PRs that still need work, these will pay larger dividends immediately than this IMO.

Later it will be a breaking change (snapshot hashes will change). Now it's easy. Even if the overall impact is tiny, why not having it? It's done and we have a good test coverage for this functionality.

The only risk I see here is that the source library is not audited. But again, this doesn't impact the security of a blockchain. Worst case this will cause problems with snapshot sync. But this we will be testing anyway soon (with the RC).

[erikgrinaker]

Snapshotting is currently IO-bound due to IAVL access patterns, so this seems to have limited upside - but feel free to benchmark it. Apart from that, LGTM.

Later it will be a breaking change (snapshot hashes will change). Now it's easy.

Snapshot formats are versioned, so the hash can be changed later by simply bumping the format number.

[ValarDragon]

I think in general, we should have a larger strategy for what cryptographic hash functions we want to use as default everywhere (aside from addresses), and also not hard-code the choice of hash function. Chains pay in complexity by using different cryptographic hash algorithms for different things, instead we ought to have a cohesive plan (even if changes get merged across different SDK/TM versions!)

There are valid reasons for the following three hashes as defaults, for which getting more input would be helpful.

• SHA2 - You have hardware implementations on many new Intel computers / servers, which beats blake2 by a large factor. Great support everywhere.
• Blake2b - Much faster in software than SHA2. Mildly (30%) better in a zkp circuit than SHA2. Supported in ~all software languages now, I don't know what the hardware support is.
• Blake3 - the in software speed is much faster than SHA2 in hardware. Very limited support across languages/ecosystems, cutting edge / not battle tested so security relies on how closely its internals mirror blake2s.

I prefer Blake2b as the default hardcoded function at this time. I'm personally in favor of holding off on Blake3 until it gets implemented / bound to more languages.

So basically ZCash is on it.

This isn't the right conclusion, they funded it. (And Zooko worked on it) There is no current plan AFAIK to use it in ZCash.

[robert-zaremba]

Initially I was thinking about blake2b (check the issue: #7215). But the I noticed that blake3 is endorsed at the beginning of the official page, so this built up my confidence.

@ValarDragon , if we don't count exotic languages, then blake2b support is close to 100%.

[robert-zaremba]

If change to blake2b will make us reaching an approval faster, then I'm for that.

[zmanian]

I'm sort of opposed to randomly adding new cryptographic dependencies to the Cosmos stack.
Blake2 & Blake3 are great but only really worth adding if the performance benefits are large.

[robert-zaremba]

This is not a random dependency add on. There is a good motivation explained here and if we go with blake2b we have a solid, verified choice.
You know... little by little, and you make a step. This one is easy and solid. Google is optimizing for fraction of a percent.

[alessio]

@zmanian dixit:

Blake2 & Blake3 are great but only really worth adding if the performance benefits are large.

That's exactly the reason why I asked for benchmarks. And I'm yet to be shown figures.

[robert-zaremba]

Do we have some scripts / tools to make such a benchmark (create blockchain, populate with data, start doing transactions and do some trace timing?)

[robert-zaremba]

@alessio , Re "And I'm yet to be shown figures.", for unit type of benchmarks, you have charts in the links above. Like I wrote, I don't expect to have this much difference in the functional setup (as we both noted with Erik, the whole process is IO bound). My motivation here was to: do some small improvements with the idea of "little by little" without adding any complexity.

[robert-zaremba]

Also if this is extremely I/O bound, why are we concerned about the CPU here?

Because we can. Also, there are other threads which may compete for CPU usage. Google pays lot of $$$ if you optimize their code by a fraction of a per-cent.

[alessio]

@ValarDragon dixit:

Also if this is extremely I/O bound, why are we concerned about the CPU here?

I have avoided commenting on that deliberately.

@robert-zaremba dixit:

Because we can.

That's not an answer to Dev's question. I'd strongly advise to focus on addressing the technical concern raised by @ValarDragon, who is a long time contributor of Tendermint Core and Cosmos SDK.

Plus:

Also, there are other threads which may compete for CPU usage.

That's always the case in most scenarios and not an excuse to patchwork new cryptographic functions because we can.

[robert-zaremba]

Because we can. Also, there are other threads which may compete for CPU usage. Google pays lot of $$$ if you optimize their code by a fraction of a per-cent.

Sorry if this doesn't sound informative enough. There is a whole thread about it here. To summarize:

1. We agree that blake2 is a better choice: it's well supported and it's faster (PoS chains don't have any advantage of using ASICS, and are rather run on standard servers / racks). New blockchains rather don't use sha2.
2. The snapshot sync use case is not related to a consensus so it's less critical for changes.
3. We can use something better, which doesn't increase a security risk - so it's a low hanging fruit, simple improvement without complexity change.

Overall this things will need to be refactor in the future to allow anyone use their own hashing algorithm in the whole chain.

[ValarDragon]

We agree that blake2 is a better choice: it's well supported and it's faster (PoS chains don't have any advantage of using ASICS, and are rather run on standard servers / racks). New blockchains rather don't use sha2.

We don't agree that its better on all fronts. See my comment:

SHA2 - You have hardware implementations on many new Intel computers / servers, which beats blake2 software speeds by a large factor. Great support everywhere.

My bad for not giving more context for my preference of a blake2b default everywhere. There are environments where you don't get to use the SHA2 in-hardware support (e.g. JS AFAIU), which would matter for lite clients. For snapshot hashes, I see no reason to not be considering the SHA2 in-hardware speed vs the blake2b software speed (where sha2 vastly outperforms) as this is not a computation lite clients perform. Things like that are reasons why I prefer blake2b in consensus rather than SHA2, and then I'd want blake2b defaulted everywhere for consistency.

I don't view an isolated change to blake2b in an area lite clients don't benefit from as an incremental improvement (even ignoring the complexity tradeoffs).

Also using different hashes & security assumptions for different things is definitely a complexity change, which now several people have noted. By using blake2b here, we aren't actually gaining anything since it doesn't improve wall time as has been noted, so I don't see why we'd take the tradeoff. (I still don't see why CPU time matters here when wall time is what is important - is there a scenario you're envisioning where the CPU time starts taking a significant portion of time?)

[sneves]

SHA2 - You have hardware implementations on many new Intel computers / servers, which beats blake2 software speeds by a large factor. Great support everywhere.

I see no reason to not be considering the SHA2 in-hardware speed vs the blake2b software speed (where sha2 vastly outperforms)

I have absolutely no dog in this fight, but I was subscribed to this thread for some reason, and this stood out to me. I'm not sure what this "vast difference" is. Let's look at some Intel numbers.

On Intel Skylake:

• SHA-256: 7.57 cycles per byte using no SHA extensions;
• BLAKE2b: 2.87 cycles per byte using AVX2.

On Intel Icelake:

• SHA-256: 2.44 cycles per byte using SHA-NI instructions;
• BLAKE2b: 2.60 cycles per byte using AVX2 + AVX512VL for the native rotation instructions.

On AMD Zen2:

• SHA-256: 2.00 cycles per byte using SHA-NI instructions;
• BLAKE2b: 3.27 cycles per byte using AVX2.

There is a difference, I suppose, but I'm not sure I would call it vast. The SHA-2 instructions really are a great upgrade compared to SHA-2 without them, though.

Parallel hashes like BLAKE2sp, BLAKE2bp, or BLAKE3 will be faster than hardware SHA-256 on any of the above platforms.

[ValarDragon]

Thank you for the correction there, I appreciate it! Must have misremembered it.

Do you know what Intel Icelake blake2b speeds are without AVX512? We definitely don't want the AVX512 downclock for hyperthreaded processes.

[sneves]

Do you know what Intel Icelake blake2b speeds are without AVX512? We definitely don't want the AVX512 downclock for hyperthreaded processes.

The code only ever works with with 256-bit vectors (the AVX512VL is purely to use new instructions on 256-bit ymm vectors), so the frequency hit from going to 512-bit wide vectors does not apply. It's the same as if only using AVX2.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[robert-zaremba]

What shall we do with this PR?

1. we remove it
2. continue with sha256 and I extract only optimization parts (hasher constructions)
3. re-discuss blake2 usage here

[zmanian]

I prefer 2.

[alessio]

I'd go with 2

[robert-zaremba]

I reverted to sha256. Please review.

[alessio]

ACK - pending tests

[erikgrinaker]

Nice cleanups, thanks!