backport: bitcoin/bitcoin#23201, bitcoin/bitcoin#25551 (external signer, user provided size of tx) #7055
Conversation
knst
changed the title
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdds per-input serialized-weight hints throughout wallet funding and signing. CCoinControl gains SetInputWeight/HasInputWeight/GetInputWeight and an internal map to store per-outpoint weights. RPC surfaces (fundrawtransaction, send, walletcreatefundedpsbt) accept and validate per-input Sequence DiagramsequenceDiagram
participant User
participant RPC as RPC Layer (spend.cpp)
participant CC as CCoinControl
participant CS as CoinSelection / FundTransaction
participant Sign as Signing (DummySignTx / wallet.cpp)
User->>RPC: Call fund/send/walletcreatefundedpsbt with inputs incl. input_sizes
RPC->>RPC: SetOptionsInputWeights(inputs, options)
RPC->>CC: SetInputWeight(outpoint, weight) for each input
RPC->>CS: FundTransaction(..., coin_control, options)
CS->>CC: HasInputWeight(outpoint)?
alt weight present
CC-->>CS: true
CS->>CC: GetInputWeight(outpoint)
CS->>CS: Use provided weight to compute input_bytes (affects selection/fees)
else no weight
CC-->>CS: false
CS->>CS: Use estimated input size
end
CS-->>RPC: Return funded tx/psbt
RPC->>Sign: DummySignTx(tx/psbt, coin_control)
Sign->>CC: HasInputWeight(outpoint)?
alt weight present
CC-->>Sign: true
Sign->>Sign: FillInputToWeight(txin, target_weight)
Sign->>Sign: Skip signing for that input
else no weight
CC-->>Sign: false
Sign->>Sign: Normal signing flow
end
Sign-->>RPC: Signed/finalized tx or PSBT
RPC-->>User: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
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: 1
🧹 Nitpick comments (3)
test/functional/wallet_send.py (1)
542-542: Use string literal for Decimal to avoid float precision issues."Unfortunately, most decimal fractions cannot be represented exactly as binary fractions." Using
Decimal(0.0001)with a float literal may introduce precision errors.- assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001)) + assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal("0.0001"))test/functional/rpc_fundrawtransaction.py (2)
10-10: Remove unused import.Static analysis correctly identifies that
ceilfrommathis imported but never used in this file.-from math import ceil
1028-1028: Use string literal for Decimal to avoid float precision issues.Same as in
wallet_send.py, usingDecimal(0.0001)with a float literal may introduce precision errors.- assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001)) + assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal("0.0001"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e3d61f2 and 151c1a59f7ea65adfbe0548e52e1d2ea513751e7.
📒 Files selected for processing (9)
src/wallet/coincontrol.h(2 hunks)src/wallet/rpc/spend.cpp(9 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/spend_tests.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_fundrawtransaction.py(3 hunks)test/functional/rpc_psbt.py(3 hunks)test/functional/wallet_send.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/wallet.hsrc/wallet/test/spend_tests.cppsrc/wallet/spend.cppsrc/wallet/coincontrol.hsrc/wallet/rpc/spend.cppsrc/wallet/wallet.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/wallet.hsrc/wallet/test/spend_tests.cppsrc/wallet/spend.cppsrc/wallet/coincontrol.hsrc/wallet/rpc/spend.cppsrc/wallet/wallet.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/spend_tests.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_send.pytest/functional/rpc_fundrawtransaction.pytest/functional/rpc_psbt.py
🧠 Learnings (7)
📓 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: 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: 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: 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.
📚 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/spend_tests.cppsrc/wallet/rpc/spend.cpptest/functional/rpc_fundrawtransaction.pytest/functional/rpc_psbt.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/test/spend_tests.cppsrc/wallet/rpc/spend.cppsrc/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/coincontrol.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/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-01-14T08:37:16.955Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Applied to files:
src/wallet/rpc/spend.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:
test/functional/rpc_fundrawtransaction.pytest/functional/rpc_psbt.py
🧬 Code graph analysis (9)
src/wallet/wallet.h (1)
src/wallet/wallet.cpp (2)
FillInputToWeight(1661-1694)FillInputToWeight(1661-1661)
src/wallet/test/spend_tests.cpp (1)
src/wallet/wallet.cpp (2)
FillInputToWeight(1661-1694)FillInputToWeight(1661-1661)
test/functional/wallet_send.py (2)
test/functional/test_framework/util.py (3)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (3)
send(797-994)send(797-797)tx(981-981)
src/wallet/spend.cpp (1)
src/policy/policy.h (1)
GetVirtualTransactionSize(118-121)
src/wallet/coincontrol.h (4)
src/wallet/spend.cpp (1)
outpoint(150-150)src/wallet/wallet.cpp (1)
outpoint(1093-1093)src/wallet/interfaces.cpp (4)
outpoint(399-399)outpoint(399-399)outpoint(400-400)outpoint(400-400)src/wallet/coinjoin.cpp (2)
outpoint(510-510)outpoint(560-560)
src/wallet/rpc/spend.cpp (2)
src/rpc/util.cpp (2)
ParseHashO(106-109)ParseHashO(106-106)src/rpc/rawtransaction_util.cpp (2)
ConstructTransaction(24-132)ConstructTransaction(24-24)
test/functional/rpc_fundrawtransaction.py (2)
test/functional/test_framework/util.py (4)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_greater_than(77-79)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (4)
fundrawtransaction(603-704)fundrawtransaction(603-603)signrawtransactionwithwallet(706-795)signrawtransactionwithwallet(706-706)
src/wallet/wallet.cpp (1)
src/serialize.h (1)
GetSizeOfCompactSize(245-251)
test/functional/rpc_psbt.py (3)
test/functional/test_framework/util.py (2)
assert_greater_than(77-79)assert_raises_rpc_error(132-148)test/functional/test_framework/test_node.py (1)
get_wallet_rpc(363-369)src/wallet/rpc/spend.cpp (2)
walletcreatefundedpsbt(1075-1208)walletcreatefundedpsbt(1075-1075)
🪛 Flake8 (7.3.0)
test/functional/rpc_fundrawtransaction.py
[error] 10-10: 'math.ceil' imported but unused
(F401)
🪛 Ruff (0.14.7)
test/functional/wallet_send.py
542-542: Decimal() called with float literal argument
Replace with string literal
(RUF032)
test/functional/rpc_fundrawtransaction.py
1028-1028: Decimal() called with float literal argument
Replace with string literal
(RUF032)
⏰ 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: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (17)
src/wallet/spend.cpp (1)
553-556: Preset-input size override via CCoinControl weight is consistent and correctUsing
HasInputWeight/GetInputWeighthere to overrideinput_byteswithGetVirtualTransactionSize(...)cleanly integrates caller‑supplied weights into coin selection and solvability checks, and preserves the existing-1error path for inputs without a weight. This matches the intended behavior for external/nonstandard inputs.src/wallet/wallet.h (1)
1134-1134: New FillInputToWeight API declaration is well-placed and consistentThe
FillInputToWeightdeclaration in the wallet namespace matches its implementation and test usage, and sits logically next toDummySignInputas another input‑sizing helper.src/wallet/wallet.cpp (2)
1661-1694: FillInputToWeight correctly pads inputs to a bounded target sizeThe helper cleanly enforces its preconditions (empty
scriptSig, nondecreasing size) and uses compact‑size accounting so that the final serialized input size is guaranteed within[target_weight-2, target_weight]. This matches the documented 2‑byte tolerance and is exercised by the new unit tests.
1704-1712: DummySignTx integration with per-input weights behaves as intendedShort‑circuiting to
FillInputToWeightwhenHasInputWeight(txin.prevout)is true lets callers fully control fee estimation for those inputs, and intentionally skips producing dummy signatures afterward. On failure, the function exits early, allowing higher‑level code to surface “missing solving data / bad weight” conditions as before.src/wallet/coincontrol.h (1)
132-147: Per-input weight tracking in CCoinControl is coherent and minimally scopedThe new
m_input_weightsmap and itsSetInputWeight/HasInputWeight/GetInputWeightaccessors integrate cleanly with existingCOutPoint-keyed state, and match how the weights are consumed in coin selection and dummy signing. The assert inGetInputWeightis appropriate given current usage patterns.Also applies to: 162-163
src/wallet/test/spend_tests.cpp (1)
61-106: FillInputToWeight tests thoroughly exercise edge cases and compact-size boundariesThe new helper and
FillInputToWeightTestcover negative/too‑small targets, the exact empty‑input size, and multiple compact‑size transitions, verifying both overall serialized size andscriptSiglength. This gives good confidence thatFillInputToWeightbehaves correctly across its range of expected inputs.src/wallet/rpc/spend.cpp (4)
531-560: LGTM - Input size parsing and validation is thorough.The validation logic correctly enforces:
voutmust be present and non-negativesizemust be present, at least 41 bytes (minimum input size), and at mostMAX_STANDARD_TX_SIZE- The
CHECK_NONFATAL(min_input_size == 41)assertion documents the expected minimumThe use of
sizein the RPC API (vs.weightin the internal API) is appropriate for Dash's non-SegWit transaction format where size and weight are equivalent.
586-601: Prevent options conflict for input sizes specified in inputs.The helper correctly:
- Rejects
input_sizesin options when inputs specify sizes (preventing ambiguity)- Early-returns if no inputs
- Extracts only inputs that have a
sizefieldOne consideration: When
inputshas elements but none containsize, this still pushes an emptyinput_sizesarray to options. This is harmless but slightly redundant.
946-947: LGTM - Input weights correctly propagated insendRPC.The call to
SetOptionsInputWeightsproperly extracts size hints fromoptions["inputs"]before passing toFundTransaction.
1174-1184: LGTM - Input weights correctly propagated inwalletcreatefundedpsbt.The options object is properly made mutable,
SetOptionsInputWeightsextracts size hints from inputs, and the modified options are passed toFundTransaction.test/functional/wallet_send.py (1)
506-542: LGTM - Comprehensive test coverage for input weight functionality.The test correctly:
- Computes input weight from the finalized PSBT
- Tests error condition when
input_sizesis in options instead of inputs- Validates funding works with explicit input weights
- Verifies mempool acceptance and fee calculation
test/functional/rpc_fundrawtransaction.py (2)
981-986: LGTM - Comprehensive error condition coverage forinput_sizesvalidation.Tests correctly verify all validation paths:
- Missing
voutkey- Negative
voutvalue- Missing
sizekey- Size below minimum (40 bytes, which is less than 41)
- Negative size
- Size exceeding maximum standard tx size
1010-1033: LGTM - Weight-based funding tests are thorough.The tests correctly verify:
- Funding with explicit input weights works and passes mempool acceptance
- Lower weight results in lower fee
- Higher weight results in higher fee
- Explicit weight overrides calculated weight when solving data is provided
- Fee rate calculation is correct with explicit weight
- Boundary values at csuint thresholds (255, 65539) don't cause issues
test/functional/rpc_psbt.py (4)
467-475: LGTM - Test setup for external input funding with PSBT.The test correctly:
- Logs the test section
- Creates a new wallet "extfund" on node 1
- Sets up the necessary infrastructure for external input testing
507-521: Input weight calculation includes buffer for potential scriptwitness variation.The formula
input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitnessincludes a 2-byte buffer. This is appropriate for weight calculation in a SegWit-aware context, though Dash doesn't use SegWit. The calculation appears adapted from Bitcoin Core's approach.For Dash's non-SegWit context, the simpler formula might be
41 + len_scriptsig, but the current approach is conservative and aligns with the backported code's comments about allowing up to 2-byte differences.Please verify that this weight calculation formula aligns with the
FillInputToWeightimplementation mentioned in the PR notes, which permits up to 2-byte differences due to missing segregated-data stack handling.
523-563: LGTM - Comprehensive input weight testing via walletcreatefundedpsbt.The tests correctly verify:
- Error when
input_sizesis specified in options instead of inputs- Funding with explicit input weight succeeds and passes mempool acceptance
- Lower weight produces lower fee
- Higher weight produces higher fee
- Explicit weight overrides calculated weight when solving data is provided
565-577: LGTM - Tests weight override for wallet inputs after descriptor import.After importing the descriptor into the wallet, the test verifies that explicitly provided weights still override the wallet's calculated weight for inputs it can solve. This ensures the weight override behavior is consistent for both external and wallet inputs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/wallet/rpc/spend.cpp (1)
643-653: RPC documentation structure matches backport pattern.Per the past review discussion and retrieved learnings, backported changes should match the original upstream PRs. This RPC argument structure for
input_sizesfollows the same pattern used in Bitcoin Core's implementation. Based on learnings, backported code should be preserved as-is unless there are genuine bugs.
🧹 Nitpick comments (2)
test/functional/wallet_send.py (1)
542-542: Use string literal for Decimal to avoid floating-point precision issues.Per static analysis hint (RUF032),
Decimal()should be called with a string literal instead of a float literal to avoid potential floating-point representation issues.- assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001)) + assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal("0.0001"))test/functional/rpc_fundrawtransaction.py (1)
1027-1027: Use string literal for Decimal to avoid floating-point precision issues.Per static analysis hint (RUF032),
Decimal()should be called with a string literal instead of a float literal.- assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001)) + assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal("0.0001"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 151c1a59f7ea65adfbe0548e52e1d2ea513751e7 and 0a38158c7cb467c2eeda0a303a998428cd89f556.
📒 Files selected for processing (9)
src/wallet/coincontrol.h(2 hunks)src/wallet/rpc/spend.cpp(9 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/spend_tests.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_fundrawtransaction.py(3 hunks)test/functional/rpc_psbt.py(3 hunks)test/functional/wallet_send.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/wallet/wallet.h
- src/wallet/wallet.cpp
- src/wallet/spend.cpp
- src/wallet/test/spend_tests.cpp
🧰 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/wallet/coincontrol.h
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/rpc/spend.cppsrc/wallet/coincontrol.h
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/rpc_fundrawtransaction.pytest/functional/rpc_psbt.pytest/functional/wallet_send.py
🧠 Learnings (8)
📓 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: 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.
📚 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.cpptest/functional/rpc_psbt.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/rpc/spend.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
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.
Applied to files:
src/wallet/rpc/spend.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/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-01-14T08:37:16.955Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Applied to files:
src/wallet/rpc/spend.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:
test/functional/rpc_fundrawtransaction.pytest/functional/rpc_psbt.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/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/wallet/coincontrol.h
🧬 Code graph analysis (3)
test/functional/rpc_fundrawtransaction.py (2)
test/functional/test_framework/util.py (4)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_greater_than(77-79)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (4)
fundrawtransaction(603-704)fundrawtransaction(603-603)signrawtransactionwithwallet(706-795)signrawtransactionwithwallet(706-706)
test/functional/rpc_psbt.py (2)
test/functional/test_framework/util.py (2)
assert_greater_than(77-79)assert_raises_rpc_error(132-148)test/functional/test_framework/test_node.py (1)
get_wallet_rpc(363-369)
src/wallet/coincontrol.h (4)
src/wallet/spend.cpp (1)
outpoint(150-150)src/wallet/wallet.cpp (1)
outpoint(1093-1093)src/wallet/interfaces.cpp (4)
outpoint(399-399)outpoint(399-399)outpoint(400-400)outpoint(400-400)src/wallet/coinjoin.cpp (2)
outpoint(510-510)outpoint(560-560)
🪛 Ruff (0.14.7)
test/functional/rpc_fundrawtransaction.py
1027-1027: Decimal() called with float literal argument
Replace with string literal
(RUF032)
test/functional/wallet_send.py
542-542: Decimal() called with float literal argument
Replace with string literal
(RUF032)
⏰ 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: linux64_tsan-build / Build source
- GitHub Check: win64-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: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (15)
src/wallet/coincontrol.h (2)
132-147: LGTM! Clean API for per-input weight tracking.The three new methods follow a consistent pattern:
SetInputWeightto store,HasInputWeightto check existence, andGetInputWeightwith an assert precondition requiring callers to verify existence first. This matches the usage pattern insrc/wallet/spend.cppwhereHasInputWeightis always checked beforeGetInputWeight.
162-163: Comment accurately reflects Dash context.The comment notes "weight (equals to size)" which is correct for Dash since there's no segregated witness data, making weight equivalent to serialized size. This aligns with the PR description noting the backport differs from Bitcoin Core due to missing segregated-data stack handling.
src/wallet/rpc/spend.cpp (4)
5-5: New include is appropriate.The
consensus/validation.hinclude is needed forMAX_STANDARD_TX_SIZEused in the size validation at line 554.
531-560: Robust input_sizes validation logic.The parsing correctly validates:
txidviaParseHashOvoutpresence, type, and non-negativitysizepresence, minimum bound (41 bytes for outpoint + sequence + empty scriptSig), and maximum bound (MAX_STANDARD_TX_SIZE)The
CHECK_NONFATAL(min_input_size == 41)is a good compile-time sanity check ensuring the constant matches expectations.
586-601: SetOptionsInputWeights correctly enforces input specification location.The guard at line 588-590 prevents users from specifying
input_sizesin options when usingsendorwalletcreatefundedpsbt, enforcing that sizes must be specified inline with inputs. The function then extracts and consolidates size specifications from inputs into the options for downstream processing.
1174-1184: Correct integration in walletcreatefundedpsbt.The code properly:
- Creates a mutable copy of options if provided, or an empty object
- Calls
SetOptionsInputWeightsto extract per-input sizes from the inputs array- Passes the enriched options to
FundTransactiontest/functional/wallet_send.py (2)
506-514: Input weight calculation logic looks correct.The code correctly:
- Decodes the PSBT to find the external input
- Locates the matching input by txid/vout
- Calculates
input_weight = 41 + len_scriptsig - 1where 41 is the base input size (outpoint + sequence + 1-byte scriptSig length), and the -1 compensates for the length byte already included in the base.
517-523: Good coverage for error path.The test correctly verifies that specifying
input_sizesin the options object (rather than inline with inputs) raises the expected RPC error with code -8.test/functional/rpc_fundrawtransaction.py (3)
980-985: Comprehensive error path coverage for input_sizes validation.The tests cover all validation scenarios:
- Missing vout key
- Negative vout
- Missing size key
- Size below minimum (40, -1)
- Size above maximum (400001)
1000-1006: Input weight calculation is well-documented.The calculation
input_weight = signed_weight - unsigned_weight + 41correctly derives the input's signing overhead by comparing signed vs unsigned transaction sizes, then adding back the base input components (prevout, sequence, 1-byte scriptSig length).
1030-1031: Good boundary testing for CompactSize encoding.Testing at sizes 255 and 65539 validates correct handling at CompactSize encoding boundaries (1-byte to 3-byte, and 3-byte to 5-byte transitions), ensuring the weight serialization doesn't have off-by-one issues at these thresholds.
test/functional/rpc_psbt.py (4)
17-17: Import added for new assertion.The
assert_greater_thanimport is needed for the fee comparison assertions in the new weight-based tests.
515-521: Input weight formula includes witness handling (safely degenerates for Dash).The formula
input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitnessis designed for Bitcoin's witness weight calculation. For Dash (no SegWit),len_scriptwitnesswill always be 0, and the formula correctly computes the input weight as(41 + len_scriptsig + 2) * 4. The +2 buffer provides margin for signature size variance.
523-531: Error path test for options-based input_sizes.Consistent with wallet_send.py, this test verifies that specifying
input_sizesin the options object raises RPC error -8 with the expected message.
533-577: Comprehensive PSBT weight-based funding tests.The tests cover:
- Funding with exact input weight → mempool acceptance
- Lower weight → lower fee (line 549)
- Higher weight → higher fee (line 556)
- Provided weight overrides solving_data calculated weight (line 563)
- Wallet inputs also respect provided weights after descriptor import (lines 566-577)
This provides excellent coverage of the new input weight functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/wallet/rpc/spend.cpp (1)
643-657:fundrawtransactioninput_sizesRPC arg now correctly models an array of objects.The
input_sizesoption is documented as anARRwhose elements areOBJcontainingtxid,vout, andsize. This matches the actual accepted format and the upstream Bitcoin structure the backport is based on, and resolves the earlier array-of-scalars mismatch.
🧹 Nitpick comments (3)
test/functional/wallet_send.py (1)
506-543: AlignDecimalusage and keep per-input-size test logic as-isThe new block that decodes the PSBT, derives an
input_weighthint fromfinal_scriptSig, checks theinput_sizeserror path, and then exercises funding with a providedsizefield is well-structured and ties directly into the new wallet behavior. The finalassert_fee_amountusingtestmempoolacceptis a good end‑to‑end check.One small nit: to avoid any float‑to‑Decimal rounding surprises and to match the rest of the file, prefer a string literal here:
- assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001)) + assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal("0.0001"))src/wallet/rpc/spend.cpp (1)
586-601:SetOptionsInputWeightshelper and its use insend/walletcreatefundedpsbtare sound, with a tiny optional polish.
SetOptionsInputWeightscorrectly:
- Rejects
optionsthat already defineinput_sizes, enforcing that hints are supplied viainputsobjects, not directly inoptions.- Scans the provided
inputsarray and buildsinput_sizesonly for entries that actually definesize, so other inputs remain unaffected.- In both
sendandwalletcreatefundedpsbt, copying the options into a localUniValue options{...}before passing it toSetOptionsInputWeightsand then toFundTransactionis the right pattern: it avoids mutatingrequest.paramswhile still lettingFundTransactionsee the synthesizedinput_sizes.- Behavior with omitted or empty
inputsis safe:inputs.size() == 0short-circuits before any work.Minor optional polish: you could skip pushing an
"input_sizes"key whensizesremains empty, but this is purely cosmetic and has no behavioral impact.Also applies to: 903-951, 1178-1189
test/functional/rpc_fundrawtransaction.py (1)
994-1032: Behavioral tests for size hints look solid; consider avoidingDecimalwith a float literal.
- The two-step signing flow (
signed_tx1from the external-funding wallet,signed_tx2from node 0) correctly isolates the external input’s contribution to transaction size.- Deriving
input_weightfrom the size delta and then checking:
- funding success with an exact hint,
- monotonicity of fees when shrinking/growing the hint,
- that an explicit
input_sizesentry overrides solving-data–based estimation, and- a specific feerate target via
assert_fee_amount,
gives strong end-to-end coverage of the new feature, including the “csuint boundary” cases.Minor nit from Ruff (RUF032):
Decimal(0.0001)is constructed from a binary float, which can introduce tiny rounding artifacts. It’s safer and more idiomatic to pass a string literal:- assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001)) + assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal("0.0001"))This keeps the test entirely in exact decimal space.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 0a38158c7cb467c2eeda0a303a998428cd89f556 and e34d649da4d90df537fc020758253114e8c872d0.
📒 Files selected for processing (9)
src/wallet/coincontrol.h(2 hunks)src/wallet/rpc/spend.cpp(9 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/spend_tests.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_fundrawtransaction.py(3 hunks)test/functional/rpc_psbt.py(3 hunks)test/functional/wallet_send.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/wallet/wallet.h
- src/wallet/coincontrol.h
- test/functional/rpc_psbt.py
🧰 Additional context used
📓 Path-based instructions (4)
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/test/spend_tests.cppsrc/wallet/wallet.cppsrc/wallet/rpc/spend.cppsrc/wallet/spend.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/spend_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/test/spend_tests.cppsrc/wallet/wallet.cppsrc/wallet/rpc/spend.cppsrc/wallet/spend.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/rpc_fundrawtransaction.pytest/functional/wallet_send.py
🧠 Learnings (15)
📓 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: 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: 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.
📚 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/test/spend_tests.cppsrc/wallet/wallet.cppsrc/wallet/rpc/spend.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:
test/functional/rpc_fundrawtransaction.pysrc/wallet/rpc/spend.cpp
📚 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.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/rpc/spend.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-08-11T17:16:36.654Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/wallet/rpc/spend.cpp
📚 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/rpc/spend.cpp
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/wallet/rpc/spend.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/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-01-14T08:37:16.955Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Applied to files:
src/wallet/rpc/spend.cpp
🧬 Code graph analysis (6)
src/wallet/test/spend_tests.cpp (1)
src/wallet/wallet.cpp (2)
FillInputToWeight(1661-1694)FillInputToWeight(1661-1661)
test/functional/rpc_fundrawtransaction.py (1)
test/functional/test_framework/util.py (4)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_greater_than(77-79)assert_fee_amount(44-52)
src/wallet/wallet.cpp (1)
src/serialize.h (1)
GetSizeOfCompactSize(245-251)
test/functional/wallet_send.py (2)
test/functional/test_framework/util.py (3)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (5)
send(801-998)send(801-801)walletprocesspsbt(1000-1077)walletprocesspsbt(1000-1000)tx(985-985)
src/wallet/rpc/spend.cpp (3)
src/rpc/util.cpp (2)
ParseHashO(106-109)ParseHashO(106-106)src/rpc/request.cpp (3)
JSONRPCError(56-62)JSONRPCError(56-56)request(31-31)src/rpc/rawtransaction_util.cpp (2)
ConstructTransaction(24-132)ConstructTransaction(24-24)
src/wallet/spend.cpp (1)
src/policy/policy.h (1)
GetVirtualTransactionSize(118-121)
🪛 Cppcheck (2.18.0)
src/wallet/test/spend_tests.cpp
[information] 80-80: Include file
(missingIncludeSystem)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
src/wallet/wallet.cpp
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
src/wallet/rpc/spend.cpp
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
src/wallet/spend.cpp
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
🪛 Ruff (0.14.8)
test/functional/rpc_fundrawtransaction.py
1027-1027: Decimal() called with float literal argument
Replace with string literal
(RUF032)
test/functional/wallet_send.py
542-542: Decimal() called with float literal argument
Replace with string literal
(RUF032)
⏰ 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: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/wallet/spend.cpp (1)
553-556: ConfirmGetInputWeightunits matchGetVirtualTransactionSizeexpectationsThe override to set
input_bytesfromcoin_control.GetInputWeight(outpoint)viaGetVirtualTransactionSize(..., 0, 0)is structurally correct and matches the intent that an explicit hint should trump the dummy estimate.Given
GetInputWeightis also passed directly intoFillInputToWeightelsewhere, please double‑check that its unit (bytes vs. BIP141 weight units) is consistent with bothFillInputToWeightandGetVirtualTransactionSizeso fee estimation can’t be skewed for preset inputs. No change requested if this already matches upstream semantics and tests.src/wallet/test/spend_tests.cpp (1)
61-106: FillInputToWeight tests precisely cover compactsize boundary behaviorThe
TestFillInputToWeighthelper andFillInputToWeightTestexercise the negative/edge cases and the 1‑byte and 3‑byte compactsize boundaries thoroughly. The expectedscriptSigsizes line up with the currentFillInputToWeightimplementation and its ±2‑byte allowance, so this gives good protection against regressions.src/wallet/wallet.cpp (2)
1661-1694: FillInputToWeight implementation aligns with tests and boundary assumptionsThe helper correctly enforces
target_weight >= current_size, adjusts for compactsize length changes, and guarantees the final serialized size is in[target_weight - 2, target_weight]. This matches the new unit test expectations and keeps behavior predictable around 1‑byte/3‑byte compactsize transitions. Theassert(txin.scriptSig.empty())precondition is appropriate given current usage in dummy‑sign flows.
1704-1711: Per-input weight integration in DummySignTx looks correct; verify unit consistencyShort‑circuiting
DummySignTxto callFillInputToWeightwhencoin_controlhas an input weight fortxin.prevoutis logically sound: it uses the explicit hint instead of constructing a dummy signature and skips signing for that input, which is fine for size estimation.As with the spend-side override, it’s worth confirming that the
GetInputWeightvalue passed here is in the same units thatFillInputToWeightand any fee/vsize calculations expect, so that hinting a size for an input cannot systematically under‑ or over‑estimate fees.src/wallet/rpc/spend.cpp (2)
5-5: Include ofconsensus/validation.his appropriate forMAX_STANDARD_TX_SIZE.The new include cleanly provides
MAX_STANDARD_TX_SIZEused in theinput_sizesvalidation block; no issues with dependency direction here.
396-427:input_sizesvalidation and coin-control plumbing look correct and match tests.
- Adding
"input_sizes"toRPCTypeCheckObjensures the new option is recognized and type-checked as an array, preserving strict-key behavior.- The parsing loop correctly:
- Enforces
txidviaParseHashO.- Requires numeric, non-negative
voutwith clear error messages that match the new functional tests.- Requires numeric
size, enforces a lower bound ofGetSerializeSize(CTxIn()) == 41and an upper bound ofMAX_STANDARD_TX_SIZE, again with messages the tests expect.- Stores the hint via
coinControl.SetInputWeight(COutPoint(txid, vout), size)so the wallet’s funding logic can consume it.This aligns with the PR’s goal of letting callers override estimated input sizes for funding and fee calculation, without impacting legacy callers.
Also applies to: 531-560
test/functional/rpc_fundrawtransaction.py (1)
971-985: Newinput_sizeserror-path coverage aligns with the C++ validation logic.The added cases (missing
vout, negativevout, missingsize, too-small or negativesize, and overly largesize) all:
- Use the new
input_sizesoption in the same shape the RPC expects.- Assert
RPC_INVALID_PARAMETER(-8) with substrings that match the C++ error messages inFundTransaction.This gives good coverage of the new validation without changing existing behavior for the solving-data error cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/wallet_send.py (1)
506-543: Minor test polish: avoid float to Decimal, optionally guard input indexTwo small suggestions here:
- At Line 542, prefer a string literal for
Decimalto avoid any floating‑point representation surprises and satisfy Ruff:- assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001)) + assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal("0.0001"))
- (Optional) If you want extra robustness, consider asserting that the external UTXO was actually found before using
input_idx, e.g. initialize it toNoneandassert input_idx is not Noneafter the loop.The overall per‑input size test logic and fee validation look solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e34d649da4d90df537fc020758253114e8c872d0 and 43d173a622f0b68a4b74df51453679f61d10dfd2.
📒 Files selected for processing (9)
src/wallet/coincontrol.h(2 hunks)src/wallet/rpc/spend.cpp(9 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/spend_tests.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_fundrawtransaction.py(3 hunks)test/functional/rpc_psbt.py(3 hunks)test/functional/wallet_send.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wallet/wallet.h
🧰 Additional context used
📓 Path-based instructions (4)
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/test/spend_tests.cppsrc/wallet/coincontrol.hsrc/wallet/rpc/spend.cppsrc/wallet/spend.cppsrc/wallet/wallet.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/spend_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/test/spend_tests.cppsrc/wallet/coincontrol.hsrc/wallet/rpc/spend.cppsrc/wallet/spend.cppsrc/wallet/wallet.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_send.pytest/functional/rpc_fundrawtransaction.pytest/functional/rpc_psbt.py
🧠 Learnings (17)
📓 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: 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: 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.
📚 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/spend_tests.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/coincontrol.hsrc/wallet/spend.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:
test/functional/rpc_fundrawtransaction.pysrc/wallet/rpc/spend.cpptest/functional/rpc_psbt.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/rpc/spend.cpp
📚 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.cpptest/functional/rpc_psbt.py
📚 Learning: 2025-10-25T07:08:51.918Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-08-11T17:16:36.654Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/wallet/rpc/spend.cpp
📚 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/rpc/spend.cpp
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/wallet/rpc/spend.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/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-01-14T08:37:16.955Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Applied to files:
src/wallet/rpc/spend.cpp
🧬 Code graph analysis (8)
src/wallet/test/spend_tests.cpp (1)
src/wallet/wallet.cpp (2)
FillInputToWeight(1661-1694)FillInputToWeight(1661-1661)
test/functional/wallet_send.py (2)
test/functional/test_framework/util.py (3)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (5)
send(800-996)send(800-800)walletprocesspsbt(998-1075)walletprocesspsbt(998-998)tx(983-983)
src/wallet/coincontrol.h (4)
src/wallet/spend.cpp (1)
outpoint(150-150)src/wallet/wallet.cpp (1)
outpoint(1093-1093)src/wallet/interfaces.cpp (4)
outpoint(399-399)outpoint(399-399)outpoint(400-400)outpoint(400-400)src/wallet/coinjoin.cpp (2)
outpoint(510-510)outpoint(560-560)
test/functional/rpc_fundrawtransaction.py (2)
test/functional/test_framework/util.py (4)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_greater_than(77-79)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (4)
fundrawtransaction(603-707)fundrawtransaction(603-603)signrawtransactionwithwallet(709-798)signrawtransactionwithwallet(709-709)
src/wallet/rpc/spend.cpp (2)
src/rpc/util.cpp (2)
ParseHashO(106-109)ParseHashO(106-106)src/rpc/rawtransaction_util.cpp (2)
ConstructTransaction(24-132)ConstructTransaction(24-24)
test/functional/rpc_psbt.py (2)
test/functional/test_framework/util.py (3)
assert_greater_than(77-79)assert_raises_rpc_error(132-148)assert_equal(69-74)test/functional/test_framework/test_node.py (1)
get_wallet_rpc(363-369)
src/wallet/spend.cpp (1)
src/policy/policy.h (1)
GetVirtualTransactionSize(118-121)
src/wallet/wallet.cpp (1)
src/serialize.h (1)
GetSizeOfCompactSize(245-251)
🪛 Cppcheck (2.18.0)
src/wallet/test/spend_tests.cpp
[information] 80-80: Include file
(missingIncludeSystem)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
src/wallet/rpc/spend.cpp
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
src/wallet/spend.cpp
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
src/wallet/wallet.cpp
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
[information] Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
(toomanyconfigs)
🪛 Ruff (0.14.8)
test/functional/wallet_send.py
542-542: Decimal() called with float literal argument
Replace with string literal
(RUF032)
test/functional/rpc_fundrawtransaction.py
1027-1027: Decimal() called with float literal argument
Replace with string literal
(RUF032)
⏰ 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_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: mac-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_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (22)
src/wallet/rpc/spend.cpp (8)
5-5: LGTM - include added for MAX_STANDARD_TX_SIZE.The new include is required for the
MAX_STANDARD_TX_SIZEconstant used in the input_sizes validation at line 554.
531-560: Input sizes parsing is well-structured with proper validation.The parsing logic correctly validates:
- Required
txidfield viaParseHashO(throws on missing/invalid)- Required
voutfield with non-negative check- Required
sizefield with bounds check [41, MAX_STANDARD_TX_SIZE]The
CHECK_NONFATALassertion at line 550 serves as a runtime sanity check that the minimum serialized input size matches the expected 41 bytes (outpoint + sequence + empty scriptSig).
586-601: SetOptionsInputWeights correctly transfers per-input sizes from inputs to options.The function properly:
- Prevents duplicate specification by checking if
input_sizesalready exists in options- Returns early if no inputs are provided
- Collects only inputs that specify a
sizefield- Adds the collected data as
input_sizesto options for downstream processingThis design keeps size hints specified alongside each input rather than requiring a separate options field in
sendandwalletcreatefundedpsbt.
643-656: RPC documentation structure is correct.The
input_sizesarray now properly documents its elements with an intermediate OBJ wrapper, matching the expected JSON format[{"txid": ..., "vout": ..., "size": ...}, ...]. This addresses the previously identified structural issue.
845-848: LGTM - size field added to send RPC inputs.The documentation clearly describes the
sizeparameter as the maximum serialized size in bytes, with appropriate guidance about DER signature sizing.
948-949: LGTM - proper integration with SetOptionsInputWeights.The call correctly processes
options["inputs"]beforeFundTransaction, ensuring per-input sizes are transferred to the options object for downstream processing.
1092-1095: LGTM - size field added to walletcreatefundedpsbt inputs.Consistent with the
sendRPC, the size parameter documentation is appropriate.
1175-1185: LGTM - proper options initialization and input weights handling.The options object is correctly initialized from params (or as empty VOBJ if null), and
SetOptionsInputWeightsis called beforeFundTransactionto transfer per-input sizes from the inputs array to the options.test/functional/rpc_fundrawtransaction.py (4)
972-973: LGTM - test now uses partial spend for external input.Using
ext_utxo["amount"] / 2allows the test to verify that funding works with an external input that doesn't cover the full output, triggering the "Insufficient funds" path correctly.
980-986: Comprehensive input_sizes validation error tests.Good coverage of error conditions:
- Missing
voutkey- Negative
voutvalue- Missing
sizekey- Size below minimum (40 < 41)
- Negative size (also < 41)
- Size exceeding MAX_STANDARD_TX_SIZE
1000-1006: Input weight calculation is appropriate for Dash.Unlike the segwit weight formula in Bitcoin Core, this calculates input size from the difference between signed and unsigned transaction sizes plus the fixed 41-byte overhead (prevout + sequence + empty scriptSig). This correctly derives the actual serialized input size for Dash.
1029-1031: LGTM - boundary tests for CompactSize encoding.Testing at size 255 and 65539 exercises the boundaries where CompactSize encoding changes from 1-byte to 3-byte and 3-byte to 5-byte representations, ensuring the weight calculations handle these correctly.
test/functional/rpc_psbt.py (6)
17-17: LGTM - import added for fee comparison assertions.The
assert_greater_thanutility is used for comparing fees between different input weight configurations.
467-474: LGTM - test setup for PSBT external input funding.Creates a dedicated "extfund" wallet on node 1 for testing PSBT funding with external inputs.
523-531: LGTM - error condition test for input_sizes in options.Verifies that specifying
input_sizesin options (rather than per-inputsizefield) correctly triggers the expected error message, ensuring users provide size hints in the proper location.
533-563: Comprehensive weight-based funding tests.Good test coverage for:
- Funding with exact input weight
- Mempool acceptance validation
- Fee comparison with lower/higher weights
- Weight override when solving_data is also provided
The relative comparisons (lower weight → lower fee, higher weight → higher fee) correctly validate the weight-to-fee relationship regardless of absolute weight values.
565-577: LGTM - wallet input weight override test.After importing the descriptor into the wallet, tests verify that explicitly provided weights override the wallet's calculated weights, ensuring consistent behavior for both external and wallet-owned inputs.
515-521: The weight calculation formula in this code is correct for Dash's non-SegWit implementation. For non-SegWit transactions, weight is properly defined as 4× the serialized transaction size in bytes; the formulainput_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitnesscorrectly yields this sincelen_scriptwitnessis always 0 in Dash. The weight unit is not intended to equal byte size—it is a standard metric where non-SegWit weight = 4× bytes. No adjustment is needed for Dash-specific handling.Likely an incorrect or invalid review comment.
src/wallet/spend.cpp (1)
553-556: Per‑input size override in SelectCoins looks correctUsing
HasInputWeight/GetInputWeightto overrideinput_bytesviaGetVirtualTransactionSize(...)for preset inputs cleanly matches the intended “caller-supplied size wins” behavior and is safely guarded.src/wallet/wallet.cpp (1)
1661-1694: FillInputToWeight and DummySignTx integration are consistent and well‑bounded
FillInputToWeight’s use ofGetSizeOfCompactSizeand the[target_weight-2, target_weight]postcondition matches the new unit tests, and theDummySignTxpath that calls it whenHasInputWeightis set cleanly bypasses normal dummy signing without violating the empty‑scriptSigprecondition. No correctness or safety issues spotted.Also applies to: 1704-1711
src/wallet/test/spend_tests.cpp (1)
61-106: FillInputToWeight tests give good edge‑case coverageThe new helper and
FillInputToWeightTestexercise negative/low targets and CompactSize boundaries, matching the implementation’s guarantees and catching regressions in serialization behavior.src/wallet/coincontrol.h (1)
132-148: Per‑input weight tracking in CCoinControl is straightforward and safeThe new
m_input_weightsmap plusSetInputWeight/HasInputWeight/GetInputWeightaccessors are minimal and correctly encapsulated; existing uses checkHasInputWeightbefore callingGetInputWeight, so the internal assert is safe.Also applies to: 162-163
| funded_tx4 = wallet.fundrawtransaction(raw_tx, {"input_sizes": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "size": high_input_weight}], "fee_rate": 10}) | ||
| input_add_weight = high_input_weight - 41 | ||
| tx4_size = wallet.decoderawtransaction(funded_tx4["hex"])["size"] + input_add_weight | ||
| assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001)) |
There was a problem hiding this comment.
Use string literal with Decimal to avoid float precision issues.
The static analysis correctly identifies that Decimal(0.0001) uses a float literal which can introduce precision errors. Use a string literal instead.
- assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001))
+ assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal("0.0001"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001)) | |
| assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal("0.0001")) |
🧰 Tools
🪛 Ruff (0.14.8)
1027-1027: Decimal() called with float literal argument
Replace with string literal
(RUF032)
🤖 Prompt for AI Agents
In test/functional/rpc_fundrawtransaction.py around line 1027, the code
constructs a Decimal using a float literal (Decimal(0.0001)), which can
introduce precision errors; change it to use a string literal instead
(Decimal("0.0001")) so the Decimal is created exactly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/functional/rpc_fundrawtransaction.py (1)
971-986: External-input size tests match C++ semantics; use string literal for DecimalThe new external-input tests:
- Correctly exercise
input_sizesvalidation (missing/negativevout, missing/too-small/too-largesize) against theFundTransactionerror messages.- Verify that providing an explicit per-input size affects fees monotonically and that the specified size overrides solving-data-based estimates, including at compactsize boundaries.
Minor (already flagged previously):
Decimal(0.0001)at line 1027 should use a string literal to avoid float-rounding issues:- assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal(0.0001)) + assert_fee_amount(funded_tx4["fee"], tx4_size, Decimal("0.0001"))Also applies to: 994-1032
🧹 Nitpick comments (1)
test/functional/wallet_send.py (1)
506-543: External-input size wiring looks correct; adjust Decimal literal for precisionThe PSBT decoding, external-input index lookup, and use of the
sizefield ininputsare consistent with the newSetOptionsInputWeights/input_sizesbehavior, including the explicit rejection ofinput_sizesinsideoptions.Minor:
Decimal(0.0001)constructs from a float and can introduce tiny precision issues. Prefer a string literal here:- assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001)) + assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal("0.0001"))
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 43d173a622f0b68a4b74df51453679f61d10dfd2 and 070e10f.
📒 Files selected for processing (9)
src/wallet/coincontrol.h(2 hunks)src/wallet/rpc/spend.cpp(9 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/spend_tests.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_fundrawtransaction.py(3 hunks)test/functional/rpc_psbt.py(3 hunks)test/functional/wallet_send.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/wallet/spend.cpp
- src/wallet/wallet.cpp
🧰 Additional context used
📓 Path-based instructions (4)
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/wallet.hsrc/wallet/test/spend_tests.cppsrc/wallet/coincontrol.hsrc/wallet/rpc/spend.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/wallet.hsrc/wallet/test/spend_tests.cppsrc/wallet/coincontrol.hsrc/wallet/rpc/spend.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/spend_tests.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/rpc_psbt.pytest/functional/rpc_fundrawtransaction.pytest/functional/wallet_send.py
🧠 Learnings (16)
📓 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: 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: 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: 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.
📚 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/coincontrol.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/{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/rpc/spend.cpp
📚 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.cpptest/functional/rpc_psbt.py
📚 Learning: 2025-10-25T07:08:51.918Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-08-11T17:16:36.654Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
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.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/wallet/rpc/spend.cpp
📚 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/rpc/spend.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/rpc/spend.cpptest/functional/rpc_psbt.pytest/functional/rpc_fundrawtransaction.py
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/wallet/rpc/spend.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/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-01-14T08:37:16.955Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Applied to files:
src/wallet/rpc/spend.cpp
🧬 Code graph analysis (6)
src/wallet/wallet.h (1)
src/wallet/wallet.cpp (2)
FillInputToWeight(1661-1692)FillInputToWeight(1661-1661)
src/wallet/test/spend_tests.cpp (1)
src/wallet/wallet.cpp (2)
FillInputToWeight(1661-1692)FillInputToWeight(1661-1661)
src/wallet/rpc/spend.cpp (1)
src/rpc/util.cpp (2)
ParseHashO(106-109)ParseHashO(106-106)
test/functional/rpc_psbt.py (3)
test/functional/test_framework/util.py (2)
assert_greater_than(77-79)assert_raises_rpc_error(132-148)test/functional/test_framework/test_node.py (1)
get_wallet_rpc(363-369)src/wallet/rpc/spend.cpp (4)
sendtoaddress(119-221)sendtoaddress(119-119)walletcreatefundedpsbt(1077-1209)walletcreatefundedpsbt(1077-1077)
test/functional/rpc_fundrawtransaction.py (2)
test/functional/test_framework/util.py (4)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_greater_than(77-79)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (2)
fundrawtransaction(603-707)fundrawtransaction(603-603)
test/functional/wallet_send.py (2)
test/functional/test_framework/util.py (3)
assert_raises_rpc_error(132-148)assert_equal(69-74)assert_fee_amount(44-52)src/wallet/rpc/spend.cpp (3)
send(800-996)send(800-800)tx(983-983)
🪛 Ruff (0.14.8)
test/functional/rpc_fundrawtransaction.py
1027-1027: Decimal() called with float literal argument
Replace with string literal
(RUF032)
test/functional/wallet_send.py
542-542: Decimal() called with float literal argument
Replace with string literal
(RUF032)
⏰ 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: win64-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: linux64_nowallet-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
🔇 Additional comments (5)
src/wallet/wallet.h (1)
1132-1135: New FillInputToWeight declaration is consistent and well-placedThe declaration matches the implementation in wallet.cpp and is correctly exposed in the wallet namespace next to DummySignInput. No issues here.
src/wallet/test/spend_tests.cpp (1)
61-106: FillInputToWeight tests thoroughly cover edge casesThe new helper and test case correctly mirror FillInputToWeight’s behavior, including failure on too-small targets and boundary behavior for compactsize encodings. This gives good coverage without overcomplicating the test.
src/wallet/coincontrol.h (1)
132-148: Per-input weight tracking in CCoinControl is straightforward and soundThe new
SetInputWeight/HasInputWeight/GetInputWeightmethods and the backingm_input_weightsmap provide a clear, minimal API for per-input sizes. The assert inGetInputWeightis appropriate as a precondition check given callers are expected to guard withHasInputWeight().Also applies to: 163-164
src/wallet/rpc/spend.cpp (1)
425-427:input_sizesplumbing and validation are coherent and match the new testsThe new
input_sizeshandling inFundTransaction, theSetOptionsInputWeightshelper, and the RPC argument/docs updates (fundrawtransactionoptions,sendinputs[].size, andwalletcreatefundedpsbtinputs[].size) are internally consistent:
- Options type-checking includes
input_sizesas an array.FundTransactionvalidatestxid/voutpresence, enforcessize∈ [GetSerializeSize(CTxIn()), MAX_STANDARD_TX_SIZE], and wires values intoCCoinControl::SetInputWeight.SetOptionsInputWeightscorrectly enforces that sizes come viainputs[*].sizerather than aninput_sizesoption, which aligns with the functional tests.send()andwalletcreatefundedpsbt()both normalize theiroptions, deriveinput_sizesfrominputs, and then delegate toFundTransaction, so the behavior is uniform across RPCs.Given the corresponding functional and unit tests, this looks like a faithful backport with correct Dash-specific renaming.
Also applies to: 531-560, 586-601, 643-656, 845-848, 940-949, 997-1207
test/functional/rpc_psbt.py (1)
17-20: PSBT external-input size tests correctly exercise the new APIThe added PSBT tests:
- Use external UTXOs and
walletcreatefundedpsbtto validate thatinput_sizesmust come viainputs[*].size, not via theoptionsobject.- Confirm that providing a per-input size affects the computed fee monotonically (smaller size → lower fee, larger size → higher fee) and that explicit sizes override solving-data-based estimates for both external and wallet-owned inputs.
The new
assert_greater_thanimport is used appropriately, and the scenarios line up with the C++SetOptionsInputWeightsandFundTransactionbehavior.Also applies to: 467-578
…n funding a transaction 3866272 tests: Test specifying input weights (Andrew Chow) 6fa762a rpc, wallet: Allow users to specify input weights (Andrew Chow) 808068e wallet: Allow user specified input size to override (Andrew Chow) 4060c50 wallet: add input weights to CCoinControl (Andrew Chow) Pull request description: When funding a transaction with external inputs, instead of providing solving data, a user may want to just provide the maximum signed size of that input. This is particularly useful in cases where the input is nonstandard as our dummy signer is unable to handle those inputs. The input weight can be provided to any input regardless of whether it belongs to the wallet and the provided weight will always be used regardless of any calculated input weight. This allows the user to override the calculated input weight which may overestimate in some circumstances due to missing information (e.g. if the private key is not known, a maximum size signature will be used, but the actual signer may be doing additional work which reduces the size of the signature). For `send` and `walletcreatefundedpsbt`, the input weight is specified in a `weight` field in an input object. For `fundrawtransaction`, a new `input_weights` field is added to the `options` object. This is an array of objects consisting of a txid, vout, and weight. Closes bitcoin#23187 ACKs for top commit: instagibbs: reACK bitcoin@3866272 glozow: reACK 3866272 via range-diff t-bast: ACK bitcoin@3866272 Tree-SHA512: 2c8b471ee537c62a51389b7c4e86b5ac1c3a223b444195042be8117b3c83e29c0619463610b950cbbd1648d3ed01ecc5bb0b3c4f39640680da9157763b9b9f9f
…shes over silent ignore fa277cd univalue: Throw exception on invalid pushes over silent ignore (MacroFake) ccccc17 refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL (MacroFake) Pull request description: The return value of the `push*` helpers is never used, but important to determine if the operation was successful. One way to fix this would be to add the "nodiscard" attribute. However, this would make the code (and this diff) overly verbose for no reason. So fix it by removing the never used return value. Also, fail verbosely in case of a programming mistake. ACKs for top commit: furszy: code ACK fa277cd Tree-SHA512: ef212a5bf5ae6bbad20acc4dafa3715521e81544185988d1eab724f440e4864a27e686aff51d5bc51b3017892c2eb8e577bcb8f37e8ddbaa0d8833bb622f2f9c
5 participants
Issue being fixed or feature implemented
When funding a transaction with external inputs, instead of providing solving data, a user may want to just provide the maximum signed size of that input. This is particularly useful in cases where the input is nonstandard as our dummy signer is unable to handle those inputs.
The input size can be provided to any input regardless of whether it belongs to the wallet and the provided size will always be used regardless of any calculated input size. This allows the user to override the calculated input size which may overestimate in some circumstances due to missing information (e.g. if the private key is not known, a maximum size signature will be used, but the actual signer may be doing additional work which reduces the size of the signature).
For
sendandwalletcreatefundedpsbt, the input size is specified in asizefield in an input object. Forfundrawtransaction, a newinput_sizesfield is added to theoptionsobject. This is an array of objects consisting of a txid, vout, and size.What was done?
Bitcoin backport bitcoin#23201 and related fixes.
This backport is not trivial to do, not trivial to dashify, and not trivial to review, so, it has own PR.
Please note, that implementation of
FillInputToWeightis different with Bitcoin Core. Due missing stack of "segregated data" we can't get exactly the same size, it maybe different up to 2 bytes.How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: