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

Allow persisting headers that are disconnected from genesis #1823

Merged

Conversation

cburgdorf
Copy link
Contributor

@cburgdorf cburgdorf commented Aug 14, 2019

What was wrong?

We want to allow persisting headers that are disconnected from genesis (with the intend to connect them back later). This is to allow syncing from a trusted checkpoint.

How was it fixed?

Add an optional genesis_parent_hash parameter to
eth.db.header.HeaderDB.persist_header_chain and
eth.db.chain.ChainDB.persist_block that allows to overwrite the hash that is used
to identify the genesis header. This allows persisting headers / blocks that aren't (yet)
connected back to the true genesis header.

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@cburgdorf cburgdorf force-pushed the christoph/spike/persist-orphan-header branch from eb574e5 to 6738c00 Compare August 14, 2019 13:41
pipermerriam
pipermerriam previously approved these changes Aug 14, 2019
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I have a very minor spider-sense reaction to the overloading of genesis_parent_hash to trick the database into doing the right thing but I don't have any better ideas or concrete objections.

eth/db/header.py Outdated
@@ -83,6 +83,10 @@ def persist_header(self,
) -> Tuple[Tuple[BlockHeader, ...], Tuple[BlockHeader, ...]]:
raise NotImplementedError("ChainDB classes must implement this method")

@abstractmethod
def persist_checkpoint_header(self, header: BlockHeader, score: int) -> None:
raise NotImplementedError("ChainDB classes must implement this method")
Copy link
Member

Choose a reason for hiding this comment

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

New paradigm uses ... for this.

@pipermerriam pipermerriam dismissed their stale review August 14, 2019 16:52

tests failing, waiting to see what tests say when conflicts are gone.

@cburgdorf cburgdorf force-pushed the christoph/spike/persist-orphan-header branch from 6738c00 to 835c9ad Compare August 15, 2019 14:29
Add an *optional* `genesis_parent_hash` parameter to
`eth.db.header.HeaderDB.persist_header_chain` and
`eth.db.chain.ChainDB.persist_block` that allows to overwrite the hash that is used
to identify the genesis header. This allows persisting headers / blocks that aren't (yet)
connected back to the true genesis header.

This feature opens up new, faster syncing techniques.
@cburgdorf cburgdorf force-pushed the christoph/spike/persist-orphan-header branch from 835c9ad to 6ac4322 Compare August 15, 2019 14:36
@cburgdorf
Copy link
Contributor Author

I have a very minor spider-sense reaction to the overloading of genesis_parent_hash to trick the database into doing the right thing

Yes, I agree. But at the same time, it's strikingly simple so it may not be that bad after all :D

tests failing, waiting to see what tests say when conflicts are gone.

Yep, I started off on the previous release. But now that you upgraded Trinity to latest py-evm code (thanks for taking care of that!) I have this rebased and polished.

@cburgdorf
Copy link
Contributor Author

I added another commit to kill the AsyncHeaderDB. It wasn't used anywhere (no, not even in Trinity). Trinity defines it's own async class and I think that may be the better place after all. So, this became just a burden to update for no real reason. I can move that out if it turns out to be controversial.

@cburgdorf cburgdorf merged commit adc4f8c into ethereum:master Aug 16, 2019
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.

None yet

2 participants