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

feat: sync: validate (early) that blocks fall within range #10691

Merged
merged 1 commit into from
Apr 22, 2023

Conversation

Stebalien
Copy link
Member

Proposed Changes

This will reject blocks in pubsub validation if they're either:

  1. Too far into the future (5 blocks beyond the expected head).
  2. Too far into the past (before finality).

Specifically:

  1. We were previously rejecting future blocks in the sync logic, but not in pubsub itself.
  2. We never used to check if a block was too old.

Motivation: Blocks that are too new/too old can cause us to perform quite a bit of unnecessary work.

We should carefully consider that second check as we may need to tweak it (e.g., to handle network issues?). We may want to base this check on the latest synced head, not the expected epoch.

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • Tests exist for new functionality or change in behavior
  • CI is green

@Stebalien Stebalien requested a review from a team as a code owner April 18, 2023 23:15
@Stebalien
Copy link
Member Author

Well, this definitely broke something.

@Stebalien
Copy link
Member Author

We should carefully consider that second check as we may need to tweak it (e.g., to handle network issues?). We may want to base this check on the latest synced head, not the expected epoch.

Ok, I needed this, but I'm concerned I'm misunderstanding something here. As far as I know, the "catch up" code path shouldn't end up calling IsEpochInConsensusRange.

@Stebalien Stebalien marked this pull request as draft April 19, 2023 00:46
@Stebalien Stebalien requested a review from magik6k April 19, 2023 00:47
chain/consensus/iface.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

I don't know this part of the system well enough to be a very useful approver but the idea and implementation seem good.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Not entirely sure about Ignore/Reject, I guess if we want to be conservative, ignore is better, but I don't have a strong opinion.

Other parts LGTM.

@vyzo
Copy link
Contributor

vyzo commented Apr 20, 2023

I think it is better to be conservative and Ignore here, otherwise it might have adverse effects with the score penalties.

Basically we should only do Reject for obviously bad behaviours, not things that can happen naturally.

Base automatically changed from fix/remove-pointless-panic to master April 21, 2023 11:47
@Stebalien Stebalien force-pushed the steb/consensus-range branch 2 times, most recently from 8e82393 to 5c86cca Compare April 21, 2023 21:23
This will reject blocks in pubsub validation if they're either:

1. Too far into the future (5 blocks beyond the expected head).
2. Too far into the past (before finality with respect to our current
   head).

Specifically:

1. We were previously rejecting future blocks in the sync logic, but not
   in pubsub itself.
2. We never used to check if a block was too _old_.

Motivation: Blocks that are too new/too old can cause us to perform
quite a bit of unnecessary work.
@Stebalien Stebalien marked this pull request as ready for review April 21, 2023 21:33
@Stebalien Stebalien enabled auto-merge (squash) April 21, 2023 21:33
@Stebalien Stebalien disabled auto-merge April 21, 2023 21:34
@Stebalien
Copy link
Member Author

Ok, this should be good to go. It ignores blocks before finality with respect to our current head and 5 blocks after the expected head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants