Skip to content

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Nov 4, 2025

Issue being fixed or feature implemented

Alternative solution to the issue described here #6901 (comment)

The fix was included in this PR to deal with sporadic errors that affect CI results for this PR (the builds cited above are from this PR). The failure seems to be because of HTTP server port conflict and because the test is short, all 3 attempts can be exhausted before the test that is already using that HTTP port exits.

What was done?

How Has This Been Tested?

Breaking Changes

n/a

Checklist:

  • 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
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@UdjinM6 UdjinM6 added this to the 23.1 milestone Nov 4, 2025
@github-actions
Copy link

github-actions bot commented Nov 4, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

The public constant PORT_RANGE in test/functional/test_framework/util.py is increased from 5000 to 10000, effectively doubling the available port range used for distributing test ports across parallel test instances. This change directly impacts the port calculation logic for p2p and RPC ports without modifying control flow or error handling mechanisms.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Verify that the new port range (10000) is sufficient and appropriate for the number of parallel test instances that can run concurrently
  • Confirm no hardcoded port assumptions elsewhere depend on the previous 5000 range
  • Check if any documentation, comments, or configuration files reference the old PORT_RANGE value and require updates

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: bumping PORT_RANGE from 5000 to 10000, which is the sole observable modification in the changeset.
Description check ✅ Passed The description is related to the changeset, explaining the rationale for the PORT_RANGE increase to address HTTP server port conflicts in CI, even though implementation details are sparse.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b1b2c6 and 15445d3.

📒 Files selected for processing (1)
  • test/functional/test_framework/util.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Functional tests should be written in Python and placed in test/functional/

Files:

  • test/functional/test_framework/util.py
🧠 Learnings (1)
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Applied to files:

  • test/functional/test_framework/util.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build container / Build container
🔇 Additional comments (1)
test/functional/test_framework/util.py (1)

314-314: LGTM! Doubling PORT_RANGE should reduce port conflicts.

This change doubles the available port distribution space from ~4,979 to ~9,979 slots, which should significantly reduce the probability of port collisions during parallel test execution. The new port ranges (p2p: ~11000-21000, rpc: ~21000-31000) are well within the valid port range and don't conflict with common reserved ports.

Please verify through CI runs that this change resolves the sporadic port conflict failures mentioned in the PR objectives.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also pick a different seed (the original proposed fix, 0574b44, modified the port seed but I think the test seed itself should also be randomised as that's closer to what a re-run of the test would achieve)

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 4, 2025

@kwvg pls check PORT_RANGE's usage. Shifting or randomizing portseed won't fix conflicts, it might make it even worse imo.

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 15445d3

PastaPastaPasta added a commit that referenced this pull request Nov 4, 2025
, bitcoin#25748, bitcoin#25863, bitcoin#24051, bitcoin#25315, bitcoin#26624, bitcoin#26894, bitcoin#26673, bitcoin#24462, bitcoin#25815, bitcoin#22949, bitcoin#26723, bitcoin#23395, bitcoin#27016, bitcoin#27189, bitcoin#27317 (auxiliary backports: part 28)

2eef89c merge bitcoin#27317: Check that the timestamp string is non-empty to avoid undefined behavior (Kittywhiskers Van Gogh)
de6c0ae merge bitcoin#27189: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk (Kittywhiskers Van Gogh)
5bed6ea merge bitcoin#27016: require miniupnpc API version 17 or later (Kittywhiskers Van Gogh)
51a9f24 merge bitcoin#23395: Add -shutdownnotify option (Kittywhiskers Van Gogh)
af0e14d merge bitcoin#26723: call `keypoolrefill` with priv key disabled should throw an error (Kittywhiskers Van Gogh)
b91c5b9 merge bitcoin#22949: Round up fee calculation to avoid a lower than expected feerate (Kittywhiskers Van Gogh)
05fb900 merge bitcoin#25815: Use existing {Chainstate,Block}Man (Kittywhiskers Van Gogh)
945798c merge bitcoin#24462: For descriptor pubkey parse errors, include context information (Kittywhiskers Van Gogh)
033e060 merge bitcoin#26673: Remove confusing getBool method (Kittywhiskers Van Gogh)
37ca4b4 merge bitcoin#26894: Remove redundant key_to_p2pkh call (Kittywhiskers Van Gogh)
2b3f925 merge bitcoin#26624: Rename local variable to distinguish it from type alias (Kittywhiskers Van Gogh)
3a58533 merge bitcoin#25315: Add warning on first startup if free disk space is less than necessary (Kittywhiskers Van Gogh)
adeebb0 merge bitcoin#24051: bitcoin-{cli,tx,util} don't need UPnP, NAT-PMP, or ZMQ (Kittywhiskers Van Gogh)
55a474f merge bitcoin#25863: remove unused norm_prv parameter in `descriptor_tests.cpp` (Kittywhiskers Van Gogh)
65299a0 merge bitcoin#25748: Avoid copies in FlatSigningProvider Merge (Kittywhiskers Van Gogh)
5d69d18 merge bitcoin#15936: Expose settings.json methods to GUI (Kittywhiskers Van Gogh)
bdf5e92 merge bitcoin#24410: Split hashing/index `GetUTXOStats` codepaths, decouple from `coinstatsindex` (Kittywhiskers Van Gogh)
43e2a19 merge bitcoin#23345: Drop unneeded dependencies for bitcoin-wallet tool (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * The minimum acceptable prune height in [bitcoin#25315](bitcoin#25315) is higher than upstream (`945` vs `550`) because minimum height is calculated using `MIN_DISK_SPACE_FOR_BLOCK_FILES` which was bumped in [dash#1621](#1621), this has been reflected in functional tests.

  * [bitcoin#27016](bitcoin#27016) raises the minimum miniupnpc version to v2.1. This is acceptable as depends already uses version v2.2.2 ([source](https://github.com/dashpay/dash/blob/f7dad69eab573c179060ff9eb1bbaccb317de6d3/depends/packages/miniupnpc.mk#L2)) and our minimum supported target based on glibc version (see description of [dash#6382](#6382)), Ubuntu 20.04 LTS ([source](https://launchpad.net/ubuntu/+source/miniupnpc/2.1.20190824-0ubuntu2)) and RHEL 9 ([source](https://rpmfind.net/linux/RPM/epel/9/x86_64/Packages/m/miniupnpc-2.2.4-2.el9.x86_64.html)) ship with the minimum required miniupnpc version or higher.

  * [bitcoin#24462](bitcoin#24462) includes changes that SegWit and Taproot-oriented that have been omitted from the backport.

  * In [bitcoin#22949](bitcoin#22949), `rpc_fundrawtransaction.py`'s `restart_node()` needs to be passed an empty set of `extra_args` to avoid the following test failure (see below)

    ```
    $ python3 ./test/functional/rpc_fundrawtransaction.py --descriptors
    2025-11-03T11:19:02.448000Z TestFramework (INFO): PRNG seed is: 4253671634984506297
    2025-11-03T11:19:02.448000Z TestFramework (INFO): Initializing test directory /var/folders/gt/rf6wpfx963x__7wg283kwnxc0000gp/T/dash_func_test_gylr91av
    2025-11-03T11:19:04.236000Z TestFramework (INFO): Connect nodes, set fees, generate blocks, and sync
    [...]
    2025-11-03T11:19:24.065000Z TestFramework (INFO): Test issue 22670 ApproximateBestSubset bug
    2025-11-03T11:20:02.261000Z TestFramework (ERROR): Unexpected exception caught during testing
    Traceback (most recent call last):
      File "/src/dash/test/functional/test_framework/test_framework.py", line 163, in main
        self.run_test()
      File "/src/dash/./test/functional/rpc_fundrawtransaction.py", line 134, in run_test
        self.test_22670()
      File "/src/dash/./test/functional/rpc_fundrawtransaction.py", line 1084, in test_22670
        self.restart_node(0)
      File "/src/dash/test/functional/test_framework/test_framework.py", line 697, in restart_node
        self.start_node(i, extra_args)
      File "/src/dash/test/functional/test_framework/test_framework.py", line 655, in start_node
        node.wait_for_rpc_connection()
      File "/src/dash/test/functional/test_framework/test_node.py", line 271, in wait_for_rpc_connection
        raise FailedToStartError(self._node_msg(
    test_framework.test_node.FailedToStartError: [node 0] dashd exited with status 1 during initialization. Error: Error loading default_wallet: You can't disable HD on an already existing HD wallet
    ************************
    ```

  * ~~To deal with frequent functional test failures due to `rpc_mempool_entry_fee_fields_deprecation.py` experiencing port conflicts ([build](https://github.com/dashpay/dash/actions/runs/19031966785/job/54350070090#step:6:1117), [build](https://github.com/dashpay/dash/actions/runs/19031966785/job/54350107927#step:6:1128)) retries will modify the port seed to try again with a different set of likely non-conflicting ports.~~ Superseded by [dash#6937](#6937).

  ## Breaking Changes

  Building with UPnP enabled requires miniupnpc API version 17 (raising the minimum version from v1.9 to v2.1).

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 2eef89c

Tree-SHA512: 8814ff6b01ac096107d0a78c89806ea1902653c0444db84c476698af4ff5cf1c7fc42b09fce4c3c62f16adce26f1e10cec15ca4c96078c58cdcce7b640a5163d
@PastaPastaPasta PastaPastaPasta merged commit 7d301e5 into dashpay:develop Nov 6, 2025
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants