backport: Merge bitcoin#31718, 25216, gui#613#7376
Conversation
|
This pull request has conflicts, please rebase. |
vijaydasmp
changed the title
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
![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: dfb30988e8
ℹ️ 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".
| argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); | ||
| argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | ||
| argsman.AddArg("-includeconf=<file>", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | ||
| argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); |
There was a problem hiding this comment.
This registers -allowignoredconf, but ArgsManager::ReadConfigFiles() still has no logic to detect an ignored datadir config file or to downgrade that error when this flag is set. In the scenario added by test_ignored_conf, -conf=<temp> points back to a datadir that already has dash.conf, but startup will succeed and silently ignore that datadir config instead of failing or warning, so the new default functional test fails and users get a no-op safety option.
Useful? React with 👍 / 👎.
| conf_path = default_datadir / "bitcoin.conf" | ||
| conf_path.write_text(f"datadir={node.datadir}\n{conf_text}") |
There was a problem hiding this comment.
Here the test writes bitcoin.conf under the helper's Bitcoin default directory, but Dash reads $HOME/.dashcore/dash.conf on Unix-like systems and ~/Library/Application Support/DashCore/dash.conf on macOS. Once the test removes -datadir, dashd ignores this file and starts from an unconfigured default datadir/RPC port, so test_config_file_log and the same pattern in test_ignored_default_conf time out instead of checking the intended config behavior.
Useful? React with 👍 / 👎.
| star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups() | ||
| if star_pos_num is not None: | ||
| max_pos = max(max_pos, int(star_pos_num)) | ||
| elif star is not None: | ||
| n += 1 |
There was a problem hiding this comment.
For non-positional format strings with both dynamic width and precision, such as %*.*f, there are two * arguments plus the value, but this regex only captures the first * in the specifier. count_format_specifiers('%*.*f') now returns 2 instead of 3, so the lint can miss calls that are missing one argument when dynamic precision is used.
Useful? React with 👍 / 👎.
WalkthroughThis PR fixes several documentation typos and refactors validation cache sizing. It introduces Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Init as init.cpp / setup_common.cpp
participant ArgsMan as ApplyArgsManOptions
participant SigCache as InitSignatureCache
participant ScriptCache as InitScriptExecutionCache
Init->>Init: construct ValidationCacheSizes
Init->>ArgsMan: apply -maxsigcachesize
ArgsMan-->>Init: signature_cache_bytes, script_execution_cache_bytes
Init->>SigCache: InitSignatureCache(signature_cache_bytes)
SigCache-->>Init: bool success
Init->>ScriptCache: InitScriptExecutionCache(script_execution_cache_bytes)
ScriptCache-->>Init: bool success
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: 3
🤖 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/node/miner.cpp`:
- Line 24: The include change removed timedata.h but miner.cpp calls
GetAdjustedTime() on lines 52 and 221, which is defined in timedata.h and not
available in util/time.h. Either restore the `#include` <timedata.h> directive at
the top of miner.cpp, or move the GetAdjustedTime() function definition from
timedata.h to util/time.h and update the corresponding header file to ensure
miner.cpp can resolve the symbol.
In `@test/functional/feature_config_args.py`:
- Around line 116-150: The test_config_file_log function and related tests
contain hardcoded references to "bitcoin.conf" which should be "dash.conf" since
the repository uses "dash.conf" as the configuration filename. Replace all
occurrences of "bitcoin.conf" with "dash.conf" in the conf_path path creation
(where it constructs the configuration file path using pathlib.Path), in the
start_node method call arguments (where -conf parameter is specified), and in
any file write operations that create the configuration file. Additionally,
update any assertion messages and expected_msgs parameters that reference the
configuration filename to use "dash.conf" instead of "bitcoin.conf".
In `@test/lint/run-lint-format-strings.py`:
- Around line 267-271: The regex match on line 267 only captures the first `*`
in the format specifier string, causing format specifiers with multiple `*`
characters (like `%*.*d`) to be undercounted. Replace the single re.match call
with re.finditer or re.findall to find all occurrences of the `*` pattern in
m.group(1), then iterate through each match to properly count all positional
arguments and increment n for each `*` specifier found.
🪄 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: 35f19a2a-66e5-4557-be3e-60a0c05e17ad
📒 Files selected for processing (33)
ci/README.mdcontrib/guix/README.mdcontrib/tracing/README.mdcontrib/tracing/log_raw_p2p_msgs.pydoc/release-notes-27302.mdsrc/chain.hsrc/init.cppsrc/net.cppsrc/net.hsrc/net_processing.cppsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/miner.cppsrc/qt/test/test_main.cppsrc/rpc/mining.cppsrc/test/blockmanager_tests.cppsrc/test/serialize_tests.cppsrc/test/util/script.hsrc/timedata.hsrc/validation.cpptest/functional/README.mdtest/functional/feature_bind_extra.pytest/functional/feature_config_args.pytest/functional/feature_init.pytest/functional/feature_notifications.pytest/functional/feature_remove_pruned_files_on_startup.pytest/functional/rpc_bind.pytest/functional/test_framework/p2p.pytest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/test_runner.pytest/functional/wallet_multiwallet.pytest/lint/run-lint-format-strings.py
💤 Files with no reviewable changes (1)
- src/timedata.h
| #include <timedata.h> | ||
| #include <util/moneystr.h> | ||
| #include <util/system.h> | ||
| #include <util/time.h> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for orphaned timedata symbols in miner.cpp after include swap
# Search for timedata usage in miner.cpp
echo "=== Checking for timedata symbol usage in miner.cpp ==="
rg -n 'timedata|dNtpOffset' src/node/miner.cpp || echo "No timedata symbol references found"
# Verify util/time.h exports needed symbols
echo ""
echo "=== Checking util/time.h for time symbols ==="
grep -E '(GetTime|GetAdjustedTime|dNtpOffset)' src/util/time.h || echo "Time symbols may not be in util/time.h"Repository: dashpay/dash
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Check what time functions miner.cpp actually uses
echo "=== Time-related function calls in miner.cpp ==="
rg -n '(GetTime|GetAdjustedTime|GetSystemTimeInSeconds|dNtpOffset)' src/node/miner.cpp
# Also check for any remaining includes that might provide time functionality
echo ""
echo "=== All includes in miner.cpp ==="
head -30 src/node/miner.cpp | grep '^`#include`'Repository: dashpay/dash
Length of output: 1009
🏁 Script executed:
#!/bin/bash
# Find GetAdjustedTime definition
echo "=== Searching for GetAdjustedTime definition ==="
rg -n 'GetAdjustedTime' --type cpp --type h | head -20
# Check full util/time.h content to see all exports
echo ""
echo "=== Full util/time.h content ==="
cat src/util/time.hRepository: dashpay/dash
Length of output: 7294
🏁 Script executed:
#!/bin/bash
# Check if timedata.h is included transitively
echo "=== Checking node/miner.h for timedata.h include ==="
rg -n 'timedata' src/node/miner.h
echo ""
echo "=== Checking validation.h for timedata.h include ==="
rg -n 'timedata' src/validation.h | head -5
echo ""
echo "=== Checking chain.h for timedata.h include ==="
rg -n 'timedata' src/chain.h
echo ""
echo "=== Full search for timedata.h includes ==="
rg '`#include`.*timedata' --type h --type cpp | grep -E '(chain\.h|validation\.h|node/miner\.h|pow\.h)'Repository: dashpay/dash
Length of output: 257
🏁 Script executed:
#!/bin/bash
# Check what headers transitively include timedata.h
echo "=== Which headers include timedata.h ==="
rg '`#include`.*timedata\.h' --type h --type cpp
echo ""
echo "=== Does pow.h include timedata ==="
cat src/pow.h | grep -E '(timedata|GetAdjustedTime)' || echo "Not found in pow.h"
echo ""
echo "=== Verify miner.cpp would have access to GetAdjustedTime ==="
# Check if any of miner's includes transitively get timedata.h
for header in consensus/consensus.h consensus/validation.h pow.h; do
if rg '`#include`.*timedata' "src/$header" > /dev/null 2>&1; then
echo "$header includes timedata.h"
fi
done
echo "Done checking transitive includes"Repository: dashpay/dash
Length of output: 1053
🏁 Script executed:
#!/bin/bash
# Check if validation.h or pow.h indirectly include timedata.h
echo "=== Check validation.h includes ==="
head -50 src/validation.h | grep '`#include`'
echo ""
echo "=== Check pow.h includes ==="
head -50 src/pow.h | grep '`#include`'
echo ""
echo "=== Comprehensive check: trace includes from miner.cpp ==="
# For key headers, check if they include timedata
for header in src/validation.h src/pow.h src/consensus/consensus.h src/chainparams.h; do
if [ -f "$header" ]; then
result=$(grep '`#include`.*timedata' "$header")
if [ -n "$result" ]; then
echo "$header INCLUDES timedata.h"
fi
fi
done
echo "Check complete"Repository: dashpay/dash
Length of output: 1050
Add timedata.h include or implement GetAdjustedTime() in util/time.h.
The include swap removes timedata.h but miner.cpp calls GetAdjustedTime() on lines 52 and 221. This function is defined in timedata.h, and util/time.h does not provide it. No transitive includes supply this symbol. Either restore #include <timedata.h> or relocate GetAdjustedTime() to util/time.h.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
🤖 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 `@src/node/miner.cpp` at line 24, The include change removed timedata.h but
miner.cpp calls GetAdjustedTime() on lines 52 and 221, which is defined in
timedata.h and not available in util/time.h. Either restore the `#include`
<timedata.h> directive at the top of miner.cpp, or move the GetAdjustedTime()
function definition from timedata.h to util/time.h and update the corresponding
header file to ensure miner.cpp can resolve the symbol.
| def test_config_file_log(self): | ||
| # Disable this test for windows currently because trying to override | ||
| # the default datadir through the environment does not seem to work. | ||
| if platform.system() == "Windows": | ||
| return | ||
|
|
||
| self.log.info('Test that correct configuration path is changed when configuration file changes the datadir') | ||
|
|
||
| # Create a temporary directory that will be treated as the default data | ||
| # directory by bitcoind. | ||
| env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "test_config_file_log")) | ||
| default_datadir.mkdir(parents=True) | ||
|
|
||
| # Write a bitcoin.conf file in the default data directory containing a | ||
| # datadir= line pointing at the node datadir. | ||
| node = self.nodes[0] | ||
| conf_text = pathlib.Path(node.bitcoinconf).read_text() | ||
| conf_path = default_datadir / "bitcoin.conf" | ||
| conf_path.write_text(f"datadir={node.datadir}\n{conf_text}") | ||
|
|
||
| # Drop the node -datadir= argument during this test, because if it is | ||
| # specified it would take precedence over the datadir setting in the | ||
| # config file. | ||
| node_args = node.args | ||
| node.args = [arg for arg in node.args if not arg.startswith("-datadir=")] | ||
|
|
||
| # Check that correct configuration file path is actually logged | ||
| # (conf_path, not node.bitcoinconf) | ||
| with self.nodes[0].assert_debug_log(expected_msgs=[f"Config file: {conf_path}"]): | ||
| self.start_node(0, ["-allowignoredconf"], env=env) | ||
| self.stop_node(0) | ||
|
|
||
| # Restore node arguments after the test | ||
| node.args = node_args | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Core config filename and config-path wiring =="
rg -n -C2 "BITCOIN_CONF_FILENAME|GetConfigFilePath|ReadConfigFiles" src/util src/init.cpp
echo
echo "== Functional framework config filename usage =="
rg -n -C2 "bitcoinconf|dash\\.conf|bitcoin\\.conf|write_config|initialize_datadir" \
test/functional/test_framework test/functional/feature_config_args.pyRepository: dashpay/dash
Length of output: 22289
Replace hardcoded "bitcoin.conf" with "dash.conf" throughout the test.
The repository uses "dash.conf" as the configuration filename (defined in src/util/system.cpp:90 and used by the test framework in test/functional/test_framework/test_node.py:83). The new tests hardcode "bitcoin.conf" in multiple locations, which will cause them to fail:
- Line 133:
conf_path = default_datadir / "bitcoin.conf"→ should be"dash.conf" - Line 335:
self.start_node(0, [f'-conf={node.datadir}/bitcoin.conf'])→ should be"dash.conf" - Line 357:
(default_datadir / "bitcoin.conf").write_text(...)→ should be"dash.conf"
Also update the assertion messages on lines 328 and 365 to reference "dash.conf" instead of "bitcoin.conf".
🤖 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/feature_config_args.py` around lines 116 - 150, The
test_config_file_log function and related tests contain hardcoded references to
"bitcoin.conf" which should be "dash.conf" since the repository uses "dash.conf"
as the configuration filename. Replace all occurrences of "bitcoin.conf" with
"dash.conf" in the conf_path path creation (where it constructs the
configuration file path using pathlib.Path), in the start_node method call
arguments (where -conf parameter is specified), and in any file write operations
that create the configuration file. Additionally, update any assertion messages
and expected_msgs parameters that reference the configuration filename to use
"dash.conf" instead of "bitcoin.conf".
| star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups() | ||
| if star_pos_num is not None: | ||
| max_pos = max(max_pos, int(star_pos_num)) | ||
| elif star is not None: | ||
| n += 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm the current single-star logic exists in the changed file
rg -n "star, star_pos_num = re.match" test/lint/run-lint-format-strings.py
# Reproduce current behavior with equivalent logic (read-only verification)
python - <<'PY'
import re
def count_like_current(format_string):
format_string = format_string.replace('%%', 'X')
n = max_pos = 0
for m in re.finditer("%(.*?)[aAcdeEfFgGinopsuxX]", format_string, re.DOTALL):
pos_num, = re.match(r"(?:(^\d+)\$)?", m.group(1)).groups()
if pos_num is not None:
max_pos = max(max_pos, int(pos_num))
else:
n += 1
star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups()
if star_pos_num is not None:
max_pos = max(max_pos, int(star_pos_num))
elif star is not None:
n += 1
return max(n, max_pos)
cases = [
("%*.*d", 3),
("%2$*3$.*4$d", 4),
]
for fmt, expected in cases:
got = count_like_current(fmt)
print(f"{fmt}: got={got}, expected={expected}, {'OK' if got == expected else 'MISMATCH'}")
PYRepository: dashpay/dash
Length of output: 224
Count all * argument specifiers, not just the first one.
The regex pattern on line 267 only captures the first * in a format specifier. Format specifiers like %*.*d and %2$*3$.*4$d are undercounted, producing false negatives in the lint check.
Proposed fix
- star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups()
- if star_pos_num is not None:
- max_pos = max(max_pos, int(star_pos_num))
- elif star is not None:
- n += 1
+ for star_match in re.finditer(r"\*(?:(\d+)\$)?", m.group(1)):
+ star_pos_num = star_match.group(1)
+ if star_pos_num is not None:
+ max_pos = max(max_pos, int(star_pos_num))
+ else:
+ n += 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.
| star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups() | |
| if star_pos_num is not None: | |
| max_pos = max(max_pos, int(star_pos_num)) | |
| elif star is not None: | |
| n += 1 | |
| for star_match in re.finditer(r"\*(?:(\d+)\$)?", m.group(1)): | |
| star_pos_num = star_match.group(1) | |
| if star_pos_num is not None: | |
| max_pos = max(max_pos, int(star_pos_num)) | |
| else: | |
| n += 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/lint/run-lint-format-strings.py` around lines 267 - 271, The regex match
on line 267 only captures the first `*` in the format specifier string, causing
format specifiers with multiple `*` characters (like `%*.*d`) to be
undercounted. Replace the single re.match call with re.finditer or re.findall to
find all occurrences of the `*` pattern in m.group(1), then iterate through each
match to properly count all positional arguments and increment n for each `*`
specifier found.
6b589b0 to
bf311f7
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
July_2026_1
branch
2 times, most recently
from
4d2b649 to
e03be9e
Compare
vijaydasmp
force-pushed
the
July_2026_1
branch
3 times, most recently
from
7b9f02e to
99b3217
Compare
vijaydasmp
changed the title
|
✅ Review complete (commit fcabe79) |
There was a problem hiding this comment.
Code Review
Source: reviewers = Claude Opus and Codex gpt-5.5[high] for general, dash-core-commit-history, and backport-reviewer (Codex lanes failed/unparseable); verifier = Claude Opus; policy gate = scripts/review_policy.enforce_backport_prereq_policy.
Policy gate restored an agent-reported missing upstream prerequisite as blocking. This is a full Bitcoin backport PR, and the bitcoin#28825 fuzz coverage hunk depends on bitcoin#28450's tx_package_eval fuzz target, which is not present in the Dash tree. The docs typo backport from bitcoin#31718 applies cleanly.
🔴 1 blocking
🤖 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/test/util/script.h`:
- [BLOCKING] src/test/util/script.h:19-30: Missing prerequisite: bitcoin#28450 (tx_package_eval fuzz target)
Upstream bitcoin#28825 modifies two files: `src/test/util/script.h` (adds the constants seen in this hunk) and `src/test/fuzz/package_eval.cpp` (uses those constants to allow fake/duplicate inputs and reach `MempoolAcceptResult::ResultType::DIFFERENT_WITNESS`). The Dash cherry-pick applies only the header changes. `src/test/fuzz/package_eval.cpp` does not exist in this tree (confirmed by `ls src/test/fuzz/*package*` returning nothing and `git log HEAD -- src/test/fuzz/package_eval.cpp` finding no commits touching that path in Dash history). That file was introduced upstream by bitcoin#28450 ("Add package evaluation fuzzer"), which has not been backported.
Evidence:
- Upstream diff `git diff 6b7bf907f5^1...6b7bf907f5 --stat` shows `src/test/fuzz/package_eval.cpp | 22 ++++++++++++++++++----` alongside the script.h change.
- Dash diff `git diff 99b3217a31^1...99b3217a31 --stat` shows only the 12-line script.h change.
- `git log HEAD --oneline -- src/test/fuzz/` shows no `package_eval.cpp` commits ever landing.
Impact: the four new constants (`EMPTY`, `P2WSH_EMPTY`, `P2WSH_EMPTY_TRUE_STACK`, `P2WSH_EMPTY_TWO_STACK`) have no consumer in Dash and the fuzz-coverage improvement the PR advertises does not take effect. This is a soft prereq from a build/runtime perspective, but the PR's stated goal ("Exercises DIFFERENT_WITNESS by using 'blank' WSH() and allowing witness to determine wtxid, and attempts to make invalid/duplicate inputs") is unachievable until bitcoin#28450 is backported. Either backport bitcoin#28450 first, or explicitly document that this PR is intentionally pre-staging only the shared script constants for a later fuzz-target backport.
---
**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.
| static const std::vector<uint8_t> EMPTY{}; | ||
| static const CScript P2WSH_EMPTY{ | ||
| CScript{} | ||
| << OP_0 | ||
| << ToByteVector([] { | ||
| uint256 hash; | ||
| CSHA256().Write(EMPTY.data(), EMPTY.size()).Finalize(hash.begin()); | ||
| return hash; | ||
| }())}; | ||
| static const std::vector<std::vector<uint8_t>> P2WSH_EMPTY_TRUE_STACK{{static_cast<uint8_t>(OP_TRUE)}, {}}; | ||
| static const std::vector<std::vector<uint8_t>> P2WSH_EMPTY_TWO_STACK{{static_cast<uint8_t>(OP_2)}, {}}; | ||
|
|
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#28450 (tx_package_eval fuzz target)
Upstream bitcoin#28825 modifies two files: src/test/util/script.h (adds the constants seen in this hunk) and src/test/fuzz/package_eval.cpp (uses those constants to allow fake/duplicate inputs and reach MempoolAcceptResult::ResultType::DIFFERENT_WITNESS). The Dash cherry-pick applies only the header changes. src/test/fuzz/package_eval.cpp does not exist in this tree (confirmed by ls src/test/fuzz/*package* returning nothing and git log HEAD -- src/test/fuzz/package_eval.cpp finding no commits touching that path in Dash history). That file was introduced upstream by bitcoin#28450 ("Add package evaluation fuzzer"), which has not been backported.
Evidence:
- Upstream diff
git diff 6b7bf907f5^1...6b7bf907f5 --statshowssrc/test/fuzz/package_eval.cpp | 22 ++++++++++++++++++----alongside the script.h change. - Dash diff
git diff 99b3217a31^1...99b3217a31 --statshows only the 12-line script.h change. git log HEAD --oneline -- src/test/fuzz/shows nopackage_eval.cppcommits ever landing.
Impact: the four new constants (EMPTY, P2WSH_EMPTY, P2WSH_EMPTY_TRUE_STACK, P2WSH_EMPTY_TWO_STACK) have no consumer in Dash and the fuzz-coverage improvement the PR advertises does not take effect. This is a soft prereq — the header additions compile cleanly and match upstream verbatim — but the PR's stated goal ("Exercises DIFFERENT_WITNESS by using 'blank' WSH() and allowing witness to determine wtxid, and attempts to make invalid/duplicate inputs") is unachievable until bitcoin#28450 is backported. Consider either (a) backporting bitcoin#28450 first and then this PR, or (b) documenting in the merge commit / PR body that only the header piece is being pre-staged for a future backport of the fuzz target.
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: ['claude-backport-reviewer']
There was a problem hiding this comment.
Resolved in this update — Missing prerequisite: bitcoin#28450 (tx_package_eval fuzz target) no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
vijaydasmp
changed the title
There was a problem hiding this comment.
💡 Codex Review
Line 60 in 2440940
Restore the walletutil declaration include
With ENABLE_WALLET, this file still imports and later calls GetWalletDir(), but this hunk removed the only include that declares it (wallet/walletutil.h; repo-wide search finds the declaration there). Wallet-enabled Qt builds will fail because wallet::GetWalletDir is undeclared unless the wallet utility header is included again.
ℹ️ 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".
| namespace node { | ||
| void ApplyArgsManOptions(const ArgsManager& argsman, ValidationCacheSizes& cache_sizes) | ||
| { | ||
| if (auto max_size = argsman.GetIntArg("-maxsigcachesize")) { |
There was a problem hiding this comment.
This new source does not compile against the current ArgsManager API: src/util/system.h only declares GetIntArg(const std::string&, int64_t), not a one-argument optional-returning overload, yet this file is added to libbitcoin_node_a_SOURCES. Any target that builds node/validation_cache_args.cpp will fail before tests run; use the existing two-argument API or backport the missing overload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
Source: reviewers = Claude Opus and Codex gpt-5.5 for general, dash-core-commit-history, and backport-reviewer; verifier = Claude Opus; policy gate = scripts/review_policy.enforce_backport_prereq_policy.
Prior Findings Reconciliation
- prior-1: FIXED. The earlier blocking bitcoin#28450 / tx_package_eval prerequisite finding no longer applies at 2440940 because bitcoin#28825 was dropped from this PR stack and the
EMPTY/P2WSH_EMPTY*constants were removed fromsrc/test/util/script.h.
Carried-Forward Prior Findings
- None.
New Findings In Latest Delta
The new push replaces the earlier fuzz-target backport with bitcoin#25216, gui#613, and bitcoin#25527. The verifier found two concrete build-blocking merge/prerequisite issues: validation_cache_args.cpp uses an upstream ArgsManager::GetIntArg overload Dash does not have, and the gui#613 include cleanup removed wallet/walletutil.h while Dash-specific GetWalletDir() users remain. The deterministic backport policy gate also restored two backport-completeness findings that verifier treated as low-severity: the omitted bitcoin#31718 doc/policy/packages.md hunk due missing bitcoin#28984 text, and the omitted Dash IWYU/tidy adaptation for the new validation-cache source.
Canonical policy output: 5 blocking and 2 nitpicks across 4 grouped root causes.
Review findings
In src/node/validation_cache_args.cpp:
- [BLOCKING] src/node/validation_cache_args.cpp:22: Missing prerequisite: single-arg ArgsManager::GetIntArg overload
The new `ApplyArgsManOptions()` calls `argsman.GetIntArg("-maxsigcachesize")` with one argument and then dereferences the optional-style result. Dash only declares `GetIntArg(const std::string&, int64_t)` in `src/util/system.h`, so this does not compile. Either backport the upstream optional-returning overload first, or adapt this code to Dash's existing API with `IsArgSet("-maxsigcachesize")` plus an explicit default.
In src/qt/rpcconsole.cpp:
- [BLOCKING] src/qt/rpcconsole.cpp:38-44: gui#613 removed walletutil.h but Dash still uses wallet::GetWalletDir
Upstream gui#613 safely removed wallet includes because upstream `rpcconsole.cpp` has no `GetWalletDir()` consumer. Dash does: `using wallet::GetWalletDir;` remains at line 60 and `GUIUtil::PathToQString(GetWalletDir())` remains around line 863. `GetWalletDir()` is declared by `src/wallet/walletutil.h`, and no transitive include provides it, so wallet-enabled GUI builds fail. Restore an `#ifdef ENABLE_WALLET` guarded `#include <wallet/walletutil.h>` or remove the Dash-specific wallet-path display in a separate scoped change.
In doc/policy/packages.md:
- [POLICY BLOCKING / verifier nitpick] doc/policy/packages.md: bitcoin#31718 package-policy typo hunk is skipped because bitcoin#28984 is missing
bitcoin#31718 also fixes `Replacements must pay more total total fees` in `doc/policy/packages.md`. Dash lacks that package-RBF section because bitcoin#28984 is not backported, so the hunk is omitted. This is documentation-only, but for full upstream backport PRs the policy gate treats missing prerequisites as blocking unless intentionally excluded or allowlisted.
In ci/dash/lint-tidy.sh:
- [POLICY BLOCKING / verifier nitpick] ci/dash/lint-tidy.sh: bitcoin#25527 IWYU/tidy source-list hunk was not adapted
Upstream bitcoin#25527 adds `src/node/validation_cache_args.cpp` to Bitcoin's IWYU/tidy source list. Dash's equivalent explicit source list is in `ci/dash/lint-tidy.sh`, but the new source file was not added there. This is not a compile/runtime issue, but it is an incomplete CI-coverage adaptation that the backport policy gate restored.
| namespace node { | ||
| void ApplyArgsManOptions(const ArgsManager& argsman, ValidationCacheSizes& cache_sizes) | ||
| { | ||
| if (auto max_size = argsman.GetIntArg("-maxsigcachesize")) { |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: single-arg ArgsManager::GetIntArg overload — build fails
argsman.GetIntArg("-maxsigcachesize") is called with a single argument, then bound via if (auto max_size = ...) and dereferenced with *max_size. This requires the upstream std::optional<int64_t> ArgsManager::GetIntArg(const std::string&) const overload, which was introduced by a separate Bitcoin Core PR (bitcoin#25409 / bitcoin#25749) that has not been backported here. Dash's ArgsManager only declares int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const (src/util/system.h:372), so this cannot compile — CI confirms failure across every build job:
node/validation_cache_args.cpp:22:42: error: no matching function for call to 'ArgsManager::GetIntArg(const char [17]) const'
./util/system.h:372:13: note: candidate: 'int64_t ArgsManager::GetIntArg(const std::string&, int64_t) const'
Either backport the single-arg GetIntArg overload first, or adapt this merge resolution to Dash's existing API (guard with IsArgSet("-maxsigcachesize") and pass an explicit default). Example adaptation:
if (argsman.IsArgSet("-maxsigcachesize")) {
int64_t max_size = argsman.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_BYTES >> 20);
size_t clamped_size_each = std::max<int64_t>(max_size, 0) * (1 << 20) / 2;
cache_sizes = {
.signature_cache_bytes = clamped_size_each,
.script_execution_cache_bytes = clamped_size_each,
};
}(add #include <script/sigcache.h> for DEFAULT_MAX_SIG_CACHE_BYTES).
source: ['claude']
There was a problem hiding this comment.
Correction: this was not resolved by 80226faa5417a7940af3d9969d52957ba82e9dea. The current head still has argsman.GetIntArg("-maxsigcachesize") with the missing optional-returning overload in src/node/validation_cache_args.cpp, so the prior finding remains valid and is carried forward in the latest review.
The earlier auto-resolved reply was posted by mistake during review reconciliation.
|
This pull request has conflicts, please rebase. |
43ff37f Update zmq.md: Fix parameter in hwm example block (mutatrum) Pull request description: Looks like a copy/paste error when `zmqpubsequence` was introduced. ACKs for top commit: theStack: ACK 43ff37f Tree-SHA512: 4d0abbba4e9fd0adc2824d0804891d677d56216d245ed0d3bffbaf76042c650edf68975d1ec4728328e69421c024fe6f8800c7e0a497934082206ea4f15cc517
…pcconsole.cpp 0994273 qt: Remove unnecessary wallet includes from rpcconsole.cpp (laanwj) Pull request description: Fixes bitcoin#25266 ACKs for top commit: MarcoFalke: cr ACK 0994273 hebasto: ACK 0994273. Tree-SHA512: 02cef4a1f3522c4cd662853eb930dfdf0866a5bd959a00f4c42d8c741b0751df4cf2e14c304b93ca3ce699c0e9730caf3d6fa2053009af312c60e861b0f79179
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80226faa54
ℹ️ 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".
| #include <wallet/walletutil.h> | ||
| #endif | ||
|
|
||
| #include <QAbstractButton> |
There was a problem hiding this comment.
In ENABLE_WALLET Qt builds this file still has using wallet::GetWalletDir and calls GetWalletDir() in onWalletChanged(), but this hunk removes the only include (wallet/walletutil.h) that declares that function; the remaining Qt/wallet interface headers only forward-declare wallet types. This makes wallet-enabled GUI builds fail to compile in this translation unit, so keep the walletutil include while dropping the unused BDB/db headers.
Useful? React with 👍 / 👎.
vijaydasmp
changed the title
There was a problem hiding this comment.
Code Review
Source: reviewers = Claude Opus failed for general, dash-core-commit-history, and backport-reviewer; Codex gpt-5.5 completed general and backport-reviewer; Codex gpt-5.5 dash-core-commit-history stalled without parseable JSON and was terminated after preserving output; verifier = Codex gpt-5.5 after Claude Opus verifier failed; policy gate = scripts/review_policy.enforce_backport_prereq_policy.
Prior Findings Reconciliation
- prior-1: STILL VALID.
src/node/validation_cache_args.cppstill calls the missing optional-returning single-argArgsManager::GetIntArg. - prior-2/prior-3: STILL VALID, consolidated.
src/qt/rpcconsole.cppstill useswallet::GetWalletDirwithout includingwallet/walletutil.h. - prior-4: STILL VALID. The bitcoin#31718
doc/policy/packages.mdtypo hunk is still omitted because Dash lacks the bitcoin#28984 package-RBF documentation section. - prior-5: STILL VALID.
src/node/validation_cache_args.cppis still absent from Dash's explicitci/dash/lint-tidy.shIWYU source list. - prior-6/prior-7: OUTDATED as standalone nitpicks. They are duplicate lower-severity forms of prior-4/prior-5 and are covered by the consolidated policy findings above.
Carried-Forward Prior Findings
src/node/validation_cache_args.cpp: missing optionalArgsManager::GetIntArgprerequisite, build-blocking.src/qt/rpcconsole.cpp: gui#613 droppedwallet/walletutil.hwhile Dash-specificGetWalletDirusage remains, build-blocking.doc/policy/packages.md: missing bitcoin#28984 prerequisite means the advertised bitcoin#31718 backport still omits an upstream hunk; policy gate keeps this blocking unless the exclusion is documented/allowlisted.ci/dash/lint-tidy.sh: bitcoin#25527's IWYU/tidy source-list hunk still has no Dash adaptation; policy gate keeps this blocking for full-backport completeness.
New Findings In Latest Delta
src/test/util/setup_common.cpp: the current rebase adds the globalNetworkSetupwrapper from bitcoin#28486 context but keeps the older per-BasicTestingSetupSetupNetworking();call, so tests initialize networking twice and ignore the[[nodiscard]]return.
Canonical policy output: 5 blocking findings across 5 root causes.
Review findings
In src/node/validation_cache_args.cpp:
- [BLOCKING] src/node/validation_cache_args.cpp:22: Missing prerequisite: optional ArgsManager accessors
The #25527 backport still calls `argsman.GetIntArg("-maxsigcachesize")` with one argument and treats the result as optional. Dash's `ArgsManager` only has the two-argument `GetIntArg(const std::string&, int64_t)` overload, so this does not compile. Backport the optional accessor prerequisite or adapt this code to Dash's existing API.
In src/qt/rpcconsole.cpp:
- [BLOCKING] src/qt/rpcconsole.cpp:36-60: gui#613 dropped walletutil.h while Dash still uses GetWalletDir
The cherry-pick removed Dash's wallet include block, including `wallet/walletutil.h`, but `using wallet::GetWalletDir` and the `GetWalletDir()` call remain. `GetWalletDir()` is declared in `wallet/walletutil.h`, so wallet-enabled GUI builds fail unless the guarded include is restored or the Dash-specific wallet path code is removed separately.
In doc/policy/packages.md:
- [BLOCKING] doc/policy/packages.md:45-49: Missing prerequisite: bitcoin#28984
Upstream bitcoin#31718 also fixes a package-RBF typo in `doc/policy/packages.md`, but Dash lacks that section because bitcoin#28984 is not backported. For a PR advertised as a full bitcoin#31718 backport, the omitted hunk remains a missing prerequisite unless the exclusion is explicitly documented or allowlisted.
In ci/dash/lint-tidy.sh:
- [BLOCKING] ci/dash/lint-tidy.sh:85-99: Omitted Dash adaptation of bitcoin#25527 IWYU source-list hunk
Upstream bitcoin#25527 adds `src/node/validation_cache_args.cpp` to Bitcoin's IWYU/tidy source list. Dash's equivalent explicit list is `ci/dash/lint-tidy.sh`, and the new source file is still missing there, so the upstream CI-coverage hunk was not adapted.
In src/test/util/setup_common.cpp:
- [BLOCKING] src/test/util/setup_common.cpp:216: Rebase reintroduces per-fixture SetupNetworking after global setup
Current `develop` already has the bitcoin#28486-style global `NetworkSetup` wrapper at lines 162-169, which calls `Assert(SetupNetworking())` once. This backport leaves the older per-`BasicTestingSetup` `SetupNetworking();` call at line 216, so networking is initialized twice and the second call ignores a `[[nodiscard]] bool` return. Keep the global setup and remove the per-fixture call.
| @@ -35,14 +35,6 @@ | |||
|
|
|||
| #include <univalue.h> | |||
There was a problem hiding this comment.
🔴 Blocking: Dropped walletutil include leaves GetWalletDir undeclared
The gui#613 cherry-pick removed Dash's wallet include block, including wallet/walletutil.h, but this file still has using wallet::GetWalletDir and still calls GetWalletDir() in onWalletChanged(). GetWalletDir() is declared in src/wallet/walletutil.h, and the currently included Qt/interface headers do not provide that declaration. Wallet-enabled GUI builds will fail at the remaining Dash-specific wallet path code.
| #include <univalue.h> | |
| #include <univalue.h> | |
| #ifdef ENABLE_WALLET | |
| #include <wallet/walletutil.h> | |
| #endif |
source: ['codex']
There was a problem hiding this comment.
Correction: this thread is still valid at fcabe799 and was carried forward in the new exact-SHA review. The earlier auto-resolved text was wrong because src/qt/rpcconsole.cpp still uses wallet::GetWalletDir without including wallet/walletutil.h.
There was a problem hiding this comment.
Code Review
Source: reviewers = Claude Opus failed for general, dash-core-commit-history, and backport-reviewer due quota; Codex gpt-5.5 completed for general and dash-core-commit-history; Codex gpt-5.5 backport-reviewer exited with useful text but no parseable JSON. Verifier = Codex gpt-5.5 after Opus verifier quota failure. Specialists: dash-core-commit-history, backport-reviewer.
Current head fcabe799bfef195a37017e1ab3ddd7f36781affe removed the bitcoin#25527 commit, so the prior validation_cache_args, IWYU coverage, and setup_common blockers tied to that removed commit are fixed. Two in-scope carried-forward backport issues remain: gui#613 removed a Dash-required walletutil include, and the advertised bitcoin#31718 backport still omits the upstream packages.md hunk because the bitcoin#28984 package-RBF documentation prerequisite is absent. No concrete CodeRabbit inline findings were provided.
🔴 2 blocking
Prior Findings Reconciliation
STILL VALID and carried forward: prior-2/prior-8 (src/qt/rpcconsole.cpp walletutil include) and prior-3/prior-6/prior-9 (doc/policy/packages.md missing bitcoin#28984 prerequisite).
FIXED by removal of bitcoin#25527: prior-1/prior-5/prior-7 (validation_cache_args / optional ArgsManager accessors), prior-4/prior-11 (SetupNetworking() reintroduced in setup_common.cpp), and prior-10/prior-12 (Dash IWYU list entry for validation_cache_args.cpp).
INTENTIONALLY DEFERRED: none.
Carried-Forward Prior Findings
In src/qt/rpcconsole.cpp:
- [BLOCKING] src/qt/rpcconsole.cpp:36: Restore walletutil include for GetWalletDir
The gui#613 backport removed Dash's guarded wallet include block, includingwallet/walletutil.h, but this file still hasusing wallet::GetWalletDirand callsGetWalletDir()inonWalletChanged(). In the current treeGetWalletDir()is declared bysrc/wallet/walletutil.h, and the remaining Qt/interfaces includes do not declare it. Wallet-enabled GUI builds will fail unless the needed include is restored or the Dash-specific wallet path code is changed.
In doc/policy/packages.md:
- [BLOCKING] doc/policy/packages.md:45-49: Handle the omitted bitcoin#31718 packages.md hunk
Upstream bitcoin#31718 changes five files, including adoc/policy/packages.mdtypo fix fromtotal total feestototal feesin the package replacement section. This PR applies only the other four files. The missing section is absent from Dash because the bitcoin#28984 package-RBF documentation prerequisite has not been backported, and there is no checked-in evidence that this upstream hunk is intentionally excluded. Since the PR advertises a full bitcoin#31718 backport, the omission needs to be documented explicitly or the prerequisite/hunk needs to be handled.
New Findings In Latest Delta
None. The latest delta from 80226faa to fcabe799 removes bitcoin#25527 and fixes the prior findings tied only to that removed commit.
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/qt/rpcconsole.cpp`:
- [BLOCKING] src/qt/rpcconsole.cpp:36: Restore walletutil include for GetWalletDir
The gui#613 backport removed Dash's guarded wallet include block, including `wallet/walletutil.h`, but this file still has `using wallet::GetWalletDir` and calls `GetWalletDir()` in `onWalletChanged()`. In the current tree `GetWalletDir()` is declared by `src/wallet/walletutil.h`, and the remaining Qt/interfaces includes do not declare it. Wallet-enabled GUI builds will fail unless the needed include is restored or the Dash-specific wallet path code is changed.
In `doc/policy/packages.md`:
- [BLOCKING] doc/policy/packages.md:45-49: Handle the omitted bitcoin#31718 packages.md hunk
Upstream bitcoin#31718 changes five files, including a `doc/policy/packages.md` typo fix from `total total fees` to `total fees` in the package replacement section. This PR applies only the other four files. The missing section is absent from Dash because the bitcoin#28984 package-RBF documentation prerequisite has not been backported, and there is no checked-in evidence that this upstream hunk is intentionally excluded. Since the PR advertises a full bitcoin#31718 backport, the omission needs to be documented explicitly or the prerequisite/hunk needs to be handled.
| @@ -35,14 +35,6 @@ | |||
|
|
|||
| #include <univalue.h> | |||
There was a problem hiding this comment.
🔴 Blocking: Restore walletutil include for GetWalletDir
The gui#613 backport removed Dash's guarded wallet include block, including wallet/walletutil.h, but this file still has using wallet::GetWalletDir and calls GetWalletDir() in onWalletChanged(). In the current tree GetWalletDir() is declared by src/wallet/walletutil.h, and the remaining Qt/interfaces includes do not declare it. Wallet-enabled GUI builds will fail unless the needed include is restored or the Dash-specific wallet path code is changed.
| #include <univalue.h> | |
| #include <univalue.h> | |
| #ifdef ENABLE_WALLET | |
| #include <wallet/walletutil.h> | |
| #endif |
source: ['codex']
5 participants
bitcoin backports