backport: bitcoin#25122, #25504, #25647, #25768, #25924, #25990, #26021, #26037, #26091, #26203, #26205 - #7550
Conversation
|
🕓 Ready for review — 5 ahead in queue (commit 6d1968b) |
![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: 60494e9db0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| QString strCoinJoinAmount = BitcoinUnits::formatHtmlWithUnit(m_display_bitcoin_unit, clientModel->coinJoinOptions().getAmount() * COIN, false, BitcoinUnits::SeparatorStyle::ALWAYS); | ||
|
|
||
| if(m_balances.balance == 0) | ||
| const auto& balances = walletModel->getCachedBalance(); |
There was a problem hiding this comment.
When only denominated_trusted or denominated_untrusted_pending changes, such as after denomination creation/confirmation during mixing, WalletModel::checkBalanceChanged() does not copy the new values because WalletBalances::balanceChanged() ignores those fields. Since this method now uses getCachedBalance() instead of fetching fresh balances, the advanced CoinJoin progress can remain stale until some unrelated tracked balance field changes.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a Dash-specific regression. Before this backport, the advanced CoinJoin path fetched wallet().getBalances() directly. The new cached path depends on WalletBalances::balanceChanged(), which does not compare denominated_trusted or denominated_untrusted_pending, so those fields can remain stale when they change independently. This needs a Dash adaptation before merge.
|
|
||
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | ||
| { | ||
| return control && control->HasSelected() ? wallet().getAvailableBalance(*control) : getCachedBalance().balance; |
There was a problem hiding this comment.
In the mixed-funds send dialog, m_coin_control->UseCoinJoin(true) is set without selecting specific inputs, so this branch bypasses wallet().getAvailableBalance(*control), whose wallet interface special-cases CoinJoin to return the anonymized balance. Returning the overall cached balance here makes “Use available balance” and the upfront balance check treat non-mixed funds as spendable for CoinJoin sends, only failing later during transaction creation.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. wallet().getAvailableBalance(*control) special-cases IsUsingCoinJoin() and returns GetBalanceAnonymized(), while the new no-manual-selection branch returns the total cached balance. Mixed-only sends therefore bypass their intended balance filter. This needs a Dash-specific adaptation before merge.
|
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:
WalkthroughThe PR centralizes Qt wallet balance access through Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WalletTimer
participant CWallet
participant Mempool
WalletTimer->>CWallet: check ShouldResend()
CWallet->>CWallet: order and filter wallet transactions
CWallet->>Mempool: ResubmitWalletTransactions(relay, force)
CWallet->>WalletTimer: SetNextResend()
sequenceDiagram
participant RPCClient
participant listsinceblock
participant ListTransactions
participant CachedTxGetAmounts
RPCClient->>listsinceblock: pass include_change
listsinceblock->>ListTransactions: forward include_change
ListTransactions->>CachedTxGetAmounts: request transaction amounts
CachedTxGetAmounts-->>ListTransactions: include or omit change outputs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (5)
src/wallet/test/walletload_tests.cpp (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the restating comments.
These comments only describe the next statements. Keep comments for non-obvious invariants or test setup rationale.
As per coding guidelines, “avoid comments that merely restate code and reserve comments for non-obvious invariants, workaround rationale, or non-local side effects.”
Also applies to: 47-47
🤖 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/wallet/test/walletload_tests.cpp` at line 38, Remove the restating comments near the unknown active descriptor setup, including both referenced locations, while leaving the test statements and behavior unchanged.Source: Coding guidelines
src/wallet/coinselection.h (1)
351-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new
only_fully_mixedparameter.The doc block lists
utxo_pool,target_value,rng, andmax_weight, but notonly_fully_mixed. This parameter is Dash-specific and controls whetherCHANGE_LOWERis added to the target, so it is not self-explanatory to a reader coming from upstream.♻️ Proposed doc addition
* `@param`[in] max_weight The maximum allowed weight for a selection result to be valid + * `@param`[in] only_fully_mixed Dash-specific: when true, no change is allowed, so the target is + * not increased by CHANGE_LOWER * `@returns` If successful, a valid SelectionResult, otherwise, util::Error */🤖 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/wallet/coinselection.h` around lines 351 - 361, Update the SelectCoinsSRD documentation to add an `@param` entry for only_fully_mixed, explaining that it is Dash-specific and controls whether CHANGE_LOWER is added to the target. Keep the existing parameter descriptions unchanged.test/functional/wallet_groups.py (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Dash terminology in the new comment.
The fixed feerate makes the fee expectations deterministic, and the values are consistent with the
maxapsfeethresholds on lines 28-29: the grouped/non-grouped difference is 5880 duffs, node 3 allows 5879, and node 4 allows 5880. The code is correct.The comment says "20 sats/vB". Dash has no witness data and no vbytes, and the unit is the duff. Consider "20 duffs/byte".
♻️ Proposed comment change
- args.append(f"-paytxfee={20 * 1e3 / 1e8}") # apply feerate of 20 sats/vB across all nodes + args.append(f"-paytxfee={20 * 1e3 / 1e8}") # apply feerate of 20 duffs/byte across all nodes🤖 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_groups.py` around lines 28 - 34, Update the comment beside the fixed paytxfee assignment in the extra_args loop to use Dash terminology, replacing “20 sats/vB” with “20 duffs/byte”; leave the fee value and surrounding whitelist comment unchanged.src/wallet/test/coinselector_tests.cpp (1)
302-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider 34 for
change_output_sizeto match the other Dash test parameters.31 bytes is the upstream P2WPKH change-output size. Dash has no segwit outputs, and the other
CoinSelectionParamsinitializations in this file use 34, which is the P2PKH change-output size (8 value + 1 script length + 25 script). The value only scales the derivedm_change_fee,m_cost_of_change, andmin_viable_changein this test, so behavior does not change. Alignment keeps the test parameters consistent.If you prefer to keep the file identical to upstream Bitcoin Core, keep 31 and ignore this note.
🤖 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/wallet/test/coinselector_tests.cpp` around lines 302 - 303, Update the CoinSelectionParams initialization containing change_output_size and change_spend_size to use 34 for change_output_size, matching the other Dash test configurations in this file; leave change_spend_size and the surrounding test logic unchanged.src/wallet/spend.cpp (1)
958-962: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestrict the change-position lookup to the case where a change output exists.
When
change_amountis 0, no change output is inserted andnChangePosInOutis set to -1, but the lookup still searches fornewTxOut, which isCTxOut(0, scriptChange). A match would restore a bogus change position. Today the dust check on recipients prevents a 0-value recipient output, so the lookup cannot match. A guard makes the intent explicit and protects the invariant if the dust rule changes.♻️ Proposed guard
// If there was a change output added before, we must update its position now - if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) { - nChangePosInOut = std::distance(txNew.vout.begin(), it); + if (nChangePosInOut != -1) { + const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); + assert(it != txNew.vout.end()); + nChangePosInOut = std::distance(txNew.vout.begin(), it); }🤖 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/wallet/spend.cpp` around lines 958 - 962, Guard the change-position lookup in the surrounding transaction-output update logic with the condition that a change output was actually created (change_amount > 0). Keep nChangePosInOut at -1 when no change output exists, and only run the std::find lookup for positive change amounts.
🤖 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 `@doc/release-notes-25504.md`:
- Around line 4-5: Update the release note’s RPC list to include listunspent
alongside listsinceblock, listtransactions, and gettransaction, preserving the
statement that each receive entry now contains parent_descs.
In `@src/wallet/wallet.cpp`:
- Line 2137: Update CWallet::GetDefaultNextResend() to return the current time
plus a 12-hour base delay and a random value below 24 hours, restoring the
intended 12–36 hour interval. Also update the GetDefaultNextResend declaration
comment in wallet.h to document that same interval.
In `@src/wallet/walletdb.cpp`:
- Around line 919-920: Update the diagnostic string in the wallet error-handling
path to say “might have been created” instead of “might had been created,”
leaving the surrounding message and conditional behavior unchanged.
- Around line 847-850: Update the wallet-loading flow around DBKeys::VERSION and
the WalletLogPrintf call so DBKeys::MINVERSION is read and applied before
pwallet->GetVersion() is logged. Alternatively, log the MINVERSION value read
directly from the database, ensuring the reported wallet file version reflects
persisted data rather than the default in-memory value.
---
Nitpick comments:
In `@src/wallet/coinselection.h`:
- Around line 351-361: Update the SelectCoinsSRD documentation to add an `@param`
entry for only_fully_mixed, explaining that it is Dash-specific and controls
whether CHANGE_LOWER is added to the target. Keep the existing parameter
descriptions unchanged.
In `@src/wallet/spend.cpp`:
- Around line 958-962: Guard the change-position lookup in the surrounding
transaction-output update logic with the condition that a change output was
actually created (change_amount > 0). Keep nChangePosInOut at -1 when no change
output exists, and only run the std::find lookup for positive change amounts.
In `@src/wallet/test/coinselector_tests.cpp`:
- Around line 302-303: Update the CoinSelectionParams initialization containing
change_output_size and change_spend_size to use 34 for change_output_size,
matching the other Dash test configurations in this file; leave
change_spend_size and the surrounding test logic unchanged.
In `@src/wallet/test/walletload_tests.cpp`:
- Line 38: Remove the restating comments near the unknown active descriptor
setup, including both referenced locations, while leaving the test statements
and behavior unchanged.
In `@test/functional/wallet_groups.py`:
- Around line 28-34: Update the comment beside the fixed paytxfee assignment in
the extra_args loop to use Dash terminology, replacing “20 sats/vB” with “20
duffs/byte”; leave the fee value and surrounding whitelist comment unchanged.
🪄 Autofix
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 Plus
Run ID: d072bb03-7ed9-4fbf-b435-cda2e63fa1c9
📒 Files selected for processing (36)
doc/release-note-25122.mddoc/release-notes-25504.mdsrc/Makefile.test.includesrc/qt/overviewpage.cppsrc/qt/overviewpage.hsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsdialog.hsrc/qt/test/wallettests.cppsrc/qt/walletmodel.cppsrc/qt/walletmodel.hsrc/rpc/client.cppsrc/wallet/coinselection.cppsrc/wallet/coinselection.hsrc/wallet/receive.cppsrc/wallet/receive.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/coins.cppsrc/wallet/rpc/transactions.cppsrc/wallet/rpc/util.cppsrc/wallet/rpc/util.hsrc/wallet/spend.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/test/fuzz/coinselection.cppsrc/wallet/test/walletload_tests.cppsrc/wallet/transaction.hsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.htest/functional/mempool_expiry.pytest/functional/test_framework/messages.pytest/functional/wallet_basic.pytest/functional/wallet_groups.pytest/functional/wallet_listreceivedby.pytest/functional/wallet_listsinceblock.pytest/functional/wallet_resendwallettransactions.py
💤 Files with no reviewable changes (1)
- src/qt/overviewpage.h
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/wallet/rpc/coins.cpp (1)
23-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove comments that restate adjacent code.
Lines 23, 27, and 35 describe the statements that immediately follow them. Remove these comments.
Proposed cleanup
if (by_label) { - // Get the set of addresses assigned to label addresses = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{LabelFromValue(params[0])}); if (addresses.empty()) throw JSONRPCError(RPC_WALLET_ERROR, "Label not found in wallet"); } else { - // Get the address CTxDestination dest = DecodeDestination(params[0].get_str()); if (!IsValidDestination(dest)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Dash address"); } addresses.emplace_back(dest); } - // Filter by own scripts only std::vector<CScript> output_scripts;As per coding guidelines, “avoid comments that merely restate code.”
🤖 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/wallet/rpc/coins.cpp` around lines 23 - 35, Remove the comments immediately preceding the address-listing, destination-decoding, and own-script filtering logic in the surrounding RPC method, leaving the executable statements and behavior unchanged.Source: Coding guidelines
src/wallet/test/walletload_tests.cpp (1)
38-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove comments that restate the test phases.
Lines 38 and 47 repeat the operations in the following statements. Remove these comments.
Proposed cleanup
std::unique_ptr<WalletDatabase> database = CreateMockWalletDatabase(); { - // Write unknown active descriptor WalletBatch batch(*database, false); std::string unknown_desc = "trx(tpubD6NzVbkrYhZ4Y4S7m6Y5s9GD8FqEMBy56AGphZXuagajudVZEnYyBahZMgHNCTJc2at82YX6s8JiL1Lohu5A3v1Ur76qguNH4QVQ7qYrBQx/86'/1'/0'/0/*)`#8pn8tzdt`"; WalletDescriptor wallet_descriptor(std::make_shared<DummyDescriptor>(unknown_desc), 0, 0, 0, 0); BOOST_CHECK(batch.WriteDescriptor(uint256(), wallet_descriptor)); BOOST_CHECK(batch.WriteActiveScriptPubKeyMan(uint256(), false)); } { - // Now try to load the wallet and verify the error. const std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, std::move(database))); BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::UNKNOWN_DESCRIPTOR);As per coding guidelines, “avoid comments that merely restate code.”
🤖 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/wallet/test/walletload_tests.cpp` around lines 38 - 47, Remove the phase-description comments in the test blocks surrounding WalletBatch setup and wallet loading, including the comments before writing the descriptor and loading the wallet. Leave the test operations and assertions unchanged.Source: Coding guidelines
🤖 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/wallet_resendwallettransactions.py`:
- Around line 91-96: Bound the wallet reload loop around listreceivedbyaddress
by tracking reload attempts and stopping after a finite limit. If the expected
order [child_txid, txid] is not observed, raise an assertion that includes the
final observed txids; retain the existing unloadwallet/loadwallet retry behavior
while attempts remain.
---
Nitpick comments:
In `@src/wallet/rpc/coins.cpp`:
- Around line 23-35: Remove the comments immediately preceding the
address-listing, destination-decoding, and own-script filtering logic in the
surrounding RPC method, leaving the executable statements and behavior
unchanged.
In `@src/wallet/test/walletload_tests.cpp`:
- Around line 38-47: Remove the phase-description comments in the test blocks
surrounding WalletBatch setup and wallet loading, including the comments before
writing the descriptor and loading the wallet. Leave the test operations and
assertions unchanged.
🪄 Autofix
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 Plus
Run ID: 33fdc6e7-2c48-4afb-89a9-e2f556f45be0
📒 Files selected for processing (36)
doc/release-notes-25122.mddoc/release-notes-25504.mdsrc/Makefile.test.includesrc/qt/overviewpage.cppsrc/qt/overviewpage.hsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsdialog.hsrc/qt/test/wallettests.cppsrc/qt/walletmodel.cppsrc/qt/walletmodel.hsrc/rpc/client.cppsrc/wallet/coinselection.cppsrc/wallet/coinselection.hsrc/wallet/receive.cppsrc/wallet/receive.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/coins.cppsrc/wallet/rpc/transactions.cppsrc/wallet/rpc/util.cppsrc/wallet/rpc/util.hsrc/wallet/spend.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/test/fuzz/coinselection.cppsrc/wallet/test/walletload_tests.cppsrc/wallet/transaction.hsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.htest/functional/mempool_expiry.pytest/functional/test_framework/messages.pytest/functional/wallet_basic.pytest/functional/wallet_groups.pytest/functional/wallet_listreceivedby.pytest/functional/wallet_listsinceblock.pytest/functional/wallet_resendwallettransactions.py
💤 Files with no reviewable changes (1)
- src/qt/overviewpage.h
🚧 Files skipped from review as they are similar to previous changes (30)
- doc/release-notes-25504.md
- src/rpc/client.cpp
- src/Makefile.test.include
- src/qt/sendcoinsdialog.h
- src/wallet/walletdb.h
- test/functional/mempool_expiry.py
- test/functional/wallet_listreceivedby.py
- src/qt/walletmodel.h
- test/functional/test_framework/messages.py
- test/functional/wallet_basic.py
- src/wallet/transaction.h
- src/wallet/test/fuzz/coinselection.cpp
- src/qt/sendcoinsdialog.cpp
- src/qt/test/wallettests.cpp
- test/functional/wallet_listsinceblock.py
- src/wallet/walletdb.cpp
- test/functional/wallet_groups.py
- src/wallet/rpc/util.h
- src/qt/walletmodel.cpp
- src/wallet/test/coinselector_tests.cpp
- src/wallet/wallet.h
- src/wallet/coinselection.cpp
- src/wallet/wallet.cpp
- src/wallet/rpc/transactions.cpp
- src/wallet/receive.h
- src/wallet/coinselection.h
- src/wallet/rpc/util.cpp
- src/wallet/rpc/backup.cpp
- src/qt/overviewpage.cpp
- src/wallet/spend.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8171fd3f5e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | ||
| { | ||
| return control && control->HasSelected() ? wallet().getAvailableBalance(*control) : getCachedBalance().balance; |
There was a problem hiding this comment.
For private-keys-disabled watch-only wallets, SendCoinsDialog::updateCoinControlState() sets fAllowWatchOnly before prepareTransaction(), and useAvailableBalance() does the same before asking for the available amount. If the user has not manually selected UTXOs, this helper now returns getCachedBalance().balance, which is the spendable balance and excludes watch_only_balance, instead of calling wallet().getAvailableBalance(*control) with fAllowWatchOnly. As a result, a watch-only wallet with funds but no selected inputs reports zero available balance and fails the upfront AmountExceedsBalance check, even though transaction creation can spend solvable watch-only coins when the control flag is honored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. fAllowWatchOnly changes AvailableCoins spendability filtering, but the new no-manual-selection cache path ignores that flag and returns balances.balance rather than the solvable watch-only amount. Private-keys-disabled watch-only wallets can fail the upfront balance check incorrectly. This needs a Dash-specific adaptation before merge.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The backport introduces one Dash-specific regression: the GUI balance cache bypasses the CoinJoin-filtered balance when no inputs are manually selected, causing mixed-send actions and prechecks to use the wallet's full balance. The rebroadcast timing observation is a pre-existing test defect, and the commit-history suggestion targets the already-merged base rather than this wallet backport.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/walletmodel.cpp`:
- [BLOCKING] src/qt/walletmodel.cpp:648-651: Use the CoinJoin balance for unselected CoinJoin controls
The CoinJoin send dialog calls `UseCoinJoin(true)` but normally has no manually selected inputs. The current `HasSelected()` check therefore returns `getCachedBalance().balance`, which includes unmixed funds, instead of the available fully mixed amount. As a result, “Use available balance” and `prepareTransaction()` can accept an amount greater than the mixed balance before transaction creation fails with insufficient mixed funds. Use the cached anonymized balance when CoinJoin is active, while retaining the wallet calculation for manually selected inputs.
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | ||
| { | ||
| return control && control->HasSelected() ? wallet().getAvailableBalance(*control) : getCachedBalance().balance; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Use the CoinJoin balance for unselected CoinJoin controls
The CoinJoin send dialog calls UseCoinJoin(true) but normally has no manually selected inputs. The current HasSelected() check therefore returns getCachedBalance().balance, which includes unmixed funds, instead of the available fully mixed amount. As a result, “Use available balance” and prepareTransaction() can accept an amount greater than the mixed balance before transaction creation fails with insufficient mixed funds. Use the cached anonymized balance when CoinJoin is active, while retaining the wallet calculation for manually selected inputs.
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | |
| { | |
| return control && control->HasSelected() ? wallet().getAvailableBalance(*control) : getCachedBalance().balance; | |
| } | |
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | |
| { | |
| if (control && control->HasSelected()) { | |
| return wallet().getAvailableBalance(*control); | |
| } | |
| if (control && control->IsUsingCoinJoin()) { | |
| return getCachedBalance().anonymized_balance; | |
| } | |
| return getCachedBalance().balance; | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Use the CoinJoin balance for unselected CoinJoin controls 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.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rechecked the force-pushed head
CodeRabbit's latest run reported no new actionable comments, but it skipped the relevant files as similar to previously reviewed changes, so that result does not clear these existing threads. |
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Carried-forward prior findings: the sole blocking finding, "Use the CoinJoin balance for unselected CoinJoin controls," is FIXED at the current head because unselected CoinJoin controls now use the cached anonymized balance. New latest-delta findings: none; the cumulative backport review found no remaining in-scope correctness, Dash-integration, test-adaptation, or prerequisite issues.
Source: reviewers gpt-5.6-sol (general, dash-core-commit-history, backport-reviewer) and claude-sonnet-5 (dash-core-commit-history, backport-reviewer, plus a fresh general retry after the initial general attempt failed the raw-JSON contract); final verifier gpt-5.6-sol (codex). The openclaw-agent coordinator is orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— final-verifier (Sol fallback after Sonnet verifier contract failures) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed),claude-sonnet-5— general (completed)
|
Follow-up on the automated final validation at
The latest CodeRabbit full review produced no new actionable comments. These two existing findings still need an author-side Dash adaptation before merge. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Carried-forward prior context: the previously indexed blocker, "Use the CoinJoin balance for unselected CoinJoin controls," is FIXED, while the two previously omitted external threads remain valid blockers because advanced CoinJoin progress can retain stale denominated balances and unselected watch-only controls still use the spendable-key cache. New latest-delta findings: none; both active blockers arise from cumulative behavior introduced by the GUI balance-cache backport rather than the latest eight-line CoinJoin fix. Source: reviewers gpt-5.6-sol (general, dash-core-commit-history, backport-reviewer) and claude-sonnet-5 (dash-core-commit-history, backport-reviewer, plus a fresh general retry after the initial general attempt failed the raw-JSON contract); final verifier gpt-5.6-sol (codex). The openclaw-agent coordinator is orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— final-verifier (Sol fallback after Sonnet verifier contract failures) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed),claude-sonnet-5— general (completed)
🔴 2 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/overviewpage.cpp`:
- [BLOCKING] src/qt/overviewpage.cpp:404: Refresh CoinJoin progress from uncached denominated balances
The backport replaces the advanced CoinJoin view's direct `wallet().getBalances()` call with `getCachedBalance()`, but `WalletBalances::balanceChanged()` does not compare `denominated_trusted` or `denominated_untrusted_pending`. These fields can change without any compared field changing: while CoinJoin is disabled, `GetBalance()` leaves both denominated fields at zero; after CoinJoin is re-enabled, a wallet containing denominated but not fully mixed outputs repopulates them while its total, pending, immature, and anonymized balances remain unchanged. `checkBalanceChanged()` then declines to assign the new value, and the advanced progress calculation continues using stale zero denominated balances. Include both denominated fields in cache invalidation, or retain an uncached balance query for the advanced CoinJoin calculation.
In `src/qt/walletmodel.cpp`:
- [BLOCKING] src/qt/walletmodel.cpp:648-656: Honor watch-only coin control without manual inputs
`SendCoinsDialog` sets `fAllowWatchOnly` for private-keys-disabled wallets before both "Use available balance" and `prepareTransaction()`. When no inputs are manually selected, this method nevertheless returns `getCachedBalance().balance`, which contains spendable-key funds and excludes the separately cached watch-only balance. The wallet-level `getAvailableBalance(*control)` path applies `fAllowWatchOnly`, filters for solvable watch-only outputs, and was used before this backport. As a result, a watch-only wallet can display funds but receive `AmountExceedsBalance` before it can create its unsigned PSBT. Delegate to the wallet calculation whenever `fAllowWatchOnly` is set.
| QString strCoinJoinAmount = BitcoinUnits::formatHtmlWithUnit(m_display_bitcoin_unit, clientModel->coinJoinOptions().getAmount() * COIN, false, BitcoinUnits::SeparatorStyle::ALWAYS); | ||
|
|
||
| if(m_balances.balance == 0) | ||
| const auto& balances = walletModel->getCachedBalance(); |
There was a problem hiding this comment.
🔴 Blocking: Refresh CoinJoin progress from uncached denominated balances
The backport replaces the advanced CoinJoin view's direct wallet().getBalances() call with getCachedBalance(), but WalletBalances::balanceChanged() does not compare denominated_trusted or denominated_untrusted_pending. These fields can change without any compared field changing: while CoinJoin is disabled, GetBalance() leaves both denominated fields at zero; after CoinJoin is re-enabled, a wallet containing denominated but not fully mixed outputs repopulates them while its total, pending, immature, and anonymized balances remain unchanged. checkBalanceChanged() then declines to assign the new value, and the advanced progress calculation continues using stale zero denominated balances. Include both denominated fields in cache invalidation, or retain an uncached balance query for the advanced CoinJoin calculation.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Refresh CoinJoin progress from uncached denominated balances 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.
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | ||
| { | ||
| if (control && control->HasSelected()) { | ||
| return wallet().getAvailableBalance(*control); | ||
| } | ||
| if (control && control->IsUsingCoinJoin()) { | ||
| return getCachedBalance().anonymized_balance; | ||
| } | ||
| return getCachedBalance().balance; |
There was a problem hiding this comment.
🔴 Blocking: Honor watch-only coin control without manual inputs
SendCoinsDialog sets fAllowWatchOnly for private-keys-disabled wallets before both "Use available balance" and prepareTransaction(). When no inputs are manually selected, this method nevertheless returns getCachedBalance().balance, which contains spendable-key funds and excludes the separately cached watch-only balance. The wallet-level getAvailableBalance(*control) path applies fAllowWatchOnly, filters for solvable watch-only outputs, and was used before this backport. As a result, a watch-only wallet can display funds but receive AmountExceedsBalance before it can create its unsigned PSBT. Delegate to the wallet calculation whenever fAllowWatchOnly is set.
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | |
| { | |
| if (control && control->HasSelected()) { | |
| return wallet().getAvailableBalance(*control); | |
| } | |
| if (control && control->IsUsingCoinJoin()) { | |
| return getCachedBalance().anonymized_balance; | |
| } | |
| return getCachedBalance().balance; | |
| CAmount WalletModel::getAvailableBalance(const CCoinControl* control) | |
| { | |
| if (control && (control->HasSelected() || control->fAllowWatchOnly)) { | |
| return wallet().getAvailableBalance(*control); | |
| } | |
| if (control && control->IsUsingCoinJoin()) { | |
| return getCachedBalance().anonymized_balance; | |
| } | |
| return getCachedBalance().balance; | |
| } |
source: ['codex']
There was a problem hiding this comment.
that's backported code as it is from mainstream
There was a problem hiding this comment.
The line does match the original gui#598 backport, but upstream later confirmed this exact cached fast path was a bug and fixed it in bitcoin#26699, commit cd98b717398f7b13ace91ea9efac9ce1e60b4d62 (“gui: getAvailableBalance, include watch only balance”). That follow-up explicitly adds the cached watch-only balance for private-keys-disabled wallets; bitcoin#26687 documents the resulting GUI PSBT failure as “The amount exceeds your balance.”
So exactness to gui#598 does not clear this finding—the PR is bringing in gui#598 without its upstream bugfix follow-up. Please backport the relevant bitcoin#26699 commit (or an equivalent fAllowWatchOnly-aware cached path) and its focused regression coverage.
There was a problem hiding this comment.
Resolved in this update — Honor watch-only coin control without manual inputs 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. |
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9119c1e49
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (output_scripts.empty()) { | ||
| throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet"); |
There was a problem hiding this comment.
When getreceivedbylabel is called for a label that exists only on non-owned address-book entries, such as an external recipient labeled through setlabel or a prior send, addresses is non-empty but every script is filtered out by wallet.IsMine(), so this new shared guard throws Address not found in wallet. The backported behavior is supposed to error only when the label is absent from the address book; existing send-only labels should still produce a zero received amount rather than turning into RPC failures.
Useful? React with 👍 / 👎.
| { | ||
| "desc": multi_a, | ||
| "active": False, | ||
| "timestamp": "now", |
There was a problem hiding this comment.
In descriptor-mode runs this block sends and mines txid_a/txid_b before creating the watch-only wallet, then imports both descriptors with timestamp: "now", which skips rescanning the already-confirmed history. As a result wo_wallet.listunspent(minconf=0) will not see those earlier outputs and the new assert_equal(len(coins), 2) path can fail; import the descriptors before funding them or use an old timestamp/rescan for this test.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Dash-specific CoinJoin denomination-cache, mixed-balance, and watch-only balance fixes are present at the exact head. One blocking test-adaptation issue remains: the rebroadcast functional test still asserts Bitcoin Core's 12–36 hour schedule even though Dash intentionally retains a 1–3 hour schedule; the full bitcoin#26205 backport also retains one obsolete explanatory comment.
Source: reviewers gpt-5.6-sol (general, dash-core-commit-history, backport-reviewer); final verifier gpt-5.6-sol (verifier). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(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 `test/functional/wallet_resendwallettransactions.py`:
- [BLOCKING] test/functional/wallet_resendwallettransactions.py:61-78: Adapt the rebroadcast test to Dash's 1–3 hour timer
The test still asserts that no rebroadcast occurs before 12 hours and advances 36 hours to guarantee one, but Dash intentionally keeps `GetDefaultNextResend()` at one hour plus a random delay below two hours. The backport also initializes `m_next_resend` during wallet construction instead of using the old first scheduler invocation solely to initialize it. In this framework, cached-chain setup advances mock time by more than the three-hour upper bound; the first `wait_p2p()` scheduler tick therefore resets the timer, and the scheduler call near 12 hours subsequently resubmits the transaction to `peer_second`, failing line 67. Test just below the one-hour lower bound and then at the three-hour upper bound, and update the later 36-hour timer assumptions to match Dash's retained interval.
In `src/wallet/wallet.cpp`:
- [NITPICK] src/wallet/wallet.cpp:2139-2143: bitcoin#26205 leaves the obsolete startup-resend explanation behind
The full bitcoin#26205 backport initializes `m_next_resend` with `GetDefaultNextResend()` and updates it only after `ShouldResend()` permits a periodic resend. Startup resubmission therefore no longer initializes the timer, but this comment still says that it does. Remove the clause stating that startup processing initializes `m_next_resend`, matching upstream commit `9245f456705b285e2d9afcc01a6155e1b3f92fad`.
| @@ -70,14 +69,56 @@ def wait_p2p(): | |||
| self.log.info("Bump time & check that transaction is rebroadcast") | |||
| # Transaction should be rebroadcast approximately 24 hours in the future, | |||
| # but can range from 12-36. So bump 36 hours to be sure. | |||
| with node.assert_debug_log(['ResendWalletTransactions: resubmit 1 unconfirmed transactions']): | |||
| with node.assert_debug_log(['resubmit 1 unconfirmed transactions']): | |||
| node.setmocktime(self.mocktime + 36 * 60 * 60) | |||
| # Tell scheduler to call MaybeResendWalletTxs now. | |||
| node.mockscheduler(60) | |||
| # Give some time for trickle to occur | |||
| node.setmocktime(self.mocktime + 36 * 60 * 60 + 600) | |||
| peer_second.wait_for_broadcast([txid]) | |||
There was a problem hiding this comment.
🔴 Blocking: Adapt the rebroadcast test to Dash's 1–3 hour timer
The test still asserts that no rebroadcast occurs before 12 hours and advances 36 hours to guarantee one, but Dash intentionally keeps GetDefaultNextResend() at one hour plus a random delay below two hours. The backport also initializes m_next_resend during wallet construction instead of using the old first scheduler invocation solely to initialize it. In this framework, cached-chain setup advances mock time by more than the three-hour upper bound; the first wait_p2p() scheduler tick therefore resets the timer, and the scheduler call near 12 hours subsequently resubmits the transaction to peer_second, failing line 67. Test just below the one-hour lower bound and then at the three-hour upper bound, and update the later 36-hour timer assumptions to match Dash's retained interval.
source: ['codex']
There was a problem hiding this comment.
tests succeed apparently?
There was a problem hiding this comment.
Resolved in this update — Adapt the rebroadcast test to Dash's 1–3 hour timer 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.
| // Resubmit transactions from the wallet to the mempool, optionally asking the | ||
| // mempool to relay them. On startup, we will do this for all unconfirmed | ||
| // transactions but will not ask the mempool to relay them. We do this on startup | ||
| // to ensure that our own mempool is aware of our transactions, and to also | ||
| // initialize m_next_resend so that the actual rebroadcast is scheduled. There |
There was a problem hiding this comment.
💬 Nitpick: bitcoin#26205 leaves the obsolete startup-resend explanation behind
The full bitcoin#26205 backport initializes m_next_resend with GetDefaultNextResend() and updates it only after ShouldResend() permits a periodic resend. Startup resubmission therefore no longer initializes the timer, but this comment still says that it does. Remove the clause stating that startup processing initializes m_next_resend, matching upstream commit 9245f456705b285e2d9afcc01a6155e1b3f92fad.
source: ['codex']
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 `@src/wallet/spend.cpp`:
- Around line 998-1002: Update the change-output tracking around nChangePosInOut
so it preserves the specific change output through BIP69 sorting instead of
rediscovering it with value-based std::find, which can select an equal recipient
output. Port the corresponding upstream identity-preserving fix or retain an
explicit marker through sorting, and add a regression test covering equal
recipient and change outputs without introducing Dash-specific behavior.
🪄 Autofix
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 Plus
Run ID: ea87965f-4980-4458-9d47-fc68499fb458
📒 Files selected for processing (5)
src/Makefile.test.includesrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsdialog.hsrc/wallet/spend.cppsrc/wallet/wallet.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/qt/sendcoinsdialog.h
- src/qt/sendcoinsdialog.cpp
- src/wallet/wallet.h
- src/Makefile.test.include
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Dash correctly retains its 1–3-hour rebroadcast interval, but the imported functional test still assumes Bitcoin Core's 12–36-hour interval, making its no-rebroadcast assertion unreliable and capable of failing when the overdue timer runs. The obsolete startup-timer comment also remains; the omitted bitcoin#26699 tests were explicitly excluded by the declared partial backport, and the CodeRabbit change-identity finding is refuted because value-identical outputs are interchangeable after sorting.
Source: reviewers gpt-5.6-sol (general, dash-core-commit-history, backport-reviewer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
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 `test/functional/wallet_resendwallettransactions.py`:
- [BLOCKING] test/functional/wallet_resendwallettransactions.py:61-78: Adapt the rebroadcast test to Dash's 1–3 hour timer
(existing thread: https://github.com/dashpay/dash/pull/7550#discussion_r3761405080)
The test still checks that no rebroadcast occurs just before 12 hours and advances 36 hours to force one, while `CWallet::GetDefaultNextResend()` intentionally schedules Dash rebroadcasts one hour plus a random delay below two hours ahead. The cached-chain setup advances mock time by more than that three-hour upper bound after the wallet constructs its initial timer. Therefore, the scheduler call near 12 hours can already relay the transaction to `peer_second`; the immediate inventory assertion either fails or passes only because P2P delivery loses the race. Reset the overdue timer before connecting `peer_second`, check immediately below Dash's one-hour lower bound, advance to the three-hour upper bound to require the rebroadcast, and update the later 36-hour timer assumption accordingly.
In `src/wallet/wallet.cpp`:
- [NITPICK] src/wallet/wallet.cpp:2139-2143: bitcoin#26205 leaves the obsolete startup-resend explanation behind
(existing thread: https://github.com/dashpay/dash/pull/7550#discussion_r3761405084)
`m_next_resend` is now initialized by its member initializer with `GetDefaultNextResend()` and is updated after `ShouldResend()` permits a periodic resend. Startup resubmission no longer initializes the timer, so the retained explanation contradicts the behavior introduced by bitcoin#26205. Remove the obsolete initialization clause, matching upstream commit `9245f456705b285e2d9afcc01a6155e1b3f92fad`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f875861c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (control && control->IsUsingCoinJoin()) { | ||
| return balances.anonymized_balance; | ||
| } | ||
| CAmount available_balance = balances.balance; |
There was a problem hiding this comment.
When no inputs are manually selected, this branch returns the cached trusted balance. That cache is built by GetBalance() and does not consult setLockedCoins, while the previous wallet().getAvailableBalance(*control) path goes through AvailableCoins(), which skips wallet.IsLockedCoin(). If a user locks UTXOs via coin control or dust protection and then uses the send dialog without selecting coins, the GUI can advertise locked funds as spendable and only fail later during transaction creation; keep the wallet availability query or subtract locked outputs here.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Two blocking issues remain: the rebroadcast functional test does not deterministically reset the initially overdue Dash timer, and the cached standard-send balance includes locked outputs that transaction creation cannot spend. The bitcoin#26205 backport also retains an obsolete timer comment from a hunk that its declared upstream commit removed.
Source: reviewers gpt-5.6-sol (general, dash-core-commit-history, backport-reviewer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
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/qt/walletmodel.cpp`:
- [BLOCKING] src/qt/walletmodel.cpp:651-662: Exclude locked outputs from the cached send balance
The unselected standard-send path returns the trusted cached balance, but `GetBalance()` does not exclude `setLockedCoins`. The previous `wallet().getAvailableBalance(*control)` path uses `AvailableCoins()`, which skips `IsLockedCoin()` outputs. Consequently, manually locked outputs—and outputs automatically locked by Dash dust protection—are advertised by “Use available balance” and counted by `prepareTransaction()`, even though transaction creation cannot select them. Preserve the cache fast path only when the standard wallet has no locked outputs; otherwise use the wallet-level availability calculation.
In `test/functional/wallet_resendwallettransactions.py`:
- [BLOCKING] test/functional/wallet_resendwallettransactions.py:45-78: Adapt the rebroadcast test to Dash's 1–3 hour timer
(existing thread: https://github.com/dashpay/dash/pull/7550#discussion_r3761405080)
The updated boundaries do not make this test deterministic. The wallet constructs `m_next_resend` before cached-chain setup advances mock time by about 8.7 hours, leaving the timer overdue. `wait_p2p()` only shifts and wakes the scheduler and does not wait for `MaybeResendWalletTxs()` to finish, so the overdue callback may run after `peer_second` is connected or during the scheduler call at line 66. It can then relay `txid` before the negative assertion and reset the timer relative to that callback, which also means the nominal three-hour check is not guaranteed to reach the newly randomized deadline. Run and synchronize an initial overdue callback before connecting `peer_second`, measure the one- and three-hour boundaries from that reset time, and replace the remaining 36-hour upper-bound assumption at lines 116–118 with Dash's three-hour bound.
In `src/wallet/wallet.cpp`:
- [NITPICK] src/wallet/wallet.cpp:2139-2144: bitcoin#26205 leaves the obsolete startup-resend explanation behind
(existing thread: https://github.com/dashpay/dash/pull/7550#discussion_r3761405084)
`m_next_resend` is initialized by its member initializer with `GetDefaultNextResend()` and is updated after `ShouldResend()` permits a periodic resend. Startup `postInitProcess()` calls `ResubmitWalletTransactions()` without initializing the timer, so the statement that startup processing initializes `m_next_resend` contradicts the implementation. Upstream bitcoin#26205 commit `9245f456705b285e2d9afcc01a6155e1b3f92fad` removed this exact clause; apply that omitted hunk.
|
Review outcome: REQUIRES CHANGES. Current CI is green, but one Dash-specific correctness blocker remains.
I mapped all declared upstream PRs against the current head. The other partial hunks are explained Dash adaptations—principally no-SegWit script forms and older wallet APIs. The CoinJoin denomination-cache and watch-only defects are fixed at the latest head. The open send-only-label and descriptor-import findings are false positives. Changed Python files compile, and the PR merges cleanly with current Codex review ^^^ |
knst
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
One blocking Dash-specific test adaptation remains: the rebroadcast test does not synchronize the initially overdue wallet timer before checking Dash's 1–3-hour boundaries. The locked-output regression is fixed at this head, while the advertised bitcoin#26205 backport still omits the upstream cleanup of an obsolete timer comment.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
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 `test/functional/wallet_resendwallettransactions.py`:
- [BLOCKING] test/functional/wallet_resendwallettransactions.py:45-78: Adapt the rebroadcast test to Dash's 1–3 hour timer
(existing thread: https://github.com/dashpay/dash/pull/7550#discussion_r3761405080)
The updated one-hour and three-hour boundaries are still nondeterministic. Cached-chain setup advances mock time by roughly 8.7 hours after the wallet initializes `m_next_resend`, so the timer is already overdue when the test begins. `mockscheduler()` only shifts the scheduler queue and wakes its worker; it does not wait for `MaybeResendWalletTxs()` to complete. The overdue callback can therefore run after `peer_second` is connected, racing the negative assertion at line 67, or reset the timer near the one-hour check so that the absolute three-hour check no longer necessarily reaches the new randomized deadline. Run and synchronize the overdue callback before connecting `peer_second`, measure both boundaries from the resulting reset time, and replace the remaining 36-hour assumptions at lines 77 and 117 with Dash's three-hour upper bound.
In `src/wallet/wallet.cpp`:
- [NITPICK] src/wallet/wallet.cpp:2139-2144: bitcoin#26205 leaves the obsolete startup-resend explanation behind
(existing thread: https://github.com/dashpay/dash/pull/7550#discussion_r3761405084)
`m_next_resend` is initialized by its member initializer with `GetDefaultNextResend()` and is updated only after `ShouldResend()` permits a periodic resend. Startup `postInitProcess()` calls `ResubmitWalletTransactions()` without initializing the timer, so the statement that startup processing initializes `m_next_resend` contradicts the implementation. Upstream bitcoin#26205 commit `9245f456705b285e2d9afcc01a6155e1b3f92fad` removed this exact clause, but the advertised full backport omitted that hunk.
|
@thepastaclaw finding: That is backported behaviour, I believe it's a bug in mainstream ; because functional test succeed even with wrong time. |
|
This pull request has conflicts, please rebase. |
…esses were found in the address book baa3ddc doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy) 8897a21 rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy) Pull request description: Built on top of bitcoin#23662, coming from comment bitcoin#23662 (review). If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away. Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty). ACKs for top commit: achow101: ACK baa3ddc theStack: re-ACK baa3ddc w0xlt: ACK bitcoin@baa3ddc Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911 Co-authored-by: Andrew Chow <achow101-github@achow101.com>
a6b0c1f doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot) 0fd2d14 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot) 55f98d0 rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot) b724476 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot) 55a82ea wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot) Pull request description: Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends). It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit. I'll add this to the output of `listunspent` too if this gets a few Concept ACKs. ACKs for top commit: instagibbs: ACK bitcoin@a6b0c1f achow101: re-ACK a6b0c1f Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37 Co-authored-by: Andrew Chow <achow101-github@achow101.com>
4fef534 wallet: use GetChange() when computing waste (S3RK) 87e0ef9 wallet: use GetChange() in tx building (S3RK) 15e97a6 wallet: add SelectionResult::GetChange (S3RK) 72cad28 wallet: calculate and store min_viable_change (S3RK) e3210a7 wallet: account for preselected inputs in target (S3RK) f8e7963 wallet: add SelectionResult::Merge (S3RK) 06f558e wallet: accurate SelectionResult::m_target (S3RK) c8cf08e wallet: ensure m_min_change_target always covers change fee (S3RK) Pull request description: Benefits: 1. more accurate waste calculation for knapsack. Waste calculation is now consistent with tx building code. Before we always assumed change for knapsack even when the solution is changeless4. 2. simpler tx building code. Only create change output when it's needed 3. makes it easier to correctly account for fees for CPFP inputs (should be done in a follow up) In the first three commits we fix the code to accurately track selection target in `SelectionResult::m_target` Then we introduce new variable `min_change` that represents the minimum viable change amount Then we introduce `SelectionResult::GetChange()` which incapsulates dropping change for fee logic and uses correct values of `SelectionResult::m_target` Then we use `SelectionResult::GetChange()` in both tx building and waste calculation code This PR is a refactoring and shouldn't change the behaviour. There is only one known small change (arguably a bug fix). Before we dropped change output if it's smaller than `cost_of_change` after paying change fees. This is incorrect as `cost_of_change` already includes `change_fee`. ACKs for top commit: achow101: ACK 4fef534 Xekyo: crACK 4fef534 furszy: Code review ACK 4fef534 w0xlt: ACK bitcoin@4fef534 Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941 Co-authored-by: Andrew Chow <github@achow101.com>
…nwallet/rescanblockchain/) e90a445 scripted-diff: rpc: fix rescan RPC name (s/rescanwallet/rescanblockchain/) (Sebastian Falbesoner) Pull request description: There is no RPC call named `rescanwallet`, i.e. fix this by renaming to the actual RPC called `rescanblockchain`. ACKs for top commit: achow101: ACK e90a445 aureleoules: ACK e90a445. promag: ACK e90a445 Tree-SHA512: abf1d1c18de32d87c29e4ff2b782dfb0e4a46dc2c2cc51ab616d12674a0f4a5d22214e00955663ae897cbb88f4f6ced913850f28ea3f5c1b3a54577a25fbf399 Co-authored-by: Andrew Chow <achow101-github@achow101.com>
…tion chains 3405f3e test: Test that an unconfirmed not-in-mempool chain is rebroadcast (Andrew Chow) 10d91c5 wallet: Deduplicate Resend and ReacceptWalletTransactions (Andrew Chow) Pull request description: Currently `ResendWalletTransactions` (used for normal rebroadcasts) will attempt to rebroadcast all of the transactions in the wallet in the order they are stored in `mapWallet`. This ends up being random as `mapWallet` is a `std::unordered_map`. However `ReacceptWalletTransactions` (used for adding to the mempool on loading) first sorts the txs by wallet insertion order, then submits them. The result is that `ResendWalletTranactions` will fail to rebroadcast child transactions if their txids happen to be lexicographically less than their parent's txid. This PR resolves this issue by combining `ReacceptWalletTransactions` and `ResendWalletTransactions` into a new `ResubmitWalletTransactions` so that the iteration code and basic checks are shared. A test has also been added that checks that such transaction chains are rebroadcast correctly. ACKs for top commit: naumenkogs: utACK 3405f3e 1440000bytes: reACK bitcoin@3405f3e furszy: Late code review ACK 3405f3e stickies-v: ACK 3405f3e Tree-SHA512: 1240d9690ecc2ae8d476286b79e2386f537a90c41dd2b8b8a5a9c2a917aa3af85d6aee019fbbb05e772985a2b197e2788305586d9d5dac78ccba1ee5aa31d77a Co-authored-by: glozow <gloriajzhao@gmail.com>
fa1ce96 test: Add missing syncwithvalidationinterfacequeue (MacroFake) faa4916 test/doc: Remove unused syncwithvalidationinterfacequeue (MacroFake) Pull request description: Fixes bitcoin#26071 ACKs for top commit: achow101: ACK fa1ce96 glozow: ACK fa1ce96 w0xlt: ACK bitcoin@fa1ce96 Tree-SHA512: d1e101b55477360ead2b99ade5d42b922aabe293ec84fb26764e29161c5be6c534aef6f22d2cc5ea63a4bd6b6e77b701f1a7a2283b8e7e815d343a604cd77656 Co-authored-by: Andrew Chow <github@achow101.com>
… BDB-only wallets 9f3a315 test: Fix `wallet_listsinceblock.py` for BDB-only wallets (Hennadii Stepanov) 1941ce6 test: Fix `wallet_basic.py` for BDB-only wallets (Hennadii Stepanov) Pull request description: Fixes bitcoin#26029. ACKs for top commit: brunoerg: crACK 9f3a315 Tree-SHA512: d31c76e558dedea689ff487644e9f2d2f1df1cc2bb9bb041ede4b272884871167fdb19ccc717394c6ba6af8b8c70e9575b344988e0ce55b241a3a4922d0b7f73 Co-authored-by: MacroFake <falke.marco@gmail.com>
…mic fees in wallet_groups.py BACKPORT NOTE: This backport pinned every node to 20 duffs/byte via `-paytxfee`, but the `maxapsfee` expectations were left at their dynamic-fee values. Upstream had already recomputed them for 20 sat/vB before that commit landed, so it carried no such update and the mismatch was invisible in the diff. The expected debug lines still spelled out the transaction sizes (225/372 and 519/813 bytes) rather than the fees the wallet now logs, and the `maxapsfee` thresholds still bracketed the un-pinned grouped/non-grouped delta of 294 duffs instead of 5880, so node 3 picked the non-grouped solution where the test expects the grouped one. ------------ 2186608 test: apply fixed feerate to avoid variable dynamic fees (stickies-v) Pull request description: Without specifying a feerate, we let the wallet decide on an appropriate feerate, which can be influenced by various factors such as what's in the mempool. Since wallet_groups.py fails when feerates are unstable, we should use a fixed feerate across all nodes. The assumed feerate was 20 sats/vbyte, so this PR adopts that. Closes bitcoin#25940. I'm not 100% sure, but I think the increased tx relay speed introduced by bitcoin#25865 caused the transactions to more quickly and often enter the other nodes' mempools, affecting their feerate calculation done in [`wallet:GetMinimumFeeRate()`](https://github.com/bitcoin/bitcoin/blob/ea67232cdb80c4bc3f16fcd823f6f811fd8903e1/src/wallet/fees.cpp#L68-L72) and thus deviating slightly from the expected 20 sats/vbyte. Ran `wallet_groups.py` over 400 times without failure. ACKs for top commit: aureleoules: ACK 2186608. glozow: Approach ACK 2186608 Tree-SHA512: 0ea467a67747e6f27369ccd0adacfb21cc36ef0ae728fb28b8ea18e409aab5bd3ede559d6cebb82da0b9703c0c8b2709d686feb3ae009ddf525aa253f44d5816 Co-authored-by: MacroFake <falke.marco@gmail.com>
…rrupt descriptor causes a fatal error e066763 wallet: coverage for loading an unknown descriptor (furszy) d26c3cc wallet: bugfix, load wallet with an unknown descriptor cause fatal error (furszy) Pull request description: Fixes bitcoin#26015 If the descriptor entry is unrecognized (due a soft downgrade) or corrupt, the unserialization fails and `LoadWallet`, instead of stop there and return the error, continues reading all the db records. As other records tied to the unrecognized or corrupt descriptor are scanned, a fatal error is being thrown. This fixes it by catching the descriptor parse failure and return which wallet failed. Logging its name/path, so the user can remove it from the settings file, to prevent its load at startup. Note: added the test in a separate file intentionally. Will continue adding coverage for the wallet load process in follow-up PRs. ACKs for top commit: achow101: ACK e066763 Sjors: re-utACK e066763 Tree-SHA512: d1f1a5d7e944c89c97a33b25b4411a36a11edae172c22f8524f69c84a035f84c570b284679f901fe60f1300f781b76a6c17b015a8e7ad44ebd25a0c295ef260f Co-authored-by: Andrew Chow <github@achow101.com>
b01682a refactor: revert m_next_resend to not be std::atomic (stickies-v) 9245f45 wallet: only update m_next_resend when actually resending (stickies-v) 7fbde8a refactor: carve out tx resend timer logic into ShouldResend (stickies-v) 01f3534 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v) c6e8e11 wallet: fix capitalization in docstring (stickies-v) Pull request description: This PR addresses the outstanding comments/issues from bitcoin#25768: - capitalization [typo](bitcoin#25768 (comment)) in docstring - remove [unused locks](bitcoin@01f3534) that we previously needed for `ReacceptWalletTransactions()` - before bitcoin#25768, only `ResendWalletTransactions()` would reset `m_next_resend` (formerly called `nNextResend`). By unifying it with `ReacceptWalletTransactions()` into `ResubmitWalletTransactions()`, the number of callsites that would reset the `m_next_resend` timer increased - since `m_next_resend` is only used in case of `relay=true` (formerly `ResendWalletTransactions()`), this is unintuitive - it leads to [unexpected behaviour](bitcoin#25768 (comment)) such as transactions potentially never being rebroadcasted. - it makes the ResubmitWalletTransactions()` logic [more complicated than strictly necessary](bitcoin#25768 (comment)) - since bitcoin#25768, we relied on an earlier call of `ResubmitWalletTransactions(relay=false, force=true)` to initialize `m_next_resend()`, I think we can more elegantly do that by just providing `m_next_resend` with a default value - just to highlight: this commit introduces behaviour change Note: the `if (!fBroadcastTransactions)` in `CWallet:ShouldResend()` is duplicated on purpose, since it potentially avoids the slightly more expensive `if (!chain().isReadyToBroadcast())` check afterwards. I don't have a strong view on it, so happy to remove that additional check to reduce the diff, too. ACKs for top commit: aureleoules: ACK b01682a achow101: ACK b01682a Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c Co-authored-by: fanquake <fanquake@gmail.com>
…g target BACKPORT NOTE to Dash Core Kept upstream's numbers even though Dash reads `SetInputWeight()` through `GetVirtualTransactionSize(nSize, 0, 0)`, which is the identity here rather than upstream's weight-to-vsize division: the input costs 148 bytes instead of 37, so the coin's effective value is 99556 rather than 99889. Either way it stays below the 99900 target, which is what the test needs. ---- d0d9cf7 test: Check external coin effective value is used in CoinSelection (Aurèle Oulès) 76b79c1 wallet: Use correct effective value when checking target (Aurèle Oulès) Pull request description: Fixes bitcoin#26185. The following assert failed because it was not checked in the parent function. https://github.com/bitcoin/bitcoin/blob/2bd9aa5a44b88c866c4d98f8a7bf7154049cba31/src/wallet/coinselection.cpp#L391 ACKs for top commit: glozow: reACK d0d9cf7 furszy: ACK d0d9cf7 Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9 Co-authored-by: glozow <gloriajzhao@gmail.com>
031a6cc to
6d1968b
Compare
3 participants
Issue being fixed or feature implemented
Multiple wallet-relevant backports
PR is slightly over-sized because some of them depends on each other ; other are conflicting if goes to own branch.
What was done?
Backports from bitcoin core v24, v25 with wallet-related code.
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
See relevant release notes changes:
Wallet
getreceivedbylabelnow returns an error, "Label not foundin wallet" (-4), if the label is not in the address book. (rpc: getreceivedbylabel, return early if no addresses were found in the address book bitcoin/bitcoin#25122)
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.