backport: Merge bitcoin#28523, 26638, 28976, 27822, 28671#7358
backport: Merge bitcoin#28523, 26638, 28976, 27822, 28671#7358vijaydasmp wants to merge 3 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
…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
|
@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 3c47eb7) |
![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 👍 / 👎.
WalkthroughThis PR bundles four independent changes. First, it adds a Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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
| # 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.
Use mock time in the assertion to avoid reintroducing flakiness.
result["time"] is produced from node mocktime, but this check compares it to wall-clock time.time(). On slow runners, this can still fail nondeterministically.
🛠️ Suggested fix
- # To avoid failing on slow test runners, use a 10s vspan here.
- assert_approx(result["time"], time.time(), vspan=10)
+ # Addrman timestamps are based on node mocktime set earlier.
+ assert_equal(result["time"], self.addr_time)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # To avoid failing on slow test runners, use a 10s vspan here. | |
| assert_approx(result["time"], time.time(), vspan=10) | |
| # Addrman timestamps are based on node mocktime set earlier. | |
| assert_equal(result["time"], self.addr_time) |
🤖 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` around lines 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().
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
2 times, most recently
from
b05b97c to
5bc72fc
Compare
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
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']
| // 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']
| // 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']
4 participants
bitcoin backports