fix(l1,l2): stop removedb from faking success on the wrong datadir - #7080
fix(l1,l2): stop removedb from faking success on the wrong datadir#7080NikhilSharmaWe wants to merge 2 commits into
Conversation
Greptile SummaryThe PR makes database removal report missing or failed deletions honestly and adds safeguards against targeting the wrong network database.
Confidence Score: 5/5The PR appears safe to merge, with the changed removal paths consistently refusing misleading success and propagating deletion failures. The new L1 guard covers every datadir suffix produced by current network resolution, explicit removal now fails for missing targets, optional import wipes remain soft, and actual filesystem failures are returned rather than hidden.
|
| Filename | Overview |
|---|---|
| cmd/ethrex/cli.rs | Adds guarded network-specific database removal, fallible deletion, explicit missing-target handling, and focused tests without an accepted defect. |
| cmd/ethrex/l2/command.rs | Propagates removal failures and avoids invoking removal for absent dev-bootstrap paths; no changed-code regression was established. |
| cmd/ethrex/Cargo.toml | Adds tempfile solely as a development dependency for the new unit tests. |
| Cargo.lock | Records the expected test dependency in the ethrex package dependency list. |
Reviews (1): Last reviewed commit: "fix(l1,l2): stop removedb from faking su..." | Re-trigger Greptile
5ef5512 to
a669b0b
Compare
|
cc @ilitteri |
| workspace = true | ||
|
|
||
| [dev-dependencies] | ||
| tempfile.workspace = true |
There was a problem hiding this comment.
tempfile lands in the root Cargo.lock, but tooling/ is a second workspace with its own lockfile and this PR doesn't touch it. Its ethrex entry still carries the pre-PR dep set, so Check Cargo.lock should fail on:
cargo metadata --locked --manifest-path tooling/Cargo.toml
(Makefile:269.) Cargo records dev-dependencies in the lockfile's dependencies array alongside normal ones, so a dev-only dep still rotates both lockfiles - I checked, and tempfile is present in the root ethrex block and absent from the tooling/ one.
Running cargo metadata --manifest-path tooling/Cargo.toml without --locked regenerates it. #7067 hit exactly this and it cost a round-trip, so flagging it before CI finishes rather than after.
The .workspace = true form itself is right, and the root lockfile entry is correct.
There was a problem hiding this comment.
Looked into it and I don't think we need to update tooling/Cargo.lock for this change.
tempfile is only added under [dev-dependencies] on ethrex. In the tooling workspace, ethrex is used as a path dependency, not as a workspace member, so Cargo does not include its [dev-dependencies] in that lockfile.
I verified locally that cargo metadata --locked --manifest-path tooling/Cargo.toml passes, and running it without --locked does not change the lockfile. #7067 needed the tooling lock update because it added a normal dependency on ethrex-dev, which is different from this case.
Happy to take another look if I missed something.
| candidates.push(base.to_path_buf()); | ||
| } | ||
|
|
||
| for suffix in Network::all_datadir_suffixes() { |
There was a problem hiding this comment.
This makes all_datadir_suffixes() load-bearing for a safety decision, and it's a hand-maintained list whose own comment says "Update this when adding new PublicNetwork variants."
Before this PR that list drove migration detection, where missing an entry means a directory doesn't get migrated - annoying, recoverable. Here a missing entry means find_valid_datadir_candidates doesn't see that sibling, guard_remove_datadir finds no alternates, and it returns Ok(()). The guard fails open, which on this code path means proceeding with a remove_dir_all the guard exists to prevent.
Worth noting the coverage is currently exact - I checked datadir_suffix() against it and every arm is reachable here: mainnet/hoodi/sepolia from the static list, chain-* from the read_dir scan (covering both L2Chain and GenesisPath), dev, and bare base. So this is about the invariant's durability, not a present gap.
Two options, either fine:
- Derive the list from the same match
datadir_suffix()uses, so adding a variant can't silently skip it. - Or invert the default: if the resolved target has no valid DB and the base directory contains any subdirectory that looks like a datadir, refuse regardless of whether it matched a known suffix. Fails closed, which is the right direction for a destructive command.
A test that adds a hypothetical unknown-suffix sibling and asserts the guard still refuses would pin whichever you pick.
There was a problem hiding this comment.
Went with fail-closed.
Candidate discovery now looks at any immediate subdirectory with a valid ethrex DB, not the hand-maintained suffix list. Added a test with an unknown sibling name (weirdnet) to lock that in.
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
…hint Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
a669b0b to
5a3ac66
Compare
Summary
Fixes #6925.
removedbcould resolve the wrong network datadir (e.g. defaultmainnetinstead ofchain-<id>), create that empty directory, delete it, and print success while the real DB was left alone.This change:
--datadirdoes (chain-*, public network dirs, or the old unsuffixed layout)--forceas “skip confirm” onlyimport --removedbsoft when the target dir is missing, so a fresh network under a shared base still worksAlso updates L2
removedbto use the same honest delete path, and adds unit tests for the main cases.Test plan
cargo test -p ethrex --lib -- remove_db_ require_datadir guard_ find_candidates explicit_removedb import_styleremovedbwithout the node’s--networkshould refuse and leavechain-*alone--networkshould actually remove the DB