Skip to content

Conversation

@ethDreamer
Copy link
Member

Issue Addressed

#829

Proposed Changes

There's now a purge subcommand for the beacon node. The user can run this when the chain_db gets in a bad state. The next time they run the client it will rebuild the chain_db.

Additional Info

This is my first-ever Rust PR, there may be better ways of doing these things so feel free to request changes / give feedback.

@CLAassistant
Copy link

CLAassistant commented Mar 28, 2020

CLA assistant check
All committers have signed the CLA.

@paulhauner paulhauner added the ready-for-review The code is ready for review label Mar 29, 2020
@paulhauner
Copy link
Member

@michaelsproul perhaps this will interest you. It seems we might be missing the freezer DB here. Do you have suggestions on the best way to get the paths for both DBs?

@michaelsproul michaelsproul self-requested a review March 31, 2020 04:13
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

This looks good!

As Paul said, we should also purge the freezer database. I think it's reasonable for now to purge whatever directory the user has provided. The function to get the freezer DB is get_freezer_db_path on ClientConfig. In the long-run, it might be worth adding a confirmation for the deletion of both DBs.

The other thing we need before merging is for CI to pass. You can enable it on your fork by enabling Github actions: https://github.com/ethDreamer/lighthouse/actions. We're waiting on Github to improve this workflow -- we can also trigger a run manually if you're confident it will pass (right now it will fail because of formatting).

let data_dir = self.get_data_dir()
.ok_or("Failed to get data_dir".to_string())?;
let trap_file = data_dir.join(CHAIN_DB_PURGED_TRAP_FILE);
fs::File::create(trap_file).map_err( |err| {
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about the case where the purge fails midway but the trap file has been created: I think it's ok, because the user will probably get errors when the partially deleted database tries to reinitialise. The alternative is creating the trap file afterwards, in which case the user will probably get errors to do with resuming from a deleted DB. In both cases, they'll probably try a re-purge.

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 1, 2020
@ethDreamer
Copy link
Member Author

This looks good!

As Paul said, we should also purge the freezer database. I think it's reasonable for now to purge whatever directory the user has provided. The function to get the freezer DB is get_freezer_db_path on ClientConfig. In the long-run, it might be worth adding a confirmation for the deletion of both DBs.

I've made these changes

The other thing we need before merging is for CI to pass. You can enable it on your fork by enabling Github actions: https://github.com/ethDreamer/lighthouse/actions. We're waiting on Github to improve this workflow -- we can also trigger a run manually if you're confident it will pass (right now it will fail because of formatting).

I believe I've enabled them but I'm not completely sure? Let me know if I need to do something else.

@ethDreamer ethDreamer requested a review from michaelsproul April 2, 2020 13:25
Improved formatting, added remove freezer db, created function
get_db_purged_trap_file_path
@ethDreamer ethDreamer force-pushed the purge_db_subcommand branch from f72b5cc to 18de29b Compare April 8, 2020 16:13
@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 8, 2020
@michaelsproul michaelsproul added ready-to-squerge and removed ready-for-review The code is ready for review labels Apr 14, 2020
@michaelsproul michaelsproul merged commit 065ea15 into sigp:master Apr 14, 2020
@ethDreamer ethDreamer deleted the purge_db_subcommand branch December 19, 2022 21:22
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