VerifyStage: headers validation using preferified hashes.

[battlmonstr]

The preverified hashes is a list of known precomputed hashes of every 192-th block in the chain:

hash(0), hash(192), hash(384), hash(576), ...

The preverified hashes are copied from:
https://github.com/ledgerwatch/erigon/blob/devel/turbo/stages/headerdownload/preverified_hashes_mainnet.go
https://github.com/ledgerwatch/erigon/blob/devel/turbo/stages/headerdownload/preverified_hashes_ropsten.go

When we have a HeaderSlice and need to verify it, the algorithm verifies that the top of the slice matches one of the preverified hashes, and that all blocks down to the root of the slice are properly connected by the parent_hash field.

For example, if we have a HeaderSlice[192...384] (with block headers from 192 to 384 inclusive), it verifies that:

1. hash(slice[384]) == preverified hash(384)
2. hash(slice[383]) == slice[384].parent_hash
3. hash(slice[382]) == slice[383].parent_hash
   ...
4. hash(slice[192]) == slice[193].parent_hash

Thus verifying hashes of all the headers.

[vorot93]

Must deserialize directly into final data model using toml::from_str, unless there is a good reason not to.

[battlmonstr]

Is there a way to do it without prepending "0x" to all the data values?

[vorot93]

https://github.com/akula-bft/akula/blob/master/src/models/chainspec.rs#L44
https://github.com/akula-bft/akula/blob/master/src/models/chainspec.rs#L158

In your case, you could make a deserializer that deserializes into String, and then parses as H256.

It will allocate, so when you have time, you could instead deserialize through a wrapper type, like UnprefixedH256, that would simply omit this check.

[blasrodri]

can this actually panic? If so, we might want to offer a more comprehensive error message.

[battlmonstr]

It can't, I'm checking for None 2 lines above. I prefer to not use match to not increase nestedness level more than necessary (some languages have a guard construct for this, which Rust doesn't have)

[blasrodri]

you can use expect("some msg")

[battlmonstr]

.expect("not None, see the line above") ?
😄

I wish Rust had something like this (inverse of "if let")

see also: https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

[battlmonstr]

@blasrodri there's a need for option, because it can be in 2 states "is_none" or "is_some", but since I already check "is_none", and since the slice is passed as a ref here, it can't be mutated between the lines, so on the unwrap line it has the same state, which is guaranteed to be is_some().

😮 the Rust gods listened to us:
rust-lang/rfcs#3137

it mentions nightly-2021-09-04 ✨ , so might become available to us soon? 🤔

with this compiler change in place this piece of code could be rewritten to:

let Some(headers) = slice.headers.as_ref() else { return }

[vorot93]

It has become available to us now 😉

[battlmonstr]

I've tried the new syntax like so:

diff --git a/src/downloader/headers/verify_stage.rs b/src/downloader/headers/verify_stage.rs
index f760307..68bd69e 100644
--- a/src/downloader/headers/verify_stage.rs
+++ b/src/downloader/headers/verify_stage.rs
@@ -85,17 +85,11 @@ impl VerifyStage {
     /// hash(slice[192]) == slice[193].parent_hash
     ///
     /// Thus verifying hashes of all the headers.
+    #[allow(clippy::needless_return)]
     fn verify_slice(&self, slice: &HeaderSlice) -> bool {
-        if slice.headers.is_none() {
-            return false;
-        }
-        let headers = slice.headers.as_ref().unwrap();
-
-        if headers.is_empty() {
-            return true;
-        }
+        let Some(headers) = slice.headers.as_ref() else { return false; };
+        let Some(last) = headers.last() else { return true; };
 
-        let last = headers.last().unwrap();
         let last_hash = last.hash();
         let expected_last_hash =
             self.preverified_hash(slice.start_block_num + headers.len() as u64 - 1);
diff --git a/src/lib.rs b/src/lib.rs
index 707b5e5..e3cc647 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -3,6 +3,7 @@
     destructuring_assignment,
     entry_insert,
     generic_associated_types,
+    let_else,
     linked_list_cursors,
     never_type,
     type_alias_impl_trait

But found 2 problems:

1. My IDE does not support it yet, so the code in this method becomes erroneous:
   
   This will be fixed eventually: Support let-else statements intellij-rust/intellij-rust#7773

2. Clippy reports needless_return by mistake. If I remove the return, it fails to compile.
   

I'd rather keep this as is for now, and change to the new syntax when it becomes more widely supported.