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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Restrict fork choice getters to finalized blocks #1475

Closed
wants to merge 10 commits into from
Closed

[Merged by Bors] - Restrict fork choice getters to finalized blocks #1475

wants to merge 10 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Aug 6, 2020 •
edited
Loading

Issue Addressed

Proposed Changes

  • Restricts the contains_block and contains_block so they only indicate a block is present if it descends from the finalized root. This helps to ensure that fork choice never points to a block that has been pruned from the database.
  • Resolves Check block ancestor is in chain before propagation #1451
  • Before importing a block, double-check that its parent is known and a descendant of the finalized root.
  • Split a big, monolithic block verification test into smaller tests.

Additional Notes

I suspect there would be a craftier way to do the is_descendant_of_finalized check, but we're a bit tight on time now and we can optimize later if it starts showing in benches.

TODO

  • Tests

@paulhauner paulhauner added work-in-progress PR is a work-in-progress consensus An issue/PR that touches consensus code, such as state_processing or block verification. labels Aug 6, 2020
@michaelsproul
Copy link
Member

This should also resolve #1451, right?

@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Aug 7, 2020
@michaelsproul michaelsproul marked this pull request as ready for review August 14, 2020 04:09
michaelsproul
michaelsproul approved these changes Aug 14, 2020
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

I couldn't fault this, despite my efforts! Let's merge!

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 14, 2020
@paulhauner
Copy link
Member Author

Woo! bors r+

@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 14, 2020
@paulhauner
Restrict fork choice getters to finalized blocks (#1475)
619ad10 
## Issue Addressed

- Resolves #1451

## Proposed Changes

- Restricts the `contains_block` and `contains_block` so they only indicate a block is present if it descends from the finalized root. This helps to ensure that fork choice never points to a block that has been pruned from the database.
- Resolves #1451
- Before importing a block, double-check that its parent is known and a descendant of the finalized root.
- Split a big, monolithic block verification test into smaller tests. 

## Additional Notes

I suspect there would be a craftier way to do the `is_descendant_of_finalized` check, but we're a bit tight on time now and we can optimize later if it starts showing in benches.

## TODO

- [x] Tests
@bors
Copy link

bors bot commented Aug 14, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title Restrict fork choice getters to finalized blocks [Merged by Bors] - Restrict fork choice getters to finalized blocks Aug 14, 2020
@bors bors bot closed this Aug 14, 2020
paulhauner added a commit that referenced this pull request Aug 15, 2020
@paulhauner
Squash in no-hash-replay
48d15f2 
Squashed commit of the following:

commit f2590e3
Author: Paul Hauner <paul@paulhauner.com>
Date:   Sat Aug 15 12:47:06 2020 +1000

    Introduced ForcedFixedLenIter

commit 52ab576
Author: Paul Hauner <paul@paulhauner.com>
Date:   Sat Aug 15 12:46:53 2020 +1000

    Fix bug in state-root-less replay

commit 765edf0
Author: Paul Hauner <paul@paulhauner.com>
Date:   Sat Aug 15 12:46:42 2020 +1000

    Update comment

commit 64393a4
Author: Paul Hauner <paul@paulhauner.com>
Date:   Sat Aug 15 12:02:36 2020 +1000

    Reject known gossip blocks quicker

commit 2ab752b
Author: Paul Hauner <paul@paulhauner.com>
Date:   Sat Aug 15 11:43:29 2020 +1000

    Fix mistake in previous commit

commit f93a681
Author: Paul Hauner <paul@paulhauner.com>
Date:   Sat Aug 15 11:34:40 2020 +1000

    Avoid cloning caches for atomic db ops

commit 8113ce6
Author: Paul Hauner <paul@paulhauner.com>
Date:   Sat Aug 15 11:12:46 2020 +1000

    Allow for skipping state roots in block replay

commit 6aeb896
Author: Paul Hauner <paul@paulhauner.com>
Date:   Fri Aug 14 22:24:27 2020 +0000

    Commit Cargo.lock changes, add build scripts (#1521)

    ## Issue Addressed

    NA

    ## Proposed Changes

    This PR commits the `Cargo.lock` file so it does not indicate a dirty git tree in the version tag. This code should be used for the `v0.2.3` release.

    Also, adds a `Makefile` command to produce tarballs for upload on release.

    ## Additional Info

    NA

commit f4a7311
Author: Paul Hauner <paul@paulhauner.com>
Date:   Fri Aug 14 08:32:31 2020 +0000

    Update to v0.2.3 (#1519)

    ## Issue Addressed

    NA

    ## Proposed Changes

    Bump versions to v0.2.3.

    ## Additional Info

    NA

commit 619ad10
Author: Paul Hauner <paul@paulhauner.com>
Date:   Fri Aug 14 06:36:38 2020 +0000

    Restrict fork choice getters to finalized blocks (#1475)

    ## Issue Addressed

    - Resolves #1451

    ## Proposed Changes

    - Restricts the `contains_block` and `contains_block` so they only indicate a block is present if it descends from the finalized root. This helps to ensure that fork choice never points to a block that has been pruned from the database.
    - Resolves #1451
    - Before importing a block, double-check that its parent is known and a descendant of the finalized root.
    - Split a big, monolithic block verification test into smaller tests.

    ## Additional Notes

    I suspect there would be a craftier way to do the `is_descendant_of_finalized` check, but we're a bit tight on time now and we can optimize later if it starts showing in benches.

    ## TODO

    - [x] Tests

commit b0a3731
Author: Paul Hauner <paul@paulhauner.com>
Date:   Fri Aug 14 04:38:45 2020 +0000

    Introduce a queue for attestations from the network (#1511)

    ## Issue Addressed

    N/A

    ## Proposed Changes

    Introduces the `GossipProcessor`, a multi-threaded (multi-tasked?), non-blocking processor for some messages from the network which require verification and import into the `BeaconChain`.

    Initial testing indicates that this massively improves system stability by (a) moving block tasks from the normal executor (b) spreading out attestation load.

    ## Additional Info

    TBC

commit e3d45ed
Author: Pawan Dhananjay <pawandhananjay@gmail.com>
Date:   Thu Aug 13 07:00:37 2020 +0000

    Log to file without json format (#1485)

    ## Issue Addressed

    N/A

    ## Proposed Changes

    Earlier, to log to a file, the only options were to redirect stdout/stderr to a file or use json logging.
    Redirecting to stdout/stderr works well but causes issues with mistakenly overwriting the file instead of appending which has resulted in loss of precious logs on multiple occasions for me.

    Json logging creates a timestamped backup of the file if it already exists, but the json format itself is hugely annoying.

    This PR modifies the `--logfile` option to log as it does in the terminal to a logfile.

commit 05a8399
Author: Adam Szkoda <adaszko@gmail.com>
Date:   Thu Aug 13 06:12:18 2020 +0000

    Wind down the SSE thread when the client disconnects (#1514)

    These started to appear when I `^C` `curl -N http://localhost:5052/beacon/fork/stream`: `Aug 12 13:00:01.539 ERRO Couldn't stream piece hyper::Error(ChannelClosed), service: http`

    Something must have changed in hyper since SSE has been implemented because I'm sure I haven't seen those errors before.

    This PR properly detects a closed SSE stream and cleans up.

commit e6f4552
Author: ladidan <46406043+ladidan@users.noreply.github.com>
Date:   Thu Aug 13 05:25:51 2020 +0000

    Update key-management.md (#1515)

    ## Issue Addressed

    consequent use of "wally"

    ## Proposed Changes

    Please list or describe the changes introduced by this PR.

    ## Additional Info

    Please provide any additional information. For example, future considerations
    or information useful for reviewers.

commit 8a1a405
Author: Adam Szkoda <adaszko@gmail.com>
Date:   Wed Aug 12 07:00:00 2020 +0000

    Fix a bug in fork pruning (#1507)

    Extracted from #1380 because merging #1380 proves to be contentious.

    Co-authored-by: Michael Sproul <michael@sigmaprime.io>
@paulhauner paulhauner deleted the finalized-desc branch October 5, 2020 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees
No one assigned
Labels
consensus An issue/PR that touches consensus code, such as state_processing or block verification. ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check block ancestor is in chain before propagation
2 participants
@paulhauner @michaelsproul