backport: Merge bitcoin#28523, 27822, 28671, 29114, 28037#7358
backport: Merge bitcoin#28523, 27822, 28671, 29114, 28037#7358vijaydasmp wants to merge 5 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
June_2026_4
branch
2 times, most recently
from
236ba33 to
9d9ed5a
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
June_2026_4
branch
6 times, most recently
from
19e166d to
daa0925
Compare
vijaydasmp
changed the title
vijaydasmp
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
✅ Review complete (commit a1053ed) |
![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: 74c2e6f097
ℹ️ 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".
| // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails | ||
| success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); | ||
| if (!success) { | ||
| success = DoMigration(*local_wallet, context, error, res); |
There was a problem hiding this comment.
When migratewallet is run on a blank legacy wallet, this branch marks the operation successful immediately after converting the database to SQLite, so DoMigration/ApplyMigrationData never runs and the wallet never gets WALLET_FLAG_DESCRIPTORS or has its legacy ScriptPubKeyMan removed. The RPC will report success while the reloaded wallet is still descriptors=false; a subsequent retry then fails at MigrateToSQLite because the database is already SQLite, leaving blank legacy wallets half-migrated.
Useful? React with 👍 / 👎.
| if self.is_sqlite_compiled(): | ||
| self.options.descriptors = True |
There was a problem hiding this comment.
With this default flipped to descriptors=True, RPCOverloadWrapper.createwallet() now passes descriptors=True for tests that omit the argument. wallet_migration.py still creates basic2 with createwallet(wallet_name="basic2", blank=True) and then calls sethdseed, which is legacy-only, so on normal SQLite+BDB test builds this test creates a descriptor blank wallet and fails before exercising the migration path.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (19)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (16)
WalkthroughThis PR introduces a new Sequence DiagramsequenceDiagram
participant Client
participant getrawaddrman RPC
participant AddrMan
participant AddrManImpl
Client->>getrawaddrman RPC: call getrawaddrman
getrawaddrman RPC->>AddrMan: GetEntries(false) — new table
AddrMan->>AddrManImpl: GetEntries(false) with LOCK(cs) + Check()
AddrManImpl-->>getrawaddrman RPC: vector<pair<AddrInfo, AddressPosition>>
getrawaddrman RPC->>AddrMan: GetEntries(true) — tried table
AddrMan->>AddrManImpl: GetEntries(true) with LOCK(cs) + Check()
AddrManImpl-->>getrawaddrman RPC: vector<pair<AddrInfo, AddressPosition>>
getrawaddrman RPC->>getrawaddrman RPC: AddrmanTableToJSON for new and tried tables
getrawaddrman RPC-->>Client: {new: {bucket/pos: entry}, tried: {bucket/pos: entry}}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/wallet_migration.py (1)
178-180:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCreate
basic2through the legacy helper before callingsethdseed.
createwallet(..., blank=True)omitsdescriptors=False, so it now creates a descriptor wallet by default; CI is failing when the next line calls legacy-onlysethdseed.Proposed fix
- self.nodes[0].createwallet(wallet_name="basic2", blank=True) - basic2 = self.nodes[0].get_wallet_rpc("basic2") + basic2 = self.create_legacy_wallet("basic2", blank=True) basic2.sethdseed(True, BASIC2_SEED_WIF)🤖 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/wallet_migration.py` around lines 178 - 180, The createwallet call for basic2 is missing the descriptors parameter, causing it to create a descriptor wallet by default when blank=True is used. Since sethdseed is a legacy-only method that cannot work with descriptor wallets, you need to explicitly add descriptors=False to the createwallet call to ensure it creates a legacy wallet. This will allow the subsequent sethdseed call with BASIC2_SEED_WIF to work correctly.Source: Pipeline failures
🤖 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/univalue/include/univalue.h`:
- Line 90: The modification to src/univalue/include/univalue.h violates the
repository rule forbidding changes to vendored dependencies. The pushKVEnd
method addition at line 90 modifies vendored UniValue code which must remain
untouched. To fix this, either revert the changes to src/univalue/** and instead
adapt all external callers to work with the original UniValue API, or obtain
explicit documented approval from the maintainers for an exception to the
vendored code policy before proceeding with this backport.
In `@test/functional/rpc_net.py`:
- Around line 453-455: The assert_approx call is comparing result["time"] (from
node mocktime) against wall-clock time.time(), which can cause nondeterministic
failures on slow runners. Replace time.time() with the corresponding mocktime
value to ensure the assertion compares mocktime against mocktime consistently.
Check the test setup or node context to determine the correct mocktime accessor
or method to use instead of time.time().
---
Outside diff comments:
In `@test/functional/wallet_migration.py`:
- Around line 178-180: The createwallet call for basic2 is missing the
descriptors parameter, causing it to create a descriptor wallet by default when
blank=True is used. Since sethdseed is a legacy-only method that cannot work
with descriptor wallets, you need to explicitly add descriptors=False to the
createwallet call to ensure it creates a legacy wallet. This will allow the
subsequent sethdseed call with BASIC2_SEED_WIF to work correctly.
🪄 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: fcbfdde3-397b-41b8-850a-c0929bd4cce7
📒 Files selected for processing (19)
src/addrman.cppsrc/addrman.hsrc/addrman_impl.hsrc/rpc/client.cppsrc/rpc/mempool.cppsrc/rpc/net.cppsrc/test/fuzz/rpc.cppsrc/test/settings_tests.cppsrc/txorphanage.cppsrc/txorphanage.hsrc/univalue/include/univalue.hsrc/univalue/lib/univalue.cppsrc/univalue/test/object.cppsrc/util/settings.cppsrc/wallet/rpc/addresses.cppsrc/wallet/wallet.cpptest/functional/rpc_net.pytest/functional/test_framework/test_framework.pytest/functional/wallet_migration.py
| void push_backV(It first, It last); | ||
|
|
||
| void __pushKV(std::string key, UniValue val); | ||
| void pushKVEnd(std::string key, UniValue val); |
There was a problem hiding this comment.
Do not modify vendored UniValue sources without an explicit exception.
Line 90 introduces an API rename inside src/univalue/**, which violates the repository rule forbidding edits to vendored dependencies. Please either (a) keep vendor code untouched and adapt callers externally, or (b) document/approve an explicit exception for this backport.
As per coding guidelines: src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue,crypto/{ctaes,x11}}/**: “Do not make changes to vendored dependencies…”.
🤖 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/univalue/include/univalue.h` at line 90, The modification to
src/univalue/include/univalue.h violates the repository rule forbidding changes
to vendored dependencies. The pushKVEnd method addition at line 90 modifies
vendored UniValue code which must remain untouched. To fix this, either revert
the changes to src/univalue/** and instead adapt all external callers to work
with the original UniValue API, or obtain explicit documented approval from the
maintainers for an exception to the vendored code policy before proceeding with
this backport.
Source: Coding guidelines
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): Two blocking issues in the backport test adaptations. (1) The bitcoin#28671 backport is incomplete: mocktime was added in test_addpeeraddress, but the paired assert_approx→assert_equal change in check_addr_information was dropped, so the test remains as flaky as it was upstream (and arguably worse, since the mocked time is now frozen while the wall-clock comparison keeps advancing). (2) The bitcoin#26638 prefer-sqlite change flipped the default wallet type to descriptors in the test framework, which breaks the Dash-specific basic2 block in wallet_migration.py — createwallet(blank=True) now produces a descriptor wallet, but the immediate sethdseed call is legacy-only. One nitpick: duplicate class AddrInfo; forward declaration in addrman.h. A soft bitcoin#26485 prerequisite claim from the Codex backport-reviewer was dropped — Dash has no remaining __pushKV references and the rename is complete for the current tree.
🔴 5 blocking | 💬 1 nitpick(s)
2 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 `test/functional/rpc_net.py`:
- [BLOCKING] test/functional/rpc_net.py:453-454: bitcoin#28671 backport is incomplete: time assertion still uses wall-clock, mocktime is dead code
The upstream PR bitcoin#28671 has two paired changes: (1) freeze the addr's nTime by setting mocktime before `addpeeraddress`, and (2) compare the returned time against that frozen mocktime instead of wall-clock `time.time()`. Commit 74c2e6f097 only applied (1) — `self.addr_time = int(time.time())` and `node.setmocktime(self.addr_time)` at lines 359-360 — but left line 454 as `assert_approx(result["time"], time.time(), vspan=10)`, and `self.addr_time` is never referenced again. The mocktime setup is therefore dead code, and the test is at least as flaky on slow runners as it was before the cherry-pick: addrman entries now have a frozen `nTime` equal to `self.addr_time`, while `time.time()` keeps advancing through the subsequent RPC steps. On slow CI (the s390x/qemu case the upstream PR targets), the delta will eventually exceed 10s and fail deterministically. Drop the partial change or finish it by comparing against `self.addr_time`.
- [BLOCKING] test/functional/rpc_net.py:453-454: Incomplete cherry-pick of bitcoin#28671: missing assert_equal replacement
Upstream PR #28671 has two paired changes:
1. Set mocktime so the addr's `nTime` is deterministic (`self.addr_time = int(time.time())` + `node.setmocktime(self.addr_time)`).
2. Replace the flaky `assert_approx(result["time"], time.time(), vspan=10)` with `assert_equal(result["time"], self.addr_time)`.
Dash's merge commit 74c2e6f097 only applied change (1). The second hunk was dropped during the cherry-pick. The relevant code at `test/functional/rpc_net.py:453-454` still reads:
To avoid failing on slow test runners, use a 10s vspan here.
assert_approx(result["time"], time.time(), vspan=10)
Upstream diff for the same region:
-
# To avoid failing on slow test runners, use a 10s vspan here. -
assert_approx(result["time"], time.time(), vspan=10)
-
assert_equal(result["time"], self.addr_time)
Consequences:
- The PR's stated purpose ("Fix failing time check") is not actually achieved — the original `assert_approx(... vspan=10)` flakiness on slow runners remains.
- With `setmocktime` now in effect, `result["time"]` is frozen at `self.addr_time`, but the assertion still compares against the wall-clock `time.time()`, which keeps advancing. On a sufficiently slow runner the delta will exceed 10s and the test will fail more deterministically than before the cherry-pick.
Fix: drop lines 453-454 and replace with `assert_equal(result["time"], self.addr_time)` (matches upstream exactly). Evidence: `git diff f4049eaf08^1...f4049eaf08 -- test/functional/rpc_net.py` shows the missing hunk; `git diff 74c2e6f097~1...74c2e6f097 -- test/functional/rpc_net.py` does not contain it.
---
**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] test/functional/rpc_net.py:453-454: Incomplete conflict resolution: bitcoin#28671
bitcoin#28671's actual fix is to compare getrawaddrman entry times against the mocked timestamp captured before addpeeraddress. Upstream f4049eaf08 changes the assertion to assert_equal(result["time"], self.addr_time). Dash's backport adds self.addr_time and node.setmocktime(self.addr_time) in test_addpeeraddress, but leaves check_addr_information using assert_approx(result["time"], time.time(), vspan=10). That preserves the slow-runner failure the upstream PR fixed: if enough time passes between insertion and validation, the addrman entry time is the mocked value, not current wall time.
---
**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.
In `test/functional/wallet_migration.py`:
- [BLOCKING] test/functional/wallet_migration.py:178-180: basic2 must be created as a legacy wallet — prefer-sqlite default breaks the subsequent sethdseed
bitcoin#26638 flips the wallet-test framework default from BDB/legacy to SQLite/descriptors when SQLite is compiled in, and `WalletMigrationTest` requires SQLite. `TestNode.createwallet()` (test_framework/test_node.py:902-907) falls back to `self.descriptors` (= `self.options.descriptors`) when the caller does not pass an explicit `descriptors=` arg, so `self.nodes[0].createwallet(wallet_name="basic2", blank=True)` now creates a descriptor wallet. The next line, `basic2.sethdseed(True, BASIC2_SEED_WIF)`, is legacy-only (gated by `EnsureLegacyScriptPubKeyMan`) and will raise before the test reaches the migration scenario. The upstream PR converted the adjacent `multisig1`, `imports0`, `watchonly0`, and `watchonly1` blocks to `self.create_legacy_wallet(...)` for exactly this reason, but the Dash-specific `basic2` block (added in the bitcoin#19602 backport) was not adapted in this PR. Route the basic2 creation through `create_legacy_wallet` so it stays BDB regardless of the framework default.
In `src/rpc/server.cpp`:
- [BLOCKING] src/rpc/server.cpp:407-455: Missing prerequisite: bitcoin#26485
bitcoin#27822 renamed UniValue::__pushKV to pushKVEnd in every upstream call site, including src/rpc/server.cpp's named-only RPC option handling: upstream changed options.__pushKV(fr->first, *fr->second) to options.pushKVEnd(fr->first, *fr->second). Dash's transformNamedArguments still takes std::vector<std::string> and has no named_only/options branch, so that upstream hunk is absent. Tracing the missing block with git log -S "named_only" points to 34ac3f438a, Merge bitcoin/bitcoin#26485: RPC: Accept options as named-only parameters. This is soft because Dash has no remaining __pushKV references and the rename itself compiles, but it is still missing upstream content touched by the backported PR.
---
**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.
| # To avoid failing on slow test runners, use a 10s vspan here. | ||
| assert_approx(result["time"], time.time(), vspan=10) |
There was a problem hiding this comment.
🔴 Blocking: bitcoin#28671 backport is incomplete: time assertion still uses wall-clock, mocktime is dead code
The upstream PR bitcoin#28671 has two paired changes: (1) freeze the addr's nTime by setting mocktime before addpeeraddress, and (2) compare the returned time against that frozen mocktime instead of wall-clock time.time(). Commit 74c2e6f only applied (1) — self.addr_time = int(time.time()) and node.setmocktime(self.addr_time) at lines 359-360 — but left line 454 as assert_approx(result["time"], time.time(), vspan=10), and self.addr_time is never referenced again. The mocktime setup is therefore dead code, and the test is at least as flaky on slow runners as it was before the cherry-pick: addrman entries now have a frozen nTime equal to self.addr_time, while time.time() keeps advancing through the subsequent RPC steps. On slow CI (the s390x/qemu case the upstream PR targets), the delta will eventually exceed 10s and fail deterministically. Drop the partial change or finish it by comparing against self.addr_time.
| # To avoid failing on slow test runners, use a 10s vspan here. | |
| assert_approx(result["time"], time.time(), vspan=10) | |
| assert_equal(result["time"], self.addr_time) |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — bitcoin#28671 backport is incomplete: time assertion still uses wall-clock, mocktime is dead code 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.
| class AddrInfo; | ||
| class AddrManImpl; | ||
| class AddrInfo; |
There was a problem hiding this comment.
💬 Nitpick: Duplicate forward declaration of class AddrInfo
The bitcoin#28523 backport added class AddrInfo; at line 39 right after class AddrManImpl;, but class AddrInfo; already appears at line 37. Harmless to the compiler, but clearly a merge-resolution artifact worth removing.
| class AddrInfo; | |
| class AddrManImpl; | |
| class AddrInfo; | |
| class AddrInfo; | |
| class AddrManImpl; |
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — Duplicate forward declaration of class AddrInfo 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.
| # To avoid failing on slow test runners, use a 10s vspan here. | ||
| assert_approx(result["time"], time.time(), vspan=10) |
There was a problem hiding this comment.
🔴 Blocking: Incomplete cherry-pick of bitcoin#28671: missing assert_equal replacement
Upstream PR bitcoin#28671 has two paired changes:
- Set mocktime so the addr's
nTimeis deterministic (self.addr_time = int(time.time())+node.setmocktime(self.addr_time)). - Replace the flaky
assert_approx(result["time"], time.time(), vspan=10)withassert_equal(result["time"], self.addr_time).
Dash's merge commit 74c2e6f only applied change (1). The second hunk was dropped during the cherry-pick. The relevant code at test/functional/rpc_net.py:453-454 still reads:
# To avoid failing on slow test runners, use a 10s vspan here.
assert_approx(result["time"], time.time(), vspan=10)
Upstream diff for the same region:
- # To avoid failing on slow test runners, use a 10s vspan here.
- assert_approx(result["time"], time.time(), vspan=10)
+ assert_equal(result["time"], self.addr_time)
Consequences:
- The PR's stated purpose ("Fix failing time check") is not actually achieved — the original
assert_approx(... vspan=10)flakiness on slow runners remains. - With
setmocktimenow in effect,result["time"]is frozen atself.addr_time, but the assertion still compares against the wall-clocktime.time(), which keeps advancing. On a sufficiently slow runner the delta will exceed 10s and the test will fail more deterministically than before the cherry-pick.
Fix: drop lines 453-454 and replace with assert_equal(result["time"], self.addr_time) (matches upstream exactly). Evidence: git diff f4049eaf08^1...f4049eaf08 -- test/functional/rpc_net.py shows the missing hunk; git diff 74c2e6f097~1...74c2e6f097 -- test/functional/rpc_net.py does not contain it.
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.
| # To avoid failing on slow test runners, use a 10s vspan here. | |
| assert_approx(result["time"], time.time(), vspan=10) | |
| - # To avoid failing on slow test runners, use a 10s vspan here. | |
| - assert_approx(result["time"], time.time(), vspan=10) | |
| + assert_equal(result["time"], self.addr_time) |
source: ['claude-backport-reviewer']
There was a problem hiding this comment.
Resolved in this update — Incomplete cherry-pick of bitcoin#28671: missing assert_equal replacement 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.
| # To avoid failing on slow test runners, use a 10s vspan here. | ||
| assert_approx(result["time"], time.time(), vspan=10) |
There was a problem hiding this comment.
🔴 Blocking: Incomplete conflict resolution: bitcoin#28671
bitcoin#28671's actual fix is to compare getrawaddrman entry times against the mocked timestamp captured before addpeeraddress. Upstream f4049ea changes the assertion to assert_equal(result["time"], self.addr_time). Dash's backport adds self.addr_time and node.setmocktime(self.addr_time) in test_addpeeraddress, but leaves check_addr_information using assert_approx(result["time"], time.time(), vspan=10). That preserves the slow-runner failure the upstream PR fixed: if enough time passes between insertion and validation, the addrman entry time is the mocked value, not current wall time.
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']
There was a problem hiding this comment.
Resolved in this update — Incomplete conflict resolution: bitcoin#28671 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
force-pushed
the
June_2026_4
branch
3 times, most recently
from
5bc72fc to
3c47eb7
Compare
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): PR #7358's title declares 5 Bitcoin backports (28523, 26638, 28976, 27822, 28671) but the head at 3c47eb7 contains merge commits for only 3 of them — bitcoin#26638 (test: prefer sqlite for wallet tests) and bitcoin#28976 (wallet: fix migration of blank wallets) are missing entirely. The 3 present backports also have an intra-PR ordering hazard: the bitcoin#28523 cherry-pick uses UniValue::pushKVEnd, which is not introduced until the following bitcoin#27822 cherry-pick, so the intermediate commit is not bisect-buildable. Final-tree behavior compiles and runs correctly, but the PR does not match its stated scope.
🔴 7 blocking | 🟡 1 suggestion(s)
5 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wallet/wallet.cpp`:
- [BLOCKING] src/wallet/wallet.cpp:1: PR title declares 5 backports but branch contains only 3 — bitcoin#26638 and bitcoin#28976 are missing
The PR title is `backport: Merge bitcoin#28523, 26638, 28976, 27822, 28671`, but `git log develop..3c47eb777a` shows only three Bitcoin merge commits: a5264f984a (#28523), c34e457774 (#27822), 3c47eb777a (#28671). The #26638 (test: prefer sqlite for wallet tests) and #28976 (wallet: Fix migration of blank wallets) merge commits are not present on this head — they exist on a sibling branch (`dac3c8d477`, `0c247480bc`) but were not pushed here.
Verification of the Dash tree at HEAD confirms the upstream behavior changes from those two PRs are absent:
- `test/functional/test_framework/test_framework.py` still prefers BDB over SQLite (no #26638 adaptation).
- `src/wallet/wallet.cpp` still gates the migration pre-check on `GetLegacyScriptPubKeyMan()` rather than `WALLET_FLAG_DESCRIPTORS`, and unconditionally runs `DoMigration` for blank legacy wallets (no #28976 fix). The upstream blank-wallet migration bug remains.
Either restore the two missing cherry-picks to this branch or update the PR title/description to accurately reflect the three backports that are actually included.
- [BLOCKING] src/wallet/wallet.cpp:4745-4794: Missing upstream backport: bitcoin#28976
The PR title includes bitcoin#28976, but at requested head 3c47eb777a the commit is not in upstream/develop..HEAD and the wallet migration fix is absent. Upstream #28976 changed the pre-migration check from GetLegacyScriptPubKeyMan() to WALLET_FLAG_DESCRIPTORS and skips DoMigration for WALLET_FLAG_BLANK_WALLET so blank legacy wallets can migrate. Dash still has the pre-PR code: it treats a missing legacy ScriptPubKeyMan as already-descriptor and always calls DoMigration after SQLite migration, so the upstream blank-wallet migration bug remains.
---
**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.
In `src/rpc/net.cpp`:
- [SUGGESTION] src/rpc/net.cpp:1202: Reorder cherry-picks: bitcoin#28523 uses pushKVEnd before bitcoin#27822 introduces it (bisect hazard)
The series orders the merges as a5264f984a (bitcoin#28523) → c34e457774 (bitcoin#27822) → 3c47eb777a (bitcoin#28671). Bitcoin#28523's new `AddrmanTableToJSON` calls `table.pushKVEnd(...)` (src/rpc/net.cpp:1202), but `UniValue::pushKVEnd` is introduced only by bitcoin#27822, which is the next commit in this PR.
Verified: `git show a5264f984a:src/univalue/include/univalue.h` still declares only `__pushKV`, not `pushKVEnd`. Commit a5264f984a therefore does not compile in isolation, breaking `git bisect` across these two commits. The final HEAD is fine because 27822 is applied.
Upstream chronological order is bitcoin#27822 (2023-06-21) → bitcoin#28523 (2023-10-03). Please reorder the cherry-picks to match upstream so every intermediate commit builds. (If you also restore #26638/#28976 per the other finding, this is a good opportunity to fix the order while re-pushing.)
- [BLOCKING] src/rpc/net.cpp:1202: Commit ordering: 28523 uses pushKVEnd before 27822 introduces it (bisect hazard)
Within this PR the commits are ordered a5264f984a (Merge bitcoin#28523) → c34e457774 (Merge bitcoin#27822) → 3c47eb777a (Merge bitcoin#28671). bitcoin#28523's new `AddrmanTableToJSON` calls `table.pushKVEnd(...)` (src/rpc/net.cpp:1202), but `pushKVEnd` is only introduced by bitcoin#27822, which is the *next* commit. Verified: `git show a5264f984a:src/univalue/include/univalue.h` shows only `__pushKV`, not `pushKVEnd`. Result: commit a5264f984a does not compile in isolation. The final tree at HEAD is fine (27822 is applied), and runtime behavior is unaffected, but this breaks `git bisect` between these two commits. Upstream chronological order is 27822 (2023-06-21) → 28523 (2023-10-03); please reorder the cherry-picks to match.
---
**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] src/rpc/net.cpp:1: PR title references 5 backports but branch contains only 3
PR #7358 title declares `bitcoin#28523, 26638, 28976, 27822, 28671`. The head SHA 3c47eb777a only includes 28523, 27822, and 28671. Sibling branch `pr-review-7358` (different head) does contain 26638 and 28976. Confirm whether 26638 and 28976 are intentionally dropped from this head or if a rebase/push lost them. If dropped intentionally, update the PR title; if not, restore the missing cherry-picks. (No prerequisite analysis was performed on 26638/28976 since they're not in the reviewed tree.)
---
**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] src/rpc/net.cpp:1202: Missing prerequisite before cherry-pick: bitcoin#27822
Dash commit a5264f984a, the bitcoin#28523 backport, adds a call to table.pushKVEnd(...). In upstream, bitcoin#28523's parent already had UniValue::pushKVEnd because bitcoin#27822 was merged earlier. In Dash, a5264f984a^ still declares only UniValue::__pushKV, and pushKVEnd is introduced later by c34e457774. This means the dependency chain is present only in the final head, not at the cherry-pick point; the 28523 commit itself is unbuildable. The same commit also adds a test using time.time() before Dash adds the import in the later 28671 adaptation.
---
**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.
In `test/functional/test_framework/test_framework.py`:
- [BLOCKING] test/functional/test_framework/test_framework.py:259-263: Missing upstream backport: bitcoin#26638
The PR title includes bitcoin#26638, but at requested head 3c47eb777a the commit is not in upstream/develop..HEAD and the hunks are absent. Upstream #26638 changed the wallet test framework to prefer SQLite over BDB and updated wallet_migration.py's create_legacy_wallet helper to explicitly create legacy BDB wallets with descriptors=False. Dash still has the pre-PR behavior here: Prefer BDB unless unavailable, and wallet_migration.py still calls createwallet(wallet_name=wallet_name) without the upstream helper changes.
---
**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.
In `src/rpc/server.cpp`:
- [BLOCKING] src/rpc/server.cpp:432-450: Missing prerequisite: bitcoin#26485
Upstream bitcoin#27822 renamed an __pushKV use inside transformNamedArguments' named-only RPC parameter handling. Dash does not have that block: current code still iterates over const std::string& argNamePattern and lacks the named_only/options handling present in upstream's pre-27822 state. That upstream block was introduced by bitcoin#26485 (OBJ_NAMED_PARAMS). This is a soft prerequisite gap because the rename backport has no remaining __pushKV use in Dash, but a full upstream-state comparison shows the modified section depends on #26485.
---
**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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🟡 Suggestion: Reorder cherry-picks: bitcoin#28523 uses pushKVEnd before bitcoin#27822 introduces it (bisect hazard)
The series orders the merges as a5264f9 (bitcoin#28523) → c34e457 (bitcoin#27822) → 3c47eb7 (bitcoin#28671). bitcoin#28523's new AddrmanTableToJSON calls table.pushKVEnd(...) (src/rpc/net.cpp:1202), but UniValue::pushKVEnd is introduced only by bitcoin#27822, which is the next commit in this PR.
Verified: git show a5264f984a:src/univalue/include/univalue.h still declares only __pushKV, not pushKVEnd. Commit a5264f9 therefore does not compile in isolation, breaking git bisect across these two commits. The final HEAD is fine because 27822 is applied.
Upstream chronological order is bitcoin#27822 (2023-06-21) → bitcoin#28523 (2023-10-03). Please reorder the cherry-picks to match upstream so every intermediate commit builds. (If you also restore bitcoin#26638/bitcoin#28976 per the other finding, this is a good opportunity to fix the order while re-pushing.)
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Reorder cherry-picks: bitcoin#28523 uses pushKVEnd before bitcoin#27822 introduces it (bisect hazard) 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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🔴 Blocking: Commit ordering: 28523 uses pushKVEnd before 27822 introduces it (bisect hazard)
Within this PR the commits are ordered a5264f9 (Merge bitcoin#28523) → c34e457 (Merge bitcoin#27822) → 3c47eb7 (Merge bitcoin#28671). bitcoin#28523's new AddrmanTableToJSON calls table.pushKVEnd(...) (src/rpc/net.cpp:1202), but pushKVEnd is only introduced by bitcoin#27822, which is the next commit. Verified: git show a5264f984a:src/univalue/include/univalue.h shows only __pushKV, not pushKVEnd. Result: commit a5264f9 does not compile in isolation. The final tree at HEAD is fine (27822 is applied), and runtime behavior is unaffected, but this breaks git bisect between these two commits. Upstream chronological order is 27822 (2023-06-21) → 28523 (2023-10-03); please reorder the cherry-picks to match.
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 — Commit ordering: 28523 uses pushKVEnd before 27822 introduces it (bisect hazard) 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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite before cherry-pick: bitcoin#27822
Dash commit a5264f9, the bitcoin#28523 backport, adds a call to table.pushKVEnd(...). In upstream, bitcoin#28523's parent already had UniValue::pushKVEnd because bitcoin#27822 was merged earlier. In Dash, a5264f9^ still declares only UniValue::__pushKV, and pushKVEnd is introduced later by c34e457. This means the dependency chain is present only in the final head, not at the cherry-pick point; the 28523 commit itself is unbuildable. The same commit also adds a test using time.time() before Dash adds the import in the later 28671 adaptation.
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']
There was a problem hiding this comment.
Resolved in this update — Missing prerequisite before cherry-pick: bitcoin#27822 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.
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): Verified 5-PR Bitcoin backport (28523, 27822, 28671, 29114, 28037). Final tree is coherent; merge resolutions are clean. Two Codex 'missing prerequisite' findings (bitcoin#26638, bitcoin#28976) were based on a misread of the PR title — the title only lists the five PRs that are actually present, so those are dropped. Verified real issue: commit ordering inside the series breaks bisectability — the 28523 backport commit calls UniValue::pushKVEnd (introduced by the later 27822 commit) and uses time.time() in rpc_net.py without an import (added by the later 28671 commit), so a5264f9 cannot build/run in isolation. Final HEAD is correct; flagging as a suggestion to reorder.
🔴 5 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
4 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 `src/rpc/net.cpp`:
- [SUGGESTION] src/rpc/net.cpp:1202: Commit ordering breaks intermediate-state build: 28523 lands before 27822 (pushKVEnd) and 28671 (import time)
The series orders the 28523 backport (a5264f984a) before the 27822 UniValue rename (83c56b97f5) and the 28671 rpc_net fix (af0874909a), but the 28523 commit already depends on both of those:
1. `src/rpc/net.cpp:1202` calls `table.pushKVEnd(key.str(), AddrmanEntryToJSON(info));`, while at a5264f984a `src/univalue/include/univalue.h` still declares only `__pushKV` (line 90). The build fails at this intermediate commit; `pushKVEnd` is only added by the next commit (83c56b97f5 / bitcoin#27822).
2. `test/functional/rpc_net.py` at a5264f984a uses `time.time()` (line 451) but the file imports only `platform` and `from itertools import product` — no `import time`. The import is added two commits later by the 28671 backport. Running the test at this revision raises `NameError: name 'time' is not defined`.
The final HEAD (42c08e1653) is self-consistent, so this only affects bisectability and commit-by-commit builds, not the merged result. Upstream had no such issue because bitcoin#27822 (Jun 2023) merged before bitcoin#28523 (Oct 2023). Easiest fix: reorder the backport commits so 27822 lands before 28523 (and pull the `import time` re-add or the 28671 commit forward as well). Alternatively, adapt the 28523 merge to use `__pushKV` plus add `import time`, and let the later renames apply on top.
- [BLOCKING] src/rpc/net.cpp:1202: Commit ordering breaks build at intermediate revision: 28523 uses pushKVEnd before 27822 renames it
Commit a5264f984a (Merge bitcoin#28523) adds `table.pushKVEnd(key.str(), AddrmanEntryToJSON(info));` to src/rpc/net.cpp, but at that revision src/univalue/include/univalue.h still declares only `__pushKV` (line 90 at a5264f984a):
void __pushKV(std::string key, UniValue val);
void pushKV(std::string key, UniValue val);
void pushKVs(UniValue obj);
`pushKVEnd` is introduced by the very next commit (83c56b97f5, scripted-diff for bitcoin#27822), so the build is broken at a5264f984a in isolation. Upstream did not have this problem because bitcoin#27822 was merged (Jun 2023) before bitcoin#28523 (Oct 2023). In this PR the order is reversed.
Not blocking — once all 5 commits are applied, the final tree is self-consistent (univalue.h declares `pushKVEnd`, all callers use the new name). But this breaks `git bisect` across this PR and any developer who builds at intermediate revisions. Either move the 27822 backport before the 28523 backport, or change 28523's adaptation to use `__pushKV` and let the scripted-diff rename it.
---
**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.
In `test/functional/rpc_net.py`:
- [BLOCKING] test/functional/rpc_net.py:499: Test file references `time.time()` without `import time` at intermediate commit (28523 before 28671)
Commit a5264f984a adds `assert_approx(result["time"], time.time(), vspan=10)` to test/functional/rpc_net.py, but at that revision the file does not `import time` (only `from itertools import product` and `import platform`). The `import time` is only added two commits later by the bitcoin#28671 backport (af0874909a), which also rewrites the assertion to use `setmocktime`.
Result: at commit a5264f984a, running rpc_net.py raises `NameError: name 'time' is not defined`. Same bisectability concern as the pushKVEnd issue — non-blocking per the agent (restored to blocking by policy), but worth aligning. Easiest fix: add the `import time` in the 28523 backport (it's an upstream-existing import that just wasn't carried over because Dash had previously dropped it), independent of the later 28671 rewrite.
---
**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.
In `src/addrman.h`:
- [BLOCKING] src/addrman.h:38-39: 28523 backport introduced a duplicate `class AddrInfo;` forward declaration; cleanup smuggled into 28671 backport
In commit a5264f984a (bitcoin#28523), src/addrman.h gains a second `class AddrInfo;` forward declaration right after `class AddrManImpl;`. The pre-existing declaration came from a prior backport (bitcoin#22950 — Pimpl AddrMan), so the line upstream 28523 added is a no-op duplicate in Dash and should have been omitted during the merge resolution.
The duplicate is silently removed by commit af0874909a (bitcoin#28671) even though upstream 28671 only touches test/functional/rpc_net.py. Mixing this cleanup into an unrelated backport is mildly confusing and could be miscredited. Pure cosmetics (duplicate forward decls are legal C++), but ideally the duplicate would have been avoided in the 28523 merge resolution itself rather than fixed up in 28671.
---
**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.
In `test/functional/test_framework/test_framework.py`:
- [BLOCKING] test/functional/test_framework/test_framework.py:259-263: Missing listed backport: bitcoin#26638
The PR title lists bitcoin#26638, but the head does not contain that cherry-pick. Upstream bitcoin#26638 changes the functional test framework to prefer SQLite wallets when available, and updates wallet_migration.py to explicitly create legacy BDB wallets with descriptors=False. Dash still has the pre-PR state here: the framework prefers BDB first, and wallet_migration.py:create_legacy_wallet still calls createwallet() without descriptors=False. This is a soft completeness issue unless the title is corrected to remove bitcoin#26638.
---
**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.
In `src/wallet/wallet.cpp`:
- [BLOCKING] src/wallet/wallet.cpp:4403-4404: Missing listed backport: bitcoin#28976
The PR title lists bitcoin#28976, but the head does not contain that cherry-pick. Upstream bitcoin#28976 fixes migration of blank wallets by replacing the hard assert on GetLegacyScriptPubKeyMan() with an Assume/error path, checking WALLET_FLAG_DESCRIPTORS instead of absence of legacy SPKM, and skipping DoMigration for blank wallets. Dash still has the old assert here and still always calls DoMigration later in MigrateLegacyToDescriptor, so the listed upstream bug fix is absent. Either include bitcoin#28976 or correct the PR title if this PR is actually intended to backport bitcoin#29114 and bitcoin#28037 instead.
---
**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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🟡 Suggestion: Commit ordering breaks intermediate-state build: 28523 lands before 27822 (pushKVEnd) and 28671 (import time)
The series orders the 28523 backport (a5264f9) before the 27822 UniValue rename (83c56b9) and the 28671 rpc_net fix (af08749), but the 28523 commit already depends on both of those:
-
src/rpc/net.cpp:1202callstable.pushKVEnd(key.str(), AddrmanEntryToJSON(info));, while at a5264f9src/univalue/include/univalue.hstill declares only__pushKV(line 90). The build fails at this intermediate commit;pushKVEndis only added by the next commit (83c56b9 / Renamed UniValue::__pushKV to UniValue::pushKVEnd. bitcoin/bitcoin#27822). -
test/functional/rpc_net.pyat a5264f9 usestime.time()(line 451) but the file imports onlyplatformandfrom itertools import product— noimport time. The import is added two commits later by the 28671 backport. Running the test at this revision raisesNameError: name 'time' is not defined.
The final HEAD (42c08e1) is self-consistent, so this only affects bisectability and commit-by-commit builds, not the merged result. Upstream had no such issue because bitcoin#27822 (Jun 2023) merged before bitcoin#28523 (Oct 2023). Easiest fix: reorder the backport commits so 27822 lands before 28523 (and pull the import time re-add or the 28671 commit forward as well). Alternatively, adapt the 28523 merge to use __pushKV plus add import time, and let the later renames apply on top.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Commit ordering breaks intermediate-state build: 28523 lands before 27822 (pushKVEnd) and 28671 (import time) 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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🔴 Blocking: Commit ordering breaks build at intermediate revision: 28523 uses pushKVEnd before 27822 renames it
Commit a5264f9 (Merge bitcoin#28523) adds table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); to src/rpc/net.cpp, but at that revision src/univalue/include/univalue.h still declares only __pushKV (line 90 at a5264f9):
void __pushKV(std::string key, UniValue val);
void pushKV(std::string key, UniValue val);
void pushKVs(UniValue obj);
pushKVEnd is introduced by the very next commit (83c56b9, scripted-diff for bitcoin#27822), so the build is broken at a5264f9 in isolation. Upstream did not have this problem because bitcoin#27822 was merged (Jun 2023) before bitcoin#28523 (Oct 2023). In this PR the order is reversed.
Not blocking — once all 5 commits are applied, the final tree is self-consistent (univalue.h declares pushKVEnd, all callers use the new name). But this breaks git bisect across this PR and any developer who builds at intermediate revisions. Either move the 27822 backport before the 28523 backport, or change 28523's adaptation to use __pushKV and let the scripted-diff rename it.
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 — Commit ordering breaks build at intermediate revision: 28523 uses pushKVEnd before 27822 renames it 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.
| "port": 8333, | ||
| "services": 1, | ||
| "network": "ipv4", | ||
| "source": "1.2.3.4", |
There was a problem hiding this comment.
🔴 Blocking: Test file references time.time() without import time at intermediate commit (28523 before 28671)
Commit a5264f9 adds assert_approx(result["time"], time.time(), vspan=10) to test/functional/rpc_net.py, but at that revision the file does not import time (only from itertools import product and import platform). The import time is only added two commits later by the bitcoin#28671 backport (af08749), which also rewrites the assertion to use setmocktime.
Result: at commit a5264f9, running rpc_net.py raises NameError: name 'time' is not defined. Same bisectability concern as the pushKVEnd issue — non-blocking per the agent (restored to blocking by policy), but worth aligning. Easiest fix: add the import time in the 28523 backport (it's an upstream-existing import that just wasn't carried over because Dash had previously dropped it), independent of the later 28671 rewrite.
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 — Test file references time.time() without import time at intermediate commit (28523 before 28671) 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.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rpc_net.py`:
- Line 473: Replace the list(filter(...)))[0] pattern on line 473 with
next(filter(...)) to avoid raising an uninformative IndexError when no matching
entry is found. Use next() with an explicit default value or assertion after it
to provide clearer failure behavior when filtering for the entry with matching
address in table_info["entries"]. This approach aligns better with Ruff's
guidance and improves debugging clarity.
🪄 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: d5f8d549-cd7f-45ca-b4c3-f51caf4d1bb6
📒 Files selected for processing (19)
src/addrman.cppsrc/addrman.hsrc/addrman_impl.hsrc/rpc/client.cppsrc/rpc/mempool.cppsrc/rpc/net.cppsrc/serialize.hsrc/span.hsrc/test/fuzz/rpc.cppsrc/test/settings_tests.cppsrc/txorphanage.cppsrc/txorphanage.hsrc/univalue/include/univalue.hsrc/univalue/lib/univalue.cppsrc/univalue/test/object.cppsrc/util/settings.cppsrc/wallet/rpc/addresses.cppsrc/wallet/rpc/wallet.cpptest/functional/rpc_net.py
✅ Files skipped from review due to trivial changes (1)
- src/wallet/rpc/wallet.cpp
🚧 Files skipped from review as they are similar to previous changes (17)
- src/test/fuzz/rpc.cpp
- src/util/settings.cpp
- src/test/settings_tests.cpp
- src/rpc/client.cpp
- src/span.h
- src/addrman_impl.h
- src/rpc/mempool.cpp
- src/addrman.h
- src/wallet/rpc/addresses.cpp
- src/univalue/include/univalue.h
- src/txorphanage.h
- src/univalue/test/object.cpp
- src/rpc/net.cpp
- src/txorphanage.cpp
- src/serialize.h
- src/addrman.cpp
- src/univalue/lib/univalue.cpp
| assert 0 <= int(position) < ADDRMAN_BUCKET_SIZE | ||
|
|
||
| entry = getrawaddrman[table_name][bucket_position] | ||
| expected_entry = list(filter(lambda e: e["address"] == entry["address"], table_info["entries"]))[0] |
There was a problem hiding this comment.
Use next(...) with an explicit assertion instead of list(filter(...))[0].
Line 473 can raise a less-informative IndexError when no match is found. Using next(...) gives clearer failure behavior and aligns with Ruff’s guidance.
Suggested change
- expected_entry = list(filter(lambda e: e["address"] == entry["address"], table_info["entries"]))[0]
+ expected_entry = next((e for e in table_info["entries"] if e["address"] == entry["address"]), None)
+ assert expected_entry is not None, f"Unexpected address {entry['address']} in {table_name}"
check_addr_information(entry, expected_entry)🧰 Tools
🪛 Ruff (0.15.17)
[warning] 473-473: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
🤖 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/rpc_net.py` at line 473, Replace the list(filter(...)))[0]
pattern on line 473 with next(filter(...)) to avoid raising an uninformative
IndexError when no matching entry is found. Use next() with an explicit default
value or assertion after it to provide clearer failure behavior when filtering
for the entry with matching address in table_info["entries"]. This approach
aligns better with Ruff's guidance and improves debugging clarity.
Source: Linters/SAST tools
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): Five Bitcoin backports (bitcoin#28523, 27822, 28671, 29114, 28037). Cumulative HEAD compiles correctly, but commit ordering still leaves two intermediate commits broken: bitcoin#28523 uses post-27822 API names (UniValue::pushKVEnd) and post-28671 fixes (import time / setmocktime) before those landing commits arrive. This breaks per-commit CI and git bisect. Also, the 27822 scripted-diff includes renames belonging to a different upstream PR.
🔴 8 blocking | 💬 2 nitpick(s)
4 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/rpc/net.cpp`:
- [BLOCKING] src/rpc/net.cpp:1202: Commit 34a9f9e1e7 (bitcoin#28523) calls UniValue::pushKVEnd before bitcoin#27822 renames __pushKV
Verified at commit 34a9f9e1e7: `src/rpc/net.cpp:1202` calls `table.pushKVEnd(key.str(), AddrmanEntryToJSON(info));`, but at the same revision `src/univalue/include/univalue.h:90` still declares only `void __pushKV(...)` — `pushKVEnd` is introduced one commit later in `54a0489e5d` (bitcoin#27822). The cumulative HEAD compiles, but per-commit CI and `git bisect` will fail at 34a9f9e1e7 with an unresolved symbol error.
Fix: reorder so the scripted-diff rename (27822) lands before 28523, or in the 28523 cherry-pick call `table.__pushKV(...)` and let 27822 rename it in the next commit.
- [BLOCKING] src/rpc/net.cpp:1202: Intermediate-state build break: bitcoin#28523 lands before bitcoin#27822 renames __pushKV -> pushKVEnd
Carried forward from prior review. The five backport commits were re-applied on the new develop base but ordering is unchanged: 34a9f9e1e7 (28523) precedes 54a0489e5d (27822). At 34a9f9e1e7, src/rpc/net.cpp:1202 calls `table.pushKVEnd(...)` while UniValue at that revision declares only `__pushKV`. Cumulative head is clean, but git bisect breaks in the intermediate window. Reorder bitcoin#27822 before bitcoin#28523, or fold the rename into 28523's adaptation. Subsumes prior finding 6.
---
**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.
In `test/functional/rpc_net.py`:
- [BLOCKING] test/functional/rpc_net.py:451: Commit 34a9f9e1e7 (bitcoin#28523) uses time.time() without `import time`; fix lives in bitcoin#28671
Verified at commit 34a9f9e1e7: `test/functional/rpc_net.py:451` calls `assert_approx(result["time"], time.time(), vspan=10)` but the file has no `import time` at that revision. Running the functional test at that intermediate commit raises `NameError: name 'time' is not defined`. The `import time` (and the deterministic `setmocktime` rewrite) only arrives in the next commit `b9a082cbd9` (bitcoin#28671), which upstream is the dedicated bugfix for this very issue.
Fix: reorder bitcoin#28671 to land immediately after (or before) 28523, or fold the `import time` adaptation directly into the 28523 cherry-pick so the test runs at every intermediate commit.
- [BLOCKING] test/functional/rpc_net.py:451: Intermediate-state test break: bitcoin#28523 uses time.time() without `import time`; fix comes from bitcoin#28671
Carried forward from prior review. At commit 34a9f9e1e7 (28523), rpc_net.py:451 references `time.time()` without `import time`. The import is added by the next commit b9a082cbd9 (28671). Cumulative head is correct, but the intermediate commit's test would NameError. Reorder bitcoin#28671 before bitcoin#28523, or include the `import time` change directly in the 28523 cherry-pick.
---
**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] test/functional/rpc_net.py:451: Commit ordering breaks intermediate test: bitcoin#28523 before bitcoin#28671
At intermediate commit 34a9f9e1e7, rpc_net.py calls time.time() in the getrawaddrman check but the file does not import time. The import and deterministic mock-time rewrite are added only later by b9a082cbd9, the bitcoin#28671 backport. This leaves the bitcoin#28523 commit with a NameError in the functional test.
---
**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.
In `src/addrman.h`:
- [BLOCKING] src/addrman.h:37-39: bitcoin#28523 backport adds duplicate `class AddrInfo;` forward declaration; cleanup smuggled into bitcoin#28671
Carried forward from prior review. At 34a9f9e1e7 (28523), src/addrman.h has duplicate `class AddrInfo;` forward declarations at lines 37 and 39 because Dash already had one from bitcoin#22950 / Pimpl AddrMan and the conflict resolution reintroduced it. The duplicate is removed in b9a082cbd9 (28671), even though upstream 28671 only touches test/functional/rpc_net.py — this Dash-only cleanup belongs inside the 28523 commit, not in an unrelated test-only backport. Subsumes prior finding 7.
---
**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] src/addrman.h:37-39: Conflict resolution smuggles addrman cleanup into bitcoin#28671
At intermediate commit 34a9f9e1e7, src/addrman.h contains class AddrInfo; class AddrManImpl; class AddrInfo;. Dash already had the AddrInfo forward declaration before bitcoin#28523, so the cherry-pick introduced a duplicate declaration. The duplicate is removed later in b9a082cbd9, even though upstream bitcoin#28671 only modifies test/functional/rpc_net.py. This cleanup belongs with the 28523 conflict resolution, not the later test-only 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.
In `src/rpc/server.cpp`:
- [BLOCKING] src/rpc/server.cpp:441-450: Missing prerequisite: bitcoin#26485
bitcoin#27822 upstream also renames an __pushKV call in transformNamedArguments' named-only RPC options branch, which was introduced by bitcoin#26485. Dash lacks that named_only/options branch, so the corresponding upstream hunk is omitted. This is non-blocking per the agent (restored to blocking by policy) for the current rename because Dash has no remaining __pushKV uses, but it is a soft missing prerequisite for a complete upstream #27822 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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🔴 Blocking: Commit 34a9f9e (bitcoin#28523) calls UniValue::pushKVEnd before bitcoin#27822 renames __pushKV
Verified at commit 34a9f9e: src/rpc/net.cpp:1202 calls table.pushKVEnd(key.str(), AddrmanEntryToJSON(info));, but at the same revision src/univalue/include/univalue.h:90 still declares only void __pushKV(...) — pushKVEnd is introduced one commit later in 54a0489e5d (bitcoin#27822). The cumulative HEAD compiles, but per-commit CI and git bisect will fail at 34a9f9e with an unresolved symbol error.
Fix: reorder so the scripted-diff rename (27822) lands before 28523, or in the 28523 cherry-pick call table.__pushKV(...) and let 27822 rename it in the next commit.
source: ['claude', 'codex']
| assert_equal(result["port"], expected["port"]) | ||
| assert_equal(result["services"], expected["services"]) | ||
| assert_equal(result["network"], expected["network"]) | ||
| assert_equal(result["source"], expected["source"]) |
There was a problem hiding this comment.
🔴 Blocking: Commit 34a9f9e (bitcoin#28523) uses time.time() without import time; fix lives in bitcoin#28671
Verified at commit 34a9f9e: test/functional/rpc_net.py:451 calls assert_approx(result["time"], time.time(), vspan=10) but the file has no import time at that revision. Running the functional test at that intermediate commit raises NameError: name 'time' is not defined. The import time (and the deterministic setmocktime rewrite) only arrives in the next commit b9a082cbd9 (bitcoin#28671), which upstream is the dedicated bugfix for this very issue.
Fix: reorder bitcoin#28671 to land immediately after (or before) 28523, or fold the import time adaptation directly into the 28523 cherry-pick so the test runs at every intermediate commit.
source: ['claude', 'codex']
|
|
||
| /** Erase an orphan by txid */ | ||
| int _EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); | ||
| int EraseTxNoLock(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); |
There was a problem hiding this comment.
💬 Nitpick: scripted-diff titled "Merge bitcoin#27822" also performs renames from bitcoin#27931
Verified: commit 54a0489e5d is titled scripted-diff: Merge bitcoin#27822 Renamed UniValue::__pushKV to UniValue::pushKVEnd, but the VERIFY SCRIPT also runs s '_EraseTx' 'EraseTxNoLock' and s '_Other' 'Other'. The _EraseTx symbol exists in Dash's tree (src/txorphanage.h:107 at the parent commit) — that rename is from upstream bitcoin#27931 (TxOrphanage refactor), not 27822. _Other matches nothing and is a no-op.
This means a separate upstream PR is being backported under another PR's name without acknowledgment in the title or description. Split the scripted-diff into one commit per upstream PR (so authorship and provenance are clear), or expand the commit message and PR title to acknowledge bitcoin#27931.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — scripted-diff titled "Merge bitcoin#27822" also performs renames from bitcoin#27931 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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🔴 Blocking: Intermediate-state build break: bitcoin#28523 lands before bitcoin#27822 renames __pushKV -> pushKVEnd
Carried forward from prior review. The five backport commits were re-applied on the new develop base but ordering is unchanged: 34a9f9e (28523) precedes 54a0489 (27822). At 34a9f9e, src/rpc/net.cpp:1202 calls table.pushKVEnd(...) while UniValue at that revision declares only __pushKV. Cumulative head is clean, but git bisect breaks in the intermediate window. Reorder bitcoin#27822 before bitcoin#28523, or fold the rename into 28523's adaptation. Subsumes prior finding 6.
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']
| assert_equal(result["port"], expected["port"]) | ||
| assert_equal(result["services"], expected["services"]) | ||
| assert_equal(result["network"], expected["network"]) | ||
| assert_equal(result["source"], expected["source"]) |
There was a problem hiding this comment.
🔴 Blocking: Intermediate-state test break: bitcoin#28523 uses time.time() without import time; fix comes from bitcoin#28671
Carried forward from prior review. At commit 34a9f9e (28523), rpc_net.py:451 references time.time() without import time. The import is added by the next commit b9a082c (28671). Cumulative head is correct, but the intermediate commit's test would NameError. Reorder bitcoin#28671 before bitcoin#28523, or include the import time change directly in the 28523 cherry-pick.
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']
| assert_equal(result["port"], expected["port"]) | ||
| assert_equal(result["services"], expected["services"]) | ||
| assert_equal(result["network"], expected["network"]) | ||
| assert_equal(result["source"], expected["source"]) |
There was a problem hiding this comment.
🔴 Blocking: Commit ordering breaks intermediate test: bitcoin#28523 before bitcoin#28671
At intermediate commit 34a9f9e, rpc_net.py calls time.time() in the getrawaddrman check but the file does not import time. The import and deterministic mock-time rewrite are added only later by b9a082c, the bitcoin#28671 backport. This leaves the bitcoin#28523 commit with a NameError in the functional test.
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']
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 415a728bd1
ℹ️ 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".
| # added. Incrementing the port changes the position in the new table bucket (bucket | ||
| # stays the same) and changes both the bucket and the position in the tried table. | ||
| while True: | ||
| if node.addpeeraddress(address=table_info["entries"][1]["address"], port=port, tried=table_name == "tried")["success"]: |
There was a problem hiding this comment.
When table_name == "tried", this uses addpeeraddress(...)["success"] as proof that the address reached the tried table, but the RPC currently sets success = true after adding to the new table and ignores a failed node.addrman->Good(address) move (src/rpc/net.cpp:1041-1045). If the chosen port collides with an existing tried bucket position—the exact case this loop is trying to avoid—the loop records the port and stops even though the address stayed in new, so the later getrawaddrman/getaddrmaninfo expectations become flaky; the test needs to verify the tried move or the RPC needs to report that failure.
Useful? React with 👍 / 👎.
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): Verified at head 415a728. Two carried-forward blocking commit-ordering issues remain: at a5264f9 the bitcoin#28523 backport introduces table.pushKVEnd(...) (src/rpc/net.cpp:1202) while UniValue still only declares __pushKV (univalue.h:90), and the new functional test calls time.time() (rpc_net.py:451) without import time — both produce a permanent intermediate build/test break on develop. One suggestion is carried forward: the duplicate class AddrInfo; cleanup is smuggled into the bitcoin#28671 test-fix commit instead of the bitcoin#28523 backport where the duplicate was introduced. Two prior findings are dropped: bitcoin#26485 as a prerequisite is OUTDATED (Dash's transformNamedArguments lacks the named_only/options branch, so the upstream rename hunk has no target in Dash — verified at src/rpc/server.cpp:407), and the alleged bitcoin#27931 smuggling in the scripted-diff is OUTDATED — upstream bitcoin#27822 is a single commit (bdea2bb) whose VERIFY SCRIPT performs all three substitutions (__pushKV, _EraseTx, _Other), so Dash's scripted-diff faithfully mirrors it. The latest delta (DKG/active-context refactor) is unrelated and does not affect these findings.
🔴 8 blocking | 🟡 1 suggestion(s)
6 additional finding(s) omitted (not in diff).
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/addrman.h`:
- [SUGGESTION] src/addrman.h:37-39: addrman.h cleanup smuggled into bitcoin#28671 instead of the bitcoin#28523 backport that introduced the duplicate
Carried forward and re-verified. At a5264f984a (bitcoin#28523), src/addrman.h:37-39 contains:
class AddrInfo;
class AddrManImpl;
class AddrInfo;
The duplicate `class AddrInfo;` is created because Dash already had a forward declaration from the prior Pimpl AddrMan backport, and the bitcoin#28523 merge resolution re-added it. Final HEAD is clean (duplicate removed), but the removal is performed by 5269052141 (Merge bitcoin#28671) whose upstream scope is solely a Python mocktime fix — the `addrman.h | 1 -` hunk is foreign to bitcoin#28671 and pollutes the audit trail/`git blame`/bisect for addrman. Not a build break, but poor backport hygiene. Fix: fold the duplicate-decl removal into the bitcoin#28523 merge resolution where the duplicate was introduced.
- [BLOCKING] src/addrman.h:37-39: Duplicate `class AddrInfo;` forward declaration at the 28523 commit; cleanup smuggled into 28671 (carried forward, STILL VALID; dedupes prior #5/#8/#10)
At commit a5264f984a (`Merge bitcoin#28523`), src/addrman.h lines 37–39 contain `class AddrInfo;\nclass AddrManImpl;\nclass AddrInfo;` — the upstream 28523 diff adds a `class AddrInfo;` forward declaration even though Dash already had one (from the prior Pimpl AddrMan backport). Duplicate forward declarations are legal C++ so this is not a build break, but it is poor hygiene. The duplicate is then silently removed by the very next commit 5269052141 (`Merge bitcoin#28671` — a test-only fix per its upstream description), whose stat is `src/addrman.h | 1 -, test/functional/rpc_net.py | 10 +/-5`. That `addrman.h` line removal has nothing to do with bitcoin#28671's stated scope (a `rpc_net.py` mocktime fix) and is smuggled cleanup that pollutes a backport's atomicity/bisectability. Resolution: drop the duplicate in 28523's conflict resolution directly, not in an unrelated test-fix 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.
- [BLOCKING] src/addrman.h:37-39: addrman.h cleanup is carried in bitcoin#28671 instead of the bitcoin#28523 backport
Carried-forward prior finding, still valid as commit-composition hygiene. The current final tree no longer has the duplicate forward declaration, but commit a5264f984a (bitcoin#28523) introduces class AddrInfo; class AddrManImpl; class AddrInfo; in src/addrman.h. Commit 5269052141, titled as bitcoin#28671's rpc_net.py time-check fix, then removes the duplicate AddrInfo line even though upstream bitcoin#28671 only touches test/functional/rpc_net.py. The conflict-resolution cleanup belongs in the bitcoin#28523 backport where the duplicate is introduced, not in the later test-only 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.
In `src/rpc/server.cpp`:
- [BLOCKING] src/rpc/server.cpp:441-450: Possible missing prerequisite: bitcoin#26485 named-only RPC options
Prior reviews flagged that bitcoin#27822 upstream also renames an __pushKV call in transformNamedArguments' named-only RPC options branch (originating from bitcoin#26485, which Dash has not backported). The omission may be correct because Dash lacks the named_only branch. This is a backport-completeness/correctness concern, not a commit-history hygiene issue, so it belongs to the backport-reviewer specialist rather than this agent.
---
**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] src/rpc/server.cpp:407-479: Soft prerequisite: bitcoin#26485 (named-only RPC `options` branch) is absent — upstream 27822 renamed a `__pushKV` call inside it (carried forward, STILL VALID; prior #9)
Upstream bitcoin#27822 (single commit bdea2bb114) renames `options.__pushKV(fr->first, *fr->second);` to `options.pushKVEnd(...)` inside the `named_only` branch of `transformNamedArguments` in src/rpc/server.cpp. That `named_only`/`options` branch was introduced by bitcoin#26485 (named-only RPC arguments). Dash's current `transformNamedArguments` (src/rpc/server.cpp:407–479, verified at HEAD) lacks the `named_only` branch entirely — there is no `options.__pushKV` call to rename. The Dash scripted-diff (b1aa2a89cd) therefore correctly does not touch server.cpp (`git grep __pushKV` would have found nothing there). This is an ADAPTATION (not a workaround) for bitcoin#27822 specifically. However, per the review skill's first-principles rule, this confirms bitcoin#26485 is a genuinely missing upstream PR in Dash; that missing PR has no functional impact on this backport but should be tracked for future backports that depend on the named-only RPC argument feature. non-blocking per the agent (restored to blocking by policy) for this PR; flagged for completeness only.
---
**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] src/rpc/server.cpp:407-450: Missing prerequisite: bitcoin#26485
Carried-forward prior finding, still valid at current head. Upstream bitcoin#27822 changes transformNamedArguments' named-only options branch from options.__pushKV(...) to options.pushKVEnd(...). Dash's b1aa2a89cd backport omits this hunk because Dash still has the older transformNamedArguments signature taking std::vector<std::string> and lacks the named_only/options path. Tracing the missing branch shows upstream merge 34ac3f438a, bitcoin#26485 (RPC: Accept options as named-only parameters), introduced it. This is not a Dash-specific adaptation shown by the backport; it is missing upstream prerequisite content for a commit titled as a full bitcoin#27822 merge.
---
**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.
In `src/rpc/net.cpp`:
- [BLOCKING] src/rpc/net.cpp:1202: Commit ordering: bitcoin#28523 lands before bitcoin#27822 — intermediate build break (carried forward, STILL VALID; dedupes prior #1/#3/#6)
At commit a5264f984a (`Merge bitcoin#28523`), src/rpc/net.cpp introduces `table.pushKVEnd(key.str(), AddrmanEntryToJSON(info));` (line 1202) and a related comment referring to `UniValue::pushKVEnd`. However, at that same commit src/univalue/include/univalue.h still only declares `void __pushKV(std::string key, UniValue val);` (line 90) — the rename `__pushKV` → `pushKVEnd` is not introduced until the later commit b1aa2a89cd (`scripted-diff: Merge bitcoin#27822`). Verified by `git show a5264f984a:src/rpc/net.cpp | grep pushKV` and `git show a5264f984a:src/univalue/include/univalue.h | grep pushKV`. Building at a5264f984a (and any bisect that lands there) fails with `error: 'class UniValue' has no member named 'pushKVEnd'`. The final HEAD (415a728b) is clean because b1aa2a89cd later renames the call, but bisectability is broken. Resolution: place the 27822 scripted-diff commit before the 28523 backport (or fold the equivalent `pushKVEnd` declaration into 28523's adaptation).
---
**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.
In `test/functional/rpc_net.py`:
- [BLOCKING] test/functional/rpc_net.py:1-451: Commit ordering: bitcoin#28523 lands before bitcoin#28671 — intermediate test break (carried forward, STILL VALID; dedupes prior #2/#4/#7)
At commit a5264f984a (`Merge bitcoin#28523`), test/functional/rpc_net.py invokes `time.time()` at line 451 (`assert_approx(result["time"], time.time(), vspan=10)`) but the file's import block (verified via `git show a5264f984a:test/functional/rpc_net.py | sed -n '1,35p'`) contains no `import time`. Running this test at that intermediate commit raises `NameError: name 'time' is not defined`. The follow-up commit 5269052141 (`Merge bitcoin#28671`) replaces the `time.time()` call with `self.addr_time` / `self.mocktime`, sidestepping the missing import. The final HEAD also explicitly adds `import time` and uses `self.mocktime`/`self.addr_time`, so HEAD is clean — but the intermediate commit is broken. Resolution: 28671's test fix must be folded into 28523 (or land before it).
---
**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.
In `src/txorphanage.h`:
- [BLOCKING] src/txorphanage.h:107: Prior finding #11 reconciliation: OUTDATED — `_EraseTx`/`_Other` renames are legitimately part of upstream bitcoin#27822
Prior review classified the scripted-diff's additional `s '_EraseTx' 'EraseTxNoLock'` and `s '_Other' 'Other'` substitutions as smuggled content from a separate PR (bitcoin#27931). Verification: upstream bitcoin#27822 is a single commit `bdea2bb1147bbd22f8b4fa406262470f9d084215` whose commit message is `scripted-diff: Following the C++ Standard rules for identifiers with _.` and whose VERIFY SCRIPT body is literally the same three substitutions (`__pushKV`→`pushKVEnd`, `_EraseTx`→`EraseTxNoLock`, `_Other`→`Other`). The merge commit `e410fb711c` titled `Merge bitcoin/bitcoin#27822` confirms this is bitcoin#27822's sole content. Dash's scripted-diff b1aa2a89cd is faithful to that single upstream commit. The Dash commit message could be improved by reproducing the upstream rationale (C++ Standard reserved-identifier rule) rather than only naming `__pushKV`, but the content is correct. This prior finding is dropped from the blocking set.
---
**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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🔴 Blocking: Commit ordering: bitcoin#28523 lands before bitcoin#27822 — intermediate build break (carried forward, STILL VALID; dedupes prior #1/#3/#6)
At commit a5264f9 (Merge bitcoin#28523), src/rpc/net.cpp introduces table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); (line 1202) and a related comment referring to UniValue::pushKVEnd. However, at that same commit src/univalue/include/univalue.h still only declares void __pushKV(std::string key, UniValue val); (line 90) — the rename __pushKV → pushKVEnd is not introduced until the later commit b1aa2a8 (scripted-diff: Merge bitcoin#27822). Verified by git show a5264f984a:src/rpc/net.cpp | grep pushKV and git show a5264f984a:src/univalue/include/univalue.h | grep pushKV. Building at a5264f9 (and any bisect that lands there) fails with error: 'class UniValue' has no member named 'pushKVEnd'. The final HEAD (415a728) is clean because b1aa2a8 later renames the call, but bisectability is broken. Resolution: place the 27822 scripted-diff commit before the 28523 backport (or fold the equivalent pushKVEnd declaration into 28523's adaptation).
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 — Commit ordering: bitcoin#28523 lands before bitcoin#27822 — intermediate build break (carried forward, STILL VALID; dedupes prior #1/#3/#6) 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.
| @@ -74,6 +76,7 @@ def run_test(self): | |||
| self.test_addpeeraddress() | |||
| self.test_sendmsgtopeer() | |||
| self.test_getaddrmaninfo() | |||
| self.test_getrawaddrman() | |||
|
|
|||
| def test_connection_count(self): | |||
| self.log.info("Test getconnectioncount") | |||
| @@ -350,9 +353,11 @@ def test_addpeeraddress(self): | |||
|
|
|||
| self.log.debug("Test that adding an address with invalid port fails") | |||
| assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=-1) | |||
| assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress,address="1.2.3.4", port=65536) | |||
| assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=65536) | |||
|
|
|||
| self.log.debug("Test that adding a valid address to the tried table succeeds") | |||
| self.addr_time = int(time.time()) | |||
| node.setmocktime(self.addr_time) | |||
| assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), {"success": True}) | |||
| with node.assert_debug_log(expected_msgs=["CheckAddrman: new 0, tried 1, total 1 started"]): | |||
| addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks | |||
| @@ -428,5 +433,114 @@ def test_getaddrmaninfo(self): | |||
| assert_equal(res[net]["tried"], 0) | |||
| assert_equal(res[net]["total"], 0) | |||
|
|
|||
| def test_getrawaddrman(self): | |||
| self.log.info("Test getrawaddrman") | |||
| node = self.nodes[1] | |||
|
|
|||
| self.log.debug("Test that getrawaddrman is a hidden RPC") | |||
| # It is hidden from general help, but its detailed help may be called directly. | |||
| assert "getrawaddrman" not in node.help() | |||
| assert "getrawaddrman" in node.help("getrawaddrman") | |||
|
|
|||
| def check_addr_information(result, expected): | |||
| """Utility to compare a getrawaddrman result entry with an expected entry""" | |||
| assert_equal(result["address"], expected["address"]) | |||
| assert_equal(result["port"], expected["port"]) | |||
| assert_equal(result["services"], expected["services"]) | |||
| assert_equal(result["network"], expected["network"]) | |||
| assert_equal(result["source"], expected["source"]) | |||
There was a problem hiding this comment.
🔴 Blocking: Commit ordering: bitcoin#28523 lands before bitcoin#28671 — intermediate test break (carried forward, STILL VALID; dedupes prior #2/#4/#7)
At commit a5264f9 (Merge bitcoin#28523), test/functional/rpc_net.py invokes time.time() at line 451 (assert_approx(result["time"], time.time(), vspan=10)) but the file's import block (verified via git show a5264f984a:test/functional/rpc_net.py | sed -n '1,35p') contains no import time. Running this test at that intermediate commit raises NameError: name 'time' is not defined. The follow-up commit 5269052 (Merge bitcoin#28671) replaces the time.time() call with self.addr_time / self.mocktime, sidestepping the missing import. The final HEAD also explicitly adds import time and uses self.mocktime/self.addr_time, so HEAD is clean — but the intermediate commit is broken. Resolution: 28671's test fix must be folded into 28523 (or land before it).
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 — Commit ordering: bitcoin#28523 lands before bitcoin#28671 — intermediate test break (carried forward, STILL VALID; dedupes prior #2/#4/#7) 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.
|
Correction to my review at Still valid / carried forward:
Dropped/outdated after recheck:
I removed the erroneous |
…n table entries 352d5eb test: getrawaddrman RPC (0xb10c) da384a2 rpc: getrawaddrman for addrman entries (0xb10c) Pull request description: Inspired by `getaddrmaninfo` (bitcoin#27511), this adds a hidden/test-only `getrawaddrman` RPC. The RPC returns information on all addresses in the address manager new and tried tables. Addrman table contents can be used in tests and during development. The RPC result encodes the `bucket` and `position`, the internal location of addresses in the tables, in the address object's string key. This allows users to choose to consume or to ignore the location information. If the internals of the address manager implementation change, the location encoding might change too. ``` getrawaddrman EXPERIMENTAL warning: this call may be changed in future releases. Returns information on all address manager entries for the new and tried tables. Result: { (json object) "table" : { (json object) buckets with addresses in the address manager table ( new, tried ) "bucket/position" : { (json object) the location in the address manager table (<bucket>/<position>) "address" : "str", (string) The address of the node "port" : n, (numeric) The port number of the node "network" : "str", (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the address "services" : n, (numeric) The services offered by the node "time" : xxx, (numeric) The UNIX epoch time when the node was last seen "source" : "str", (string) The address that relayed the address to us "source_network" : "str" (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the source address }, ... }, ... } Examples: > bitcoin-cli getrawaddrman > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawaddrman", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/ ``` ACKs for top commit: willcl-ark: reACK 352d5eb amitiuttarwar: reACK 352d5eb stratospher: reACK 352d5eb. achow101: ACK 352d5eb Tree-SHA512: cc462666b5c709617c66b0e3e9a17c4c81e9e295f91bdd9572492d1cb6466fc9b6d48ee805ebe82f9f16010798370effe5c8f4db15065b8c7c0d8637675d615e
fa4c683 test: Fix failing time check in rpc_net.py (MarcoFalke) Pull request description: This check fails on slow runners, such as s390x qemu. Fix it by using mocktime. See bitcoin#28523 (comment) ACKs for top commit: 0xB10C: ACK fa4c683 pinheadmz: ACK fa4c683 brunoerg: crACK fa4c683 Tree-SHA512: 83fb534682e11e97537dc89de8c16f206f38af1a892a2d5970c02684c05eaea8fc9adba3159f16b2440ca0b3871d513a0562a6f3a38f19a5574a47be0919e42f
…alue::pushKVEnd
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s '__pushKV' 'pushKVEnd'
s '_EraseTx' 'EraseTxNoLock'
s '_Other' 'Other'
-END VERIFY SCRIPT-
fab4169 Allow int8_t optimized vector serialization (MarcoFalke) facaa14 Faster std::byte (pre)vector (un)serialize (MarcoFalke) Pull request description: Currently, large vectors of `std::byte` are (un)serialized byte-by-byte, which is slow. Fix this, by enabling the already existing optimization for them. On my system this gives a 10x speedup for `./src/bench/bench_bitcoin --filter=PrevectorDeserializeTrivial`, when `std::byte` are used: ```diff diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp index 2524e21..76b16bc34e 100644 --- a/src/bench/prevector.cpp +++ b/src/bench/prevector.cpp @@ -17,7 +17,7 @@ struct nontrivial_t { static_assert(!std::is_trivially_default_constructible<nontrivial_t>::value, "expected nontrivial_t to not be trivially constructible"); -typedef unsigned char trivial_t; +typedef std::byte trivial_t; static_assert(std::is_trivially_default_constructible<trivial_t>::value, "expected trivial_t to be trivially constructible"); ``` However, the optimization does not cover `signed char`. Fix that as well. ACKs for top commit: sipa: utACK fab4169 achow101: ACK fab4169 TheCharlatan: ACK fab4169 Tree-SHA512: a3e20f375fd1d0e0dedb827a8ce528de1173ea69660c8c891ad1343a86b422072f6505096fca0d3f8af4442fbe1378a02e32d5974919d4e88ff06934d0258cba
f1684bb rpc: mention that migratewallet can take a while (Andrew Chow) 9ecff99 rpc: Drop migratewallet experimental warning (Andrew Chow) Pull request description: The migration process itself hasn't fundamentally changed since it was added, so I think it's reasonable to say that it is no longer experimental. ACKs for top commit: maflcko: lgtm ACK f1684bb josibake: ACK bitcoin@f1684bb furszy: ACK bitcoin@f1684bb ryanofsky: Code review ACK f1684bb willcl-ark: ACK f1684bb Tree-SHA512: 99b176cddbf3878c76bd4c80c030106200bf03139785e26dbae3341e1a675b623a13cd6dc7a0bb78344335bf859ae7548d97b2b58eb650c6e7b305d7cdc86e40
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): Backport stack of 5 Bitcoin Core PRs (bitcoin#28523, bitcoin#28671, bitcoin#27822, bitcoin#29114, bitcoin#28037). Final-tree state at 7bd3106 is clean, but the three prior findings about the intermediate state at e71252c (Merge bitcoin#28523) all reproduce: src/rpc/net.cpp:1202 calls table.pushKVEnd(...) while src/univalue/include/univalue.h at that SHA still declares only __pushKV (rename lands later in 6d72900); test/functional/rpc_net.py uses time.time() with no import time (added later in ae1f78a); and src/addrman.h has a duplicate class AddrInfo; forward declaration that is only cleaned up in the bitcoin#28671 merge. No new latest-delta defects in the in-scope backport diff (the active/dkg/llmq churn shown in delta stats is develop-rebase noise outside this PR's 19 changed files).
🔴 3 blocking | 🟡 1 suggestion(s)
3 additional finding(s) omitted (not in diff).
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/addrman.h`:
- [SUGGESTION] src/addrman.h:36-38: addrman.h duplicate `class AddrInfo;` cleanup smuggled into bitcoin#28671 instead of bitcoin#28523
STILL VALID at head 7bd3106a3d (cleanup persisted, so final tree is clean — this is purely commit-history hygiene). Verified directly: at e71252c03b (Merge bitcoin#28523) src/addrman.h contains:
class AddrInfo;
class AddrManImpl;
class AddrInfo;
The duplicate exists because Dash already had a forward declaration of AddrInfo from the prior Pimpl AddrMan backport, and the #28523 adaptation appended a fresh one instead of reusing it. The removal of the duplicate then lands in ae1f78ab2d (Merge bitcoin#28671), whose stated scope is a Python test time fix — upstream bitcoin#28671 only touches test/functional/rpc_net.py and removes `friend class AddrManTest;` from addrman.h, not a forward-declaration dedup. C++ tolerates redundant forward declarations so this is not a build break, but the cleanup belongs with the commit that introduced the duplicate. Fix: fold the `-class AddrInfo;` hunk into the #28523 merge resolution so each merge commit's diff matches its upstream PR.
- [BLOCKING] src/addrman.h:36-38: Carried-forward: addrman.h duplicate `class AddrInfo;` cleanup smuggled into bitcoin#28671
STILL VALID at head 7bd3106a3d. At e71252c03b (Merge bitcoin#28523), src/addrman.h contains:
class AddrInfo;
class AddrManImpl;
class AddrInfo;
The duplicate `class AddrInfo;` arises because Dash already had a forward declaration of AddrInfo from the earlier Pimpl AddrMan backport, and the 28523 adaptation appended a fresh one rather than reusing the existing one. The dedup happens only in the next commit (ae1f78ab2d, Merge bitcoin#28671), where one of the two lines is removed — a change unrelated to bitcoin#28671's stated purpose (test/functional/rpc_net.py time fix). C++ tolerates redundant forward declarations so this is not a build break, but the cleanup belongs in the 28523 backport that introduced the duplicate, not silently in 28671. Suggest folding the dedup into e71252c03b. (Verified again at HEAD: src/addrman.h:36-37 is now the clean two-line declaration, confirming the cleanup did persist into 28671; the issue is purely commit-attribution / bisect hygiene.)
---
**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] src/addrman.h:37-39: Addrman conflict cleanup is placed in bitcoin#28671 instead of bitcoin#28523
Carried-forward prior finding: STILL VALID as commit-history hygiene. Dash already had class AddrInfo; before bitcoin#28523, but the bitcoin#28523 cherry-pick still adds another class AddrInfo; after class AddrManImpl;, leaving src/addrman.h in e71252c03b with class AddrInfo; class AddrManImpl; class AddrInfo;. The final head removes the duplicate in ae1f78ab2d, but upstream bitcoin#28671 only changes test/functional/rpc_net.py; this addrman.h cleanup is a conflict-resolution fix for bitcoin#28523 and should live with that commit rather than being smuggled into the follow-up 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.
In `src/rpc/net.cpp`:
- [BLOCKING] src/rpc/net.cpp:1202: Missing prerequisite ordering: bitcoin#27822 must precede bitcoin#28523
Carried-forward prior finding: STILL VALID at current head. In Dash commit e71252c03b (Merge bitcoin#28523), src/rpc/net.cpp calls table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)) in getrawaddrman. At that same intermediate commit, src/univalue/include/univalue.h still only declares void __pushKV(...); the pushKVEnd rename from bitcoin#27822 is not applied until later commit 6d72900b9f. Upstream bitcoin#28523 was based on a starting state that already had bitcoin#27822, so the Dash cherry-pick order breaks intermediate compilation even though the final head has the symbol.
---
**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.
| // Address manager tables have unique entries so there is no advantage | ||
| // in using UniValue::pushKV, which checks if the key already exists | ||
| // in O(N). UniValue::pushKVEnd is used instead which currently is O(1). | ||
| table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)); |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite ordering: bitcoin#27822 must precede bitcoin#28523
Carried-forward prior finding: STILL VALID at current head. In Dash commit e71252c (Merge bitcoin#28523), src/rpc/net.cpp calls table.pushKVEnd(key.str(), AddrmanEntryToJSON(info)) in getrawaddrman. At that same intermediate commit, src/univalue/include/univalue.h still only declares void __pushKV(...); the pushKVEnd rename from bitcoin#27822 is not applied until later commit 6d72900. Upstream bitcoin#28523 was based on a starting state that already had bitcoin#27822, so the Dash cherry-pick order breaks intermediate compilation even though the final head has the symbol.
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']
There was a problem hiding this comment.
Resolved in this update — Missing prerequisite ordering: bitcoin#27822 must precede bitcoin#28523 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.
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): Cumulative review at a1053ed. The latest delta from 7bd3106..a1053ed is empty, so no new defects were introduced. The 9 prior findings collapse into 3 distinct root causes (all carried-forward, all STILL VALID): the bitcoin#28523 merge (928555b) calls UniValue::pushKVEnd before the rename lands in bitcoin#27822 (bea095a), uses time.time() without import time (added by bitcoin#28671 at 8982514), and leaves a duplicate class AddrInfo; forward declaration in src/addrman.h that is only removed by bitcoin#28671. The final tree at head is clean and builds — these are commit-ordering / merge-resolution hygiene issues that break git bisect across the intermediate commits in the backport stack.
🔴 4 blocking | 🟡 1 suggestion(s)
3 additional finding(s) omitted (not in diff).
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/rpc_net.py`:
- [BLOCKING] test/functional/rpc_net.py:451: Carried-forward: bitcoin#28523 test uses time.time() before bitcoin#28671 adds `import time` and the mocktime adaptation
STILL VALID at head a1053ede4d. Verified directly against the worktree:
- At intermediate commit 928555b296 (Merge bitcoin#28523), test/functional/rpc_net.py:451 contains `assert_approx(result["time"], time.time(), vspan=10)`.
- At that same SHA, the file's import block contains no `import time` (only `import test_framework.messages`, `import platform`, and `from itertools import product`).
- `import time` and the switch to `setmocktime` / `self.addr_time` are introduced only in 8982514cab (Merge bitcoin#28671).
Consequence: running test/functional/rpc_net.py at 928555b296 raises `NameError: name 'time' is not defined`. As with the pushKVEnd issue, the final head test runs fine because bitcoin#28671's adaptation arrives in the very next commit — but per-commit CI and `git bisect` across this range fail.
Fix: fold bitcoin#28671's adaptation (import time + mocktime usage) into the bitcoin#28523 backport commit. Reordering #28671 before #28523 isn't semantically right because #28671 is upstream's fix to #28523, so folding/squashing is the natural option.
- [BLOCKING] test/functional/rpc_net.py:451: Missing prerequisite/adaptation: bitcoin#28523 uses time.time() before time is imported
Carried-forward prior finding, STILL VALID. In intermediate commit 928555b29698a674435d158ee7ffbbd8652ad902, test_getrawaddrman calls `assert_approx(result["time"], time.time(), vspan=10)` at test/functional/rpc_net.py:451, but the import block at that same commit has no `import time`, so running rpc_net.py at that commit raises NameError. Upstream bitcoin#28523's pre-PR rpc_net.py already had `import time`; Dash's base did not. Dash adds the import only in the later bitcoin#28671 adaptation commit 8982514cab3a2a7757d73171481a9a4e1759e10e, so the final tree is fixed but the intermediate bitcoin#28523 cherry-pick remains test-broken.
---
**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.
In `src/addrman.h`:
- [SUGGESTION] src/addrman.h:37-39: Carried-forward: duplicate `class AddrInfo;` cleanup smuggled into the wrong backport commit
STILL VALID at head a1053ede4d as a commit-history hygiene issue. Verified directly:
- Dash's tree before this PR already had `class AddrInfo;` (from an earlier Pimpl AddrMan backport).
- At intermediate commit 928555b296 (Merge bitcoin#28523), src/addrman.h:37-39 contains:
```
class AddrInfo;
class AddrManImpl;
class AddrInfo;
```
The bitcoin#28523 cherry-pick added a second `class AddrInfo;` without noticing the pre-existing one.
- At head a1053ede4d the duplicate is gone — but it was removed by 8982514cab (Merge bitcoin#28671), whose upstream PR is purely a test fix in test/functional/rpc_net.py and does not touch src/addrman.h.
Final tree is clean, so this is non-blocking. But it muddies both backport commits' diffs: bitcoin#28523 carries an unnecessary forward-declaration, and bitcoin#28671 diverges from upstream by carrying a header cleanup it shouldn't own. Fix: move the `-class AddrInfo;` hunk from 8982514cab into 928555b296's conflict resolution so each backport commit's diff faithfully reflects what its upstream PR changed.
- [BLOCKING] src/addrman.h:36-39: Carried-forward: addrman.h duplicate `class AddrInfo;` cleanup smuggled into bitcoin#28671 instead of bitcoin#28523
Dedupe of prior findings #5, #8, #9 (same root cause). STILL VALID at head a1053ede4d as commit-history hygiene (final tree is clean). Dash already had a forward declaration of AddrInfo prior to bitcoin#28523 (from the earlier Pimpl AddrMan backport), so the #28523 cherry-pick in 928555b296 adds a second `class AddrInfo;` after `class AddrManImpl;`, leaving src/addrman.h in that intermediate commit with `class AddrInfo; class AddrManImpl; class AddrInfo;`. The duplicate is removed only in the later 8982514cab (Merge bitcoin#28671), even though #28671 upstream is purely a test fix (test/functional/rpc_net.py) and does not touch addrman.h. The cleanup belongs in the #28523 conflict resolution, not smuggled into #28671. Final head is clean, so this is non-blocking per the agent (restored to blocking by policy) — flagged for adaptation-hygiene only.
---
**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] src/addrman.h:37-39: Conflict cleanup misplaced: duplicate AddrInfo forward declaration removed in bitcoin#28671 instead of bitcoin#28523
Carried-forward prior finding, STILL VALID as commit-history hygiene. Dash's base already had `class AddrInfo;` before `class AddrManImpl;`, while upstream bitcoin#28523 adds `class AddrInfo;` after `class AddrManImpl;`. The Dash bitcoin#28523 commit 928555b29698a674435d158ee7ffbbd8652ad902 therefore leaves src/addrman.h with `class AddrInfo; class AddrManImpl; class AddrInfo;` at lines 37-39. The duplicate is harmless C++, and the final tree removes it, but that cleanup lands in the later bitcoin#28671 commit rather than in the bitcoin#28523 conflict resolution where the duplicate was introduced.
---
**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.
| assert_equal(result["port"], expected["port"]) | ||
| assert_equal(result["services"], expected["services"]) | ||
| assert_equal(result["network"], expected["network"]) | ||
| assert_equal(result["source"], expected["source"]) |
There was a problem hiding this comment.
🔴 Blocking: Carried-forward: bitcoin#28523 test uses time.time() before bitcoin#28671 adds import time and the mocktime adaptation
STILL VALID at head a1053ed. Verified directly against the worktree:
- At intermediate commit 928555b (Merge rpc: add hidden getrawaddrman RPC to list addrman table entries bitcoin/bitcoin#28523), test/functional/rpc_net.py:451 contains
assert_approx(result["time"], time.time(), vspan=10). - At that same SHA, the file's import block contains no
import time(onlyimport test_framework.messages,import platform, andfrom itertools import product). import timeand the switch tosetmocktime/self.addr_timeare introduced only in 8982514 (Merge test: Fix failing time check in rpc_net.py bitcoin/bitcoin#28671).
Consequence: running test/functional/rpc_net.py at 928555b raises NameError: name 'time' is not defined. As with the pushKVEnd issue, the final head test runs fine because bitcoin#28671's adaptation arrives in the very next commit — but per-commit CI and git bisect across this range fail.
Fix: fold bitcoin#28671's adaptation (import time + mocktime usage) into the bitcoin#28523 backport commit. Reordering bitcoin#28671 before bitcoin#28523 isn't semantically right because bitcoin#28671 is upstream's fix to bitcoin#28523, so folding/squashing is the natural option.
source: ['claude', 'codex']
|
Correction to my automated review at a1053ed ( The latest delta from
The policy gate incorrectly re-expanded duplicate agent phrasings in the review body and made the addrman hygiene duplicate read as blocking. Please treat the addrman item as a suggestion and ignore duplicate restatements of the same root causes. I also removed the duplicate inline comment on |
|
bitcoin#28671 which is exactly to fixe test: Fix failing time check in rpc_net.py, So bitcoin#28523 should be fine @thepastaclaw |
|
Thanks — agreed that bitcoin#28671 is the upstream fix for the flaky time check added by bitcoin#28523. The blocker I’m calling out is narrower: the Dash I rechecked current head
So I still think the fix is to fold the minimal |
4 participants
bitcoin backports