Reject many skipped blocks #4020
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## unstable #4020 +/- ##
================================
================================
|
Performance Report✔️ no performance regression detected Full benchmark results
|
dadepo
commented
May 14, 2022
| @@ -66,6 +66,12 @@ Will double processing times. Use only for debugging purposes.", | |||
| group: "chain", | |||
| }, | |||
|
|
|||
| "chain.maxSkipSlots": { | |||
There was a problem hiding this comment.
@dapplion should this option to be hidden? Also should it have a default value? I do not see a default value set in LH implementation though. If there is a need to set a default, what range of value will be advisable?
There was a problem hiding this comment.
Lighthouse doesn't have a default value because it's disabled by default. If the CLI value is not set the check does not apply
There was a problem hiding this comment.
Yeah. I was asking, should we also implement it that way (that is have it disabled by default, and hence do not have a default value)? Or do we consider the possibility of a DoS via this avenue high enough that we want to have a default value? (and hence have the check enabled by default)
There was a problem hiding this comment.
With this enabled by default you can also reject valid blocks so it's a dangerous feature if miss-used. I imagine that's why Lighthouse has disabled it by default
wemeetagain
previously approved these changes
Jun 3, 2022
dapplion
suggested changes
Jun 4, 2022
| "safe-slots-to-import-optimistically": number; | ||
| // this is defined as part of IBeaconPaths | ||
| // "chain.persistInvalidSszObjectsDir": string; |
There was a problem hiding this comment.
Why is this comment deleted?
There was a problem hiding this comment.
Because I was not sure of the value of keeping the property commented out...it felt like something that was marked for removal.
I have brought it back and updated it a bit to be more clear.
| @@ -35,6 +36,9 @@ describe("gossip block validation", function () { | |||
| chain.forkChoice = forkChoice; | |||
| regen = chain.regen = sinon.createStubInstance(StateRegenerator); | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |||
| (chain as any).opts = {maxSkipSlots}; | |||
There was a problem hiding this comment.
You can cast to the full class, which should allow editing. Better than any
There was a problem hiding this comment.
Unfortunately on the full class ie BeaconChain, opts is a readonly value, so casting it to the full class will not allow editing
There was a problem hiding this comment.
Please keep trying other strategies before casting to any, for example
((chain as Pick<BeaconChain, "opts">) as {opts: BeaconChain["opts"]}).opts = {maxSkipSlots}Or what about just
chain.opts.maxSkipSlots = maxSkipSlotsJust keep iterating until you find the strongest chain of casting that will not compile if something changes
dadepo
force-pushed
the
dadepo/reject-many-skipped-blocks
branch
from
1aa7dd0
to
05ebc6f
Compare
|
@dapplion I responded to your recent comments...if you are fine by them...is it possible to help merge? |
wemeetagain
added
status-needs-review
help-wanted
dapplion
force-pushed
the
dadepo/reject-many-skipped-blocks
branch
from
b1e598a
to
309170d
Compare
dapplion
removed
the
help-wanted
dapplion
force-pushed
the
dadepo/reject-many-skipped-blocks
branch
from
348a895
to
94dc8e0
Compare
dapplion
approved these changes
Jul 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Description
Closes #3988
Steps to test or reproduce