backport: Merge bitcoin/bitcoin#(partial) 30291, 30397, 22729, 29633, 29459#7300
backport: Merge bitcoin/bitcoin#(partial) 30291, 30397, 22729, 29633, 29459#7300vijaydasmp wants to merge 5 commits into
Conversation
|
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
|
✅ Review complete (commit 086d42e) |
![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: 314e0a3b81
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if extra_args is None: | ||
| extra_args = self.extra_args | ||
|
|
There was a problem hiding this comment.
start() appends auto-generated -bind arguments directly into extra_args, and when extra_args is omitted this aliases self.extra_args (line 232). In the framework, add_nodes() commonly seeds node args with shared empty lists ([[]] * num_nodes), so the first node that starts can mutate the list used by other nodes; subsequent nodes then inherit the first node’s bind ports and may fail with port-collision startup errors. Copy extra_args before appending so each node gets an isolated argument list.
Useful? React with 👍 / 👎.
WalkthroughThis pull request refactors network bind configuration across the test framework and core initialization. It introduces automatic bind address setup for test nodes by adding a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
test/functional/mempool_accept.py (1)
80-86: 💤 Low valueClarify or remove the incomplete comment.
Line 86 contains the comment
# ... 0.99 passes, but the subsequent test case (lines 87-90) doesn't actually pass amaxfeerateparameter to demonstrate this. The comment may confuse readers who expect to see a positive feerate test immediately following the negative feerate test.Consider either:
- Removing the comment
- Expanding it to clarify that any positive value would pass (without needing explicit demonstration here)
- Adding an explicit test with a positive maxfeerate value
🤖 Prompt for 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. In `@test/functional/mempool_accept.py` around lines 80 - 86, The inline comment "# ... 0.99 passes" is misleading because no positive maxfeerate test follows; update the test around the assert_raises_rpc_error call that uses check_mempool_result so the comment matches reality: either remove the comment, replace it with a clarifying note that positive maxfeerate values would pass without an explicit test, or add an explicit passing assertion that calls check_mempool_result with maxfeerate=0.99 (using the same raw_tx_in_block and expected successful result_expected) to demonstrate the positive-case; reference check_mempool_result and raw_tx_in_block when making the change.
🤖 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/test_framework/test_node.py`:
- Around line 94-98: The current logic only sets has_explicit_bind by checking
extra_conf for entries starting with "bind=", causing auto-injected binds to
ignore effective settings like listen, port, and whitebind; update the code in
the block that calls append_config(datadir, extra_conf) so it computes the
effective configuration after appending and derives has_explicit_bind from that
effective config: consider any explicit "bind=" or "-bind=" entries, any
whitebind entries, whether listen=0 is set, and whether a custom port is set
(i.e., port != p2p_port(self.index)); if any of those conditions apply, treat
the node as having an explicit bind and do not inject default
p2p_port(self.index). Use the existing symbols append_config, extra_conf,
has_explicit_bind, p2p_port(self.index), listen, port, whitebind to find and
change the logic.
In `@test/functional/test_runner.py`:
- Line 490: Replace the bare assertion on results_filepath.parent.exists() with
explicit error handling: check if not results_filepath.parent.exists() and raise
a clear exception (e.g., ValueError or FileNotFoundError) or call the same
error-reporting helper used elsewhere in this module, using the same message
"Results file parent directory does not exist" so the check on
results_filepath.parent is enforced even when Python is run with -O; update the
code near the results_filepath usage (the results_filepath variable check) to
follow the existing pattern used around the other validations in this file.
---
Nitpick comments:
In `@test/functional/mempool_accept.py`:
- Around line 80-86: The inline comment "# ... 0.99 passes" is misleading
because no positive maxfeerate test follows; update the test around the
assert_raises_rpc_error call that uses check_mempool_result so the comment
matches reality: either remove the comment, replace it with a clarifying note
that positive maxfeerate values would pass without an explicit test, or add an
explicit passing assertion that calls check_mempool_result with maxfeerate=0.99
(using the same raw_tx_in_block and expected successful result_expected) to
demonstrate the positive-case; reference check_mempool_result and
raw_tx_in_block when making the change.
🪄 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: ea4c1c31-e458-42d1-b146-76c59c425be7
📒 Files selected for processing (10)
src/init.cppsrc/logging.hsrc/net.cppsrc/test/util/net.cpptest/functional/feature_bind_extra.pytest/functional/mempool_accept.pytest/functional/test-shell.mdtest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/test_runner.py
| if args.resultsfile: | ||
| results_filepath = pathlib.Path(args.resultsfile) | ||
| # Stop early if the parent directory doesn't exist | ||
| assert results_filepath.parent.exists(), "Results file parent directory does not exist" |
There was a problem hiding this comment.
Replace assertion with explicit error handling for consistency.
Using assert for user input validation is inconsistent with the error handling pattern used elsewhere in this file (e.g., lines 497-498, 546-547), and assertions can be disabled with python -O.
Suggested fix
- assert results_filepath.parent.exists(), "Results file parent directory does not exist"
+ if not results_filepath.parent.exists():
+ print("ERROR: Results file parent directory does not exist")
+ sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert results_filepath.parent.exists(), "Results file parent directory does not exist" | |
| if not results_filepath.parent.exists(): | |
| print("ERROR: Results file parent directory does not exist") | |
| sys.exit(1) |
🤖 Prompt for 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.
In `@test/functional/test_runner.py` at line 490, Replace the bare assertion on
results_filepath.parent.exists() with explicit error handling: check if not
results_filepath.parent.exists() and raise a clear exception (e.g., ValueError
or FileNotFoundError) or call the same error-reporting helper used elsewhere in
this module, using the same message "Results file parent directory does not
exist" so the check on results_filepath.parent is enforced even when Python is
run with -O; update the code near the results_filepath usage (the
results_filepath variable check) to follow the existing pattern used around the
other validations in this file.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Five-PR Bitcoin Core backport (bitcoin#30291, bitcoin#30397, bitcoin#22729, bitcoin#29633, bitcoin#29459) that merges cleanly against Dash's existing extensions in src/net.cpp and src/init.cpp. The behavioral change (bitcoin#22729: fail-fast on bind failures plus per-node bind injection in the test framework) is correctly carried over. One real but latent footgun in the test framework's bind injection (shared list mutation) plus two cosmetic comment issues. No blocking issues identified — Codex's 'blocking' framing of the shared-list bug is overstated because every current call site either short-circuits via has_explicit_bind or passes its own non-shared extra_args.
Reviewed commit: 314e0a3
🟡 1 suggestion(s) | 💬 2 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 `test/functional/test_framework/test_node.py`:
- [SUGGESTION] lines 231-243: start() mutates the caller's extra_args list — latent shared-reference footgun
`start()` calls `extra_args.append(...)` directly on the list reference assigned from `self.extra_args` (or the caller-supplied list). In `BitcoinTestFramework.add_nodes`, the default path is `extra_args = [[]] * num_nodes`, which produces N references to the same empty list, all of which are stored as each node's `self.extra_args`.
In the current tree this does not blow up: with the default `bind_to_localhost_only=True`, `extra_conf` contains `bind=127.0.0.1`, so `self.has_explicit_bind` is True in `__init__` and the new injection block is skipped. The five tests that set `bind_to_localhost_only=False` (feature_discover, feature_bind_extra, feature_bind_port_discover, feature_bind_port_externalip, rpc_bind) either run a single node or supply per-node `extra_args` lists that already contain `-bind=...` (so the `any(e.startswith('-bind=')...)` short-circuit kicks in). So no test currently mutates a shared list.
But any future test that sets `bind_to_localhost_only=False` while relying on the default `extra_args` would silently get every node binding to node 0's `p2p_port`/`tor_port`, producing confusing port-collision failures. Copy the list before mutating to make the new injection block defensive against the shared-default.
| rawtxs=[raw_tx_in_block], | ||
| maxfeerate=-0.01, | ||
| )) | ||
| # ... 0.99 passes |
There was a problem hiding this comment.
💬 Nitpick: Orphaned '# ... 0.99 passes' comment from bitcoin#29459 cherry-pick
Upstream bitcoin#29459 inserts the new negative-feerate assertion immediately before a pre-existing self.check_mempool_result(..., maxfeerate=0.99) call, where the trailing # ... 0.99 passes comment naturally describes the following line. Dash's mempool_accept.py never carried the maxfeerate=1 fails / 0.99 passes pair (pre-existing divergence), so the cherry-pick leaves the comment dangling above a check_mempool_result call that has no maxfeerate=0.99. Drop the comment or replace it with one that matches what the next call actually does. Cosmetic only.
source: ['claude']
There was a problem hiding this comment.
29459: address thepastaclaw's comment comment please
| break | ||
|
|
||
| print_results(test_results, max_len_name, (int(time.time() - start_time))) | ||
| runtime = int(time.time() - start_time) |
There was a problem hiding this comment.
nit: mark 30291 as partial due to missing fix of typo on this line:
sys.exit(f"Early exiting after test failure due to insufficient free space in {tmpdir}\n"
| rawtxs=[raw_tx_in_block], | ||
| maxfeerate=-0.01, | ||
| )) | ||
| # ... 0.99 passes |
There was a problem hiding this comment.
29459: address thepastaclaw's comment comment please
… csv ad06e68 test: write functional test results to csv (tdb3) Pull request description: Adds argument `--resultsfile` to test_runner.py. Enables functional test results to be written to a (csv) file for processing by other applications (or for historical archiving). Test name, status, and duration are written to the file provided with the argument. Since `test_runner.py` is being touched, also fixes a misspelling (linter warning). Can split into its own commit if desired. #### Notes - Total runtime of functional tests has seemed to have increased on my development machines over the past few months (more tests added, individual test runtime increase, etc.). Was interested in recording test runtime data over time to detect trends. Initially searched `doc/benchmarking.md`, existing PRs, and Issues, but didn't immediately see this type of capability or alternate solutions (please chime in if you know of one!). Thought it would be beneficial to add this capability to `test_runner` to facilitate this type of data analysis (and potentially other use cases) - Saw https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#benchmarking-with-perf, and this PR's higher level data seems complimentary. - Was on the fence as to whether to expand `print_results()` (i.e. take advantage of the same loop over `test_results`) or implement in a separate `write_results()` function. Decided on the latter for now, but interested in reviewers' thoughts. #### Example 1: all tests pass ``` $ test/functional/test_runner.py --resultsfile functional_test_results.csv --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp feature_blocksdir wallet_startup feature_config_args mempool_accept Temporary test directory at /mnt/tmp/test_runner_₿_🏃_20240614_201625 Test results will be written to functional_test_results.csv ... $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,29 mempool_accept.py,Passed,9 wallet_startup.py,Passed,2 ALL,Passed,29 ``` #### Example 2: one test failure ``` $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,28 wallet_startup.py,Passed,2 mempool_accept.py,Failed,1 ALL,Failed,28 ``` ACKs for top commit: maflcko: re-ACK ad06e68 kevkevinpal: tACK [ad06e68](bitcoin@ad06e68) achow101: ACK ad06e68 rkrux: tACK [ad06e68](bitcoin@ad06e68) brunoerg: ACK ad06e68 marcofleon: Good idea, tested ACK ad06e68 Tree-SHA512: 561194406cc744905518aa5ac6850c07c4aaecdaf5d4d8b250671b6e90093d4fc458f050e8a85374e66359cc0e0eaceba5eb24092c55f0d8f349d744a32ef76c
…l/net.cpp e233ec0 refactor: Use designated initializer (Hodlinator) Pull request description: Block was recently touched (e2d1f84) and the codebase recently switched to C++20 which allows this to improve robustness. Follow-up suggested in bitcoin#29625 (comment) ACKs for top commit: maflcko: ACK e233ec0 Tree-SHA512: ce3a18f513421e923710a43c8f97db1badb7ff5c6bdbfd62d9543312d2225731db5c14bef16feb47c43b84fad4dc24485086634b680feba422d2b7b363e13fa6
…startup on bind failure bca346a net: require P2P binds to succeed (Vasil Dimov) af55253 net: report an error if unable to bind on the Tor listening addr:port (Vasil Dimov) 9a7e5f4 net: don't extra bind for Tor if binds are restricted (Vasil Dimov) Pull request description: Make it possible to disable the Tor binding on `127.0.0.1:8334` and stop startup if any P2P bind fails instead of "if all P2P binds fail". Fixes bitcoin#22726 Fixes bitcoin#22727 ACKs for top commit: achow101: ACK bca346a cbergqvist: ACK bca346a pinheadmz: ACK bca346a Tree-SHA512: fabef89a957191eea4f3e3b6109d2b8389f27ecc74440a920b0c10f31fff00a85bcfd1eb3c91826c7169c618f4de8a8d0a260e2caf40fd854f07ea9a980d8603
d0e6564 log: Remove error() reference (Fabian Jahr) Pull request description: Mini-followup to bitcoin#29236 that was just merged. Removes a reference to `error()` that was missed in a comment. ACKs for top commit: ryanofsky: Code review ACK d0e6564. Just dropped LogPrintf reference since last review stickies-v: ACK d0e6564 Empact: ACK bitcoin@d0e6564 Tree-SHA512: 8abe4895951013c2ceca9a57743aacabaf8af831d07eee9ae8372c121c16e88b7226f0e537200c3464792e19ac7e03b57ba0be31f43add8802753972b0aefc48
bf264e0 test: check_mempool_result negative feerate (kevkevin) Pull request description: Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result` Asserts "Amount out of range" error message and `-3` error code Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250) ACKs for top commit: maflcko: lgtm ACK bf264e0 brunoerg: nice, utACK bf264e0 davidgumberg: Looks great, ACK bitcoin@bf264e0 Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78
vijaydasmp
changed the title
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 086d42e6cf
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | ||
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | ||
| if will_listen and not has_explicit_bind: | ||
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") |
There was a problem hiding this comment.
When a test opts out with bind_to_localhost_only = False, some scripts are explicitly exercising the daemon path where no -bind is present: feature_bind_port_discover.py node 0 expects bind_on_any to call Discover(), and feature_bind_port_externalip.py cases rely on GetListenPort() honoring -port. Appending this normal -bind=0.0.0.0:<p2p_port> makes args.GetArgs("-bind") nonempty for those starts, so discovery is suppressed and -port is ignored in favor of the injected bind port. This should be opt-in or avoided for tests that intentionally requested the no-bind path.
Useful? React with 👍 / 👎.
| if (!Bind(addr_bind, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None)) { | ||
| return false; |
There was a problem hiding this comment.
AppInitMain() still inserts DefaultOnionServiceTarget() into options.onion_binds when no -bind/-whitebind is supplied, even if the user sets -listenonion=0; making every onion bind failure fatal here means a conflict on the implicit 127.0.0.1 onion target port aborts startup before the later 0.0.0.0 bind can succeed. This regresses running another node with a different -port and onion listening disabled, where the implicit Tor target is occupied but normal P2P listening would otherwise work; only explicit onion binds should be fatal, or the default should be skipped when onion listening is off.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
Correction: the first version of this review over-promoted duplicate carried-forward test-framework findings while applying the backport-prerequisite policy gate. I edited the duplicate inline comments as superseded; please use the corrected finding list below.
Cumulative review at 086d42e confirms the five-PR backport stack (bitcoin#30291 partial, bitcoin#30397, bitcoin#22729, bitcoin#29633, bitcoin#29459) merges cleanly with the expected Dash adaptations in InitBinds and onion-service target selection. The prior mempool_accept comment nitpick is fixed. Two prior test-framework issues still apply, one new test-runner argument edge case was found, and the policy gate keeps one missing-prerequisite finding for bitcoin#29434 as blocking.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 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 `test/functional/test_framework/test_node.py`:
- [SUGGESTION] test/functional/test_framework/test_node.py:231-243: start() mutates caller's extra_args list — shared-reference footgun
Carried forward from the prior review at 314e0a3b and still valid at 086d42e6. When `extra_args` is None, `start()` aliases `self.extra_args` and appends the auto-injected `-bind=...` flags onto that referenced list. `BitcoinTestFramework.add_nodes` can install shared inner lists via `[[]] * num_nodes`, so the first node/restart can leak stale bind flags into later starts. Copy the selected list before appending, e.g. `extra_args = list(self.extra_args) if extra_args is None else list(extra_args)`.
- [NITPICK] test/functional/test_framework/test_node.py:234-238: Carry-over comment references Bitcoin regtest port 18445 and bitcoind
Still valid at 086d42e6. The comment imported with bitcoin#22729 still mentions `bitcoind`, `bitcoin.conf`, and `127.0.0.1:18445`; the port is Bitcoin Core's regtest onion-service target, while Dash's ports differ by network and the code correctly uses `tor_port(self.index)`. Prefer Dash-neutral wording.
- [SUGGESTION] test/functional/test_framework/test_node.py:239-243: Implicit -bind injection ignores --dashd-extra-args (extra_args_from_options)
`will_listen` and `has_explicit_bind` inspect only per-node `extra_args`, but the process is launched with `self.args + self.extra_args_from_options + extra_args`. A global `--dashd-extra-args=-listen=0` or `--dashd-extra-args=-bind=...` therefore does not suppress the auto-injected binds, which can fight user-supplied global args. Base the bind/listen decision on the same combined argument set passed to dashd.
In `test/functional/mempool_accept.py`:
- [BLOCKING] test/functional/mempool_accept.py:80-85: Missing prerequisite: bitcoin#29434
bitcoin#29459 was added upstream immediately after the maxfeerate coverage introduced by bitcoin#29434. Upstream #29434 added the 1 BTC/kvB rejection test, the 0.99 BTC/kvB passing test, and the `src/rpc/mempool.cpp` `ParseFeeRate` path that rejects fee rates >= 1 BTC/kvB. Dash's current backport of bitcoin#29459 only adds the negative maxfeerate assertion and adapts around the missing #29434 context: the following already-known check still has no `maxfeerate=0.99`, and `src/rpc/mempool.cpp` still parses maxfeerate with `CFeeRate(AmountFromValue(...))` instead of `ParseFeeRate`. Per the review policy for full upstream backports, this missing prerequisite needs either backporting or explicit PR-description documentation as an intentional exclusion.
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | ||
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | ||
| # a unique port chosen by the test framework and configured as port=P in | ||
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | ||
| # 127.0.0.1:tor_port(). |
There was a problem hiding this comment.
💬 Nitpick: Carry-over comment references Bitcoin regtest port 18445 and bitcoind
The block comment cherry-picked from bitcoin#22729 still mentions bitcoind, bitcoin.conf, and 127.0.0.1:18445 — the latter is Bitcoin Core's regtest OnionServiceTargetPort. Dash's onion-service target ports differ per network (9996 / 19996 / 19796 / 19896) and the code already uses the dynamic tor_port(self.index) helper, so the literal 18445 is misleading for Dash test authors. Rephrase to reference dashd / dash.conf and a generic onion-service target port instead of the Bitcoin-specific literal.
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | |
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | |
| # a unique port chosen by the test framework and configured as port=P in | |
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | |
| # 127.0.0.1:tor_port(). | |
| # If listening and no -bind is given, then dashd would bind P2P ports on | |
| # 0.0.0.0:P and 127.0.0.1:<default onion-service target port> (for incoming | |
| # Tor connections), where P is a unique port chosen by the test framework | |
| # and configured as port=P in dash.conf. To avoid collisions on the default | |
| # onion target port, change it to 127.0.0.1:tor_port(). |
source: ['claude', 'codex']
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | ||
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | ||
| if will_listen and not has_explicit_bind: | ||
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | ||
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") |
There was a problem hiding this comment.
🟡 Suggestion: Implicit -bind injection ignores --dashd-extra-args (extra_args_from_options)
will_listen and has_explicit_bind inspect only the per-node extra_args, but on line 258 the process is launched with self.args + self.extra_args_from_options + extra_args. That means a global --dashd-extra-args=-listen=0 or --dashd-extra-args=-bind=... passed via the test runner does not influence the implicit-bind decision: the auto-injected -bind= flags get appended anyway, which can fight a user-supplied -listen=0 (dashd will warn/refuse listening with explicit binds) or duplicate a user-supplied bind. Decide based on the same combined argument set that is actually passed to dashd.
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | |
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | |
| if will_listen and not has_explicit_bind: | |
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | |
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") | |
| startup_args = [*self.extra_args_from_options, *extra_args] | |
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in startup_args) | |
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in startup_args) | |
| if will_listen and not has_explicit_bind: | |
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | |
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") |
source: ['codex']
| if extra_args is None: | ||
| extra_args = self.extra_args | ||
|
|
||
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | ||
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | ||
| # a unique port chosen by the test framework and configured as port=P in | ||
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | ||
| # 127.0.0.1:tor_port(). | ||
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | ||
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | ||
| if will_listen and not has_explicit_bind: | ||
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | ||
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") |
There was a problem hiding this comment.
🟡 Suggestion: start() mutates caller's extra_args list — shared-reference footgun
Carried forward from the prior review at 314e0a3 and still valid at 086d42e. When extra_args is None, start() aliases self.extra_args and appends the auto-injected -bind=... flags onto that referenced list. BitcoinTestFramework.add_nodes can install shared inner lists via [[]] * num_nodes, so the first node/restart can leak stale bind flags into later starts. Copy the selected list before appending, e.g. extra_args = list(self.extra_args) if extra_args is None else list(extra_args).
Correction note: this is a suggestion, not a backport-prerequisite blocker; the earlier policy-gate text on this comment was incorrect.
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | ||
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | ||
| # a unique port chosen by the test framework and configured as port=P in | ||
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | ||
| # 127.0.0.1:tor_port(). |
There was a problem hiding this comment.
Superseded duplicate
This inline comment was a duplicate created by an over-broad policy-gate promotion. Please ignore this thread and use the corrected nitpick comment on the same block instead.
| # Check negative feerate | ||
| assert_raises_rpc_error(-3, "Amount out of range", lambda: self.check_mempool_result( | ||
| result_expected=None, | ||
| rawtxs=[raw_tx_in_block], | ||
| maxfeerate=-0.01, | ||
| )) |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#29434
bitcoin#29459 was added upstream immediately after the maxfeerate coverage introduced by bitcoin#29434. In upstream, bitcoin#29434 added the 1 BTC/kvB rejection test, the 0.99 BTC/kvB passing test, and the src/rpc/mempool.cpp ParseFeeRate path that rejects fee rates >= 1 BTC/kvB. Dash's current backport of bitcoin#29459 only adds the negative maxfeerate assertion and adapts around the missing bitcoin#29434 context: the following already-known check still has no maxfeerate=0.99, and src/rpc/mempool.cpp still parses maxfeerate with CFeeRate(AmountFromValue(...)) instead of ParseFeeRate. This is a soft prerequisite gap because the new negative-feerate test is cleanly adapted and should pass, but the upstream dependency chain around this test is incomplete.
Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. intentional_exclusion: true or a matching entry in policy_overrides). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
source: ['codex-backport-reviewer']
| if extra_args is None: | ||
| extra_args = self.extra_args | ||
|
|
||
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | ||
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | ||
| # a unique port chosen by the test framework and configured as port=P in | ||
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | ||
| # 127.0.0.1:tor_port(). | ||
| will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) | ||
| has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) | ||
| if will_listen and not has_explicit_bind: | ||
| extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") | ||
| extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") |
There was a problem hiding this comment.
Superseded duplicate
This inline comment duplicated the carried-forward extra_args mutation finding and incorrectly labeled it as a blocking backport-prerequisite issue. Please ignore this thread and use the corrected suggestion comment instead.
| # If listening and no -bind is given, then bitcoind would bind P2P ports on | ||
| # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is | ||
| # a unique port chosen by the test framework and configured as port=P in | ||
| # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to | ||
| # 127.0.0.1:tor_port(). |
There was a problem hiding this comment.
Superseded duplicate
This inline comment duplicated the stale-comment nitpick and incorrectly labeled it as a blocking backport-prerequisite issue. Please ignore this thread and use the corrected nitpick comment instead.
6 participants
bitcoin backports