backport: Merge bitcoin#27254#7432
Conversation
There was a problem hiding this comment.
Code Review
Source: exact-SHA automated review for 173bd17f8f3d023626255ece0b48ed50916afb57; dash-core-commit-history and backport-reviewer specialists ran. Opus/Claude reviewer and verifier lanes failed due quota (resets Jul 10, 8am America/Chicago); Codex gpt-5.5 reviewer lanes and Codex verifier completed. No existing GitHub review-thread context was present for this PR.
Note: posted as a COMMENT review because GitHub does not allow formal approval/request-changes on my own PR.
🔴 1 blocking
-
Missing prerequisite: bitcoin#27254 (
src/common/args.cpp:39-41)Upstream bitcoin#27419 starts after bitcoin#27254, where
src/util/fs.handsrc/util/fs_helpers.hexist andcommon/args.cppincludes them directly. At this exact Dash SHA, the full Dash mergef5dec3bce1anamed in the PR description is not an ancestor; the branch only contains the older partial 27254 compatibility commit.The current backport works around that by forward-declaring
RenameOverandGetSpecialFolderPathinsrc/common/args.cpp, whilesrc/common/args.hstill includes the old<fs.h>and exportsGetDefaultDataDirinstead of getting it fromutil/fs_helpers.h. That may be an intentional Dash adaptation, but then the PR/commit description needs to explicitly say Dash is keeping the partial 27254 layout and explain these deviations. Otherwise, backport the full bitcoin#27254 prerequisite first.Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless explicitly allowlisted, for example with
intentional_exclusion/policy_overrideevidence.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/common/args.cpp`:
- [BLOCKING] src/common/args.cpp:39-41: Missing prerequisite: bitcoin#27254
Upstream bitcoin#27419 starts after bitcoin#27254, where `src/util/fs.h` and `src/util/fs_helpers.h` exist and `common/args.cpp` includes them directly. At this exact Dash SHA, the full Dash merge `f5dec3bce1a` named in the PR description is not an ancestor; the branch only contains the older partial 27254 compatibility commit. Either backport the full bitcoin#27254 prerequisite first or explicitly document that Dash is intentionally keeping the partial 27254 layout and that these declarations/header placements are deliberate.
|
🕓 Ready for review — 2 ahead in queue (commit c6475c6) |
thepastaclaw
changed the title
e86f8b3 to
8b4b8ad
Compare
thepastaclaw
changed the title
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This is a clean, correctly scoped extraction of util/fs and util/fs_helpers from util/system (bitcoin#27254 backport). At the exact head, GetDefaultDataDir is declared exactly once, fs_helpers.cpp is fully decoupled from util/system.h (using LogPrintf directly rather than error()), and all Dash-specific call sites and filesystem-helper behavior are correctly preserved. No blocking source-code issues were found; the only carried-forward observation is a commit-hygiene suggestion that six same-author corrective commits sitting on top of the aggregate merge commit should be squashed so every intermediate commit is an independently buildable bisect point.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— backport-reviewer (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit-range:93ff581f9e4..8b4b8ad02fc>`:
- [SUGGESTION] <commit-range:93ff581f9e4..8b4b8ad02fc>:1: Squash the six corrective commits into the bitcoin#27254 backport merge
The aggregate merge commit 93ff581f9e4 (Merge bitcoin/bitcoin#27254) is followed by six same-author commits — 58090d04297, ceb7cf18e1a, d4d87627ece, 7f72cef2246, 9d0afc49719, and 8b4b8ad02fc — made within a short window, each of which completes or corrects a defect in that same merge: finishing omitted extraction hunks and a missing direct include, restoring Dash-specific FileCommit/RenameOver/LockDirectory log messages the merge silently dropped, removing a stray unrelated include, restoring a stripped trailing newline, finishing include cleanup across several files, and finally removing a duplicate GetDefaultDataDir() declaration. That last point matters concretely: at 93ff581f9e4, GetDefaultDataDir is declared in both util/system.h and the newly added util/fs_helpers.h, which is a genuine -Wredundant-decls hazard under configure.ac's warning flags (promoted to an error with --enable-werror) — only fixed six commits later. Because Dash merges commit stacks without squashing, this leaves permanent bisect hazards: `git bisect` landing on 93ff581f9e4 finds a commit that may not build cleanly with strict flags, and `git blame` on src/util/fs_helpers.cpp will misattribute Dash-specific behavior restoration to a chain of small, dependent fixups rather than one coherent commit. Recommend folding all six corrective commits into 93ff581f9e4 via interactive rebase (preserving the upstream merge's authorship/message for the cherry-picked series) so the backport lands as a single, independently buildable commit.
8b4b8ad to
35be6bc
Compare
|
Addressed the clean-history review finding in
CI is running on the rewritten head. CodeRabbit follow-through is left to the automated |
35be6bc to
b1a9383
Compare
441dafe to
8e22882
Compare
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
reviewed commit 8e22882
@thepastaclaw, address ONLY mentioned concerns, don't create over-complications and unrelated fixes
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The filesystem extraction is buildable and preserves LockDirectory() runtime behavior, but it is not a complete backport of bitcoin#27254 because the missing bitcoin#27238 prerequisite forces prerequisite-dependent upstream hunks to be altered or retained. The declaration move also drops Dash-specific documentation explaining that ReleaseDirectoryLocks() is used during runtime restarts. The earlier commit-history finding is resolved because the current PR range contains one coherent backport commit.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/util/fs_helpers.cpp`:
- [BLOCKING] src/util/fs_helpers.cpp:72-73: Missing prerequisite: bitcoin#27238
Upstream commit aaced5633b8 from bitcoin#27238 moves error() from util/system.h to logging.h. The bitcoin#27254 extraction then moves LockDirectory() unchanged with its return error(...) path and removes util/system.h from flatfile.cpp. Dash contains neither prerequisite change at the PR base nor at this head: error() remains defined only in util/system.h, so this backport replaces the upstream LockDirectory() call with LogPrintf() plus return false and retains util/system.h in flatfile.cpp. Although the workaround is behavior-equivalent, the PR advertises the complete bitcoin#27254 backport while omitting its prerequisite-dependent decoupling hunk. Backport bitcoin#27238, or at least its error() relocation, and then preserve the upstream bitcoin#27254 changes.
Note: GitHub does not allow PastaClaw to request changes on their own PR, so this canonical blocking preliminary result is transported as a COMMENT review.
| LogPrintf("ERROR: Error while attempting to lock directory %s: %s\n", fs::PathToString(directory), lock->GetReason()); | ||
| return false; |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#27238
Upstream commit aaced56 from bitcoin#27238 moves error() from util/system.h to logging.h. The bitcoin#27254 extraction then moves LockDirectory() unchanged with its return error(...) path and removes util/system.h from flatfile.cpp. Dash contains neither prerequisite change at the PR base nor at this head: error() remains defined only in util/system.h, so this backport replaces the upstream LockDirectory() call with LogPrintf() plus return false and retains util/system.h in flatfile.cpp. Although the workaround is behavior-equivalent, the PR advertises the complete bitcoin#27254 backport while omitting its prerequisite-dependent decoupling hunk. Backport bitcoin#27238, or at least its error() relocation, and then preserve the upstream bitcoin#27254 changes.
source: ['codex']
| /** Release all directory locks. This is used for unit testing only, at runtime | ||
| * the global destructor will take care of the locks. | ||
| */ | ||
| void ReleaseDirectoryLocks(); |
There was a problem hiding this comment.
💬 Nitpick: Restore the Dash runtime-use note for ReleaseDirectoryLocks
Moving this declaration from util/system.h dropped the Dash-specific note explaining its runtime use. The remaining comment says the function is used only for unit testing, but init.cpp calls ReleaseDirectoryLocks() when a client restart is requested so the replacement process can acquire the directory lock. Preserve that context in the moved declaration.
| /** Release all directory locks. This is used for unit testing only, at runtime | |
| * the global destructor will take care of the locks. | |
| */ | |
| void ReleaseDirectoryLocks(); | |
| /** Release all directory locks. This is used for unit testing and to release | |
| * locks before restarting the client. At normal shutdown, the global destructor | |
| * will take care of the locks. | |
| */ | |
| void ReleaseDirectoryLocks(); |
source: ['codex']
b6b6ac5 to
8e22882
Compare
00e9b97 refactor: Move fs.* to util/fs.* (TheCharlatan) 106b46d Add missing fs.h includes (TheCharlatan) b202b3d Add missing cstddef include in assumptions.h (TheCharlatan) 18fb363 refactor: Extract util/fs_helpers from util/system (Ben Woosley) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin#24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR bitcoin#25152. #### Context There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (bitcoin#25290, bitcoin#25487, bitcoin#25527, bitcoin#25862, bitcoin#26177, and bitcoin#27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in bitcoin#27238. #### Changes Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files. There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory. Further commits splitting more functionality out of `system.h` are still in bitcoin#25152 and will be submitted in separate PRs once this PR has been processed. ACKs for top commit: hebasto: ACK 00e9b97 Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
8e22882 to
c6475c6
Compare
|
Addressed the requested review follow-up in signed commit c6475c6. The patch is limited to restoring the two Dash-specific details in |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (97)
💤 Files with no reviewable changes (1)
WalkthroughThe change extracts filesystem helper APIs from Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CI classification for |
Backport of bitcoin#27150 (merge `fc037c8c83`). Squashes the four upstream topic commits in order: - `c361df90b9` scripted-diff: Remove double newlines after some init errors - `3db2874bd7` Extend bilingual_str support for tinyformat - `d172b5c671` Add InitError(error, details) overload - `802cc1ef53` Deduplicate bitcoind and bitcoin-qt init code Dash adaptations: - Keep Dash CoreContext construction and -printcrashinfo handling in AppInit, and retain dashd/dash-qt/dash.conf naming in user-facing text. - Apply the InitError fatal-index change to Dash current BaseIndex::FatalErrorImpl implementation. - Qualify nested tfm::format() calls because Dash builds as C++20, where upstream unqualified calls collide with std::format through ADL. This was fixed later upstream by `fa8ef7d138`. - Register src/common/init.cpp with IWYU in ci/dash/lint-tidy.sh; upstream ci/test/06_script_b.sh was removed when Dash migrated CI. - After dashpay#7432 (bitcoin#27254 util/fs extract), include util/fs.h instead of fs.h in the new common/init.cpp, and do not reintroduce the pre-extract <fs.h> include in qt/bitcoin.cpp. No source or test hunks are excluded. The AbortError-to-InitError shutdown change is retained here with its upstream owner, d172b5c.
4 participants
Issue being fixed or feature implemented
Backports bitcoin#27254, the earliest remaining prerequisite in the bitcoin#29034 backport chain.
This PR previously bundled bitcoin#27254 and bitcoin#27419. It has been split back to one prerequisite at a time; bitcoin#27419 will follow separately.
What was done?
util/systemintoutil/fsandutil/fs_helpers.node/chainstate.cppinclude.FileCommit()errors andRenameOver()behavior.util/bitdeque.hinclude copied from later history and the unused inherited filesystem include fromsrc/net.h.The final change is scoped to bitcoin#27254 only: 97 files changed, 474 insertions, and 400 deletions.
The include audit found 84 upstream matches, 12 documented Dash-only deviations, and one file with no include change.
src/wallet/walletdb.cppnow placesutil/fs.hin the same upstream-relative position betweensync.handutil/system.h, while retaining the Dash-only governance include in canonical order.How Has This Been Tested?
On macOS:
The include-only scope check confirmed that every review-addressing delta line is an include directive or include-block blank line. The non-GUI core and unit-test builds passed; local Qt development packages are unavailable, so fresh GitHub CI will provide the Qt build coverage.
Independent exact-head backport prerequisite gate: passed at
8e2288222a8e35d9f359c5121bb87a91a1715cb7with no missing prerequisites or unexplained deviations.Breaking Changes
None expected. This is a filesystem-helper extraction with Dash-specific behavior preserved.
Checklist: