backport: Merge bitcoin#29041#7429
Closed
thepastaclaw wants to merge 1 commit into
Closed
Conversation
…oin#29030) ea00f98 test: fix intermittent error in rpc_net.py (bitcoin#29030) (Sebastian Falbesoner) Pull request description: Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point. Solve this by using the recently introduced `wait_for_new_peer` helper (see bitcoin#29006, commit 00e0658), which is more robust. Fixes bitcoin#29030. ACKs for top commit: maflcko: lgtm ACK ea00f98 Tree-SHA512: dda307949a466fb3b24408a8c213d307e0af2155f2e8b4e52c836a22397f9d218bf9d8c54ca55bae62a96d7566f27167db9311dd8801785c327234783af5ed00
PastaPastaPasta
approved these changes
Jul 8, 2026
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
Member
There was a problem hiding this comment.
utACK 5722f88; trivial
PastaPastaPasta
marked this pull request as ready for review
July 8, 2026 18:54
Author
|
✅ Review complete (commit 5722f88) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis change modifies a single line in the functional test file test/functional/rpc_net.py. In test_getpeerinfo, the mechanism for waiting on a new peer connection is switched from an assert_debug_log context manager checking for a specific log entry to a wait_for_new_peer() context manager, applied before adding a p2p connection without sending a version. Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 24, 2026
5bbf2bb (partial) Merge bitcoin#29034: test: detect OS in functional tests consistently using `platform.system()` (Ava Chow) 70867eb Merge bitcoin#28956: Nuke adjusted time from validation (attempt 2) (Ava Chow) Pull request description: ## 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? - Backported bitcoin#28956: `Nuke adjusted time from validation (attempt 2)`. - Removed the adjusted-time callback from `ChainstateManager::Options`. - Removed the callback from `SnapshotTestSetup::SimulateNodeRestart()` now that Dash's current `develop` includes the bitcoin#25667 test setup. - Updated validation, mining, block-template, and test callsites to use local time where upstream does. - Kept Dash-specific `GetAdjustedTime()` users outside validation, such as governance, sporks, LLMQ debug, and CoinJoin. - Backported bitcoin#29034: `test: detect OS in functional tests consistently using platform.system()`. - Updated the matching Dash functional-test and framework callsites, including `feature_remove_pruned_files_on_startup.py` now that bitcoin#26533 is in Dash's current `develop`. - Kept `os.name` only for the existing POSIX helper exception, matching the upstream guideline. Notes on intentionally omitted upstream hunks: - bitcoin#28956 updates `src/headerssync.cpp` from `GetAdjustedTime()` to `NodeClock::now()`, but that file and subsystem were introduced by bitcoin#25717 and are not present in Dash. That prerequisite-dependent hunk is intentionally excluded here and should be carried with bitcoin#25717 if/when it is backported. - bitcoin#29034 also updates `feature_config_args.py` and `test_framework/util.py` around `get_temp_default_datadir()`, but those hunks depend on bitcoin#27302 test/helper context that Dash does not currently have. - Those prerequisite-dependent hunks are not imported here; they should come with their prerequisite backports if/when those are taken. ## How Has This Been Tested? Local environment: macOS arm64, configured from a fresh worktree with: ```bash ./autogen.sh ./configure --without-gui --disable-bench --disable-fuzz-binary --without-miniupnpc --without-natpmp ``` Validation: ```bash git diff --check upstream/develop...HEAD COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py PYTHONPYCACHEPREFIX=/tmp/tracker-1912-pycache python3 -m py_compile test/functional/feature_bind_extra.py test/functional/feature_init.py test/functional/feature_notifications.py test/functional/feature_remove_pruned_files_on_startup.py test/functional/rpc_bind.py test/functional/test_framework/p2p.py test/functional/test_framework/test_node.py test/functional/test_runner.py test/functional/wallet_multiwallet.py test/lint/lint-python.py test/functional/feature_bind_extra.py test/functional/feature_init.py test/functional/feature_notifications.py test/functional/feature_remove_pruned_files_on_startup.py test/functional/rpc_bind.py test/functional/test_framework/p2p.py test/functional/test_framework/test_node.py test/functional/test_runner.py test/functional/wallet_multiwallet.py make -C src -j8 test/test_dash make -C src -j8 dashd dash-cli ./src/test/test_dash --run_test=validation_tests/ --run_test=miner_tests/ --run_test=bls_tests/ --run_test=validation_chainstatemanager_tests/ test/functional/test_runner.py feature_init.py feature_bind_extra.py rpc_bind.py wallet_multiwallet.py feature_notifications.py feature_remove_pruned_files_on_startup.py ``` 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: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 5bbf2bb Tree-SHA512: 976a2c89a5a437a18a564105bfb19ce2c6c5cbf32a67757c84c0748dae5f618e47c2cdcdc72022af60fc8e4284a9dec7b31702a8a9f7a0f5b4e30518ecca6c6e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
backport: Merge bitcoin#29041
Issue being fixed or feature implemented
rpc_net.pyrace where waiting for the"Added connection peer="debug log does not guarantee the new peer isalready visible through
getpeerinfo().platform.system()bitcoin/bitcoin#29034 from the original PR title.Review on backport: Merge bitcoin#29041 #7190 noted test: detect OS in functional tests consistently using
platform.system()bitcoin/bitcoin#29034 depends on prerequisite context,including Nuke adjusted time from validation (attempt 2) bitcoin/bitcoin#28956, so this replacement keeps that separate instead of
backporting an incomplete fragment.
What was done?
test: fix intermittent error in rpc_net.py (#29030)wait_for_new_peer()helperfrom the already-merged test: fix v2 transport intermittent test failure (#29002) bitcoin/bitcoin#29006 backport.
How Has This Been Tested?
git diff --check upstream/develop...HEADCOMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.pypython3 -m py_compile test/functional/rpc_net.py./autogen.sh./configure --without-gui --disable-bench --disable-fuzz-binarymake -C src/dashbls -j4make -C src/secp256k1 -j4make -C src dashd -j4test/functional/rpc_net.pycode-review dashpay/dash upstream/develop takeover-7190-backport-29041Breaking Changes
Checklist