backport: Merge bitcoin#28755, 28865, 31225, (partial) 31718, 28966#7364
backport: Merge bitcoin#28755, 28865, 31225, (partial) 31718, 28966#7364vijaydasmp wants to merge 5 commits into
Conversation
|
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
June_2026_5
branch
6 times, most recently
from
983388d to
a0cb8c5
Compare
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
|
✅ Review complete (commit 088b945) |
|
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 (7)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis pull request makes several independent improvements across documentation, build configuration, and testing. Documentation typos and wording are corrected across Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Faithful backport of four small upstream PRs: typo/grammar doc fixes (bitcoin#31718, bitcoin#31225), configure.ac restructure for miniupnpc (bitcoin#28755), and per-symbol UBSan suppressions migration (bitcoin#28865). All agent findings were verified against upstream diffs and current source; none represent defects introduced by the Dash adaptation. The flagged 'incomplete' UBSan migration for policy/fees.cpp is in fact a faithful match of upstream — that line was intentionally not migrated in bitcoin#28865 (the narrowing to TxConfirmStats::removeTx happened in the later, unbackported bitcoin#28966). The packages.md/tracing-script notes are upstream prerequisite gaps that do not affect this PR.
vijaydasmp
changed the title
There was a problem hiding this comment.
Code Review
Incremental + cumulative re-review of 2e0dcdc0 after prior clean review at cad42458. No carried-forward prior findings exist, and CodeRabbit had zero inline findings. The latest delta only retunes test/sanitizer_suppressions/ubsan; the UBSan suppression state matches the relevant upstream backport after verification.
One cumulative backport completeness issue remains: the PR claims bitcoin#31718, but omits its doc/policy/packages.md hunk because Dash is missing the prerequisite package-RBF documentation from bitcoin#28984. This is documentation-only and not a build/runtime defect, but for a full upstream backport it should either bring the prerequisite doc state or explicitly document the intentional exclusion.
🔴 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 `doc/policy/packages.md`:
- [BLOCKING] doc/policy/packages.md:38-39: Missing prerequisite: bitcoin#28984
bitcoin#31718 changes five files upstream, including doc/policy/packages.md. The Dash cherry-pick applies the other four doc/tracing typo fixes but omits the packages.md hunk. Upstream pre-31718 has a package replacement subsection after the package conflict rules, including the typo line `Replacements must pay more total total fees at the incremental relay fee`; Dash current still jumps from the older package conflict/ancestor-limit rules to `When packages are evaluated against ancestor/descendant limits...` and has no package replacement subsection to fix. Upstream blame shows the omitted paragraph was introduced by commit afd52d8e63 (`doc: update package RBF comment`), first merged via 41544b8f96 (`Merge bitcoin/bitcoin#28984: Cluster size 2 package rbf`). This is a soft prerequisite gap: the current cherry-pick handles it by omitting a documentation-only hunk, so it should not break build/runtime behavior, but the claimed full backport of bitcoin#31718 is incomplete without either bitcoin#28984's relevant package-RBF doc state or an explicit exclusion of that hunk.
---
**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`). Either backport the prerequisite or annotate the intentional exclusion in the PR description.
| unsigned-integer-overflow:EvalScript | ||
| unsigned-integer-overflow:txmempool.cpp | ||
| unsigned-integer-overflow:util/strencodings.cpp | ||
| unsigned-integer-overflow:xoroshiro128plusplus.h |
There was a problem hiding this comment.
🔴 Blocking: 28865: kept broader policy/fees.cpp suppression instead of upstream's TxConfirmStats::removeTx narrowing
Upstream PR 28865 replaces 'implicit-integer-sign-change:policy/fees.cpp' with the narrower per-symbol 'implicit-integer-sign-change:TxConfirmStats::removeTx'. The Dash backport (commit 73add31) keeps the original file-scoped 'implicit-integer-sign-change:policy/fees.cpp' entry and does not add 'TxConfirmStats::removeTx'. This is the only ubsan line where Dash diverges from upstream's per-symbol migration. It is more permissive (suppresses any sign-change UBSan finding in fees.cpp) rather than less, so it is not a missing prerequisite and will not break the build or tests. Worth a note in case the divergence is unintentional — upstream's stated goal of 28865 was precisely to narrow these suppressions to specific symbols.
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']
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Code Review
Incremental + cumulative re-review of 1bd4ff8c after the prior review at 2e0dcdc0.
The latest delta only changes test/sanitizer_suppressions/ubsan; that change matches the upstream per-symbol UBSan suppression migration and did not introduce a new issue.
Carried-forward prior finding: STILL VALID. The PR still claims a full backport of bitcoin#31718, but the upstream doc/policy/packages.md hunk remains omitted because Dash is missing the package replacement subsection introduced by bitcoin#28984. That is a missing prerequisite unless the omission is explicitly documented as intentionally deferred.
🔴 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 `doc/policy/packages.md`:
- [BLOCKING] doc/policy/packages.md:33-39: Incomplete bitcoin#31718 backport: missing doc/policy/packages.md hunk (prerequisite bitcoin#28984)
Carried-forward prior finding, revalidated at head 1bd4ff8c — STILL VALID.
Upstream bitcoin#31718 modifies five files: ci/README.md, contrib/guix/README.md, contrib/tracing/README.md, contrib/tracing/log_raw_p2p_msgs.py, and doc/policy/packages.md. The fifth file fixes the duplicated word in “Replacements must pay more total total fees …” inside the “Only limited package replacements are currently considered. (#28984)” subsection.
Verified in this worktree: Dash's current doc/policy/packages.md jumps from the package-conflict rule to the ancestor/descendant rule with no #28984 package replacement subsection in between. Greps for “total total”, “package replacement”, “Package Replacement”, and “pay more” return no matching subsection.
The Dash cherry-pick applies only the four other bitcoin#31718 typo/doc hunks; the packages.md hunk is silently dropped because the targeted text does not exist. Because the PR title claims a faithful full `Merge bitcoin/bitcoin#31718` backport and the PR description does not document an intentional omission, this is still a missing prerequisite rather than a harmless Dash adaptation.
Remediation options: (a) also backport bitcoin#28984 and any prerequisites before merging, then include the corresponding packages.md typo fix; or (b) explicitly document that this hunk is intentionally deferred until #28984 lands, so permanent history does not record an incomplete merge as a faithful full backport.
CodeRabbit reported no actionable inline findings for this SHA, so no CR reactions were needed.
b74e449 build: remove potential for duplciate natpmp linking (fanquake) 4e95096 build: remove duplicate -lminiupnpc linking (fanquake) Pull request description: Having the link check in the header check loop means we get `-lminiupnpc -lminiupnpc -lminiupnpc` on the link line. This is unnecessary, and results in warnings, i.e: ```bash ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc' ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc' ld: warning: ignoring duplicate libraries: '-levent', '-lminiupnpc' ``` These warnings have been occurring since the new macOS linker released with Xcode 15, and also came up in hebasto#34. There are other duplicate lib issues, i.e with `-levent` + `-levent_pthreads -levent`, but those are less straight forward to solve, and won't be included here. ACKs for top commit: jonatack: ACK b74e449 hebasto: ACK b74e449, it fixes one issue mentioned in hebasto#34 (comment). TheCharlatan: ACK b74e449 theuni: ACK b74e449 Tree-SHA512: 987a56ef17cbaf273cb672c41016f3f615b16889317325a9e88135d0c41f01af3840ad44a6f811a7df97f5873c9cd957e60aaa1b99bd408b17b4b1ffe2c68f36
fd30e96 test: migrate to some per-symbol ubsan suppressions (fanquake) Pull request description: Now that the symbolizer should be hanging around (bitcoin#28814), migrate some file-wide suppressions to be symbol specific. Should assist in catching new issues that may otherwise go unnoticed due to file-wide suppression. Only tested (so far) on aarch64 using the native ASAN & FUZZ CI. ACKs for top commit: maflcko: lgtm ACK fd30e96 dergoegge: utACK fd30e96 (if CI is green) Tree-SHA512: fbc44464d22813969dd4d1cdeab00042fa45f0af9bf1aed4fd3b688dc7b3c377a7c0f5f0c0a37ba65b649cfb5c7ff8ab2774500fe182d702c4340ca19f08479f
ac286e0 doc: Fix grammatical errors in multisig-tutorial.md (secp512k2) Pull request description: This pull request fixes grammatical errors in the `multisig-tutorial.md` document. ACKs for top commit: Abdulkbk: ACK ac286e0 Tree-SHA512: 684fe16e802431109957b9cde441353edeb16ffffde4282310c1a6f104adffc53347d00a2cf3a5969a78803f3177d6056ca37d3b7e8be52c2ec43ec0b9fcf4d9
…BlockTx suppression fa9dc92 test: Add missing CBlockPolicyEstimator::processBlockTx suppression (MarcoFalke) Pull request description: Fixes bitcoin#28865 (comment) ``` # FUZZ=policy_estimator UBSAN_OPTIONS="suppressions=/root/fuzz_dir/scratch/fuzz_gen/code/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 ... policy/fees.cpp:632:27: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed) #0 0x55cbbe10daee in CBlockPolicyEstimator::processBlockTx(unsigned int, CTxMemPoolEntry const*) src/policy/fees.cpp:632:27 #1 0x55cbbe10e361 in CBlockPolicyEstimator::processBlock(unsigned int, std::vector<CTxMemPoolEntry const*, std::allocator<CTxMemPoolEntry const*>>&) src/policy/fees.cpp:680:13 #2 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>)::$_1::operator()() const src/test/fuzz/policy_estimator.cpp:69:40 #3 0x55cbbd84af48 in unsigned long CallOneOf<policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3>(FuzzedDataProvider&, policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3) src/./test/fuzz/util.h:43:27 #4 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>) src/test/fuzz/policy_estimator.cpp:38:9 #5 0x55cbbda1cc18 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #6 0x55cbbda1cc18 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #7 0x55cbbd26a944 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x190e944) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #8 0x55cbbd253916 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f7916) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #9 0x55cbbd25945a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18fd45a) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #10 0x55cbbd284026 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x1928026) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #11 0x7fe4aa8280cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #12 0x7fe4aa828188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #13 0x55cbbd24e494 in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f2494) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change policy/fees.cpp:632:27 in ``` ``` # base64 /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 AQEAAAAAADkFlVwAAQEAAAAAADkFlZVcACTDSSsP3746IAZrH48khwMAAQEB/QEALQAACwAAAAAA FgAAAAAAAQAABgAAAAAAAAAAAAAAAAAAACcQAAAAAAAAAAAAAAAAAAAAAAD6AAAAOQWVXAABAQAA AAAAOQWVlVwAIMNJKw/fvjogBmsfjySHAwABAQH9AQAtAAALAAAAAAAAAAABAAAGAAAAAAAAAAAA AAAAAAAAJxAAAAAAAAAAAAAAAAAAAAAAAPr/AAAAAAAAAAAAAAQAAAAA/wAAAAAAAAAAAAAEAAAA AAEBAeAIAVwBXAAA/jbSBvwBKABSKBwBYgEB2wAEkvXInHYAAAAAAAAAvgAAAAAA/9//6v8e/xIk MgAlAiUAOw== ACKs for top commit: fanquake: ACK fa9dc92 dergoegge: utACK fa9dc92 Tree-SHA512: 3898c17c928ecc2bcc8c7086359e9ae00da2197b4d8e10c7bf6d12415326c9bca3ef6e1d8d3b83172ccfa604ce7e7371415262ba705225f9ea4da8b1a7eb0306 Signed-off-by: Shailendra Kumar Gupta <guptashail05@gmail.com>
vijaydasmp
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): Small backport PR bundling 5 trivial upstream changes (bitcoin#28755, bitcoin#28865, bitcoin#31225, bitcoin#31718-partial, bitcoin#28966) covering doc typos, configure.ac UPnP/NATPMP restructuring, and ubsan suppression migration. All hunks faithfully mirror upstream. The sole prior blocking finding (missing doc/policy/packages.md hunk from bitcoin#31718) is reconciled as INTENTIONALLY DEFERRED: the upstream typo target text does not exist in Dash's tree because prerequisite bitcoin#28984 (package-RBF) is unbackported, and the commit/PR title's explicit '(partial)' prefix is maintainer-provided acknowledgement of the deliberate omission. No new in-scope defects in the latest delta.
🔴 3 blocking
3 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `doc/policy/packages.md`:
- [BLOCKING] doc/policy/packages.md:33-39: Carried-forward prior finding (backport-completeness, not commit-hygiene): missing packages.md hunk / prerequisite bitcoin#28984
Prior review at 1bd4ff8c flagged that bitcoin#31718 changes 5 files upstream (including doc/policy/packages.md), but the Dash cherry-pick at e88406e7d8 only applies 4 (ci/README.md, contrib/guix/README.md, contrib/tracing/README.md, contrib/tracing/log_raw_p2p_msgs.py). From a commit-history hygiene standpoint, this is handled correctly — the subject is explicitly labeled `(partial) Merge bitcoin/bitcoin#31718`, which is the right convention for partial cherry-picks; whether the omitted hunk is acceptable (i.e. whether prerequisite bitcoin#28984 is missing) is a backport-completeness question, owned by the backport-reviewer specialist, not this commit-history specialist. Forwarding here only because the run prompt required explicit reconciliation of prior findings; classify as OUT-OF-SCOPE for commit-history review (the labeling itself is correct).
---
**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.
- [BLOCKING] doc/policy/packages.md:1: Carried-forward: bitcoin#31718 packages.md hunk omitted (prereq bitcoin#28984 missing) — INTENTIONALLY DEFERRED
Revalidation of prior blocking finding at 1bd4ff8c. STATUS: INTENTIONALLY DEFERRED.
Upstream bitcoin#31718 (merge 796e1a4c5d) touches 5 files, including a 'total total' → 'total' fix in doc/policy/packages.md (around the package-RBF replacement rules block). Dash commit e88406e7d8 backports 4 of the 5 hunks and is explicitly titled '(partial) Merge bitcoin/bitcoin#31718: Docs: fix typos in documentation files'. The packages.md hunk is absent because the target string ('Replacements must pay more total total fees ...') does not exist in Dash's doc/policy/packages.md — Dash has not backported the originating upstream PR bitcoin#28984 (Cluster size 2 package rbf), which introduced that documentation block and its typo (verified via `git log bitcoin/master -S 'Replacements must pay more total' -- doc/policy/packages.md` → afd52d8e63, contained in Merge bitcoin#28984 / followup #30295).
Why this is treated as intentionally deferred rather than carried forward as still-blocking:
- The PR title and the Dash commit message both use the '(partial)' prefix, which is explicit maintainer acknowledgement that one upstream hunk is being skipped on purpose.
- The missing prerequisite (bitcoin#28984) is a substantial mempool / package-RBF feature, not a doc-only change; pulling it in solely to enable a one-word doc typo fix would be a disproportionate dependency expansion.
- The omitted hunk is purely cosmetic documentation text and has no runtime / consensus / build impact.
No action required for this PR. If/when bitcoin#28984 is later backported, the 'total total' typo should be fixed at that time (or as a trailing follow-up).
---
**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.
- [BLOCKING] doc/policy/packages.md:33-39: Missing prerequisite: bitcoin#28984
Prior finding status: STILL VALID. Upstream bitcoin#31718 changes doc/policy/packages.md by fixing the package-RBF rule text from “total total fees” to “total fees”. That hunk is inside the “Only limited package replacements are currently considered. (#28984)” section. In current Dash HEAD, doc/policy/packages.md jumps from the package-conflict rule at lines 33-36 into different package policy text and does not contain that package-RBF section at all. Tracing the missing section shows it was introduced by upstream merge 41544b8f96 (bitcoin#28984: Cluster size 2 package rbf), with follow-up context in bitcoin#30295. The current partial cherry-pick of bitcoin#31718 still omits the packages.md hunk rather than carrying the prerequisite chain or explicitly documenting a Dash-specific exclusion, so the previous missing-prerequisite finding remains unresolved.
---
**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.
3 participants
bitcoin back ports