-
Notifications
You must be signed in to change notification settings - Fork 20.3k
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: add read-only option to database #22407
Conversation
@holiman it's rebased. Please take another look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR also closes #22498 |
Oh wait
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, previously, func MakeChain(ctx *cli.Context, stack *node.Node, readOnly bool) (chain *core.BlockChain, chainDb ethdb.Database) {
existed and took a readOnly
boolean. However, it was mostly ignored, just stopped the tx indexing from happening.
As a result, some operations call it with readOnly
set to true, snapshot prune-state
is one of them. It now fails, because the readOnly
is interpreted strictly (correctly).
I guess we need some way to say "I do want to modify the db, but please do not spin up tx indexer/unindexer or snapshot generator/wiper, just the core stuff".
Here are the callers of
So afaict it's only |
thanks, I will check the pruner command and fix it |
There are several PR's that touch the same things, which maybe should be merged into one?
|
It's still problematic. The readonly notion should also be passed into the freezer. Otherwise the background migrator will be enabled by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I just need do a bit more testing before I click the approve
|
Looks like the datadir is empty, so that the initialization is enabled by default. I think it should be the correct behavior? Previously we have lots of "underlying" database operations. Now the policy is a bit stricter. |
It looks like that, yes, but it's not that simple, for some reason:
|
@holiman I think introducing the "Read-Only" chain is an immature idea. The blockchain is self-contained. It has a lot of recovery logic:
All of these will require the disk write permission. So instead of fixing the So I think this version is safer and easier to review. It's just too dangerous to introduce the "read-only" to chain without enough refactoring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* all: add read-only option to database * all: fixes tests * cmd/geth: migrate flags * cmd/geth: fix the compact * cmd/geth: fix the format * cmd/geth: fix log * cmd: add chain-readonly * core: add readonly notion to freezer * core/rawdb: add log * core/rawdb: fix freezer close * cmd: fix * cmd, core: construct db * core: update tests
This PR introduces the "Read-Only" database and fixes a few issues in the db commands.
Test cases
MakeChainDatabase
MakeChain