backport: Merge bitcoin/bitcoin#28644 , 28184, (partial) 27944#7091
backport: Merge bitcoin/bitcoin#28644 , 28184, (partial) 27944#7091vijaydasmp wants to merge 3 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
vijaydasmp
force-pushed
the
Jan_2026_5
branch
2 times, most recently
from
2904f22 to
1a61563
Compare
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2026_5
branch
3 times, most recently
from
343e254 to
1bf441f
Compare
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2026_5
branch
3 times, most recently
from
2aa6e19 to
0ebc185
Compare
vijaydasmp
force-pushed
the
Jan_2026_5
branch
from
0ebc185 to
8ac187e
Compare
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
Jan_2026_5
branch
from
8ac187e to
6b3af01
Compare
vijaydasmp
force-pushed
the
Jan_2026_5
branch
from
6b3af01 to
5d091d3
Compare
WalkthroughThe pull request contains changes across the Qt GUI layer, functional tests, and configuration scripts. In the main functional change, Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/functional/interface_usdt_validation.py (1)
99-137:⚠️ Potential issue | 🔴 Critical
eventsis never populated, so Line 136 will always fail.Line 136 asserts
len(events) == BLOCKS_EXPECTED, buthandle_blockconnected()never appends toevents. This makes the test fail even when tracepoints are correct.Suggested fix
def handle_blockconnected(_, data, __): event = ctypes.cast(data, ctypes.POINTER(Block)).contents self.log.info(f"handle_blockconnected(): {event}") - block_hash = bytes(event.hash[::-1]).hex() - block = expected_blocks[block_hash] - assert_equal(block["hash"], block_hash) - assert_equal(block["height"], event.height) - assert_equal(len(block["tx"]), event.transactions) - assert_equal(len([tx["vin"] for tx in block["tx"]]), event.inputs) - assert_equal(0, event.sigops) # no sigops in coinbase tx - # only plausibility checks - assert event.duration > 0 - del expected_blocks[block_hash] + events.append(event)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/interface_usdt_validation.py` around lines 99 - 137, The test fails because the events list is never populated; initialize a shared list (e.g., events = []) before defining handle_blockconnected and append the received event (or a lightweight copy of it) inside handle_blockconnected (the callback registered via bpf["block_connected"].open_perf_buffer(handle_blockconnected)); ensure you append the same event object shape expected later (the variable named event used in the later loop) so the final assertions and len(events) == BLOCKS_EXPECTED succeed.src/qt/bitcoingui.cpp (1)
555-620:⚠️ Potential issue | 🔴 CriticalUse
thisas connection context for wallet actions instead of null m_wallet_controller.
createActions()executes during construction (line 157) whenm_wallet_controlleris stillnullptr(initialized at line 122). Usingnullptras the Qt::connect context object on lines 555, 583, 609, 612, 618 bypasses Qt's automatic signal-slot lifecycle management. When the context object is null, Qt cannot properly manage connection cleanup, creating a resource management issue.Replace
m_wallet_controllerwiththisas the context object and add null checks inside each lambda, as the suggested fix shows. This ensures wallet actions are properly lifecycle-managed throughBitcoinGUI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/bitcoingui.cpp` around lines 555 - 620, The connect calls for wallet-related actions (the QMenu aboutToShow lambda using m_wallet_controller->listWalletDir(), and the triggered handlers for m_restore_wallet_action/CreateWalletActivity/RestoreWalletActivity/CloseWalletActivity/closeAllWallets via m_close_wallet_action, m_create_wallet_action, m_close_all_wallets_action) currently use m_wallet_controller as the connection context even though it can be nullptr during construction; change the context argument in these connect(...) calls from m_wallet_controller to this, and inside each lambda add a guard that returns early if m_wallet_controller is nullptr before calling methods like m_wallet_controller->listWalletDir(), creating activities that take m_wallet_controller, or calling m_wallet_controller->closeWallet()/closeAllWallets(); this preserves Qt's automatic connection cleanup while avoiding dereferencing a null m_wallet_controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 555-620: The connect calls for wallet-related actions (the QMenu
aboutToShow lambda using m_wallet_controller->listWalletDir(), and the triggered
handlers for
m_restore_wallet_action/CreateWalletActivity/RestoreWalletActivity/CloseWalletActivity/closeAllWallets
via m_close_wallet_action, m_create_wallet_action, m_close_all_wallets_action)
currently use m_wallet_controller as the connection context even though it can
be nullptr during construction; change the context argument in these
connect(...) calls from m_wallet_controller to this, and inside each lambda add
a guard that returns early if m_wallet_controller is nullptr before calling
methods like m_wallet_controller->listWalletDir(), creating activities that take
m_wallet_controller, or calling
m_wallet_controller->closeWallet()/closeAllWallets(); this preserves Qt's
automatic connection cleanup while avoiding dereferencing a null
m_wallet_controller.
In `@test/functional/interface_usdt_validation.py`:
- Around line 99-137: The test fails because the events list is never populated;
initialize a shared list (e.g., events = []) before defining
handle_blockconnected and append the received event (or a lightweight copy of
it) inside handle_blockconnected (the callback registered via
bpf["block_connected"].open_perf_buffer(handle_blockconnected)); ensure you
append the same event object shape expected later (the variable named event used
in the later loop) so the final assertions and len(events) == BLOCKS_EXPECTED
succeed.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/qt/bitcoingui.cpptest/functional/interface_usdt_net.pytest/functional/interface_usdt_utxocache.pytest/functional/interface_usdt_validation.pytest/fuzz/test_runner.pytest/lint/lint-python.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified 2 findings: (1) wallet menu action lambdas connected before m_wallet_controller is initialized — may crash or silently no-op, (2) USDT validation test assertion now vacuous after event recording refactor.
Reviewed commit: 5d091d3
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [BLOCKING] lines 555-618: Wallet menu action lambdas are connected before `m_wallet_controller` is initialized
Verified. `BitcoinGUI` calls `createActions()` from the constructor at line 157, but `m_wallet_controller` is only assigned later in `setWalletController()` at lines 967-972. The backport changed the wallet menu/action connections at lines 555, 571, 583, 609, 612, and 618 to use `m_wallet_controller` as the context object at connection time, when it is still null. That means the `Open Wallet`, `Restore Wallet`, `Close Wallet`, `Create Wallet`, and `Close All Wallets` lambda connections are not established correctly. This matches the upstream revert of commit `7066e8996d` in `f09bfab4af` / merge `565c55119b`.
In `test/functional/interface_usdt_validation.py`:
- [SUGGESTION] lines 88-136: Final block-connected assertion was changed to use an event list that is never populated
Verified. The refactor removed `blocks_checked` from the callback, but the replacement check at line 136 now asserts `len(events) == BLOCKS_EXPECTED` even though `events` is only initialized at line 90 and never appended to in `handle_blockconnected()`. The callback still performs inline validation and deletes from `expected_blocks`, so the final `len(events)` assertion is guaranteed to stay at 0 and does not measure traced callbacks correctly.
| @@ -568,7 +568,7 @@ void BitcoinGUI::createActions() | |||
| continue; | |||
| } | |||
|
|
|||
| connect(action, &QAction::triggered, [this, path] { | |||
| connect(action, &QAction::triggered, m_wallet_controller, [this, path] { | |||
| auto activity = new OpenWalletActivity(m_wallet_controller, this); | |||
| connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection); | |||
| connect(activity, &OpenWalletActivity::opened, rpcConsole, &RPCConsole::setCurrentWallet, Qt::QueuedConnection); | |||
| @@ -580,7 +580,7 @@ void BitcoinGUI::createActions() | |||
| action->setEnabled(false); | |||
| } | |||
| }); | |||
| connect(m_restore_wallet_action, &QAction::triggered, [this] { | |||
| connect(m_restore_wallet_action, &QAction::triggered, m_wallet_controller, [this] { | |||
| //: Name of the wallet data file format. | |||
| QString name_data_file = tr("Wallet Data"); | |||
|
|
|||
| @@ -606,16 +606,16 @@ void BitcoinGUI::createActions() | |||
| auto backup_file_path = fs::PathFromString(backup_file.toStdString()); | |||
| activity->restore(backup_file_path, wallet_name.toStdString()); | |||
| }); | |||
| connect(m_close_wallet_action, &QAction::triggered, [this] { | |||
| connect(m_close_wallet_action, &QAction::triggered, m_wallet_controller, [this] { | |||
| m_wallet_controller->closeWallet(walletFrame->currentWalletModel(), this); | |||
| }); | |||
| connect(m_create_wallet_action, &QAction::triggered, [this] { | |||
| connect(m_create_wallet_action, &QAction::triggered, m_wallet_controller, [this] { | |||
| auto activity = new CreateWalletActivity(m_wallet_controller, this); | |||
| connect(activity, &CreateWalletActivity::created, this, &BitcoinGUI::setCurrentWallet); | |||
| connect(activity, &CreateWalletActivity::created, rpcConsole, &RPCConsole::setCurrentWallet); | |||
| activity->create(); | |||
| }); | |||
| connect(m_close_all_wallets_action, &QAction::triggered, [this] { | |||
| connect(m_close_all_wallets_action, &QAction::triggered, m_wallet_controller, [this] { | |||
There was a problem hiding this comment.
🔴 Blocking: Wallet menu action lambdas are connected before m_wallet_controller is initialized
Verified. BitcoinGUI calls createActions() from the constructor at line 157, but m_wallet_controller is only assigned later in setWalletController() at lines 967-972. The backport changed the wallet menu/action connections at lines 555, 571, 583, 609, 612, and 618 to use m_wallet_controller as the context object at connection time, when it is still null. That means the Open Wallet, Restore Wallet, Close Wallet, Create Wallet, and Close All Wallets lambda connections are not established correctly. This matches the upstream revert of commit 7066e8996d in f09bfab4af / merge 565c55119b.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [BLOCKING] lines 555-618: Wallet menu action lambdas are connected before `m_wallet_controller` is initialized
Verified. `BitcoinGUI` calls `createActions()` from the constructor at line 157, but `m_wallet_controller` is only assigned later in `setWalletController()` at lines 967-972. The backport changed the wallet menu/action connections at lines 555, 571, 583, 609, 612, and 618 to use `m_wallet_controller` as the context object at connection time, when it is still null. That means the `Open Wallet`, `Restore Wallet`, `Close Wallet`, `Create Wallet`, and `Close All Wallets` lambda connections are not established correctly. This matches the upstream revert of commit `7066e8996d` in `f09bfab4af` / merge `565c55119b`.
| @@ -98,7 +97,6 @@ def __repr__(self): | |||
| usdt_contexts=[ctx], debug=0, cflags=["-Wno-error=implicit-function-declaration"]) | |||
|
|
|||
| def handle_blockconnected(_, data, __): | |||
| nonlocal events, blocks_checked | |||
| event = ctypes.cast(data, ctypes.POINTER(Block)).contents | |||
| self.log.info(f"handle_blockconnected(): {event}") | |||
| block_hash = bytes(event.hash[::-1]).hex() | |||
| @@ -111,7 +109,6 @@ def handle_blockconnected(_, data, __): | |||
| # only plausibility checks | |||
| assert event.duration > 0 | |||
| del expected_blocks[block_hash] | |||
| blocks_checked += 1 | |||
|
|
|||
| bpf["block_connected"].open_perf_buffer( | |||
| handle_blockconnected) | |||
| @@ -136,7 +133,7 @@ def handle_blockconnected(_, data, __): | |||
| # only plausibility checks | |||
| assert event.duration > 0 | |||
| del expected_blocks[block_hash] | |||
| assert_equal(BLOCKS_EXPECTED, blocks_checked) | |||
| assert_equal(BLOCKS_EXPECTED, len(events)) | |||
There was a problem hiding this comment.
🟡 Suggestion: Final block-connected assertion was changed to use an event list that is never populated
Verified. The refactor removed blocks_checked from the callback, but the replacement check at line 136 now asserts len(events) == BLOCKS_EXPECTED even though events is only initialized at line 90 and never appended to in handle_blockconnected(). The callback still performs inline validation and deletes from expected_blocks, so the final len(events) assertion is guaranteed to stay at 0 and does not measure traced callbacks correctly.
source: ['codex']
🤖 Fix this 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/interface_usdt_validation.py`:
- [SUGGESTION] lines 88-136: Final block-connected assertion was changed to use an event list that is never populated
Verified. The refactor removed `blocks_checked` from the callback, but the replacement check at line 136 now asserts `len(events) == BLOCKS_EXPECTED` even though `events` is only initialized at line 90 and never appended to in `handle_blockconnected()`. The callback still performs inline validation and deletes from `expected_blocks`, so the final `len(events)` assertion is guaranteed to stay at 0 and does not measure traced callbacks correctly.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
- should drop gui 765
- 27944 should be partial because we don't have
test/functional/interface_usdt_mempool.pyyet
faa190b test: Fuzz merge with -use_value_profile=0 for now (MarcoFalke) Pull request description: Seems odd that this has to be done, but for now there are (unknown) size limits on the qa-assets repo. Also, a larger size means that cloning and iterating over the files takes a longer time. Not sure how to measure the net impact of this, but with some backups reverting this commit, it can be limited on the downside? ACKs for top commit: dergoegge: ACK faa190b Tree-SHA512: 9f8b3f4526f60e4ff6fca97859a725d145a8339c216bd15c92fad7e53f84308745fee47727527de459c0245ef9d474a9dc836fee599ab2b556b519bd900b9a33
f904777 lint: fix custom mypy cache dir setting (Fabian Jahr) Pull request description: fixes bitcoin#28183 The custom cache dir for `mypy` can only be set via an environment variable, setting the `MYPY_CACHE_DIR` variable in the program is not sufficient. This error was introduced while translating the shell script to python. See also the mypy documentation: https://mypy.readthedocs.io/en/stable/config_file.html#confval-cache_dir ACKs for top commit: MarcoFalke: lgtm ACK f904777 Tree-SHA512: 7e8fb0cd06688129bd46d1afb8647262eb53d0f60b1ef6f288fedaa122d906fb62c9855e8bb0d6c6297d41a87a47d3cec7a00df55a7d033947937dfe23d07ba7
…anups (27831 follow-ups) 9f55773 test: refactor: usdt_mempool: store all events (stickies-v) bc43270 test: refactor: remove unnecessary nonlocal (stickies-v) 326db63 test: log sanity check assertion failures (stickies-v) f5525ad test: store utxocache events (stickies-v) f1b99ac test: refactor: deduplicate handle_utxocache_* logic (stickies-v) ad90ba3 test: refactor: rename inbound to is_inbound (stickies-v) afc0224 test: refactor: remove unnecessary blocks_checked counter (stickies-v) Pull request description: Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to bitcoin#27831 (review). Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable. The rationale for each change is in the corresponding commit message. Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired. ACKs for top commit: 0xB10C: ACK 9f55773. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable. Tree-SHA512: 6c37a0265b6c26d4f9552a056a690b8f86f7304bd33b4419febd8b17369cf6af799cb87c16df35d0c2a1b839ad31de24661d4384eafa88816c2051c522fd3bf5
vijaydasmp
changed the title
|
⏳ Review in progress (commit 1490033) |
|
Re-checked current head
I prepared the minimal fix on my fork here:
Cherry-pick command if helpful: git fetch https://github.com/thepastaclaw/dash.git tracker-974 && git cherry-pick 62c1a52875I also tried pushing directly to |
|
The earlier comment from pastaclaw stating that “one real issue still remains in |
4 participants
Bitcoin Backporting work