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 👍 / 👎.
WalkthroughAdds asset-lock payload versioning (INITIAL_VERSION=1, CURRENT_VERSION=2) and a v24 activation gate that rejects v2 payloads before activation. V2 relaxes credit-output rules to accept P2PKH or P2SH; v1 remains P2PKH-only. Extends asset-lock JSON to include platform addresses for v2, updates validateaddress to recognize platform addresses, adds wallet parsing/recipient flag for platform transfers, builds asset-lock transactions (nVersion=3, TRANSACTION_ASSET_LOCK) from platform recipients, and adds unit and functional tests for pre/post-v24 and v1/v2 behaviors. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/evo/assetlocktx.cppsrc/evo/assetlocktx.cpp:5:10: fatal error: 'evo/assetlocktx.h' file not found ... [truncated 2200 characters] ... cters 10-1023 src/evo/core_write.cppsrc/evo/core_write.cpp:5:10: fatal error: 'evo/assetlocktx.h' file not found ... [truncated 1071 characters] ... lang/install/lib/clang/18/include" src/evo/specialtxman.cppsrc/evo/specialtxman.cpp:5:10: fatal error: 'evo/specialtxman.h' file not found ... [truncated 2200 characters] ... 4784, characters 10-1023
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.
| 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.
yes, it's expected. Wallet creates only v2 asset locks. |
|
Thanks for the clarification — I missed this top-level reply, not intentional. You’re right that wallet-created asset locks being v2-only is expected, so the v1 fallback part of my review was wrong. I’m retracting that as a blocker. The narrower thing I was trying to flag is only the pre-V24 RPC behavior: platform addresses appear to be accepted, a v2 payload is built, consensus rejects it with I tried to edit the original review body to reflect this, but GitHub returned 404 for the review update endpoint, so leaving the correction here instead. The separate |
Co-authored-by: thephez <thephez@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/spend.cpp`:
- Around line 786-810: Detecting platform recipients (has_platform) currently
always builds a v2 asset-lock payload (opt_assetLockPayload) and inserts an
OP_RETURN output into vecSend even on pre-v24 nodes; change this so you first
check the v24 activation flag (use the existing consensus/activation helper
available in the codebase) and only construct opt_assetLockPayload, set
nExtraPayloadSize, and prepend the OP_RETURN output when v24 is active; if v24
is not active and has_platform is true, abort early with a clear RPC error
(return/throw the existing RPC error path) instead of building the payload so
the RPC fails fast and does not produce an invalid v2 asset-lock transaction.
In `@test/functional/feature_asset_locks.py`:
- Around line 800-803: Remove the unused local variables causing Flake8 F841:
either delete the assignment to fee_test_amount (and the unused COIN reference)
and/or remove the unused txid_subtract variable, or use the returned value;
specifically update the code around fee_test_amount and the call to
node_wallet.sendtoaddress (which assigns txid_subtract) — e.g., drop the
fee_test_amount line and replace txid_subtract = node_wallet.sendtoaddress(...)
with just node_wallet.sendtoaddress(address=platform_addr_from_key, ...) or
store the result in a used variable/underscore if you intend to ignore it.
🪄 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: 8ada7e55-42fe-48cb-bd18-38602116c367
📒 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
🚧 Files skipped from review as they are similar to previous changes (5)
- src/wallet/wallet.h
- src/evo/assetlocktx.cpp
- src/evo/assetlocktx.h
- src/evo/core_write.cpp
- src/test/evo_assetlocks_tests.cpp
| const bool has_platform = std::any_of(vecSend_in.cbegin(), vecSend_in.cend(), | ||
| [](const auto& r) { return r.fPlatformTransfer; }); | ||
| if (has_platform) { | ||
| std::vector<CTxOut> creditOutputs; | ||
| CAmount platform_total{0}; | ||
|
|
||
| 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 { | ||
| vecSend = vecSend_in; | ||
| } |
There was a problem hiding this comment.
Gate platform-asset-lock construction on v24 activation.
On Line 788, platform recipients always trigger v2 asset-lock payload creation. Pre-v24, that payload is consensus-invalid, so RPC send paths can still build/sign/return a txid even though relay will reject it (bad-assetlocktx-version-2). Add an explicit pre-v24 guard before payload creation and fail fast with a clear RPC error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/spend.cpp` around lines 786 - 810, Detecting platform recipients
(has_platform) currently always builds a v2 asset-lock payload
(opt_assetLockPayload) and inserts an OP_RETURN output into vecSend even on
pre-v24 nodes; change this so you first check the v24 activation flag (use the
existing consensus/activation helper available in the codebase) and only
construct opt_assetLockPayload, set nExtraPayloadSize, and prepend the OP_RETURN
output when v24 is active; if v24 is not active and has_platform is true, abort
early with a clear RPC error (return/throw the existing RPC error path) instead
of building the payload so the RPC fails fast and does not produce an invalid v2
asset-lock transaction.
| fee_test_amount = COIN | ||
| try: | ||
| txid_subtract = node_wallet.sendtoaddress( | ||
| address=platform_addr_from_key, |
There was a problem hiding this comment.
Remove unused locals to satisfy Flake8 F841.
Line 800 (fee_test_amount) and Line 802 (txid_subtract) are assigned but never used.
Proposed fix
- fee_test_amount = COIN
try:
- txid_subtract = node_wallet.sendtoaddress(
+ node_wallet.sendtoaddress(
address=platform_addr_from_key,
amount=Decimal("1.0"),
subtractfeefromamount=True,📝 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.
| fee_test_amount = COIN | |
| try: | |
| txid_subtract = node_wallet.sendtoaddress( | |
| address=platform_addr_from_key, | |
| try: | |
| node_wallet.sendtoaddress( | |
| address=platform_addr_from_key, |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 800-800: local variable 'fee_test_amount' is assigned to but never used
(F841)
[error] 802-802: local variable 'txid_subtract' is assigned to but never used
(F841)
🤖 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 800 - 803, Remove the
unused local variables causing Flake8 F841: either delete the assignment to
fee_test_amount (and the unused COIN reference) and/or remove the unused
txid_subtract variable, or use the returned value; specifically update the code
around fee_test_amount and the call to node_wallet.sendtoaddress (which assigns
txid_subtract) — e.g., drop the fee_test_amount line and replace txid_subtract =
node_wallet.sendtoaddress(...) with just
node_wallet.sendtoaddress(address=platform_addr_from_key, ...) or store the
result in a used variable/underscore if you intend to ignore it.
Source: Linters/SAST tools
|
✅ Review complete (commit 834d56a) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: wallet still builds v2 asset-lock payloads unconditionally (pre-V24 platform sends will be silently mempool-rejected after RPC reports success), duplicate-recipient guard remains bypassable through Platform encodings, and the synthetic OP_RETURN insertion still shifts caller-supplied change_pos semantics. New findings in the latest delta: the new functional-test block introduces two unused locals (fee_test_amount, txid_subtract) which fail flake8 F841 that is enabled in this repo's Python lint, so CI will break. Fixed prior finding: subtractfeefromamount is now explicitly rejected for Platform recipients in ParseRecipients and has functional-test coverage.
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/spend.cpp`:
- [BLOCKING] src/wallet/spend.cpp:788-810: Wallet builds a v2 asset-lock payload unconditionally; pre-V24 sends to platform addresses return a txid for a tx that can never enter the mempool
`opt_assetLockPayload.emplace(creditOutputs)` (line 801) relies on `CAssetLockPayload`'s default ctor, which sets `nVersion = CURRENT_VERSION = 2` (`src/evo/assetlocktx.h:32,40`). Consensus rejects v2 asset-lock payloads with `bad-assetlocktx-version-2` while `DEPLOYMENT_V24` is inactive (`src/evo/assetlocktx.cpp` + `specialtxman`). Nothing in this code path or in `ParseRecipients` (`src/wallet/rpc/spend.cpp:25-76`) gates platform-recipient decoding on V24, and `CommitTransaction()` only logs broadcast failure (`src/wallet/wallet.cpp:2400-2403`). Net effect: pre-V24, `sendtoaddress`/`sendmany` to a `dash1…`/`tdash1…` address constructs and records a wallet transaction, returns a txid to the user, and the transaction is permanently mempool-rejected with no error surfaced. The functional test only exercises this path after V24 activation (`test/functional/feature_asset_locks.py:780-836`), so the regression has no coverage. Either gate platform-recipient decoding on `DeploymentActiveAfter(..., DEPLOYMENT_V24)` and return an explicit RPC error pre-V24, or fall back to a v1 payload when the credit outputs only contain P2PKH scripts.
In `test/functional/feature_asset_locks.py`:
- [BLOCKING] test/functional/feature_asset_locks.py:800-807: New test block introduces unused locals that fail enabled flake8 F841
The new subtractfeefromamount sub-test assigns `fee_test_amount = COIN` (line 800) and `txid_subtract = node_wallet.sendtoaddress(...)` (line 802), but neither value is read. `test/lint/lint-python.py` enables flake8 `F841` (line 89), so the repository's Python lint will fail on this delta, blocking CI before functional tests run. Remove both unused assignments. (The assertion-message typo `subtracfeefromamount` on line 808 is cosmetic — only present in the local error message — and not worth blocking on by itself, but worth cleaning up in the same touch.)
In `src/wallet/rpc/spend.cpp`:
- [SUGGESTION] src/wallet/rpc/spend.cpp:33-58: Duplicate-recipient protection is bypassed for Platform addresses
The duplicate-recipient guard only tracks decoded `CTxDestination`s in the `destinations` set (lines 35-38). The Platform branch (lines 40-58) derives a `script_pub_key` but inserts nothing into a dedup structure, so a caller can specify the same payout twice via different encodings — e.g. a regular base58 `yX…` address and the `tdash1…` encoding of the same hash, or two different Platform encodings of the same script — without tripping the duplicate check. This is inconsistent with the existing RPC contract and can silently allow accidental duplicate payouts into the credit pool. Track an additional identifier (the derived `script_pub_key` or the underlying hash) and reject duplicates across both regular and Platform encodings.
| if (has_platform) { | ||
| std::vector<CTxOut> creditOutputs; | ||
| CAmount platform_total{0}; | ||
|
|
||
| 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 { | ||
| vecSend = vecSend_in; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Wallet builds a v2 asset-lock payload unconditionally; pre-V24 sends to platform addresses return a txid for a tx that can never enter the mempool
opt_assetLockPayload.emplace(creditOutputs) (line 801) relies on CAssetLockPayload's default ctor, which sets nVersion = CURRENT_VERSION = 2 (src/evo/assetlocktx.h:32,40). Consensus rejects v2 asset-lock payloads with bad-assetlocktx-version-2 while DEPLOYMENT_V24 is inactive (src/evo/assetlocktx.cpp + specialtxman). Nothing in this code path or in ParseRecipients (src/wallet/rpc/spend.cpp:25-76) gates platform-recipient decoding on V24, and CommitTransaction() only logs broadcast failure (src/wallet/wallet.cpp:2400-2403). Net effect: pre-V24, sendtoaddress/sendmany to a dash1…/tdash1… address constructs and records a wallet transaction, returns a txid to the user, and the transaction is permanently mempool-rejected with no error surfaced. The functional test only exercises this path after V24 activation (test/functional/feature_asset_locks.py:780-836), so the regression has no coverage. Either gate platform-recipient decoding on DeploymentActiveAfter(..., DEPLOYMENT_V24) and return an explicit RPC error pre-V24, or fall back to a v1 payload when the credit outputs only contain P2PKH scripts.
source: ['claude', 'codex']
| fee_test_amount = COIN | ||
| try: | ||
| txid_subtract = node_wallet.sendtoaddress( | ||
| address=platform_addr_from_key, | ||
| amount=Decimal("1.0"), | ||
| subtractfeefromamount=True, | ||
| fee_rate=1000, | ||
| ) |
There was a problem hiding this comment.
🔴 Blocking: New test block introduces unused locals that fail enabled flake8 F841
The new subtractfeefromamount sub-test assigns fee_test_amount = COIN (line 800) and txid_subtract = node_wallet.sendtoaddress(...) (line 802), but neither value is read. test/lint/lint-python.py enables flake8 F841 (line 89), so the repository's Python lint will fail on this delta, blocking CI before functional tests run. Remove both unused assignments. (The assertion-message typo subtracfeefromamount on line 808 is cosmetic — only present in the local error message — and not worth blocking on by itself, but worth cleaning up in the same touch.)
| fee_test_amount = COIN | |
| try: | |
| txid_subtract = node_wallet.sendtoaddress( | |
| address=platform_addr_from_key, | |
| amount=Decimal("1.0"), | |
| subtractfeefromamount=True, | |
| fee_rate=1000, | |
| ) | |
| self.log.info("Test subtractfeefromamount for platform sendtoaddress") | |
| try: | |
| node_wallet.sendtoaddress( | |
| address=platform_addr_from_key, | |
| amount=Decimal("1.0"), | |
| subtractfeefromamount=True, | |
| fee_rate=1000, | |
| ) | |
| raise AssertionError("subtractfeefromamount for platform address expected to generate error") | |
| except JSONRPCException as e: | |
| assert "subtractfeefromamount is not supported for Platform addresses" in e.error['message'] |
source: ['codex']
| 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 Platform 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.
🟡 Suggestion: Duplicate-recipient protection is bypassed for Platform addresses
The duplicate-recipient guard only tracks decoded CTxDestinations in the destinations set (lines 35-38). The Platform branch (lines 40-58) derives a script_pub_key but inserts nothing into a dedup structure, so a caller can specify the same payout twice via different encodings — e.g. a regular base58 yX… address and the tdash1… encoding of the same hash, or two different Platform encodings of the same script — without tripping the duplicate check. This is inconsistent with the existing RPC contract and can silently allow accidental duplicate payouts into the credit pool. Track an additional identifier (the derived script_pub_key or the underlying hash) and reject duplicates across both regular and Platform encodings.
source: ['claude', 'codex']
| vecSend.insert(vecSend.begin(), | ||
| {CScript() << OP_RETURN << ParseHex(""), platform_total, /*fSubtractFeeFromAmount=*/false}); |
There was a problem hiding this comment.
💬 Nitpick: Inserting the synthetic OP_RETURN at vecSend.begin() shifts caller-provided change_pos semantics
When Platform recipients are present, the OP_RETURN is unconditionally inserted at vecSend.begin(), while change_pos (from the caller) is later interpreted relative to the post-insertion vector. A caller passing change_pos = 0 intending 'before my first recipient' will now place change before the OP_RETURN. Benign for current sendtoaddress/sendmany callers (they leave change_pos = -1), but CreateTransaction accepts explicit positions, so either document the shift or assert change_pos == -1 when has_platform is true to make the contract explicit.
source: ['claude', 'codex']
3 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: