Add block and attestation validation by paulhauner · Pull Request #33 · sigp/lighthouse

paulhauner · 2018-10-02T07:44:31Z

This adds block and attestation validation code that was built
previously.

There were many non-validation specific changes made whilst
building these functions (e.g., db, hashing, etc) -- these changes have
already been merged into master and this branch has been created just to
make it easy to review this code.

The two primary files in this PR are:

beacon_chain/validation/src/attestation_validation.rs: validates an attestations.
beacon_chain/validation/src/block_validation.rs: validates a block (and its attestations)

There are also three other files which provide supporting functions to those two primary files:

beacon_chain/validation/src/attestation_parent_hashes.rs: gets relevant parent_hashes for some attestation.
beacon_chain/validation/src/message_generation.rs: generates the message hash that should be signed across in an attestation.
beacon_chain/validation/src/signature_verification.rs: verifies the signature on an attestation.

There are ~27 tests that should all pass.

This adds block and attestation validation code that was written previously. There were many non-validation specific changes made whilst building these functions (e.g., db, hashing, etc) -- these changes have already been merged into master and this branch has been created just to make it easy to review this code.

djrtwo

My review is primarily in the higher level logic rather than digging into what's going on underneath the interfaces (for example I'm just assuming your bitfield stuff works).

This all looks excellent! Really solid foundations and code. Generally very readable.

I noted some various suggestions along with some small errors I've found regarding the spec.

Happy to discuss anything as needed

As per Danny's request

Previously we were just checking it exists in the DB. This is incorrect because the last_justified_block_hash _must_ be in the chain referenced by the block. I.e., it's not OK for a block to reference a justified block in another chain.

This allows skipping back in the chain to find a block.

Previously there was not a check that the hash was in the chain, just that it was known (in the database in any chain)

Comment was suggested by Danny R.

As per comments by Danny Ryan on PR#33

There was no check that the attestation is within an appropriate distance from its parent block.

djrtwo · 2018-10-15T01:42:21Z

+         * from the parent_slot of block that contained it.
+         */
+        if a.slot < self.parent_block_slot
+            .saturating_sub(u64::from(self.cycle_length).saturating_add(1)) {


I think the order of these "saturating" operations will give you a slight error wrt the spec.
"Verify that slot >= max(parent.slot - CYCLE_LENGTH + 1, 0)" -- the max operation here allows for a.slot to be 0, whereas your implementation does not.

For example say parent_block_slot = 5, cycle_length = 10 then I believe the right side of your boolean is 1, and prevents a.slot from equalling 0. If instead your logic matched the spec, the right side would be 0 and a.slot would not be less and would thus be valid.

That was a little verbose.. hope it makes sense.

Unless stringing together saturating ops does something different than I expect.

I had a look and I think what I have is correct. For reference, here's your parameters returning 0:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=90463a4a4df484611637787a97d021e8

Am I missing something?

ah, interesting so the calls to saturating string together before resolving.

I was expecting parent_block.saturating_sub(cycle_length) to give 0 and then 0.saturating_add(1) to give 1.

Sorry. Learning rust on the fly

EDIT: Scrap this comment. read below

oh wait.
I missed the parens. Thought it was parent_slot.saturating_sub(cycle_length).saturating_add(1)

Sneaky parens

djrtwo

a few small notes. Looks good! very close to ready

paulhauner · 2018-10-16T04:40:17Z

@djrtwo are you happy with this or would you like some more time? No pressure, just following up :)

djrtwo · 2018-10-16T13:32:22Z

No should be merged. It's a good foundation

* some blob reprocessing work * remove ForceBlockLookup * reorder enum match arms in sync manager * a lot more reprocessing work * impl logic for triggerng blob lookups along with block lookups * deal with rpc blobs in groups per block in the da checker. don't cache missing blob ids in the da checker. * make single block lookup generic * more work * add delayed processing logic and combine some requests * start fixing some compile errors * fix compilation in main block lookup mod * much work * get things compiling * parent blob lookups * fix compile * revert red/stevie changes * fix up sync manager delay message logic * add peer usefulness enum * should remove lookup refactor * consolidate retry error handling * improve peer scoring during certain failures in parent lookups * improve retry code * drop parent lookup if either req has a peer disconnect during download * refactor single block processed method * processing peer refactor * smol bugfix * fix some todos * fix lints * fix lints * fix compile in lookup tests * fix lints * fix lints * fix existing block lookup tests * renamings * fix after merge * cargo fmt * compilation fix in beacon chain tests * fix * refactor lookup tests to work with multiple forks and response types * make tests into macros * wrap availability check error * fix compile after merge * add random blobs * start fixing up lookup verify error handling * some bug fixes and the start of deneb only tests * make tests work for all forks * track information about peer source * error refactoring * improve peer scoring * fix test compilation * make sure blobs are sent for processing after stream termination, delete copied tests * add some tests and fix a bug * smol bugfixes and moar tests * add tests and fix some things * compile after merge * lots of refactoring * retry on invalid block/blob * merge unknown parent messages before current slot lookup * get tests compiling * penalize blob peer on invalid blobs * Check disk on in-memory cache miss * Update beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs * Update beacon_node/network/src/sync/network_context.rs Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com> * fix bug in matching blocks and blobs in range sync * pr feedback * fix conflicts * upgrade logs from warn to crit when we receive incorrect response in range * synced_and_connected_within_tolerance -> should_search_for_block * remove todo * add data gas used and update excess data gas to u64 * Fix Broken Overflow Tests * payload verification with commitments * fix merge conflicts * restore payload file * Restore payload file * remove todo * add max blob commitments per block * c-kzg lib update * Fix ef tests * Abstract over minimal/mainnet spec in kzg crate * Start integrating new KZG * checkpoint sync without alignment * checkpoint sync without alignment * add import * add import * query for checkpoint state by slot rather than state root (teku doesn't serve by state root) * query for checkpoint state by slot rather than state root (teku doesn't serve by state root) * loosen check * get state first and query by most recent block root * Revert "loosen check" This reverts commit 069d13d. * get state first and query by most recent block root * merge max blobs change * simplify delay logic * rename unknown parent sync message variants * rename parameter, block_slot -> slot * add some docs to the lookup module * use interval instead of sleep * drop request if blocks and blobs requests both return `None` for `Id` * clean up `find_single_lookup` logic * add lookup source enum * clean up `find_single_lookup` logic * add docs to find_single_lookup_request * move LookupSource our of param where unnecessary * remove unnecessary todo * query for block by `state.latest_block_header.slot` * fix lint * fix merge transition ef tests * fix test * fix test * fix observed blob sidecars test * Add some metrics (#33) * fix protocol limits for blobs by root * Update Engine API for 1:1 Structure Method * make beacon chain tests to fix devnet 6 changes * get ckzg working and fix some tests * fix remaining tests * fix lints * Fix KZG linking issues * remove unused dep * lockfile * test fixes * remove dbgs * remove unwrap * cleanup tx generator * small fixes * fixing fixes * more self reivew * more self review * refactor genesis header initialization * refactor mock el instantiations * fix compile * fix network test, make sure they run for each fork * pr feedback * fix last test (hopefully) --------- Co-authored-by: Pawan Dhananjay <pawandhananjay@gmail.com> Co-authored-by: Mark Mackey <mark@sigmaprime.io> Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com> Co-authored-by: Michael Sproul <michael@sigmaprime.io>

* Add blob sidecar inclusion test. * Fix lint

* Verify inclusion proof should not be fallible * Add blob sidecar inclusion test (#33) * Add blob sidecar inclusion test. * Fix lint

paulhauner added 3 commits October 2, 2018 17:35

Deleted some unnecessary files

eaa694b

Remove failing benchmarks

50d7252

paulhauner changed the title ~~Add pre-written validation code~~ Add block and attestation validation Oct 2, 2018

paulhauner requested a review from AgeManning October 2, 2018 07:49

Add doc comments to attestation validation

49737fb

paulhauner mentioned this pull request Oct 2, 2018

Implement block validation #11

Closed

djrtwo reviewed Oct 3, 2018

View reviewed changes

paulhauner added 17 commits October 4, 2018 14:46

Update comment for rejecting future block

c30a9a7

As per Danny's request

Separate parent block checking and proposer checking

e2d45ea

Add InvalidBitfieldEndBits tests

a998ac2

Rename voted_hashmap to voted_hashset

9a59908

Fix a type in sig. ver. comment

f13a4ff

Fix issue with last_justified_block_hash

cf9f8c1

Previously we were just checking it exists in the DB. This is incorrect because the last_justified_block_hash _must_ be in the chain referenced by the block. I.e., it's not OK for a block to reference a justified block in another chain.

Merge branch 'master' into validation

739abc0

Add block_at_slot function to block store

550ca79

This allows skipping back in the chain to find a block.

Return block hash with block_at_slot()

8c1d72a

Add test to block_store, export BlockAtSlotError

d729355

Verify attestation justified_block_hash.

6ef4268

Previously there was not a check that the hash was in the chain, just that it was known (in the database in any chain)

Replace hashmap with hashset

3139d0e

Fix "indicies" type

68ca51c

Add comment regarding tightening block val.

b544153

Comment was suggested by Danny R.

Ensure attestation val. check parent.slot

1f089d4

As per comments by Danny Ryan on PR#33

Ensure that parent_block.slot < block.slot

e8daca4

Fix big in attestation validation

90010ce

There was no check that the attestation is within an appropriate distance from its parent block.

djrtwo reviewed Oct 15, 2018

View reviewed changes

paulhauner mentioned this pull request Oct 15, 2018

Beacon block #50

Merged

paulhauner merged commit 2fb9dfb into master Oct 17, 2018

paulhauner deleted the validation branch October 17, 2018 23:07

ethDreamer pushed a commit to ethDreamer/lighthouse that referenced this pull request Jun 20, 2023

Add some metrics (sigp#33)

4553b40

mergify Bot pushed a commit that referenced this pull request Jun 17, 2024

Add blob sidecar inclusion test (#33)

8c26db7

* Add blob sidecar inclusion test. * Fix lint

mergify Bot pushed a commit that referenced this pull request Jun 17, 2024

Verify inclusion proof should not be fallible (#5787)

474c1b4

* Verify inclusion proof should not be fallible * Add blob sidecar inclusion test (#33) * Add blob sidecar inclusion test. * Fix lint

Uh oh!

Conversation

paulhauner commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djrtwo Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

djrtwo Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

paulhauner Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

djrtwo Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djrtwo Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

paulhauner Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

paulhauner commented Oct 16, 2018

Uh oh!

djrtwo commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

paulhauner commented Oct 2, 2018 •

edited

Loading

djrtwo Oct 16, 2018 •

edited

Loading