backport: bitcoin#22323, #22929, #23172, #23202, #23659, #23688, #23835, #24083, #25679, #25931, #26156 (improvements for external signer: rpc, tests) #7025
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis PR makes widespread wallet and script API changes: const-correctness for signature checkers; replaces global dummy signature objects with reference-bound dummy creators; changes ScriptPubKeyMan lookup from single-manager to multi-manager ( Sequence Diagram(s)(omitted) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (3)
test/functional/wallet_abandonconflict.py (1)
120-120: Optional: simplify membership test for lint (not invsnot X in)Line 120 currently uses
assert not txABC2 in [...], which flake8/ruff flag as E713. You can make this more idiomatic (and silence the lint) without changing behavior:- assert not txABC2 in [utxo["txid"] for utxo in alice.listunspent(0)] + assert txABC2 not in [utxo["txid"] for utxo in alice.listunspent(0)]src/wallet/rpc/spend.cpp (1)
358-382:FundTxDochelper cleanly deduplicates funding RPC docsCentralizing the
conf_target/estimate_mode/solving_dataargument descriptions inFundTxDoc()reduces duplication and keeps the shared funding‑related RPC docs in sync without affecting behavior.src/wallet/rpc/backup.cpp (1)
1989-2107: Deterministiclistdescriptorsoutput and extended metadata look consistentThe two‑phase collection into
WalletDescInfo, lexicographic sort bydescriptor, and emission of optionalinternal,range,next(_index),coinjoin, and mnemonic fields are internally consistent and align with the stated behavior change (sorted descriptors and extra metadata). Note thatcoinjoin/mnemonic fields are only emitted for ranged descriptors viainfo.range.has_value(), which matches current usage where only keypool‑managed (ranged) BIP39 descriptor sets carry mnemonics; if you ever introduce non‑ranged mnemonic descriptors, you may want to move those fields outside theif (info.range)block or document this assumption.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/node/interfaces.cpp(1 hunks)src/script/interpreter.h(1 hunks)src/script/sign.cpp(1 hunks)src/script/sign.h(1 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/spend.cpp(7 hunks)src/wallet/scriptpubkeyman.cpp(5 hunks)src/wallet/scriptpubkeyman.h(5 hunks)src/wallet/spend.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)src/wallet/wallet.cpp(5 hunks)src/wallet/wallet.h(3 hunks)test/functional/rpc_fundrawtransaction.py(1 hunks)test/functional/wallet_abandonconflict.py(5 hunks)test/functional/wallet_listdescriptors.py(1 hunks)test/functional/wallet_listtransactions.py(2 hunks)test/lint/spelling.ignore-words.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/script/sign.hsrc/wallet/test/wallet_tests.cppsrc/wallet/scriptpubkeyman.cppsrc/script/interpreter.hsrc/wallet/wallet.hsrc/wallet/spend.cppsrc/wallet/rpc/spend.cppsrc/node/interfaces.cppsrc/wallet/rpc/addresses.cppsrc/wallet/wallet.cppsrc/script/sign.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/rpc/backup.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/wallet/test/wallet_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/test/wallet_tests.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/wallet.hsrc/wallet/spend.cppsrc/wallet/rpc/spend.cppsrc/wallet/rpc/addresses.cppsrc/wallet/wallet.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/rpc/backup.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/wallet_abandonconflict.pytest/functional/wallet_listdescriptors.pytest/functional/rpc_fundrawtransaction.pytest/functional/wallet_listtransactions.py
src/node/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Files:
src/node/interfaces.cpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/test/wallet_tests.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/wallet.hsrc/wallet/rpc/spend.cppsrc/wallet/rpc/addresses.cpptest/functional/rpc_fundrawtransaction.pysrc/wallet/scriptpubkeyman.hsrc/wallet/rpc/backup.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/wallet/test/wallet_tests.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/wallet/test/wallet_tests.cpptest/functional/rpc_fundrawtransaction.py
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/test/wallet_tests.cppsrc/wallet/wallet.hsrc/wallet/rpc/addresses.cppsrc/wallet/wallet.cpp
📚 Learning: 2025-06-20T23:32:16.225Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Applied to files:
test/functional/wallet_abandonconflict.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/wallet/scriptpubkeyman.cppsrc/wallet/wallet.hsrc/wallet/spend.cppsrc/wallet/scriptpubkeyman.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/wallet/spend.cppsrc/wallet/scriptpubkeyman.h
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/wallet/spend.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/scriptpubkeyman.hsrc/wallet/rpc/backup.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/wallet/scriptpubkeyman.h
🧬 Code graph analysis (9)
src/script/sign.h (1)
src/script/sign.cpp (1)
DUMMY_CHECKER(364-364)
src/wallet/test/wallet_tests.cpp (1)
src/rpc/request.cpp (1)
error(58-58)
test/functional/wallet_abandonconflict.py (4)
test/functional/test_framework/test_node.py (1)
get_wallet_rpc(363-369)test/functional/test_framework/test_framework.py (2)
sync_mempools(834-860)disconnect_nodes(730-764)test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)src/wallet/rpc/transactions.cpp (4)
abandontransaction(803-842)abandontransaction(803-803)gettransaction(694-801)gettransaction(694-694)
src/wallet/wallet.h (2)
src/wallet/wallet.cpp (13)
IsMine(1484-1488)IsMine(1484-1484)IsMine(1490-1494)IsMine(1490-1490)IsMine(1496-1504)IsMine(1496-1496)IsMine(1506-1513)IsMine(1506-1506)IsMine(1515-1526)IsMine(1515-1515)outpoint(1094-1094)IsInternalScriptPubKeyMan(4005-4027)IsInternalScriptPubKeyMan(4005-4005)src/wallet/scriptpubkeyman.h (5)
ScriptPubKeyMan(170-233)ScriptPubKeyMan(178-178)bool(185-185)bool(230-230)bool(236-236)
test/functional/rpc_fundrawtransaction.py (2)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)src/wallet/rpc/spend.cpp (2)
fundrawtransaction(554-644)fundrawtransaction(554-554)
test/functional/wallet_listtransactions.py (2)
src/wallet/rpc/addresses.cpp (4)
getnewaddress(17-56)getnewaddress(17-17)getaddressinfo(418-563)getaddressinfo(418-418)src/wallet/rpc/transactions.cpp (2)
listtransactions(442-552)listtransactions(442-442)
src/wallet/wallet.cpp (4)
src/wallet/scriptpubkeyman.cpp (8)
IsMine(165-177)IsMine(165-165)IsMine(179-183)IsMine(179-179)IsMine(1854-1861)IsMine(1854-1854)IsMine(1863-1867)IsMine(1863-1863)src/wallet/scriptpubkeyman.h (2)
IsMine(180-180)IsMine(181-181)src/wallet/wallet.h (1)
cs_wallet(604-604)src/wallet/external_signer_scriptpubkeyman.cpp (2)
GetExternalSigner(42-51)GetExternalSigner(42-42)
src/wallet/scriptpubkeyman.h (1)
src/wallet/scriptpubkeyman.cpp (6)
MarkUnusedAddresses(309-344)MarkUnusedAddresses(309-309)MarkUnusedAddresses(2045-2071)MarkUnusedAddresses(2045-2045)MarkReserveKeysAsUsed(1650-1675)MarkReserveKeysAsUsed(1650-1650)
src/wallet/rpc/backup.cpp (2)
src/wallet/hdchain.h (1)
BIP32_PURPOSE_FEATURE(145-145)src/script/descriptor.cpp (1)
nullopt(656-656)
🪛 Flake8 (7.3.0)
test/functional/wallet_abandonconflict.py
[error] 79-79: block comment should start with '# '
(E265)
[error] 120-120: test for membership should be 'not in'
(E713)
🪛 Ruff (0.14.6)
test/functional/wallet_abandonconflict.py
120-120: Test for membership should be not in
Convert to not in
(E713)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (30)
src/node/interfaces.cpp (1)
583-589: Minor documentation fix approved.This comment correction (spelling fix and clarity improvement) is a standard backport improvement. The change has no functional impact and aligns with typical upstream Bitcoin PR cleanups.
test/functional/wallet_abandonconflict.py (2)
35-175: Multi‑walletalice/bobrefactor and wallet‑scoped RPC usage look soundThe changes to introduce
aliceandbobwallet RPC handles, route balance/tx calls throughalice, and re‑acquirealiceafter eachrestart_node()are consistent and preserve the original test semantics. Balance math, use ofgetbalances()['mine'], and the abandon/unabandon flows all line up correctly with the expected confirmation and mempool states.
177-235: Receiver‑side (bob) walletconflicts coverage is correct and well‑scopedThe new
bobwallet flow—explicitloadwallet("bob"), paying the double‑spend tobob, and then assertingconfirmations == -1/walletconflictsfortxAB1plus a single confirmed double‑spend entry—matches the intended conflict behavior from the receiver’s perspective and doesn’t interfere with the existing sender‑side checks.src/wallet/rpc/addresses.cpp (1)
509-546: Multi-ScriptPubKeyManhandling ingetaddressinfolooks correctUsing
GetScriptPubKeyMans(scriptPubKey)and picking the first manager, then reusingspk_manboth forparent_descand metadata, is null‑safe and consistent with the new multi‑manager API. No issues spotted here.test/functional/wallet_listtransactions.py (3)
6-21: Imports and node topology updates are appropriate for new scenariosAdding
os,shutil, andassert_raises_rpc_errormatches later usage, and bumpingself.num_nodesto 3 is required for the wallet copy onto node2.extra_argsis already derived fromself.num_nodes, so startup semantics remain consistent.
105-161: Externally generated address test is soundThe new
run_externally_generated_address_testcorrectly:
- Refills the keypool before copying
wallet.datto node2.- Stops nodes before file operations and restarts them with the same datadirs.
- Reconnects the 3‑node topology explicitly.
- Normalizes
listtransactionsby stripping label/time fields and sorting bytxidbefore comparing node0 vs node2.- Verifies that address labels remain only on the node that actually generated the addresses.
This matches expected external‑signer / multi‑wallet semantics.
162-167:listtransactionsparameter validation coverage is adequate
run_invalid_parameters_testexercises:
- Empty
label→ specific RPC error."*"as a valid label wildcard.- Negative
countandskip(“from”) arguments.The error codes/messages align with the
listtransactionsRPC implementation, so this adds useful regression coverage.src/wallet/spend.cpp (2)
528-547: Preset input sizing correctly incorporates external signing providerThe updated preset‑input loop now:
- Uses wallet metadata when the input is in
mapWallet.- Falls back to
coin_control.GetExternalOutputwhen it is not.- If
input_bytesis still-1, computes it viacoin_control.m_external_provider.This ensures fee estimation works for externally provided inputs while still rejecting unsignable inputs (
input_bytes == -1) early.
1168-1177:FundTransactioninternal vs external input handling is clearer and saferClassifying inputs as:
- Internal when
wallet.IsMine(outPoint)is true (wallet‑owned/watched).- External when not mine but present in the UTXO set, via
coinControl.SelectExternal.- Failing with a clear error if the external UTXO cannot be found,
removes the previous heuristic and makes external‑signer flows and error reporting more explicit.
src/wallet/scriptpubkeyman.cpp (3)
309-344: LegacyMarkUnusedAddresses’ new return value is consistent with keypool semanticsReturning
std::vector<WalletDestination>and populating it from the keys removed byMarkReserveKeysAsUsed(withPKHashandfInternal) gives callers precise knowledge of which addresses were actually consumed while preserving existing keypool‑topup and metadata‑update behavior.
1650-1675:MarkReserveKeysAsUsedcorrectly tracks and returns consumed keypool entriesRefactoring
MarkReserveKeysAsUsedto:
- Determine the correct internal vs external pool.
- Remove all entries up to
keypool_idfrom the chosen set and DB.- Erase
m_pool_key_to_indexmappings.- Accumulate the corresponding
CKeyPoolrecords in a vector,provides the data needed by
MarkUnusedAddresseswithout changing observable keypool behavior.
2045-2071: DescriptorMarkUnusedAddressesmatches descriptor wallet expectationsFor descriptor wallets,
MarkUnusedAddresses:
- Detects used scripts via
IsMine(script)/m_map_script_pub_keys.- Advances
next_indexup to the used index, expanding from the descriptor cache per index.- Collects
WalletDestinationentries withstd::nulloptfor the internal flag (as expected for single‑descriptor managers).- Calls
TopUp()afterwards to replenish the keypool.This is in line with the descriptor keypool model; the unused
block_timeparameter is benign here.src/wallet/wallet.h (2)
808-818: Outpoint-levelIsMineoverload is a sensible additionDeclaring
isminetype IsMine(const COutPoint& outpoint) const(withcs_walletlock requirement) aligns with the implementation inwallet.cppand simplifies callers likeFundTransactionthat work directly withCOutPoints.
1000-1004: Multi-manager and internal-SPK-man helpers are well-factoredThe new APIs:
GetScriptPubKeyMans(const CScript& script) constfor multi‑manager lookups, andstd::optional<bool> IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man) constto query internal/external status for active descriptor managers,provide the right abstractions for the RPC and spending code to reason about multiple script managers without leaking internal details.
Also applies to: 1068-1075
test/lint/spelling.ignore-words.txt (1)
14-14: Add of “keypair” to ignore list is appropriateThis just prevents spurious spelling lint failures for a common cryptographic term; no behavior impact.
src/wallet/test/wallet_tests.cpp (1)
853-853: ZapSelectTx localerrortype change is benignSwitching the unused local from
bilingual_strtostd::stringdoes not affect test behavior and keeps the code aligned with upstream.src/script/interpreter.h (1)
195-212: Const‑correctness improvement inDeferringSignatureCheckerStoring and accepting a
const BaseSignatureChecker&is consistent with how checkers are used (no mutation) and works with existing callers; behavior is unchanged.src/wallet/rpc/spend.cpp (2)
570-597:fundrawtransactionoptions now reuse shared funding docsUsing
Cat<...>(..., FundTxDoc())for theoptionsobject keeps the documented snake_case keys (includingsolving_data) consistent with other funding RPCs and with the parsing inFundTransaction, with no runtime change.
763-797:sendandwalletcreatefundedpsbtoption docs aligned viaFundTxDocExtending both RPCs’
optionsargument lists withFundTxDoc()makes the help text forconf_target,estimate_mode, andsolving_datauniform across funding calls, matching the sharedFundTransactionimplementation.Also applies to: 1047-1069
src/script/sign.cpp (1)
345-385: Dummy checker/creator refactor is consistent and well‑scopedThe new
DUMMY_CHECKERandDummySignatureCreatorimplementations produce valid, fixed‑size dummy DER signatures (71/72 bytes) behindconstinterface references, matching their use for size estimation and solvability checks without changing script semantics.src/script/sign.h (1)
53-58: ExposeDUMMY_CHECKERalongside dummy signature creatorsAdding
extern const BaseSignatureChecker& DUMMY_CHECKER;keeps the header in sync with sign.cpp and makes the shared dummy checker available to callers in the same way as the dummy signature creators.test/functional/wallet_listdescriptors.py (1)
60-63: Good addition to assert lexicographic ordering of descriptorsThe new assertion that
descstrings equal their sorted order directly verifies the intended deterministic sorting behavior oflistdescriptorswithout overconstraining other fields.test/functional/rpc_fundrawtransaction.py (1)
389-394: Updated error expectation matches more specific external‑input failureSwitching the expected message in
test_invalid_inputto"Unable to find UTXO for external input"reflects the new, clearer error path for a vin pointing to a non‑existent UTXO, while keeping “Insufficient funds” for real balance shortfalls.src/wallet/wallet.cpp (5)
1143-1158: Multi‑manager address marking and address‑book backfill look correctThe new loop over
GetScriptPubKeyMans(txout.scriptPubKey)plusMarkUnusedAddressesandWalletDestination.internalhandling cleanly supports multiple SPK managers. InferringinternalviaIsInternalScriptPubKeyManonly when not provided by the manager, and only auto‑adding non‑internal destinations to the address book when they’re absent, gives the desired behavior for restored backups and newly detected receive addresses without mislabeling change or CoinJoin paths.
1515-1526: NewIsMine(const COutPoint&)helper is safe and well‑scopedThe helper checks for a known wallet transaction and bounds‑checks the output index before delegating to
IsMine(CTxOut), so there’s no risk of out‑of‑range access and it reuses existing ownership logic.
2573-2585:DisplayAddresscorrectly routes through external signer ScriptPubKeyMansSwitching to
GetScriptPubKeyMans(scriptPubKey)and then dynamic_casting toExternalSignerScriptPubKeyManensures this RPC only uses external‑signer managers that can actually handle the script, and immediately returns the first successfulDisplayAddressresult. Non‑external SPK managers are ignored, and afalsereturn cleanly signals no capable signer was found.
3771-3781:GetScriptPubKeyMans(const CScript&)generalization looks soundCollecting all SPK managers whose
CanProvidereturns true for a script and returning them in astd::set<ScriptPubKeyMan*>is consistent with the existingGetSolvingProviderpattern. Reusing a singleSignatureDatainstance here matches that pattern and is safe, sinceCanProvideis used as a capability check rather than depending on prior contents.
4005-4027:IsInternalScriptPubKeyMancorrectly classifies active descriptor managersThe helper returns
std::nulloptfor legacy wallets and non‑active SPK managers, and for descriptor wallets asserts that active managers have a defined output type before comparing against theinternalmanager pointer. This matches the import/activation constraints (e.g., combo descriptors and CoinJoin descriptors are not activated) and gives a reliable internal/external flag for use by higher‑level code likeAddToWalletIfInvolvingMeandlistdescriptors.src/wallet/scriptpubkeyman.h (2)
150-161:WalletDestinationand path type enum are a good fit for new address metadataIntroducing
WalletDestination { CTxDestination dest; std::optional<bool> internal; }provides a simple way to pipe both the address and (optionally) its internal/external classification back to wallet‑level code. Together with the existingPathDerivationType(includingDIP0009_CoinJoinfor CoinJoin paths), this matches how descriptor managers distinguish external, internal, and CoinJoin derivation paths in the rest of the wallet.
197-205:MarkUnusedAddresses/MarkReserveKeysAsUsedAPI changes align with callersChanging
ScriptPubKeyMan::MarkUnusedAddressesto returnstd::vector<WalletDestination>andLegacyScriptPubKeyMan::MarkReserveKeysAsUsedto returnstd::vector<CKeyPool>matches the implementations inscriptpubkeyman.cppand the new usage inCWallet::AddToWalletIfInvolvingMe. Legacy SPKMs can report concrete destinations and theirfInternalflag, while descriptor SPKMs can return destinations withinternalleft unset for wallet‑level inference. The documented behavior (“all keys up to and including the provided key”) also matches the backported logic.Also applies to: 353-354, 499-507, 597-598
BACKPORT NOTE: changes from script/standard is excluded due to not backported yet taproot 184d453 script, doc: spelling update (Jon Atack) Pull request description: Clears out the report from `test/lint/lint-spelling.sh` and touches up the leftover nits in bitcoin#22166 (review). Happy to add any others people are aware of. ACKs for top commit: MarcoFalke: cr ACK 184d453 Sjors: utACK 184d453 Tree-SHA512: 3b0ef6bd5ff227363b0bda79eeb66763151c74f607bc3a2a7bfe7823e3eef196587bccfe639e714e8e27b918ba57e8317eda06f225143c32c736685087dbcd24
fafff13 doc: Extract FundTxDoc (MarcoFalke) Pull request description: No need to duplicate the documentation for the same field(s) three times. Fix that by de-duplicating it for the fields: conf_target, estimate_mode, replaceable, and solving_data. Can be reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`. ACKs for top commit: fanquake: ACK fafff13 Tree-SHA512: 098ddad3904b80b24c9e7b57ca8e807a6ccc3899eac2c9986d71ba3873c2b580bbb95f2fdfbf94b2db02f81c7b0ebf438a90324c23389b7b968ca85ae8475373
… to the address book 3d71d16 test: listtranscations with externally generated addresses (S3RK) d045664 Add to spends only transcations from me (S3RK) 9f3a622 Automatically add labels to detected receiving addresses (S3RK) c1b99c0 Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses (S3RK) 03840c2 Add CWallet::IsInternalScriptPubKeyMan (S3RK) 456e350 wallet: resolve ambiguity of two ScriptPubKey managers providing same script (S3RK) Pull request description: This PR fixes certain use-cases when **send-to-self** transactions are missing from `listtransactions` output. 1. When a receiving address is generated externally to the wallet (e.g. same wallet running on two nodes, or by 3rd party from xpub) 2. When restoring backup with lost metadata, but keypool gap is not exceeded yet When the block is connected or tx added to mempool we already mark used keys. This PR extends this logic to determine whether the destination is a receiving one and if yes add it to the address book with empty label. Works both for legacy and descriptors wallets. - For legacy it uses the internal flag from the keypool entry. Caveat: because we don't know which script type would be used we add all possible destinations for such keys. - For descriptor wallets it uses internal flag for the script pub key manager. Caveat: it only works for active descriptors. fixes bitcoin#19856 fixes bitcoin#20293 ACKs for top commit: laanwj: Code review ACK 3d71d16 Tree-SHA512: 03fafd5548ead0c4ffe9ebcc9eb2849f1d2fa7270fda4166419b86877d4e57dcf04460e465fbb9c90b42031f3c05d1b83f1b67a9f82c2a42980825ed1e7b52e6
4e1cb90 test: fix: remove outdated TestNode.generate calls (James O'Beirne) Pull request description: Currently failing on CI. After this change the test itself still fails, but at least it's apparently for a non-incidental reason. ACKs for top commit: meshcollider: ACK 4e1cb90 theStack: Tested ACK 4e1cb90 Tree-SHA512: 5e7059d334d571ca92f250d298292ce1653da8257cbfb218d28cc9c5816c21c718c36482da31fcaf78e0714cc9b67ff04b91405e820accaf4d8321a354af9441
…_listtransactions.py 0ba98ed test: remove unneeded sync_all() calls in wallet_listtransactions.py (Sebastian Falbesoner) Pull request description: This is a small follow-up to bitcoin#23659. The `self.sync_all()` calls after generating blocks can be removed, since that happens automatically per default by the test framework's generate function (if no explicit sync_fun is passed). On the course of touching the file, imports are sorted and the grammar of a log message is fixed. ACKs for top commit: fanquake: ACK 0ba98ed - thanks for following up. shaavan: ACK 0ba98ed Tree-SHA512: 451e733865dcb1e424d90289c8c89272837a9af6fd4b77d6c60728c84524d9c792d684b7e601b02a0efda67231183c42dd9040d96214ac7d9473b2808cabe73f
…ameters c27bba9 test: check for invalid listtransactions RPC parameters (Sebastian Falbesoner) Pull request description: This PR adds missing test coverage for RPC errors that are thrown if invalid parameters are passed to `listtransactions`: https://github.com/bitcoin/bitcoin/blob/887796a5ffcbafcd281b920f8d55fcb6e8347584/src/wallet/rpc/transactions.cpp#L508 https://github.com/bitcoin/bitcoin/blob/887796a5ffcbafcd281b920f8d55fcb6e8347584/src/wallet/rpc/transactions.cpp#L524 https://github.com/bitcoin/bitcoin/blob/887796a5ffcbafcd281b920f8d55fcb6e8347584/src/wallet/rpc/transactions.cpp#L526 ACKs for top commit: shaavan: ACK c27bba9 Tree-SHA512: e5a23590186b4d9663261ff6cea52ac45e9bf2f2ef693c22b3452bb07af9b800fdabc2a94fd2852c686c28214a496d7afe296e41831759f2182feac2482635d0
…are also in the wallet ef8e2a5 tests: Test that external inputs of txs in wallet is handled correctly (Andrew Chow) eb87963 wallet: Try estimating input size with external data if wallet fails (Andrew Chow) a537d7a wallet: SelectExternal actually external inputs (Andrew Chow) f2d00bf wallet: Add CWallet::IsMine(COutPoint) (Andrew Chow) Pull request description: if a transaction is being funded that has an external input, and that input's parent is also in the wallet, we will fail to detect that and fail to fund the transaction. In order to correctly detect such inputs, we need to be doing `IsMine` on all specified inputs in order to use `Select` and `SelectExternal` correctly. Additionally `SelectCoins` needs to call `CalculateMaximumSignedInputSize` with the correct parameters which depends on whether the wallet is able to solve for the input. Because there are some situations where the wallet could find an external input to belong to it (e.g. watching an address - unable to solve, but will be ISMINE_WATCHONLY), instead of switching which `CalculateMaximumSignedInputSize` to use, we should call the one that uses the wallet, and if that fails, try again with the one that uses external solving data. Also adds a test for this case. ACKs for top commit: instagibbs: ACK bitcoin@ef8e2a5 furszy: ACK ef8e2a5 ishaanam: reACK ef8e2a5 Tree-SHA512: a43c4aefeed4605f33a36ce87ebb916e2c153fea6d415b02c9a89275e84a7e3bf12840b33c296d2d2bde46350390da48d9262f9567338e3f21d5936aae4caa1e
…external inputs BACKPORT NOTE: Dash have no bumpfee functionality. This PR is backported just to keep in sync with bitcoin's codebase and it includes minor refactorings c3b099a wallet, tests: Test bumpfee's max input weight calculation (Andrew Chow) 116a620 Make DUMMY_CHECKER availble outside of script/sign.cpp (Andrew Chow) ff63832 test, bumpfee: Check that psbtbumpfee can bump txs with external inputs (Andrew Chow) 1bc8106 bumpfee: be able to bump fee of a tx with external inputs (Andrew Chow) 31dd3dc bumpfee: Clear scriptSigs and scriptWitnesses before calculated max size (Andrew Chow) a0c3afb bumpfee: extract weights of external inputs when bumping fee (Andrew Chow) 612f1e4 bumpfee: Calculate fee by looking up UTXOs (Andrew Chow) Pull request description: This PR allows `psbtbumpfee` to return a PSBT for transactions that contain external inputs. This does not work for bumping in the GUI nor `bumpfee` because these need private keys available to sign and send the transaction. But `psbtbumpfee` returns a psbt, so it is fine to not be able to sign. In order to correctly estimate the size of the inputs for coin selection, the fee bumper will use the size of the inputs of the transaction being bumped. Because the sizes of signatures are not guaranteed, for external inputs, the fee bumper will verify the scripts with a special SignatureChecker which will compute the weight of all of the signatures in that input, and compute their weights if those signatures were maximally sized. This allows the fee bumper to obtain a max size estimate for each external input. Builds on bitcoin#23201 as it relies on the ability to pass weights in to coin selection. Closes bitcoin#23189 ACKs for top commit: ishaanam: reACK c3b099a t-bast: Re-ran my tests agains bitcoin@c3b099a, ACK Tree-SHA512: 40016ec52d351430977579cfa2694c7e6764f42c9ce09d3a6f1753b767f86053f296d9de988248df033be6d725d67badbf2a5ef82c8ace23c61487729b7691e5
5099624 rpc: sort listdescriptors result (Sjors Provoost) Pull request description: This puts receive and change descriptors directly below each other. The change would be simpler if `UniValue` arrays were sortable. ACKs for top commit: achow101: ACK 5099624 S3RK: reACK 5099624 furszy: utACK 5099624 w0xlt: reACK bitcoin@5099624 Tree-SHA512: 71246a48ba6f97c3e7c76ee32ff9e958227a14ca5a6eec638215dbfee57264d4e918ea5837f4d030eddc9c797c93df1791ddd55b5a499522ce2a35bcf380670b
…rings are sorted 810c3dc doc, rpc: mention that `listdescriptors` result is sorted by string representation (Sebastian Falbesoner) d99af86 test: check that `listdescriptors` descriptor strings are sorted (Sebastian Falbesoner) Pull request description: This small PR adds a test for the change introduced in PR bitcoin#25931 ("rpc: sort listdescriptors result", commit 5099624). The correctness of the test can easily be verified by commenting out the `std::sort` call in the `listdescriptors` RPC implementation: ```diff diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 09c74ea..3ed1a69b26 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -1829,9 +1829,11 @@ RPCHelpMan listdescriptors() }); } + /* std::sort(wallet_descriptors.begin(), wallet_descriptors.end(), [](const auto& a, const auto& b) { return a.descriptor < b.descriptor; }); + */ UniValue descriptors(UniValue::VARR); for (const WalletDescInfo& info : wallet_descriptors) { ``` leading to a fail of the functional test `wallet_listdescriptors.py`. ACKs for top commit: jarolrod: ACK 810c3dc aureleoules: ACK 810c3dc Tree-SHA512: 31770e3149b8a0251ecfa8662a2270c149f778eb910985f48a91d6a5d288b7b1c2244f9f1b798ebe3f1aa9f0b935cb4d6f12d5d28f78bcde3c4a61af76d11d0a
3ee6d07 test: add more wallet conflicts assertions (S3RK) 3b98bf9 Revert "Add to spends only transcations from me" (S3RK) Pull request description: This reverts commit d045664 from bitcoin#22929. This commit was based on invalid assumption that `mapTxSpends` should contain only outgoing txs and broke wallet conflicts feature. ACKs for top commit: achow101: ACK 3ee6d07 Tree-SHA512: bf5a77ced6bac57d5eb85771d9189c53e1edc295d179ed5a1bdce18e365794a9101b4cecf35387b27f67260db3b47f7214e7876e490494529b748cceeb95632d
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/functional/wallet_abandonconflict.py (2)
79-79: Add space after#in comment.Static analysis reports a formatting issue: block comments should have a space after
#.Apply this diff:
- #Create a child tx spending AB1 and C + # Create a child tx spending AB1 and C
120-120: Prefernot inoperator for membership test.Python style guidelines recommend
x not in yovernot x in yfor better readability.Apply this diff:
- assert not txABC2 in [utxo["txid"] for utxo in alice.listunspent(0)] + assert txABC2 not in [utxo["txid"] for utxo in alice.listunspent(0)]src/wallet/scriptpubkeyman.cpp (1)
315-350: Minor robustness nit in legacy keypool usage reportingThe refactor of
LegacyScriptPubKeyMan::MarkUnusedAddresses/MarkReserveKeysAsUsedto return affected destinations andCKeyPoolentries is functionally fine and matches the new wallet‑level APIs. One small robustness concern: inMarkReserveKeysAsUsed,result.push_back(std::move(keypool));is executed even whenbatch.ReadPool(index, keypool)fails, soresultmay contain default‑initializedCKeyPoolobjects on DB read errors. That’s only reachable under wallet DB corruption and won’t affect normal operation, but if you ever want to harden this path, you could gate the push on a successfulReadPoolto avoid propagating dummy entries. Given this is a straight backport, leaving it as‑is to stay aligned with upstream is also reasonable.Also applies to: 1656-1681
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/node/interfaces.cpp(1 hunks)src/script/interpreter.h(1 hunks)src/script/sign.cpp(1 hunks)src/script/sign.h(1 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/spend.cpp(7 hunks)src/wallet/scriptpubkeyman.cpp(5 hunks)src/wallet/scriptpubkeyman.h(5 hunks)src/wallet/spend.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)src/wallet/wallet.cpp(5 hunks)src/wallet/wallet.h(3 hunks)test/functional/rpc_fundrawtransaction.py(1 hunks)test/functional/wallet_abandonconflict.py(5 hunks)test/functional/wallet_listdescriptors.py(1 hunks)test/functional/wallet_listtransactions.py(2 hunks)test/lint/spelling.ignore-words.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- test/functional/rpc_fundrawtransaction.py
- test/lint/spelling.ignore-words.txt
- src/wallet/test/wallet_tests.cpp
- test/functional/wallet_listdescriptors.py
- src/node/interfaces.cpp
- src/script/sign.h
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/wallet/rpc/spend.cppsrc/script/interpreter.hsrc/wallet/spend.cppsrc/wallet/wallet.hsrc/script/sign.cppsrc/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cppsrc/wallet/rpc/addresses.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/rpc/spend.cppsrc/wallet/spend.cppsrc/wallet/wallet.hsrc/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cppsrc/wallet/rpc/addresses.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/wallet_abandonconflict.pytest/functional/wallet_listtransactions.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/rpc/spend.cppsrc/wallet/wallet.hsrc/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cppsrc/wallet/rpc/addresses.cpp
📚 Learning: 2025-06-20T23:32:16.225Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Applied to files:
test/functional/wallet_abandonconflict.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/wallet/spend.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/wallet/spend.cppsrc/wallet/scriptpubkeyman.h
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/wallet/spend.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/wallet.hsrc/wallet/wallet.cppsrc/wallet/rpc/addresses.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/wallet/scriptpubkeyman.h
🧬 Code graph analysis (7)
test/functional/wallet_abandonconflict.py (5)
test/functional/test_framework/test_node.py (1)
get_wallet_rpc(363-369)src/wallet/test/wallet_tests.cpp (1)
getnewaddress(43-43)test/functional/test_framework/test_framework.py (1)
sync_mempools(834-860)test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)src/wallet/rpc/transactions.cpp (2)
abandontransaction(805-844)abandontransaction(805-805)
src/wallet/wallet.h (2)
src/wallet/wallet.cpp (13)
IsMine(1483-1487)IsMine(1483-1483)IsMine(1489-1493)IsMine(1489-1489)IsMine(1495-1503)IsMine(1495-1495)IsMine(1505-1512)IsMine(1505-1505)IsMine(1514-1525)IsMine(1514-1514)outpoint(1093-1093)GetScriptPubKeyMans(3766-3776)GetScriptPubKeyMans(3766-3766)src/wallet/scriptpubkeyman.h (5)
ScriptPubKeyMan(170-233)ScriptPubKeyMan(178-178)bool(185-185)bool(230-230)bool(236-236)
src/wallet/rpc/backup.cpp (2)
src/wallet/hdchain.h (1)
BIP32_PURPOSE_FEATURE(145-145)src/script/descriptor.cpp (1)
nullopt(656-656)
src/wallet/scriptpubkeyman.cpp (1)
src/script/descriptor.cpp (3)
assert(767-771)dest(693-703)nullopt(656-656)
src/wallet/scriptpubkeyman.h (2)
src/wallet/scriptpubkeyman.cpp (6)
MarkUnusedAddresses(315-350)MarkUnusedAddresses(315-315)MarkUnusedAddresses(2047-2073)MarkUnusedAddresses(2047-2047)MarkReserveKeysAsUsed(1656-1681)MarkReserveKeysAsUsed(1656-1656)src/wallet/walletdb.h (1)
WalletBatch(143-256)
src/wallet/wallet.cpp (1)
src/wallet/scriptpubkeyman.cpp (8)
IsMine(168-180)IsMine(168-168)IsMine(182-186)IsMine(182-182)IsMine(1856-1863)IsMine(1856-1856)IsMine(1865-1869)IsMine(1865-1865)
src/wallet/rpc/addresses.cpp (1)
src/wallet/interfaces.cpp (1)
spk_man(216-223)
🪛 Flake8 (7.3.0)
test/functional/wallet_abandonconflict.py
[error] 79-79: block comment should start with '# '
(E265)
[error] 120-120: test for membership should be 'not in'
(E713)
🪛 Ruff (0.14.7)
test/functional/wallet_abandonconflict.py
120-120: Test for membership should be not in
Convert to not in
(E713)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (31)
test/functional/wallet_abandonconflict.py (3)
35-38: LGTM! Well-structured dual-wallet test setup.The introduction of separate wallet handles for alice (sender) and bob (receiver) enables comprehensive testing of wallet conflicts from both perspectives.
177-178: LGTM! Correct wallet reload after node restart.The explicit
loadwallet("bob")call is necessary because bob is not the default wallet. After node restarts, only the default wallet (alice) is automatically loaded.
221-230: LGTM! Excellent receiver-side wallet conflict verification.The added test coverage properly validates that wallet conflicts are correctly tracked from bob's (receiver's) perspective, ensuring both sender and receiver wallets maintain consistent conflict information.
src/wallet/rpc/backup.cpp (5)
1990-1990: Documentation accurately reflects the new sorted output behavior.
2031-2041: Well-structured intermediate type for collecting descriptor data.The struct appropriately uses
std::optionalfor conditionally-present fields andSecureStringfor sensitive mnemonic data.
2064-2065: CoinJoin detection via derivation path pattern matching.The detection relies on finding the pattern
/<BIP32_PURPOSE_FEATURE>'/<ExtCoinType>'/4'/0'in the descriptor string. While string-based matching is somewhat fragile compared to inspecting the descriptor structure directly, this approach works correctly for the current descriptor format.
2081-2083: Correct lexicographic sorting provides deterministic output.The sorting ensures consistent ordering across calls, which is noted in the PR objectives as a non-breaking change since previous ordering was undefined.
2086-2110: Emission loop correctly outputs all descriptor fields in documented order.Key observations:
- Mnemonic fields are conditionally emitted only when mnemonic is non-empty (addresses past review feedback)
- Range end is correctly converted from exclusive (
range_end) to inclusive (range_end - 1) for display- Both
nextandnext_indexfields are emitted for backward compatibility as documented in the RPC helpsrc/wallet/rpc/spend.cpp (4)
358-382: FundTxDoc helper correctly centralizes common funding argument docsThe helper’s argument names and structure (
conf_target,estimate_mode,solving_data.pubkeys/scripts/descriptors) line up with whatFundTransactionalready parses and uses, so this de-duplication keeps the RPC help consistent without changing behavior. Based on learnings, this matches the intended Bitcoin backport pattern.
569-595: Refactoring fundrawtransaction options docs to use FundTxDoc looks consistentThe
Cat<std::vector<RPCArg>>composition preserves all previously documentedoptionskeys while adding the sharedconf_target,estimate_mode, andsolving_datadocs viaFundTxDoc(). This aligns the help text with the options actually consumed byFundTransactionand avoids duplication.
762-796: send() options now share the unified funding docs and remain compatible with existing logicReusing
FundTxDoc()here correctly documentsconf_target,estimate_mode, andsolving_datainside theoptionsobject, matching howsend()merges top‑level arguments intooptionsbefore callingFundTransaction. The dual-path handling (positional vsoptions) is enforced by the existing conflict checks, so this is a pure help-text consolidation.
1045-1068: walletcreatefundedpsbt gains consistent docs for shared funding optionsAttaching
FundTxDoc()to theoptionsobject cleanly adds documentation forconf_target,estimate_mode, andsolving_datathatFundTransactionalready understands, without touching the established mixed‑case keys. This improves help coverage while keeping the backport faithful to upstream.src/script/sign.cpp (2)
345-356: DummySignatureChecker global reference is safe and consistent with upstream patternBinding
DUMMY_CHECKERasconst BaseSignatureChecker&to aDummySignatureChecker()temporary is OK: the temporary’s lifetime is extended to static duration, and the dynamic type remainsDummySignatureChecker, so virtual dispatch works correctly. This matches the upstream dummy-checker pattern and avoids extra global objects.Based on learnings, this looks like a faithful Bitcoin Core backport, so no local refactor is suggested.
357-385: DummySignatureCreator DER layout and globals look correct
DummySignatureCreator::CreateSigsizesvchSigasm_r_len + m_s_len + 7and only writes up to index6 + m_r_len + m_s_len, so no out-of-bounds access. The constructed bytes form a valid DERSEQUENCEof twoINTEGERs, with the final byte used forSIGHASH_ALL. The two globals
DUMMY_SIGNATURE_CREATOR = DummySignatureCreator(32, 32)andDUMMY_MAXIMUM_SIGNATURE_CREATOR = DummySignatureCreator(33, 32)are also safe: both are
const BaseSignatureCreator&bound directly to temporaries, so their lifetimes are extended for the whole program, and they rely onDUMMY_CHECKERonly at call time (not during static init).Based on learnings, this aligns with the Bitcoin Core backport and should not be altered locally.
src/script/interpreter.h (1)
195-199: Const-correctness for DeferringSignatureChecker is coherentSwitching
m_checkerand the constructor toconst BaseSignatureChecker&matches how checkers are used elsewhere (only via const methods) and aligns withVerifyScript/EvalScriptsignatures. Existing callers that had a non-const checker still bind fine by implicit conversion to const, so this is a safe tightening of the API.test/functional/wallet_listtransactions.py (1)
7-16: New listtransactions functional coverage looks consistent with upstream behaviorThe extra node, wallet.dat cloning, normalization helper, and invalid-parameter assertions all look correct and in line with the external-signer backports. The tests exercise keypool gap handling, cross-node wallet consistency, label persistence, and RPC argument validation without introducing Dash-specific divergences.
Also applies to: 21-25, 105-168
src/wallet/rpc/addresses.cpp (1)
509-546: Multi‑ScriptPubKeyMan handling in getaddressinfo is correctUsing
GetScriptPubKeyMans(scriptPubKey)with a best‑effort selection of the first manager, then downcasting toDescriptorScriptPubKeyManforparent_descand reusing the samespk_manfor metadata, is internally consistent and matches the intended multi‑SPKM design. No obvious correctness or lifetime issues here.src/wallet/spend.cpp (2)
525-569: Preset input sizing now correctly supports external solving providersThe revised preset‑input loop in
SelectCoinsfirst uses wallet solving data when available, then falls back tocoin_control.m_external_providerfor inputs not in the wallet or not solvable by it. Aborting wheninput_bytesremains-1avoids fee underestimation for unsignable presets. This matches the external signer use case without regressing existing behavior.
1166-1177: FundTransaction internal vs external input classification is well‑definedClassifying each vin by
wallet.IsMine(outPoint)and otherwise consultingfindCoinsto either fail with a clear error or callcoinControl.SelectExternalis a clean separation between wallet‑owned and external UTXOs. It ensures external inputs have theirCTxOutmetadata registered for later sizing and signing, and introduces a sensible error when a referenced external UTXO cannot be found.src/wallet/wallet.h (1)
808-818: Wallet public API extensions are coherent with multi‑SPKM designAdding
IsMine(const COutPoint&), exposingGetScriptPubKeyMans(const CScript&), and introducingIsInternalScriptPubKeyMancleanly support the new multi‑manager and internal/external semantics used elsewhere in this PR. Signatures and lock annotations are consistent with existing implementations.Also applies to: 1000-1002, 1068-1072
src/wallet/scriptpubkeyman.cpp (1)
2047-2073: Descriptor MarkUnusedAddresses correctly returns affected destinationsFor descriptor wallets,
DescriptorScriptPubKeyMan::MarkUnusedAddressesnow returns astd::vector<WalletDestination>by expanding cached descriptors fromnext_indexup to the detected used index and marking each derived destination as affected (withinternalleft asstd::nullopt). This mirrors the legacy behavior at the descriptor level and integrates cleanly with the higher‑level wallet logic that consumes these destinations.src/wallet/wallet.cpp (5)
1141-1157: Correct use of multi‑SPKMan MarkUnusedAddresses and address‑book inferenceThe new loop over
GetScriptPubKeyMans(txout.scriptPubKey)and consumption ofMarkUnusedAddressesresults is consistent: it correctly infersinternalviaIsInternalScriptPubKeyMan(spk_man)when unset, skips undetermined destinations, and auto-adds external receiving addresses to the address book only when not already present. This matches the intended descriptor/external‑signer behavior while keeping DIP0009/CoinJoin managers out of the address book via the active‑SPKMan check inIsInternalScriptPubKeyMan.
1514-1525: NewIsMine(const COutPoint&)helper is safe and boundedThe helper safely looks up the wallet tx and bounds‑checks
outpoint.nbefore delegating toIsMine(const CTxOut&), and enforcescs_walletbeing held viaAssertLockHeld. This is a straightforward, correct addition to the ownership helpers.
2571-2580: DisplayAddress now correctly considers all matching ScriptPubKeyMansIterating
GetScriptPubKeyMans(scriptPubKey)and selecting the firstExternalSignerScriptPubKeyManis the right generalization from the previous single‑manager lookup. It avoids false negatives when the relevant external‑signer SPKMan isn’t the default external manager while preserving previous behavior (returning false when none apply).
3766-3776: GetScriptPubKeyMans(script) implementation is consistent with CanProvide semanticsUsing a local
SignatureData sigdataand selecting all managers whereCanProvide(script, sigdata)is true correctly returns the set of SPKMs that can solve the script. This aligns with how other helpers (e.g. GetSolvingProvider) driveCanProvideand fits the new callers.
4000-4022: IsInternalScriptPubKeyMan correctly classifies active descriptor managersThe helper appropriately:
- Returns
std::nulloptfor legacy wallets.- Restricts classification to active SPKMs only.
- Requires a
DescriptorScriptPubKeyMan(including external‑signer subclasses) and asserts it has an output type.- Determines “internal” by pointer equality with
GetScriptPubKeyMan(/*internal=*/true).This yields reliable
internalflags for descriptor wallets and lets callers safely skip non‑classifiable managers (e.g. inactive CoinJoin descriptors), matching the intended behavior forWalletDestination.internal.src/wallet/scriptpubkeyman.h (5)
150-155: WalletDestination struct is minimal and fits call‑sites
WalletDestinationcleanly encapsulates the destination plus an optionalinternalflag. This matches howMarkUnusedAddressesimplementations populate it (legacy settingfInternal, descriptors usingstd::nullopt) and how wallet code later infers / consumes the flag.
197-205: Base MarkUnusedAddresses signature and docs align with new usageChanging
MarkUnusedAddressesto returnstd::vector<WalletDestination>and documenting that it returns “all of the addresses affected” matches the implementations and the new caller inAddToWalletIfInvolvingMe. The default implementation returning{}is safe for SPKMs that don’t participate.
353-353: LegacyScriptPubKeyMan override matches contractThe override of
MarkUnusedAddressesinLegacyScriptPubKeyMancorrectly adopts the new signature, and (per the .cpp snippet) returns concreteWalletDestinationentries tagged withkeypool.fInternal, which the wallet later uses to decide whether to add addresses to the book. This is consistent and preserves legacy semantics.
499-507: MarkReserveKeysAsUsed return type and docs reflect new behaviorUpdating
MarkReserveKeysAsUsedto returnstd::vector<CKeyPool>and documenting that it returns “all affected keys” matches the provided implementation, which now exposes the consumed keypool entries so callers (likeMarkUnusedAddresses) can derive all possible destinations. Signature and comment are coherent.
597-597: DescriptorScriptPubKeyMan::MarkUnusedAddresses override matches interfaceThe descriptor SPKMan override uses the new
std::vector<WalletDestination>return type and (per the .cpp snippet) fills it with derived destinations andinternal = std::nullopt, which fits the wallet‑side inference viaIsInternalScriptPubKeyMan. The declaration is consistent with the base class and implementation.
6 participants
What was done?
More backports to improve support of External signer.
This PR have mostly backports related to tests and RPC
listdescriptorsreturns descriptors now sorted. It's not a breaking change, because before they has been returned in "undefined" sorting order.How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: