backport: Bitcoin Core v0.26 backports (batch 3)#7433
Conversation
|
This pull request has conflicts, please rebase. |
cb684ab to
5f660ae
Compare
|
CI scope check for tracker #1918: the failing job is the known flaky |
|
🕓 Ready for review — 1 ahead in queue (commit 208f73b) |
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed), claude/backport-reviewer=opus(failed), codex/backport-reviewer=gpt-5.5(completed); verifier=gpt-5.5.
Verified head 5f660ae62355b8698bbed4acc4150a48ff242859. The dashd init split contains a real merge-resolution regression, and several advertised full Bitcoin PR backports omit upstream hunks because Dash is missing prerequisite Bitcoin infrastructure. The deterministic backport-prereq policy gate was applied and its restored prerequisite evidence has been merged into the findings below. No CodeRabbit inline findings were present.
5 blocking | 1 nitpick
Note: GitHub does not allow me to request changes on my own PR, so this is posted as a COMMENT while preserving the verified blocking findings.
Verified Findings
blocking: -printcrashinfo no longer exits after printing
src/bitcoind.cpp:121-123
ParseArgs() prints decoded -printcrashinfo output and returns true, but main() treats that only as successful argument parsing and then continues through ProcessInitCommands() and AppInit(). ProcessInitCommands() only handles help/version, so dashd -printcrashinfo=... can proceed into normal node startup after printing. The same split also moved help/version handling behind datadir/config/settings initialization, while the pre-backport code handled these informational commands before datadir/config validation.
blocking: Missing prerequisite: bitcoin#18044 for bitcoin#27625 wtxid coverage
test/functional/p2p_leak_tx.py:7-8
Upstream bitcoin#27625 changes p2p_leak_tx.py to import MSG_WTX and P2PTxInvStore, adds test_notfound_on_replaced_tx(), and requests both txid and wtxid inventory for the replaced transaction. The Dash cherry-pick only adds disconnect_p2ps() to the older txid-only test because Dash lacks the wtxid relay/test framework; tracing MSG_WTX points to bitcoin#18044. Since the PR advertises a full backport of bitcoin#27625, this omitted upstream test path is a missing prerequisite that needs either the prerequisite backport or an explicit documented exclusion.
blocking: Missing prerequisite: bitcoin#27150 common::InitConfig for bitcoin#27708
src/bitcoind.cpp:126-142
In upstream bitcoin#27708, ParseArgs delegates datadir/config/settings setup to common::InitConfig(args). Dash substitutes the older local sequence (CheckDataDirOption(), ReadConfigFiles(), SelectParams(), InitSettings()) because common::InitConfig is absent; that helper and supporting common/init split were introduced by bitcoin#27150. For a full advertised backport, this is a missing prerequisite rather than a faithful application of the upstream starting state.
blocking: Missing prerequisite: bitcoin#19988 txrequest.cpp hunk for bitcoin#27783
test/lint/lint-includes.py:26-41
Upstream bitcoin#27783 adds explicit Boost public headers in src/txrequest.cpp, including boost/tuple/tuple.hpp, and updates the lint allowlist accordingly. Dash has no src/txrequest.cpp, so the cherry-pick omits that upstream hunk and does not add the matching lint entry. The missing module comes from bitcoin#19988, making this a missing prerequisite for a full backport of bitcoin#27783 unless the exclusion is documented.
blocking: Missing prerequisite: bitcoin#27556/bitcoin#26715 wallet test hunks for bitcoin#27783
src/wallet/test/walletdb_tests.cpp:30-30
Upstream bitcoin#27783 changes BOOST_ASSERT uses in walletdb_read_write_deadlock and wallet_load_ckey. In this Dash tree, walletdb_tests.cpp ends after walletdb_readkeyvalue, and src/wallet/test/walletload_tests.cpp is absent, so both upstream hunks were omitted. Bitcoin history shows walletdb_read_write_deadlock was introduced by bitcoin#27556, while the wallet_load_ckey shape touched by bitcoin#27783 depends on wallet-test work from bitcoin#26715. These are test-only omissions, but they are missing prerequisites under the full-backport completeness rule unless explicitly excluded.
nitpick: Remove transient recovery note from commit message
<commit:6be2980>:1-1
Commit 6be29805584 includes an operational recovery note about a Codex worker failure, parent-session staging, and a stray Qt conflict artifact. That is permanent git log noise and does not help future backport archaeology. Keep the durable backport provenance and validation, but drop the internal recovery details before merge.
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:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddf030f4c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
WalkthroughThe pull request centralizes daemon and GUI configuration initialization, makes configuration path handling instance-based, and propagates process exit status through node startup and shutdown. Fatal initialization, indexing, and block-import failures now use abort handling. GUI settings errors use structured callbacks and configuration statuses. Bilingual formatting gains translated argument handling with unit coverage. Functional tests, fuzz limits, sanitizer settings, build files, and IWYU coverage are updated. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MAIN_FUNCTION
participant ParseArgs
participant InitConfig
participant AppInit
participant NodeContext
MAIN_FUNCTION->>ParseArgs: parse command-line arguments
ParseArgs->>InitConfig: load config and settings
MAIN_FUNCTION->>AppInit: initialize node
AppInit->>NodeContext: publish exit status
MAIN_FUNCTION->>NodeContext: wait, interrupt, and shutdown
NodeContext-->>MAIN_FUNCTION: return process exit status
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shutdown.cpp`:
- Around line 25-38: Initialize the shutdown state for standalone chainstate
execution before any validation or blockstorage work can call AbortNode. Locate
the chainstate entry point in bitcoin-chainstate.cpp and invoke
InitShutdownState with the appropriate exit-status storage during startup,
ensuring g_exit_status is set before failures are handled; preserve the existing
initialization path in init.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: faecc176-cea3-4d9f-ae2e-e77d77a67d34
📥 Commits
Reviewing files that changed from the base of the PR and between 2bbf4a4 and ddf030f4c5e3138e992914bab1d968fc7786f0c6.
📒 Files selected for processing (21)
ci/test/00_setup_env_native_asan.shci/test/00_setup_env_native_fuzz.shsrc/bitcoin-chainstate.cppsrc/bitcoind.cppsrc/index/base.cppsrc/init.cppsrc/init.hsrc/interfaces/node.hsrc/net_processing.cppsrc/node/blockstorage.cppsrc/node/context.hsrc/node/interfaces.cppsrc/qt/bitcoin.cppsrc/qt/bitcoin.hsrc/shutdown.cppsrc/shutdown.hsrc/test/fuzz/utxo_total_supply.cppsrc/wallet/test/wallet_tests.cpptest/functional/feature_abortnode.pytest/functional/p2p_leak_tx.pytest/functional/test_framework/test_node.py
💤 Files with no reviewable changes (1)
- src/qt/bitcoin.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.5(failed: ACP refresh token revoked), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.5(failed: ACP refresh token revoked), claude/backport-reviewer=opus(completed), codex/backport-reviewer=gpt-5.5(failed: ACP refresh token revoked); verifier=opus.
Full cumulative verification at head ddf030f4c5e. Delta from prior 5f660ae6 is one empty CI-retrigger commit with zero tree changes, so all prior verified blocking findings still apply. Confirmed the -printcrashinfo merge-resolution regression at src/bitcoind.cpp:121-124/286-288 (ParseArgs returns true, main() only bypasses AppInit for ParseArgs failure or ProcessInitCommands success, so -printcrashinfo falls through to normal node startup). Confirmed four missing upstream prerequisites via direct filesystem/grep checks: no src/txrequest.cpp (bitcoin#19988), no src/wallet/test/walletload_tests.cpp and no walletdb_read_write_deadlock (bitcoin#27556/bitcoin#26715), no common::InitConfig (bitcoin#27150), and p2p_leak_tx.py lacks wtxid coverage (bitcoin#18044). Two style findings on commit hygiene (empty CI-retrigger commit at HEAD, transient recovery note in 6be2980) also verified. Codex lanes failed to produce JSON due to ACP refresh token revocation; no attribution possible. CodeRabbit had no findings.
Commit-history nit: 6be29805584 still embeds transient Codex worker/recovery diagnostics in its permanent commit message; retain durable provenance/validation and remove that operational note before merge.
🔴 5 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
5 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 `src/bitcoind.cpp`:
- [BLOCKING] src/bitcoind.cpp:121-123: -printcrashinfo no longer exits after printing
`ParseArgs()` prints decoded `-printcrashinfo` output and returns `true` (lines 121-124), but `main()` at lines 286-288 only bypasses `AppInit` when `ParseArgs` returns false or when `ProcessInitCommands` returns true. `ProcessInitCommands` (lines 153-172) only handles `-help`/`-version`, so `dashd -printcrashinfo=...` proceeds through `AppInit(node)` into full node startup after printing the decoded crash info. Before the bitcoin#27708 split, the `-printcrashinfo` branch lived inside `AppInit` and returned `true`, and `main` skipped `WaitForShutdown` on that path; the split moved the branch to `ParseArgs` without preserving the exit semantics. The Dash-only clause needs its own early-exit path — either return a sentinel that `main` treats as EXIT_SUCCESS, or handle `-printcrashinfo` in `ProcessInitCommands` alongside help/version. The same split also moves help/version behind datadir/config/settings initialization; the pre-backport code handled those informational commands before datadir validation, which is a secondary behavioral regression worth revisiting as part of the fix.
- [BLOCKING] src/bitcoind.cpp:126-142: Missing prerequisite: bitcoin#27150 common::InitConfig for bitcoin#27708
Upstream bitcoin#27708 delegates datadir/config/settings setup to `common::InitConfig(args)`, which centralizes error handling and exit semantics. Dash substitutes the older local sequence — `CheckDataDirOption()`, `ReadConfigFiles()`, `SelectParams()`, `InitSettings()` — because `common::InitConfig` is absent (grep for `common::InitConfig` returns no hits). The helper and supporting `common/init.{h,cpp}` split were introduced by bitcoin#27150. For a full advertised backport of bitcoin#27708, this is a missing prerequisite rather than a faithful application of the upstream starting state. Either backport bitcoin#27150 (and the short chain leading to it) so `ParseArgs` can use `common::InitConfig`, or document the omission and its scope in the commit/PR body.
In `test/functional/p2p_leak_tx.py`:
- [BLOCKING] test/functional/p2p_leak_tx.py:7-8: Missing prerequisite: bitcoin#18044 for bitcoin#27625 wtxid coverage
Upstream bitcoin#27625 modifies `p2p_leak_tx.py` to import `MSG_WTX` and `P2PTxInvStore`, adds `test_notfound_on_replaced_tx()`, and requests both txid and wtxid inventory for the replaced transaction. The Dash cherry-pick only adds `self.nodes[0].disconnect_p2ps()` to the older txid-only test and omits the wtxid coverage entirely because Dash's node lacks wtxid relay (bitcoin#18044). Even though `MSG_WTX` and `P2PTxInvStore` exist in Dash's `test_framework/p2p.py`, the omitted test path exercises wtxid `notfound` delivery from the node, which requires the wtxid-relay backport. Since the PR advertises a full backport of bitcoin#27625, this is a missing prerequisite: either backport bitcoin#18044's wtxid-relay path or document the intentional exclusion in the commit/PR body.
In `test/lint/lint-includes.py`:
- [BLOCKING] test/lint/lint-includes.py:26-41: Missing prerequisite: bitcoin#19988 txrequest.cpp hunk for bitcoin#27783
Upstream bitcoin#27783 adds explicit public Boost headers (including `boost/tuple/tuple.hpp`) in `src/txrequest.cpp` and updates the `lint-includes.py` `EXPECTED_BOOST_INCLUDES` allowlist accordingly. Dash has no `src/txrequest.cpp` (confirmed absent by `ls`), so the cherry-pick omits both the source hunk and the matching lint-allowlist entry. The missing module was introduced by bitcoin#19988 (Overhaul transaction request logic). For a full advertised backport of bitcoin#27783 this is a missing prerequisite: either backport bitcoin#19988's txrequest overhaul, or explicitly note in the commit/PR body that the txrequest.cpp hunk is intentionally excluded because the module does not exist in Dash.
In `src/wallet/test/walletdb_tests.cpp`:
- [BLOCKING] src/wallet/test/walletdb_tests.cpp:30: Missing prerequisite: bitcoin#27556/#26715 wallet test hunks for bitcoin#27783
Upstream bitcoin#27783 changes `BOOST_ASSERT` uses in `walletdb_read_write_deadlock` and `wallet_load_ckey`. In this Dash tree, `walletdb_tests.cpp` lacks `walletdb_read_write_deadlock` entirely, and `src/wallet/test/walletload_tests.cpp` is absent (confirmed by `ls`), so both upstream hunks were omitted. Bitcoin history shows `walletdb_read_write_deadlock` was introduced by bitcoin#27556, and the `wallet_load_ckey` shape touched by #27783 depends on the MockableDatabase wallet-test work from bitcoin#26715. Under the full-backport completeness rule these are missing prerequisites: either backport the wallet-test prerequisites, or document that these `BOOST_ASSERT` hunks are intentionally excluded because the receiving tests do not yet exist in Dash.
In `<commit:ddf030f>`:
- [SUGGESTION] <commit:ddf030f>:1: Drop empty CI-retrigger commit before merge
Commit `ddf030f4c5e` ("ci: retrigger linux64_ubsan-test after feature_governance flake") introduces zero tree changes — `git show --stat` confirms no files touched. Because Dash Core merges without squashing, this empty commit will land in permanent `develop` history where it becomes noise for `git log`, `git bisect`, and `git blame`. CI reruns should be triggered from the CI UI or a comment trigger rather than by pushing an empty commit. Please drop this commit before merge so the PR tip is the actual backport `5f660ae6`. If a marker is needed for reviewer visibility, use a PR comment instead.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking findings.
ddf030f to
a1f31c5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1f31c56cc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.5(failed), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.5(failed), claude/backport-reviewer=opus(completed), codex/backport-reviewer=gpt-5.5(failed); verifier=opus.
Cumulative review at head a1f31c56cc8ea56651b20353e71ea5eba6ea6221. Carried-forward prior findings: none — all 7 prior findings from ddf030f4 are reconciled (2 FIXED for the -printcrashinfo relocation and commit-hygiene cleanup, 4 INTENTIONALLY DEFERRED for missing prerequisites now explicitly documented in both PR description and per-commit bodies, 1 FIXED for the dropped empty CI-retrigger commit). New findings in latest delta: none — the ddf030f4..a1f31c56 delta only relocates the -printcrashinfo handler from ParseArgs() into ProcessInitCommands() so main() returns EXIT_SUCCESS before AppInit(), and wires InitShutdownState() into the standalone dash-chainstate binary so AbortNode()'s Assert(g_exit_status) has a valid target. Both changes were verified against the checked-out worktree (src/bitcoind.cpp:150-153,288 and src/bitcoin-chainstate.cpp:62-69,308). CodeRabbit has no actionable inline findings.
Prior finding reconciliation
- FIXED — -printcrashinfo no longer exits after printing: src/bitcoind.cpp:150-153 relocates the -printcrashinfo handler into ProcessInitCommands(); src/bitcoind.cpp:288 short-circuits main() with
return EXIT_SUCCESSwhen ProcessInitCommands() returns true. - INTENTIONALLY DEFERRED — Missing prerequisite: bitcoin#18044 for bitcoin#27625 wtxid coverage: Both the PR description under
Intentional exclusions / Dash adaptationsand commit 00d6eee0101's body explicitly document that upstreamtest_notfound_on_replaced_tx()and the MSG_WTX/P2PTxInvStore imports are omitted because Dash disables RBF and has not backported wtxid relay (bitcoin#18044). - INTENTIONALLY DEFERRED — Missing prerequisite: bitcoin#27150 common::InitConfig for bitcoin#27708: PR description and commit 8d9ba7d1f85 body both explicitly state that
common::InitConfig()comes from bitcoin#27150, which is not backported, so ParseArgs() (src/bitcoind.cpp:112-146) keeps Dash's existing CheckDataDirOption()/ReadConfigFiles()/SelectParams()/InitSettings() sequence with unchanged semantics. - INTENTIONALLY DEFERRED — Missing prerequisite: bitcoin#19988 txrequest.cpp hunk for bitcoin#27783: Commit a1f31c56cc8 body explicitly documents omission of the src/txrequest.cpp hunk and the boost/tuple/tuple.hpp lint-includes.py allowlist entry because Dash has no txrequest module (bitcoin#19988 not backported) and adding the allowlist entry with no user would fail lint-includes.py's unused-dependency check.
- INTENTIONALLY DEFERRED — Missing prerequisite: bitcoin#27556/bitcoin#26715 wallet test hunks for bitcoin#27783: Commit a1f31c56cc8 body explicitly excludes the walletdb_read_write_deadlock and wallet_load_ckey hunks because the receiving tests do not exist in Dash (bitcoin#27556 and the bitcoin#26715 wallet-test rework not backported); the coinselector_tests.cpp BOOST_ASSERT line touched upstream also does not exist in Dash.
- FIXED — Drop empty CI-retrigger commit before merge:
git log develop..a1f31c56shows the branch was rebuilt with new SHAs (e9f53a6df08, 7d29963f285, 00d6eee0101, 8d9ba7d1f85, 58cf2cdf8a9, 7370b87a287, a1f31c56cc8). - FIXED — Remove transient recovery note from commit message: The prior 6be29805584 commit was replaced during rebase by 8d9ba7d1f85.
Posting note: review_poster could not post inline/top-level review cleanly; posting exact-SHA body-only review from the same verifier output.
Verified Findings
No active findings.
|
This pull request has conflicts, please rebase. |
a1f31c5 to
f8000e2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8000e2d7f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
f8000e2 to
3dc9d29
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The current macOS failure is CI infrastructure, not a #7433 source failure. The
|
✅ Action performedReviews resumed. |
|
@knst #7433 is mergeable at 3dc9d29 and every check except mac-build is green. The macOS job failed before compilation because the exact SDK cache key was unavailable; depends/hosts/darwin.mk is unchanged from develop. Could you please rerun the failed jobs on Actions run 29308372256 (or seed the trusted SDK cache if a rerun still cannot write it)? The failure trace is documented in tracker #1918. |
Likely fixed by #7463 (merged) ; try to rebase on the top of develop @thepastaclaw That looks like issue with mac's CI |
|
This The rerun of Actions job https://github.com/dashpay/dash/actions/runs/29308372256/job/88561596764 stopped before checkout or compilation: The PR's actual diff from its merge base does not modify The backport branch is proper as-is and was left completely untouched: no commit, push, rebase, force-push, or CI retrigger was performed for this unrelated failure. |
3dc9d29 to
12cead7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12cead79f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
12cead7 to
1a23858
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/feature_help.py`:
- Line 58: Update the extra_args list in the nodes[0].start call to remove the
unnecessary f-string prefix from "-printcrashinfo=invalid", while leaving the
datadir interpolation as an f-string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3695b7df-7686-472d-b691-69c4f340a3e2
📒 Files selected for processing (38)
ci/dash/lint-tidy.shci/test/00_setup_env_native_asan.shci/test/00_setup_env_native_fuzz.shsrc/Makefile.amsrc/Makefile.test.includesrc/bitcoin-chainstate.cppsrc/bitcoin-cli.cppsrc/bitcoin-wallet.cppsrc/bitcoind.cppsrc/common/init.cppsrc/common/init.hsrc/index/base.cppsrc/init.cppsrc/init.hsrc/init/common.cppsrc/interfaces/node.hsrc/net_processing.cppsrc/node/blockstorage.cppsrc/node/context.hsrc/node/interface_ui.cppsrc/node/interface_ui.hsrc/node/interfaces.cppsrc/qt/bitcoin.cppsrc/qt/bitcoin.hsrc/qt/guiutil.cppsrc/rpc/request.cppsrc/shutdown.cppsrc/shutdown.hsrc/test/fuzz/utxo_total_supply.cppsrc/test/translation_tests.cppsrc/util/system.cppsrc/util/system.hsrc/util/translation.htest/functional/feature_abortnode.pytest/functional/feature_help.pytest/functional/p2p_leak_tx.pytest/functional/test_framework/test_node.pytest/lint/run-lint-format-strings.py
💤 Files with no reviewable changes (1)
- src/qt/bitcoin.h
🚧 Files skipped from review as they are similar to previous changes (32)
- ci/dash/lint-tidy.sh
- test/lint/run-lint-format-strings.py
- src/init.h
- src/shutdown.h
- ci/test/00_setup_env_native_fuzz.sh
- test/functional/p2p_leak_tx.py
- src/Makefile.test.include
- src/bitcoin-wallet.cpp
- src/Makefile.am
- ci/test/00_setup_env_native_asan.sh
- src/node/blockstorage.cpp
- test/functional/feature_abortnode.py
- src/interfaces/node.h
- src/index/base.cpp
- src/node/interface_ui.cpp
- src/qt/guiutil.cpp
- src/node/interface_ui.h
- src/shutdown.cpp
- src/test/fuzz/utxo_total_supply.cpp
- src/util/translation.h
- src/node/context.h
- src/rpc/request.cpp
- test/functional/test_framework/test_node.py
- src/node/interfaces.cpp
- src/bitcoin-chainstate.cpp
- src/common/init.h
- src/init.cpp
- src/util/system.h
- src/bitcoind.cpp
- src/qt/bitcoin.cpp
- src/util/system.cpp
- src/net_processing.cpp
1a23858 to
1a2cb76
Compare
✅ Action performedReviews resumed. |
1a2cb76 to
0fe81d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/p2p_leak_tx.py`:
- Around line 55-64: Update test_notfound_on_unannounced_tx so the test
explicitly fails when all MAX_REPEATS iterations complete without observing a
notfound response. Add a loop else clause or an equivalent flag/assertion after
the loop, while preserving the existing success path when an unannounced
transaction is detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de150378-dc1e-4966-82fc-9bfbf36834e7
📒 Files selected for processing (39)
ci/dash/lint-tidy.shci/test/00_setup_env_native_asan.shci/test/00_setup_env_native_fuzz.shsrc/Makefile.amsrc/Makefile.test.includesrc/bitcoin-chainstate.cppsrc/bitcoin-cli.cppsrc/bitcoin-wallet.cppsrc/bitcoind.cppsrc/common/init.cppsrc/common/init.hsrc/index/base.cppsrc/init.cppsrc/init.hsrc/init/common.cppsrc/interfaces/node.hsrc/net_processing.cppsrc/node/blockstorage.cppsrc/node/context.hsrc/node/interface_ui.cppsrc/node/interface_ui.hsrc/node/interfaces.cppsrc/qt/bitcoin.cppsrc/qt/bitcoin.hsrc/qt/guiutil.cppsrc/rpc/request.cppsrc/shutdown.cppsrc/shutdown.hsrc/test/fuzz/utxo_total_supply.cppsrc/test/translation_tests.cppsrc/util/system.cppsrc/util/system.hsrc/util/translation.htest/functional/feature_abortnode.pytest/functional/feature_filelock.pytest/functional/feature_help.pytest/functional/p2p_leak_tx.pytest/functional/test_framework/test_node.pytest/lint/run-lint-format-strings.py
💤 Files with no reviewable changes (1)
- src/qt/bitcoin.h
🚧 Files skipped from review as they are similar to previous changes (29)
- src/Makefile.test.include
- src/shutdown.h
- src/bitcoin-wallet.cpp
- src/interfaces/node.h
- src/index/base.cpp
- ci/dash/lint-tidy.sh
- src/common/init.h
- src/util/translation.h
- test/lint/run-lint-format-strings.py
- src/init.h
- ci/test/00_setup_env_native_asan.sh
- src/Makefile.am
- ci/test/00_setup_env_native_fuzz.sh
- test/functional/feature_abortnode.py
- src/rpc/request.cpp
- src/node/interface_ui.cpp
- src/shutdown.cpp
- src/node/interface_ui.h
- src/node/blockstorage.cpp
- src/bitcoin-cli.cpp
- src/qt/guiutil.cpp
- test/functional/feature_help.py
- src/util/system.h
- test/functional/test_framework/test_node.py
- src/node/interfaces.cpp
- src/util/system.cpp
- src/bitcoin-chainstate.cpp
- src/net_processing.cpp
- src/qt/bitcoin.cpp
Pass chainman directly instead of wrapping it in std::ref when calling Dash’s VerifyLoadedChainstate(ChainstateManager&, ...) function.
Backport of bitcoin#27778 Automated backport - cherry-pick of f08bde7.
…ystem.cpp Backport of bitcoin#27170 (merge `8303f11e10`). Squashes the two upstream topic commits in order: - `b20b34f5b3` refactor: use ArgsManager::GetConfigFilePath() - `9a9d5da11f` refactor: Stop using gArgs global in system.cpp Dash adaptation: - Pass the current ArgsManager to Dash’s existing empty dash.conf creation path after changing GetConfigFile() to require an explicit ArgsManager. No upstream hunks are excluded; upstream bitcoin#27170 added no tests.
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. No source or test hunks are excluded. The AbortError-to-InitError shutdown change is retained here with its upstream owner, d172b5c.
… errors Backport of bitcoin#27708 Automated backport — cherry-pick of `c92fd63886`. Prerequisite: - bitcoin#27150 is backported in the immediately preceding commit, so ParseArgs() now uses common::InitConfig() exactly as upstream intends. The AbortError-to-InitError conversion remains in that prerequisite with its upstream owner, d172b5c. Dash adaptations: - Preserve Dash-only -printcrashinfo as an early successful-return command: skip config/datadir/loose-token handling after command-line syntax parses, then print it from ProcessInitCommands(). Focused regression coverage is retained in feature_help.py. - Retain Dash CoreContext context{node}; in AppInit() for Dash subsystem lifetime management after upstream splits argument handling from startup. - Adapt the fatal-index change to Dash current BaseIndex::FatalErrorImpl(). - Keep Dash interfaces::Node and AppInitParameterInteraction API shapes while threading the new exit status through them. Excluded / deferred (intentional): - No invented src/bitcoin-chainstate.cpp InitShutdownState/exit_status wiring; that has no upstream counterpart and is not part of this backport. - No bitcoin#28946 PID-file ownership fix (g_generated_pid / feature_filelock coverage); that already belongs to separate dashpay#7354. - src/qt/bitcoin.cpp does not add node/context.h because Dash reads the status through interfaces::Node::getExitStatus(), so the header would be unused.
Backport of bitcoin#27780 Automated backport — cherry-pick of `2a786ea349`.
Backport of bitcoin#27810 Automated backport — cherry-pick of `a1db4476bd`.
0fe81d5 to
208f73b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 208f73b608
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| node.exit_status = EXIT_FAILURE; | ||
| } | ||
| Interrupt(node); | ||
| Shutdown(node); |
There was a problem hiding this comment.
When AppInit(node) returns false before AppInitMain() calls CreatePidFile()—for example, a second dashd fails AppInitLockDataDirectory() because another process holds the datadir—this unconditional Shutdown(node) still runs. Shutdown() unconditionally removes GetPidFile(*node.args), so the failed second start can delete the running daemon's dashd.pid; fresh evidence in this revision is that this call remains unconditional and the tree has no pid-ownership guard.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
|
The two red source-build checks at head
The PR branch is clean and was left completely untouched. The last fully green head ( |
2 participants
Batch 3 of Bitcoin Core v0.26 backports.
Dash support commit:
This keeps the experimental
dash-chainstateverification call aligned with Dash's currentVerifyLoadedChainstate(ChainstateManager&, ...)signature.6 Bitcoin backports:
Removed / deferred from this batch:
Intentional exclusions / Dash adaptations:
dash.confcreation path now passes the currentArgsManagertoGetConfigFile()after the upstream API is made explicit. No upstream hunks are excluded.-printcrashinfo, and Dash-specific binary/config names. Its sharedcommon::InitConfig()usesCheckDataDirOption(args)from the immediately preceding refactor: Stop using gArgs global in system.cpp bitcoin/bitcoin#27170 prerequisite. The fatal-index change is adapted to Dash's currentBaseIndex::FatalErrorImpl(). Nestedtfm::format()calls are qualified for Dash's C++20 build, matching the later upstream ADL fix.src/common/init.cppis registered in Dash's IWYU script because upstream's CI script no longer exists here.ParseArgs()usescommon::InitConfig()as upstream intends. Dash-only-printcrashinforemains an early successful-return command (skip config/datadir/loose-token handling after command-line syntax parses, then print fromProcessInitCommands()) with focused regression coverage infeature_help.py. Dash's CoreContext lifetime handling is retained. Interfaces/Node and AppInitParameterInteraction shapes keep the new exit-status threading. No inventedsrc/bitcoin-chainstate.cppInitShutdownState/exit_status wiring (no upstream counterpart). No init: don't delete PID file if it was not generated bitcoin/bitcoin#28946 PID-file ownership fix (g_generated_pid/feature_filelockcoverage); that already belongs to separate backport: Merge bitcoin#29144, 28946, 28980, 28554, 28414 #7354.src/qt/bitcoin.cppdoes not addnode/context.hbecause Dash reads status throughinterfaces::Node::getExitStatus().Automated backport via PastaClaw backport pipeline. Each Bitcoin backport was individually cherry-picked and reviewer-verified before batching.