feat: asset lock tx v2 with basic RPC support + tests#7294
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
This pull request has conflicts, please rebase. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 303a71cb69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| opt_assetLockPayload.emplace(creditOutputs); |
There was a problem hiding this comment.
When a platform address is passed to sendtoaddress/sendmany before v24 is active, this default construction creates a version-2 CAssetLockPayload. The consensus path rejects v2 asset locks until v24 (bad-assetlocktx-version-2), but SendMoney still commits the signed transaction to the wallet before only logging the mempool rejection, so users get a txid and their inputs are marked spent by a transaction that cannot relay until the fork activates. Please reject platform recipients or otherwise guard this path until v24 is active.
Useful? React with 👍 / 👎.
| vecSend.insert(vecSend.begin(), | ||
| {CScript() << OP_RETURN << ParseHex(""), platform_total, /*fSubtractFeeFromAmount=*/false}); |
There was a problem hiding this comment.
When the caller sets subtractfeefromamount for a platform recipient, ParseRecipients records that flag, but this replacement removes the original recipient and hard-codes the OP_RETURN recipient to not subtract fees. As a result the fee deduction loop never sees the selected platform output, so exact-spend cases can fail with insufficient funds and non-exact sends charge the fee to change/extra inputs instead of reducing the locked amount requested by the RPC option.
Useful? React with 👍 / 👎.
WalkthroughThis PR implements DIP-27 v2 asset-lock transactions with v24 activation support. It updates the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/assetlocktx.cpp (1)
56-80: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPin consensus checks to explicit payload versions.
These branches are keyed off
CURRENT_VERSIONand>= 2, so a future version bump can silently change historical validation behavior. In this path, please dispatch on exact versions instead of relative comparisons, so v1/v2 rules stay fixed unless someone intentionally edits them.Suggested change
- if (opt_assetLockTx->getVersion() == 0 || opt_assetLockTx->getVersion() > CAssetLockPayload::CURRENT_VERSION) { + const uint8_t version = opt_assetLockTx->getVersion(); + if (version == 0 || version > CAssetLockPayload::CURRENT_VERSION) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-version"); } - if (!is_v24_active && opt_assetLockTx->getVersion() == CAssetLockPayload::CURRENT_VERSION) { - return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-version-2"); + switch (version) { + case CAssetLockPayload::INITIAL_VERSION: + break; + case 2: + if (!is_v24_active) { + return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-version-2"); + } + break; + default: + return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-version"); } @@ - if (opt_assetLockTx->getVersion() >= 2) { - if (!out.scriptPubKey.IsPayToPublicKeyHash() && !out.scriptPubKey.IsPayToScriptHash()) { - return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-script-pubkey"); - } - } else { + switch (version) { + case CAssetLockPayload::INITIAL_VERSION: if (!out.scriptPubKey.IsPayToPublicKeyHash()) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-pubKeyHash"); } + break; + case 2: + if (!out.scriptPubKey.IsPayToPublicKeyHash() && !out.scriptPubKey.IsPayToScriptHash()) { + return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-assetlocktx-script-pubkey"); + } + break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/assetlocktx.cpp` around lines 56 - 80, The code currently gates behavior using CAssetLockPayload::CURRENT_VERSION and a relative check (opt_assetLockTx->getVersion() >= 2), which will change historical rules when CURRENT_VERSION increments; update the validation in the asset lock payload path to dispatch on exact payload versions instead of relative comparisons: read the payload version once (e.g., auto ver = opt_assetLockTx->getVersion()) and replace the CURRENT_VERSION equality/inequality and the ">= 2" branch with explicit version checks (e.g., ver == 1, ver == 2, etc.), ensuring getCreditOutputs and the scriptPubKey checks (IsPayToPublicKeyHash / IsPayToScriptHash) are applied only for the exact intended versions so past rules remain fixed until intentionally modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/evo/assetlocktx.h`:
- Line 158: The declaration of CheckAssetLockTx currently has a defaulted
parameter is_v24_active = false which allows callers to omit the activation
flag; remove the default so the signature becomes bool CheckAssetLockTx(const
CTransaction& tx, TxValidationState& state, bool is_v24_active); then update all
call sites that invoke CheckAssetLockTx(...) to pass the explicit
deployment/activation boolean (thread the v24 activation state from callers that
know consensus state), ensuring compilation fails if a caller neglects to
propagate the flag.
In `@src/evo/core_write.cpp`:
- Around line 103-105: The RPC JSON currently emits the new DIP-18 address as
creditOutputs[].address; change emission so the address is nested under
creditOutputs[].scriptPubKey.address for v2 asset-locks. Update the
RPCResult/JSON building in src/evo/core_write.cpp where the RPCResult::Type::STR
"address" entry is added (both occurrences around the two blocks noted) to
instead add "scriptPubKey" as an object containing the existing script fields
plus the "address" field when version >= 2, and ensure the produced JSON
structure uses creditOutputs[].scriptPubKey.address so consumers see the
documented path.
In `@src/wallet/rpc/spend.cpp`:
- Around line 27-57: The duplicate-recipient check only uses the CTxDestination
set `destinations`, so platform-decoded recipients never get inserted and
duplicates are allowed; after decoding a platform address in
`DecodePlatformDestination` and when handling `PlatformP2PKHDestination` or
`PlatformP2SHDestination`, construct the corresponding CTxDestination (e.g.,
`CTxDestination dest2 = PKHash(hash)` or `CTxDestination dest2 =
ScriptHash(hash)`), run the same duplicate check `if (destinations.count(dest2))
{ throw JSONRPCError(... duplicated address ...) }`, and then
`destinations.insert(dest2)` before creating `script_pub_key` (keep existing
`is_platform` handling and use the same error message and flow).
In `@src/wallet/spend.cpp`:
- Around line 792-808: The loop that moves platform recipients out of vecSend_in
(checking recipient.fPlatformTransfer) ignores recipient.fSubtractFeeFromAmount
so fee-subtraction is silently dropped; add a fail-fast guard inside that loop:
when iterating vecSend_in in the block that builds creditOutputs, detect if any
recipient has fPlatformTransfer == true AND fSubtractFeeFromAmount == true and
return an explicit RPC error (or propagate an error) explaining that
subtract-fee-from-amount is not supported for platform transfers; reference the
loop over vecSend_in, recipient.fPlatformTransfer,
recipient.fSubtractFeeFromAmount, creditOutputs, platform_total and the
vecSend.insert code path so the guard is placed before removing platform
recipients.
- Around line 801-805: When computing fees the extra payload size set via
opt_assetLockPayload.emplace(creditOutputs) and written into nExtraPayloadSize
is not re-applied when nBytes is recomputed during the second fee-pass, causing
underestimation; update the fee-recalculation path that recomputes nBytes (the
block that uses nBytes after the second pass) to include the payload overhead by
recomputing/adding nExtraPayloadSize (derived from serializing
*opt_assetLockPayload into CDataStream) to nBytes before the fee check, and
ensure opt_assetLockPayload is only cleared/dropped after the final fee decision
so nExtraPayloadSize remains available for the second pass.
In `@test/functional/feature_asset_locks.py`:
- Around line 776-777: The multiline calls to check_mempool_result (e.g.,
check_mempool_result(tx=lock_tx, result_expected={'allowed': False,
'reject-reason': 'bad-assetlocktx-version-2'}) and the similar call around lines
791-792) violate Flake8 E128 continuation indentation; fix by re-indenting the
continued lines so they align with the first element after the opening
parenthesis (or use a hanging indent) so the continuation line(s) are visually
aligned with the start of the arguments inside check_mempool_result.
---
Outside diff comments:
In `@src/evo/assetlocktx.cpp`:
- Around line 56-80: The code currently gates behavior using
CAssetLockPayload::CURRENT_VERSION and a relative check
(opt_assetLockTx->getVersion() >= 2), which will change historical rules when
CURRENT_VERSION increments; update the validation in the asset lock payload path
to dispatch on exact payload versions instead of relative comparisons: read the
payload version once (e.g., auto ver = opt_assetLockTx->getVersion()) and
replace the CURRENT_VERSION equality/inequality and the ">= 2" branch with
explicit version checks (e.g., ver == 1, ver == 2, etc.), ensuring
getCreditOutputs and the scriptPubKey checks (IsPayToPublicKeyHash /
IsPayToScriptHash) are applied only for the exact intended versions so past
rules remain fixed until intentionally modified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bde9e45c-0174-4bee-84a1-f5087beff469
📒 Files selected for processing (10)
src/evo/assetlocktx.cppsrc/evo/assetlocktx.hsrc/evo/core_write.cppsrc/evo/specialtxman.cppsrc/rpc/output_script.cppsrc/test/evo_assetlocks_tests.cppsrc/wallet/rpc/spend.cppsrc/wallet/spend.cppsrc/wallet/wallet.htest/functional/feature_asset_locks.py
| }; | ||
|
|
||
| bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state); | ||
| bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state, bool is_v24_active = false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Require the activation flag at every call site.
Defaulting is_v24_active to false makes a missed caller silently apply pre-v24 rules instead of failing to compile. For consensus validation, I'd drop the default so new call paths must thread deployment state explicitly.
Suggested change
-bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state, bool is_v24_active = false);
+bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state, bool is_v24_active);📝 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.
| bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state, bool is_v24_active = false); | |
| bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state, bool is_v24_active); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/evo/assetlocktx.h` at line 158, The declaration of CheckAssetLockTx
currently has a defaulted parameter is_v24_active = false which allows callers
to omit the activation flag; remove the default so the signature becomes bool
CheckAssetLockTx(const CTransaction& tx, TxValidationState& state, bool
is_v24_active); then update all call sites that invoke CheckAssetLockTx(...) to
pass the explicit deployment/activation boolean (thread the v24 activation state
from callers that know consensus state), ensuring compilation fails if a caller
neglects to propagate the flag.
| }}, | ||
| {RPCResult::Type::STR, "address", /*optional=*/true, "DIP-18 Platform address (version >= 2 only)"}, | ||
| }}}} |
There was a problem hiding this comment.
Nest platform address under scriptPubKey for v2 asset-lock JSON.
The new field is currently emitted as creditOutputs[].address instead of creditOutputs[].scriptPubKey.address, which can break consumers depending on the documented path.
Suggested patch
@@
{RPCResult::Type::OBJ, "scriptPubKey", "", {
{RPCResult::Type::STR, "asm", "The asm"},
{RPCResult::Type::STR_HEX, "hex", "The hex"},
{RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
- }},
- {RPCResult::Type::STR, "address", /*optional=*/true, "DIP-18 Platform address (version >= 2 only)"},
+ {RPCResult::Type::STR, "address", /*optional=*/true, "DIP-18 Platform address (version >= 2 only)"},
+ }},
}}}}
@@
UniValue spk(UniValue::VOBJ);
ScriptToUniv(credit_output.scriptPubKey, spk, /*include_hex=*/true, /*include_address=*/false);
- out.pushKV("scriptPubKey", spk);
if (nVersion >= 2) {
CTxDestination dest;
if (ExtractDestination(credit_output.scriptPubKey, dest)) {
if (const auto* pkh = std::get_if<PKHash>(&dest)) {
- out.pushKV("address", EncodePlatformDestination(PlatformP2PKHDestination(uint160(*pkh))));
+ spk.pushKV("address", EncodePlatformDestination(PlatformP2PKHDestination(uint160(*pkh))));
} else if (const auto* sh = std::get_if<ScriptHash>(&dest)) {
- out.pushKV("address", EncodePlatformDestination(PlatformP2SHDestination(uint160(*sh))));
+ spk.pushKV("address", EncodePlatformDestination(PlatformP2SHDestination(uint160(*sh))));
}
}
}
+ out.pushKV("scriptPubKey", spk);Also applies to: 117-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/evo/core_write.cpp` around lines 103 - 105, The RPC JSON currently emits
the new DIP-18 address as creditOutputs[].address; change emission so the
address is nested under creditOutputs[].scriptPubKey.address for v2 asset-locks.
Update the RPCResult/JSON building in src/evo/core_write.cpp where the
RPCResult::Type::STR "address" entry is added (both occurrences around the two
blocks noted) to instead add "scriptPubKey" as an object containing the existing
script fields plus the "address" field when version >= 2, and ensure the
produced JSON structure uses creditOutputs[].scriptPubKey.address so consumers
see the documented path.
| std::set<CTxDestination> destinations; | ||
| int i = 0; | ||
| for (const std::string& address: address_amounts.getKeys()) { | ||
| CTxDestination dest = DecodeDestination(address); | ||
| if (!IsValidDestination(dest)) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Dash address: ") + address); | ||
| } | ||
| CScript script_pub_key; | ||
| bool is_platform = false; | ||
|
|
||
| if (destinations.count(dest)) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); | ||
| CTxDestination dest = DecodeDestination(address); | ||
| if (IsValidDestination(dest)) { | ||
| if (destinations.count(dest)) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); | ||
| } | ||
| destinations.insert(dest); | ||
| script_pub_key = GetScriptForDestination(dest); | ||
| } else { | ||
| std::string platform_error; | ||
| PlatformDestination platform_dest = DecodePlatformDestination(address, platform_error); | ||
| if (!IsValidPlatformDestination(platform_dest)) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Dash address: ") + address); | ||
| } | ||
| is_platform = true; | ||
| if (const auto* pkh = std::get_if<PlatformP2PKHDestination>(&platform_dest)) { | ||
| uint160 hash; | ||
| std::copy(pkh->begin(), pkh->end(), hash.begin()); | ||
| script_pub_key = GetScriptForDestination(PKHash(hash)); | ||
| } else if (const auto* sh = std::get_if<PlatformP2SHDestination>(&platform_dest)) { | ||
| uint160 hash; | ||
| std::copy(sh->begin(), sh->end(), hash.begin()); | ||
| script_pub_key = GetScriptForDestination(ScriptHash(hash)); | ||
| } else { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Unsupported Platform address type: ") + address); | ||
| } |
There was a problem hiding this comment.
Platform recipients bypass duplicate-recipient validation.
destinations tracks only CTxDestination; platform-decoded recipients are never inserted, so duplicate platform recipients are accepted.
Suggested fix
- std::set<CTxDestination> destinations;
+ std::set<CScript> destination_scripts;
@@
- if (IsValidDestination(dest)) {
- if (destinations.count(dest)) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address);
- }
- destinations.insert(dest);
+ if (IsValidDestination(dest)) {
script_pub_key = GetScriptForDestination(dest);
} else {
@@
} else {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Unsupported Platform address type: ") + address);
}
}
+
+ if (destination_scripts.count(script_pub_key)) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address);
+ }
+ destination_scripts.insert(script_pub_key);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/rpc/spend.cpp` around lines 27 - 57, The duplicate-recipient check
only uses the CTxDestination set `destinations`, so platform-decoded recipients
never get inserted and duplicates are allowed; after decoding a platform address
in `DecodePlatformDestination` and when handling `PlatformP2PKHDestination` or
`PlatformP2SHDestination`, construct the corresponding CTxDestination (e.g.,
`CTxDestination dest2 = PKHash(hash)` or `CTxDestination dest2 =
ScriptHash(hash)`), run the same duplicate check `if (destinations.count(dest2))
{ throw JSONRPCError(... duplicated address ...) }`, and then
`destinations.insert(dest2)` before creating `script_pub_key` (keep existing
`is_platform` handling and use the same error message and flow).
| for (const auto& recipient : vecSend_in) { | ||
| if (recipient.fPlatformTransfer) { | ||
| creditOutputs.emplace_back(recipient.nAmount, recipient.scriptPubKey); | ||
| platform_total += recipient.nAmount; | ||
| } else { | ||
| vecSend.push_back(recipient); | ||
| } | ||
| } | ||
|
|
||
| opt_assetLockPayload.emplace(creditOutputs); | ||
| CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); | ||
| ss << *opt_assetLockPayload; | ||
| nExtraPayloadSize = ss.size(); | ||
|
|
||
| vecSend.insert(vecSend.begin(), | ||
| {CScript() << OP_RETURN << ParseHex(""), platform_total, /*fSubtractFeeFromAmount=*/false}); | ||
| } else { |
There was a problem hiding this comment.
subtractfeefromamount is ignored for platform recipients.
Platform recipients are removed from vecSend, so fee-subtraction logic never applies to them, which silently breaks RPC semantics.
Suggested guard (fail fast until full support is implemented)
for (const auto& recipient : vecSend_in) {
if (recipient.fPlatformTransfer) {
+ if (recipient.fSubtractFeeFromAmount) {
+ return util::Error{_("subtractfeefromamount is not supported for platform recipients")};
+ }
creditOutputs.emplace_back(recipient.nAmount, recipient.scriptPubKey);
platform_total += recipient.nAmount;
} else {
vecSend.push_back(recipient);
}
}📝 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.
| for (const auto& recipient : vecSend_in) { | |
| if (recipient.fPlatformTransfer) { | |
| creditOutputs.emplace_back(recipient.nAmount, recipient.scriptPubKey); | |
| platform_total += recipient.nAmount; | |
| } else { | |
| vecSend.push_back(recipient); | |
| } | |
| } | |
| opt_assetLockPayload.emplace(creditOutputs); | |
| CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); | |
| ss << *opt_assetLockPayload; | |
| nExtraPayloadSize = ss.size(); | |
| vecSend.insert(vecSend.begin(), | |
| {CScript() << OP_RETURN << ParseHex(""), platform_total, /*fSubtractFeeFromAmount=*/false}); | |
| } else { | |
| for (const auto& recipient : vecSend_in) { | |
| if (recipient.fPlatformTransfer) { | |
| if (recipient.fSubtractFeeFromAmount) { | |
| return util::Error{_("subtractfeefromamount is not supported for platform recipients")}; | |
| } | |
| creditOutputs.emplace_back(recipient.nAmount, recipient.scriptPubKey); | |
| platform_total += recipient.nAmount; | |
| } else { | |
| vecSend.push_back(recipient); | |
| } | |
| } | |
| opt_assetLockPayload.emplace(creditOutputs); | |
| CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); | |
| ss << *opt_assetLockPayload; | |
| nExtraPayloadSize = ss.size(); | |
| vecSend.insert(vecSend.begin(), | |
| {CScript() << OP_RETURN << ParseHex(""), platform_total, /*fSubtractFeeFromAmount=*/false}); | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/spend.cpp` around lines 792 - 808, The loop that moves platform
recipients out of vecSend_in (checking recipient.fPlatformTransfer) ignores
recipient.fSubtractFeeFromAmount so fee-subtraction is silently dropped; add a
fail-fast guard inside that loop: when iterating vecSend_in in the block that
builds creditOutputs, detect if any recipient has fPlatformTransfer == true AND
fSubtractFeeFromAmount == true and return an explicit RPC error (or propagate an
error) explaining that subtract-fee-from-amount is not supported for platform
transfers; reference the loop over vecSend_in, recipient.fPlatformTransfer,
recipient.fSubtractFeeFromAmount, creditOutputs, platform_total and the
vecSend.insert code path so the guard is placed before removing platform
recipients.
| opt_assetLockPayload.emplace(creditOutputs); | ||
| CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); | ||
| ss << *opt_assetLockPayload; | ||
| nExtraPayloadSize = ss.size(); | ||
|
|
There was a problem hiding this comment.
Extra payload bytes are missed in the second fee calculation pass.
You set nExtraPayloadSize here, but if change is dropped later, nBytes is recomputed without re-adding payload overhead, underestimating required fee.
Suggested fix
nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control);
+ if (nExtraPayloadSize != 0) {
+ nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize;
+ }
fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/spend.cpp` around lines 801 - 805, When computing fees the extra
payload size set via opt_assetLockPayload.emplace(creditOutputs) and written
into nExtraPayloadSize is not re-applied when nBytes is recomputed during the
second fee-pass, causing underestimation; update the fee-recalculation path that
recomputes nBytes (the block that uses nBytes after the second pass) to include
the payload overhead by recomputing/adding nExtraPayloadSize (derived from
serializing *opt_assetLockPayload into CDataStream) to nBytes before the fee
check, and ensure opt_assetLockPayload is only cleared/dropped after the final
fee decision so nExtraPayloadSize remains available for the second pass.
| self.check_mempool_result(tx=lock_tx, | ||
| result_expected={'allowed': False, 'reject-reason': 'bad-assetlocktx-version-2'}) |
There was a problem hiding this comment.
Fix Flake8 E128 continuation indentation on multiline check_mempool_result calls.
These lines are currently under-indented and will fail lint checks.
Proposed fix
- self.check_mempool_result(tx=lock_tx,
- result_expected={'allowed': False, 'reject-reason': 'bad-assetlocktx-version-2'})
+ self.check_mempool_result(
+ tx=lock_tx,
+ result_expected={'allowed': False, 'reject-reason': 'bad-assetlocktx-version-2'},
+ )
@@
- self.check_mempool_result(tx=lock_tx_v2,
- result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})
+ self.check_mempool_result(
+ tx=lock_tx_v2,
+ result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}},
+ )Also applies to: 791-792
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 777-777: continuation line under-indented for visual indent
(E128)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/feature_asset_locks.py` around lines 776 - 777, The multiline
calls to check_mempool_result (e.g., check_mempool_result(tx=lock_tx,
result_expected={'allowed': False, 'reject-reason':
'bad-assetlocktx-version-2'}) and the similar call around lines 791-792) violate
Flake8 E128 continuation indentation; fix by re-indenting the continued lines so
they align with the first element after the opening parenthesis (or use a
hanging indent) so the continuation line(s) are visually aligned with the start
of the arguments inside check_mempool_result.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two blocking issues in the new wallet send path for platform addresses: the wallet always builds a v2 asset-lock payload even when DEPLOYMENT_V24 is inactive (the node will reject the tx but RPC reports success), and subtractfeefromamount is silently dropped because the platform recipients are replaced by a synthetic OP_RETURN with the flag hard-coded to false. A further suggestion: the duplicate-recipient guard in ParseRecipients does not track platform addresses, so the same destination can be specified twice through different encodings.
Reviewed commit: 303a71c
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
4 finding(s)
🔴 blocking: Wallet builds a v2 asset-lock payload unconditionally; pre-V24 sends to platform addresses appear to succeed but are rejected
src/wallet/spend.cpp (lines 788-810)
opt_assetLockPayload.emplace(creditOutputs) uses the default CAssetLockPayload::CURRENT_VERSION, which is now 2 (src/evo/assetlocktx.h:32). Consensus rejects v2 asset locks with bad-assetlocktx-version-2 when DEPLOYMENT_V24 is inactive (src/evo/assetlocktx.cpp:59-61, gated in src/evo/specialtxman.cpp:140-141). Address decoding (DecodePlatformDestination) is not gated on V24, so a user sending to a dash1…/tdash1… address before V24 activates will have a syntactically valid tx constructed that the mempool always rejects. Because CommitTransaction() only logs broadcast failure and SendMoney() still returns a txid (src/wallet/wallet.cpp:2399-2402, src/wallet/rpc/spend.cpp:177-182), sendtoaddress/sendmany will report success for a wallet-tx that never enters the mempool. Either gate the platform-recipient path on DeploymentActiveAfter(..., DEPLOYMENT_V24) and return a friendly RPC error pre-V24, or fall back to building a v1 payload when only P2PKH credit outputs are required.
source: claude, codex
🔴 blocking: `subtractfeefromamount` is silently ignored for platform recipients
src/wallet/spend.cpp (lines 792-807)
Platform recipients are pulled out of vecSend_in and copied verbatim into creditOutputs at their original nAmount. They are then replaced in vecSend by a single synthetic OP_RETURN recipient whose fSubtractFeeFromAmount is hard-coded to false (line 807). The fee-subtraction loop later iterates only sortedVecSend and checks each recipient's flag (src/wallet/spend.cpp:1051-1083), so the flag set by the caller on a platform recipient is dropped completely. Result: sendtoaddress(tdash1..., amount, ..., subtractfeefromamount=true) still locks the full amount into the credit outputs and charges the fee from change/other recipients, violating the documented RPC contract and turning some valid calls into unexpected insufficient-funds failures. Either reject subtractfeefromamount on platform recipients in ParseRecipients with an explicit error, or implement consistent fee subtraction on both the credit output nAmount and the OP_RETURN platform_total.
source: claude, codex
🟡 suggestion: Duplicate-recipient protection is bypassed for platform addresses
src/wallet/rpc/spend.cpp (lines 33-58)
The duplicate-address guard tracks only decoded CTxDestinations in the destinations set. The platform-address branch derives a script_pub_key but never inserts anything into that set, so callers can specify the same destination twice — for example a regular yX… address and the equivalent tdash1… encoding, or two different platform encodings of the same hash — without tripping the duplicate check. This is inconsistent with the existing RPC behaviour and silently allows accidental duplicate payouts. Track an additional identifier (e.g. the derived script_pub_key or the underlying hash) in the dedup set for platform recipients and reject duplicates against both regular and platform encodings.
source: codex
💬 nitpick: Inserting the OP_RETURN at the front shifts caller-provided `change_pos` semantics
src/wallet/spend.cpp (lines 806-807)
When platform recipients are present, the OP_RETURN is unconditionally inserted at vecSend.begin(), but change_pos (passed in by the caller) is then interpreted relative to the post-insertion vector. A caller that passes change_pos = 0 intending 'before my first recipient' will now place change before the OP_RETURN. Likely benign for current RPC callers that leave change_pos = -1, but worth either documenting near the insertion or asserting that change_pos == -1 when has_platform is true to make the contract explicit.
source: claude
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/spend.cpp`:
- [BLOCKING] lines 788-810: Wallet builds a v2 asset-lock payload unconditionally; pre-V24 sends to platform addresses appear to succeed but are rejected
`opt_assetLockPayload.emplace(creditOutputs)` uses the default `CAssetLockPayload::CURRENT_VERSION`, which is now 2 (`src/evo/assetlocktx.h:32`). Consensus rejects v2 asset locks with `bad-assetlocktx-version-2` when `DEPLOYMENT_V24` is inactive (`src/evo/assetlocktx.cpp:59-61`, gated in `src/evo/specialtxman.cpp:140-141`). Address decoding (`DecodePlatformDestination`) is not gated on V24, so a user sending to a `dash1…`/`tdash1…` address before V24 activates will have a syntactically valid tx constructed that the mempool always rejects. Because `CommitTransaction()` only logs broadcast failure and `SendMoney()` still returns a txid (`src/wallet/wallet.cpp:2399-2402`, `src/wallet/rpc/spend.cpp:177-182`), `sendtoaddress`/`sendmany` will report success for a wallet-tx that never enters the mempool. Either gate the platform-recipient path on `DeploymentActiveAfter(..., DEPLOYMENT_V24)` and return a friendly RPC error pre-V24, or fall back to building a v1 payload when only P2PKH credit outputs are required.
In `src/wallet/spend.cpp`:
- [BLOCKING] lines 792-807: `subtractfeefromamount` is silently ignored for platform recipients
Platform recipients are pulled out of `vecSend_in` and copied verbatim into `creditOutputs` at their original `nAmount`. They are then replaced in `vecSend` by a single synthetic OP_RETURN recipient whose `fSubtractFeeFromAmount` is hard-coded to `false` (line 807). The fee-subtraction loop later iterates only `sortedVecSend` and checks each recipient's flag (`src/wallet/spend.cpp:1051-1083`), so the flag set by the caller on a platform recipient is dropped completely. Result: `sendtoaddress(tdash1..., amount, ..., subtractfeefromamount=true)` still locks the full `amount` into the credit outputs and charges the fee from change/other recipients, violating the documented RPC contract and turning some valid calls into unexpected insufficient-funds failures. Either reject `subtractfeefromamount` on platform recipients in `ParseRecipients` with an explicit error, or implement consistent fee subtraction on both the credit output `nAmount` and the OP_RETURN `platform_total`.
In `src/wallet/rpc/spend.cpp`:
- [SUGGESTION] lines 33-58: Duplicate-recipient protection is bypassed for platform addresses
The duplicate-address guard tracks only decoded `CTxDestination`s in the `destinations` set. The platform-address branch derives a `script_pub_key` but never inserts anything into that set, so callers can specify the same destination twice — for example a regular `yX…` address and the equivalent `tdash1…` encoding, or two different platform encodings of the same hash — without tripping the duplicate check. This is inconsistent with the existing RPC behaviour and silently allows accidental duplicate payouts. Track an additional identifier (e.g. the derived `script_pub_key` or the underlying hash) in the dedup set for platform recipients and reject duplicates against both regular and platform encodings.
In `src/wallet/spend.cpp`:
- [NITPICK] lines 806-807: Inserting the OP_RETURN at the front shifts caller-provided `change_pos` semantics
When platform recipients are present, the OP_RETURN is unconditionally inserted at `vecSend.begin()`, but `change_pos` (passed in by the caller) is then interpreted relative to the post-insertion vector. A caller that passes `change_pos = 0` intending 'before my first recipient' will now place change before the OP_RETURN. Likely benign for current RPC callers that leave `change_pos = -1`, but worth either documenting near the insertion or asserting that `change_pos == -1` when `has_platform` is true to make the contract explicit.
Inline posting hit GitHub HTTP 422, so I posted the verified findings as a top-level review body.
2 participants
Issue being fixed or feature implemented
Implements asset lock txes v2, see dashpay/dips#182
What was done?
addressfield for ToJson helper for asset locksAlso prior works: #7285, #7284
How Has This Been Tested?
See updates for unit & functional tests
Breaking Changes
Changes consensus rules for v24 activation to enable assetlock txes v2.
Checklist: