backport: Merge bitcoin#28956, (partial) 29034#7430
Conversation
|
Prerequisite branch opened as #7431 for bitcoin#26533 + bitcoin#27302. I’m leaving this PR draft until that lands, then I’ll rebuild the bitcoin#28956/bitcoin#29034 branch on top so bitcoin#29034 has proper diff-on-diff hunks instead of carrying prereq-adapted context here. |
|
Correction on the prerequisite chain: #7431 is now scoped to bitcoin#26533 only. The next prerequisite before bitcoin#27302 / bitcoin#29034 is bitcoin#27419 (the common/args split), so I pulled bitcoin#27302 out of #7431 to avoid carrying cross-layout hunks. |
|
🕓 Ready for review — 10 ahead in queue (commit 5bbf2bb) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At the exact head, three blocking issues remain: the test runner restores Windows behavior removed by an ancestor backport, and the advertised full bitcoin#28956 backport omits two upstream hunks without documenting those omissions as intentional exclusions. The two omitted bitcoin#29034 areas are explicitly excluded by the PR body, and the author comments correctly refine their prerequisite chains, so those claims are dropped.
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)
🔴 3 blocking
3 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 `test/functional/test_runner.py`:
- [BLOCKING] test/functional/test_runner.py:46-68: Preserve the newer Windows terminal logic while changing OS detection
Commit e189624b5e58, which is an ancestor of this PR's base, deliberately removed the `sys.getwindowsversion()` guard and moved the ANSI formatting assignments into the non-Windows branch when Dash backported bitcoin#31172. This hunk reapplies the older pre-#31172 structure from bitcoin#29034 instead of changing only the OS detector. As a result, the obsolete version gate returns and Windows now receives the ANSI formatting assignments that the current base deliberately leaves disabled. Preserve the base structure and replace only `os.name == 'nt'` with `platform.system() == 'Windows'`.
In `src/headerssync.cpp`:
- [BLOCKING] src/headerssync.cpp:44: Missing prerequisite: bitcoin#25717
Upstream bitcoin#28956 changes the `HeadersSyncState` commitment bound from `GetAdjustedTime()` to `NodeClock::now()` at `src/headerssync.cpp:44`. The block was introduced by bitcoin#25717, and neither `src/headerssync.cpp` nor its corresponding subsystem exists in this PR's Dash base or head. The PR advertises a full bitcoin#28956 backport, while its documented intentional exclusions cover only bitcoin#29034, so this omitted hunk is an undeclared prerequisite gap. Backport bitcoin#25717 and carry the bitcoin#28956 transformation, or explicitly re-scope and document this area as intentionally excluded.
In `src/test/validation_chainstatemanager_tests.cpp`:
- [BLOCKING] src/test/validation_chainstatemanager_tests.cpp:383-386: Missing prerequisite: bitcoin#25667
Upstream bitcoin#28956 removes `.adjusted_time_callback = GetAdjustedTime` from the snapshot restart setup at `src/test/validation_chainstatemanager_tests.cpp:383`. A first-parent diff of the bitcoin#25667 merge shows that PR introduced `SnapshotTestSetup::SimulateNodeRestart()` and this callback assignment. Dash's exact base and head lack that test section, and the PR body does not declare the bitcoin#25667-dependent bitcoin#28956 hunk as intentionally excluded. Because the PR advertises the full upstream backport, the missing test prerequisite must be restored or explicitly excluded and tracked.
Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
c32a42a to
df310c5
Compare
|
Addressed the preliminary backport-review findings in the rewritten two-commit stack:
Fresh local validation passed: whitespace/diff checks, Python compile + lint/mypy, signed-commit verification, |
|
This pull request has conflicts, please rebase. |
ff9039f Remove GetAdjustedTime (dergoegge) Pull request description: This picks up parts of bitcoin#25908. The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains. ACKs for top commit: naumenkogs: ACK ff9039f achow101: ACK ff9039f maflcko: lgtm ACK ff9039f 🤽 stickies-v: ACK ff9039f Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc (cherry picked from commit 3c13f5d)
df310c5 to
0f0b5c1
Compare
|
Rebased the rewritten two-commit stack onto current |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR backports two upstream Bitcoin Core PRs (bitcoin#28956 removing adjusted time from validation, and bitcoin#29034 consolidating OS detection via platform.system()) as two clean, provenance-preserving commits. All three carried-forward prior findings from the c32a42a review are now reconciled against exact head 0f0b5c1: PF-1 is FIXED (test_runner.py now changes only the os.name -> platform.system() detector while preserving the base's existing Windows ctypes terminal-control block byte-for-byte), PF-3 is FIXED (the SnapshotTestSetup::SimulateNodeRestart() ChainstateManager::Options initializer no longer sets .adjusted_time_callback, matching upstream's net diff exactly), and PF-2 is INTENTIONALLY_DEFERRED (src/headerssync.cpp and the HeadersSync subsystem from bitcoin#25717 do not exist anywhere in this repository's actual ancestry for base 646cc8d or head 0f0b5c1 -- a commit with that title found via git log --all belongs to an unrelated sibling worktree and is confirmed NOT an ancestor of either base or head via git merge-base -- and this exclusion is explicitly documented rather than silently dropped). No new blocking or suggestion-level findings were identified at the current head by either agent line; a repo-wide grep confirms zero dangling adjusted_time_callback/GetAdjustedTime references remain in validation/mining code, while Dash's own timedata.cpp GetAdjustedTime() is correctly retained for spork/coinjoin/governance/llmq use (out of scope for upstream's validation-only removal). One Sonnet reviewer lane (general, and its first retry) failed/was unparseable; a successful second retry (sonnet-retry2-general) plus completed specialist lanes provide full coverage alongside Codex's completed three-lane checkpoint, so both Codex and Sonnet coverage are combined in this final verification.
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— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— backport-reviewer (completed),claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed),claude-sonnet-5— general (completed)
Note: GitHub does not allow PastaClaw to submit an approval or request changes on their own PR, so the canonical verifier result is transported as a COMMENT review.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
LGTM overall; add "partial...." to 29034 title
…nsistently using `platform.system()` 878d914 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner) 4c65ac9 test: detect OS consistently using `platform.system()` (Sebastian Falbesoner) 37324ae test: use `skip_if_platform_not_linux` helper where possible (Sebastian Falbesoner) Pull request description: There are at least three ways to detect the operating system in Python3: - `os.name` (https://docs.python.org/3.9/library/os.html#os.name) - `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform) - `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system) We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system()`, as it appears to be one most consistent and easy to read (see also [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-12-08#989301;) and table below). `sys.platform` is inconsistent as it has the major version number encoded for BSD systems, which doesn't make much sense for e.g. OpenBSD, where there is no concept of major versions, but instead the version is simply increased by 0.1 on each release. Note that `os.name` is still useful to detect whether we are running a POSIX system (see `BitcoinTestFramework.skip_if_platform_not_posix`), so for this use-case it is kept as only exception. The following table shows values for common operating systems, found via ``` $ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())" ``` | OS | os.name | sys.platform | platform.system() | |--------------|---------|--------------|--------------------| | Linux 6.2.0 | posix | linux | Linux | | MacOS* | posix | darwin | Darwin | | OpenBSD 7.4 | posix | openbsd7 | OpenBSD | | Windows* | nt | win32 | Windows | \* = I neither have a MacOS nor a Windows machine available, so I extracted the values from documentation and our current code. Also I'm relying on CI for testing the relevant code-paths. Having reviewers to this this locally would be very appreciated, if this gets Concept ACKed. ACKs for top commit: kevkevinpal: ACK [878d914](bitcoin@878d914) achow101: ACK 878d914 hebasto: ACK 878d914, I have reviewed the code and it looks OK. pablomartin4btc: tACK 878d914 Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc (cherry picked from commit bb6de1b)
0f0b5c1 to
5bbf2bb
Compare
thepastaclaw
changed the title
|
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 (10)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughThe change removes Estimated code review effort: 3 (Moderate) | ~20 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 |
4 participants
Issue being fixed or feature implemented
Follow-up replacement work for #7190 after splitting bitcoin#29041 into #7429.
This PR backports bitcoin#28956, which removes adjusted network time from validation/mining paths, and bitcoin#29034, which makes functional-test OS detection consistently use
platform.system().What was done?
Nuke adjusted time from validation (attempt 2).ChainstateManager::Options.SnapshotTestSetup::SimulateNodeRestart()now that Dash's currentdevelopincludes the assumeutxo: snapshot initialization bitcoin/bitcoin#25667 test setup.GetAdjustedTime()users outside validation, such as governance, sporks, LLMQ debug, and CoinJoin.platform.system()bitcoin/bitcoin#29034:test: detect OS in functional tests consistently using platform.system().feature_remove_pruned_files_on_startup.pynow that prune: scan and unlink already pruned block files on startup bitcoin/bitcoin#26533 is in Dash's currentdevelop.os.nameonly for the existing POSIX helper exception, matching the upstream guideline.Notes on intentionally omitted upstream hunks:
src/headerssync.cppfromGetAdjustedTime()toNodeClock::now(), but that file and subsystem were introduced by p2p: Implement anti-DoS headers sync bitcoin/bitcoin#25717 and are not present in Dash. That prerequisite-dependent hunk is intentionally excluded here and should be carried with p2p: Implement anti-DoS headers sync bitcoin/bitcoin#25717 if/when it is backported.platform.system()bitcoin/bitcoin#29034 also updatesfeature_config_args.pyandtest_framework/util.pyaroundget_temp_default_datadir(), but those hunks depend on init: Error if ignored bitcoin.conf file is found bitcoin/bitcoin#27302 test/helper context that Dash does not currently have.How Has This Been Tested?
Local environment: macOS arm64, configured from a fresh worktree with:
Validation:
The two-commit stack was rebased onto current
develop, both rewritten commits are GPG-signed, and upstream-vs-Dash file coverage was rechecked after the rebase. The bitcoin#25667 snapshot restart hunk and bitcoin#26533 prune-test hunk are now included because those prerequisites have since landed in Dash. The only remaining exclusions are the explicitly documented bitcoin#25717 and bitcoin#27302 prerequisite-dependent hunks above.Breaking Changes
None expected.
Checklist: