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

Check that block numbers are consecutive #1571

Closed
mrBliss opened this issue Feb 4, 2020 · 3 comments
Closed

Check that block numbers are consecutive #1571

mrBliss opened this issue Feb 4, 2020 · 3 comments
Assignees
Labels
bug Something isn't working byron Required for a Byron mainnet: replace the old core nodes with cardano-node consensus issues related to ouroboros-consensus
Milestone

Comments

@mrBliss
Copy link
Contributor

mrBliss commented Feb 4, 2020

Apparently, nowhere in the Byron or Shelley ledger is it verified that blocks have consecutive block numbers.

We do check it in an assertion ChainFragment, but in production, assertions will be disabled.

We should verify this at the earliest opportunity we have, which is applyChainState (called for synced headers but also when we apply blocks we produced ourselves, as part of applyExtLedgerState). In addition to check whether block numbers are consecutive, we should also check that the first block has the right block number, i.e., genesisBlockNo.

It is important that we check this, because we're assuming that block numbers are a reliable proxy for chain length. So if this is not the case, we might prefer a chain that is actually shorter. So both consecutiveness and the right initial block number are important. For example, an attacker might forge a single block with a very large block number, and we would incorrectly prefer it over all other chains.

@mrBliss mrBliss added bug Something isn't working consensus issues related to ouroboros-consensus priority high labels Feb 4, 2020
@mrBliss mrBliss added this to the S6 2020-02-13 milestone Feb 4, 2020
@mrBliss mrBliss assigned edsko and nc6 Feb 4, 2020
@mrBliss mrBliss changed the title Check that blocks are consecutive Check that block numbers are consecutive Feb 4, 2020
@edsko
Copy link
Contributor

edsko commented Feb 5, 2020

@nc6 , change of plan, no need to do this for each ledger (although the spec should still verify it, I think). I will do this once and for all for all ledgers. You actually discussed doing that when discussing the Byron integration, albeit for hashes, not block numbers; I'm coming round to your point of view now :) PR to follow, unassigning you from this ticket.

@edsko edsko unassigned nc6 Feb 5, 2020
@edsko
Copy link
Contributor

edsko commented Feb 5, 2020

WIP PR at #1577 .

@edsko edsko added the byron Required for a Byron mainnet: replace the old core nodes with cardano-node label Feb 7, 2020
@edsko
Copy link
Contributor

edsko commented Feb 7, 2020

So #1578 cleans up a lot of our BlockNo hygiene, but I'd like to fix IntersectMBO/cardano-base#78 before fixing this ticket, in order to ensure that we don't have other places with bad BlockNo hygiene.

edsko added a commit that referenced this issue Feb 11, 2020
edsko added a commit that referenced this issue Feb 11, 2020
@iohk-bors iohk-bors bot closed this as completed in 7cca829 Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working byron Required for a Byron mainnet: replace the old core nodes with cardano-node consensus issues related to ouroboros-consensus
Projects
None yet
Development

No branches or pull requests

3 participants