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

[Merged by Bors] - Fix a bug in fork pruning #1507

Closed
wants to merge 3 commits into from

Conversation

adaszko
Copy link
Contributor

@adaszko adaszko commented Aug 11, 2020

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

A bug in a test case hid a bug in the code:
prunes_abandoned_fork_between_two_finalized_checkpoints: contrary to its
name, the abandoned fork isn't actually wholly contained between two
finalized checkpoints in terms of slot values.
@adaszko adaszko added bug Something isn't working ready-for-review The code is ready for review labels Aug 11, 2020
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.

Nice! Just a few small changes reqd

Think of it this way, small PRs are good for your GitHub stats 😉

beacon_node/beacon_chain/src/migrate.rs Show resolved Hide resolved
beacon_node/beacon_chain/tests/store_tests.rs Show resolved Hide resolved
adaszko and others added 2 commits August 11, 2020 16:22
Pruning stopped short of pruning the entire abandoned chain. It only
pruned the head.  This has to do with get_state() returning unexpected
states based on slots for states that that lie below the "split point".
Fix by doing pruning on the hot database (as it should be from the
start) and not touching the cold database.
@adaszko adaszko force-pushed the pr/fork-prunning-bugfix branch from b28d392 to 5950960 Compare August 11, 2020 14:22
@adaszko
Copy link
Contributor Author

adaszko commented Aug 12, 2020

bors r+

@bors
Copy link

bors bot commented Aug 12, 2020

🔒 Permission denied

Existing reviewers: click here to make adaszko a reviewer

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've run this on my Medalla node and it worked well

bors r+

bors bot pushed a commit that referenced this pull request Aug 12, 2020
Extracted from #1380 because merging #1380 proves to be contentious.

Co-authored-by: Michael Sproul <michael@sigmaprime.io>
@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 12, 2020
@bors bors bot changed the title Fix a bug in fork pruning [Merged by Bors] - Fix a bug in fork pruning Aug 12, 2020
@bors bors bot closed this Aug 12, 2020
paulhauner added a commit that referenced this pull request Aug 13, 2020
Squashed commit of the following:

commit 6433619
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 14:50:00 2020 +1000

    Tidy

commit dc95d3e
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 14:36:56 2020 +1000

    Fix comment

commit 79e0e28
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 13:26:29 2020 +1000

    Fix warning

commit 55caf5d
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 13:21:59 2020 +1000

    Ignore gossip messages when syncing

commit 8e59b5d
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 12:02:50 2020 +1000

    Add attestation error metrics

commit beb0874
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 11:26:51 2020 +1000

    Tidy, add new metric

commit 69d64d9
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 11:08:45 2020 +1000

    Fix queue size constants

commit 84e16b9
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 11:03:55 2020 +1000

    Tidy, improve docs

commit 5dbcd29
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 10:54:23 2020 +1000

    Separate idle/event messages

commit a6091dd
Author: Paul Hauner <paul@paulhauner.com>
Date:   Thu Aug 13 09:24:16 2020 +1000

    Tidy, add metric, debounce errors

commit f299d8b
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 20:49:15 2020 +1000

    Fix clippy lint

commit 21acf3c
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 19:40:45 2020 +1000

    Try address clippy ling

commit 2c1ddc7
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 19:38:55 2020 +1000

    Run cargofmt

commit 9f922d1
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 19:36:24 2020 +1000

    Tidy, add comments

commit 6dbd3cc
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 18:37:47 2020 +1000

    Fix clippy lints

commit 99b1bcb
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 18:05:54 2020 +1000

    Move aggregates into processor

commit a9789f8
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 16:18:23 2020 +1000

    Fix bugs with work count

commit b4f8bfe
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 15:03:52 2020 +1000

    Modify metrics

commit 8bec89e
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 14:51:39 2020 +1000

    Add metrics

commit 7328c94
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 14:25:27 2020 +1000

    Increase work queue size

commit 59875eb
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 14:21:50 2020 +1000

    Integrate into processor

commit 77cc8a4
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 13:56:38 2020 +1000

    Progress with queue

commit e45d58b
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 12:01:22 2020 +1000

    Remove atmoic

commit e7667c9
Author: Paul Hauner <paul@paulhauner.com>
Date:   Wed Aug 12 11:59:53 2020 +1000

    Add first draft of beacon queue

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>

commit 61367ef
Author: ladidan <46406043+r0oN@users.noreply.github.com>
Date:   Wed Aug 12 01:24:36 2020 +0000

    Update key-management.md (#1508)

    ## Issue Addressed

    minor documentation changes in order to have identical command prompts and description below

    ## Proposed Changes

    adjust description "wally" to align with command prompt

    ## Additional Info

    devs might give it a thought whether command line should be "mywallet"
    I personally prefer "wally" for minimization reasons =)
paulhauner added a commit that referenced this pull request Aug 15, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants