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

all: extend ancient store API with freezer identifier routing #24481

Closed
wants to merge 1 commit into from

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Feb 28, 2022

It's a PR based on #23954

Currently, ancient store is only used to store ancient chain data, but actually it can be extended.
In the near future, we will introduce reverse diff notion which is perfectly suitable with ancient
store. Before that we need to extend the current ancient store API with freezer routing.

In this PR, the routing is implemented by a freezer identifier, although there is only a single freezer
instance right now.

Only the last commit is relevant in this PR. 2dd348d

@@ -35,14 +35,73 @@ import (
// freezerdb is a database wrapper that enabled freezer data retrievals.
type freezerdb struct {
ethdb.KeyValueStore
ethdb.AncientStore
chain *freezer
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit curious (and lazy:) .. What made this change necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because for ancient store APIs, it requires an additional parameter typ and it's used for freezer instance routing. Right now there is only a single instance for chain and later we can add reverse diff and even more.

@@ -72,58 +72,78 @@ type KeyValueStore interface {
type AncientReader interface {
// HasAncient returns an indicator whether the specified data exists in the
// ancient store.
HasAncient(kind string, number uint64) (bool, error)
HasAncient(typ string, kind string, number uint64) (bool, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

You've probably given this more thought that I have (since you implemented it), so perhaps you can explain to me...

It strikes me as a strange choice to place the abstraction here. I mean, you could leave these interfaces intact, and have the 'chain' be one ancientreader, and the 'state' be another ancientreader. So the 'decision' of what data to access would be made higher up in the hierarchy.

I just think it seems a bit odd to have the 'typ string' selector so far down. Like, if we had two separate databases, for storing different confent, the database api would still be identical. We'd handle two databases distinct from eachother, not expect the db API to provide this kind of multiplexing.

But as I said, I haven't dived deep into this, so I'm probably missing some nuances.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main consideration for putting the multiplexing in database interface itself is to avoid passing multiple database instances in our codebase all around.

If we want to keep a single database instance, then we must define the interface somewhere which can accept the parameter(typ here) for freezer instance routing.

Copy link
Contributor

@fjl fjl Mar 31, 2022

Choose a reason for hiding this comment

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

I also think it's weird to plug it through the "type" parameter.

The problem here is that the database implements the Ancient* interfaces, representing an is-a relationship (database is ancients). We now want to convert this to a has-a relationship. Another way to make this refactoring would be adding methods like

type DatabaseReader interface{
    ...
    AncientReader() AncientReader
}

type DatabaseWriter interface{
    ...
    AncientWriter() AncientWriter
}

Copy link
Contributor

@fjl fjl Mar 31, 2022

Choose a reason for hiding this comment

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

I always disliked the ancients being to tied in with database. While I understand that you want to avoid passing multiple databases in code, it's also important to think about where the new, second freezer will be used. If we will only use it for reverse diffs, not all code needs access to reverse diffs. Only core.BlockChain needs it to perform reorgs.

With this in mind, the cleanest solution would be having a standalone way of opening a freezer instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

With this in mind, the cleanest solution would be having a standalone way of opening a freezer instance.

I can try with these approach. Expose a public API for opening a standalone freezer instance, and only use it in the "path-based-trie-database" only for reverse diff. Hopefully it can remove most of hacks and looks clean.

@holiman holiman requested a review from fjl March 31, 2022 07:29
@rjl493456442
Copy link
Member Author

Close it right now, we don't want to mess up the database interface in this approach.

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

3 participants