fix: make huge asset lock tx non-standard#7359
Conversation
|
✅ Review complete (commit c8801b2) |
|
![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: 5f0e2fcf97
ℹ️ 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".
WalkthroughThis PR introduces standardness validation for asset lock special transactions in the Dash mempool. A new IsStandardSpecialTx function enforces three constraints on asset lock transactions: maximum 100 inputs, maximum 20,480 bytes total serialized size, and rejection of payload version >= 2. The function is integrated into MemPoolAccept::PreChecks when fRequireStandard is enabled to reject non-standard asset locks with descriptive reason codes. Functional tests and helper updates verify mempool rejection/acceptance behavior and block inclusion. Sequence DiagramsequenceDiagram
participant Client
participant MemPool as MemPoolAccept
participant Validator as IsStandardSpecialTx
participant Mempool
Client->>MemPool: Submit asset lock transaction
MemPool->>MemPool: PreChecks: check fRequireStandard
alt fRequireStandard enabled
MemPool->>Validator: Validate special transaction
Validator->>Validator: Check if TRANSACTION_ASSET_LOCK
alt Asset lock with ≤100 inputs, ≤20480 bytes, version<2
Validator->>MemPool: return true
MemPool->>Mempool: Accept to mempool
else Exceeds limits or invalid version
Validator->>MemPool: return false with reason
MemPool->>Client: Reject TX_NOT_STANDARD
end
else fRequireStandard disabled
MemPool->>Mempool: Accept to mempool
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/feature_asset_locks.py`:
- Around line 765-792: Extend test_non_standard to also exercise the new
serialized-size rule: construct an asset lock transaction whose serialized size
exceeds 20,480 bytes (use the existing helpers in the test — e.g., build a
funding/split tx with many outputs or add a large OP_RETURN/payload and call
create_assetlock to produce the oversized asset lock), assert its vin length if
useful, then call node_wallet.testmempoolaccept and nodes[1].testmempoolaccept
and verify the permissive node allows it but the standard node rejects it with
reject-reason 'assetlocktx-too-large' (follow the existing pattern used for
lock_v2 and lock_many_inputs in test_non_standard).
🪄 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: 27836ae4-8a8a-4eb5-80ed-191953376c14
📒 Files selected for processing (4)
src/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/validation.cpptest/functional/feature_asset_locks.py
| def test_non_standard(self, node_wallet, node, pubkey): | ||
| self.log.info("Testing that v2 and >100-input asset locks are non-standard...") | ||
| assert softfork_active(node_wallet, 'v24') | ||
|
|
||
| coin = node_wallet.listunspent(query_options={'minimumAmount': 1}).pop() | ||
| lock_v2 = self.create_assetlock(coin, COIN, pubkey, version=2) | ||
| # reserve this coin so funding the split below can not spend it | ||
| node_wallet.lockunspent(False, [{'txid': coin['txid'], 'vout': coin['vout']}]) | ||
|
|
||
| self.log.info("Split one coin into 101 outputs to build an asset lock with >100 inputs") | ||
| raw = node_wallet.createrawtransaction([], [{node_wallet.getnewaddress(): 1} for _ in range(101)]) | ||
| funded = node_wallet.fundrawtransaction(raw, {'change_position': 101})['hex'] | ||
| split_txid = node_wallet.sendrawtransaction(node_wallet.signrawtransactionwithwallet(funded)['hex']) | ||
| self.generate(node, 1) | ||
| many_coins = [{'txid': split_txid, 'vout': i, 'amount': 1} for i in range(101)] | ||
| lock_many_inputs = self.create_assetlock(many_coins, COIN, pubkey) | ||
| assert_equal(len(lock_many_inputs.vin), 101) | ||
|
|
||
| self.log.info("A standard node (-acceptnonstdtxn=0) rejects them; the permissive node accepts them") | ||
| self.restart_node(1, self.extra_args[1] + ["-acceptnonstdtxn=0"]) | ||
| self.connect_nodes(1, 0) | ||
| for tx, reason in [(lock_v2, 'assetlocktx-version-2'), (lock_many_inputs, 'assetlocktx-too-many-inputs')]: | ||
| tx_hex = tx.serialize().hex() | ||
| assert_equal(node_wallet.testmempoolaccept([tx_hex])[0]['allowed'], True) | ||
| rejected = self.nodes[1].testmempoolaccept([tx_hex])[0] | ||
| assert_equal(rejected['allowed'], False) | ||
| assert_equal(rejected['reject-reason'], reason) | ||
|
|
There was a problem hiding this comment.
Missing functional coverage for the assetlocktx-too-large standardness rule.
test_non_standard currently validates only two reject paths (Line 786): version 2 and >100 inputs. The PR introduces a third mempool standardness rule (serialized size > 20,480), but this test does not assert that rejection path, so that policy can regress unnoticed.
Suggested test extension
@@
- for tx, reason in [(lock_v2, 'assetlocktx-version-2'), (lock_many_inputs, 'assetlocktx-too-many-inputs')]:
+ # TODO: also construct a tx with <=100 inputs but serialized size > 20480
+ # and assert reject-reason == 'assetlocktx-too-large'
+ for tx, reason in [
+ (lock_v2, 'assetlocktx-version-2'),
+ (lock_many_inputs, 'assetlocktx-too-many-inputs'),
+ # (lock_too_large, 'assetlocktx-too-large'),
+ ]:🤖 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 765 - 792, Extend
test_non_standard to also exercise the new serialized-size rule: construct an
asset lock transaction whose serialized size exceeds 20,480 bytes (use the
existing helpers in the test — e.g., build a funding/split tx with many outputs
or add a large OP_RETURN/payload and call create_assetlock to produce the
oversized asset lock), assert its vin length if useful, then call
node_wallet.testmempoolaccept and nodes[1].testmempoolaccept and verify the
permissive node allows it but the standard node rejects it with reject-reason
'assetlocktx-too-large' (follow the existing pattern used for lock_v2 and
lock_many_inputs in test_non_standard).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Codex's blocking finding is correct: the PR's stated goal is for v2 asset locks to be non-standard but consensus-valid, yet CAssetLockPayload::CURRENT_VERSION is still 1 and CheckAssetLockTx rejects any getVersion() > CURRENT_VERSION. CheckSpecialTx is invoked unconditionally inside MemPoolAccept::PreChecks (validation.cpp:959) and during block validation, so a v2 asset lock will be rejected via consensus regardless of the new policy gate — both nodes in the new functional test will fail to mine lock_v2. Both reviewers also converge on the missing functional coverage for the assetlocktx-too-big branch. Claude's nitpicks about the magic-number naming and 'silent pass-through' on bad payload deserialization were dropped: payload-deserialization failures are caught later in the same PreChecks invocation by CheckAssetLockTx ("bad-assetlocktx-payload"), so no relay leakage occurs.
🔴 1 blocking | 🟡 1 suggestion(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/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:1259-1263: v2 asset locks remain consensus-invalid, contradicting the PR goal and breaking the new test
The PR's header doc and commit message state that v2 asset locks should be "non-standard but consensus-valid" so they can be activated later without another hard fork. The implementation only adds a relay/policy check in `IsStandardSpecialTx`, but does not relax consensus: `CAssetLockPayload::CURRENT_VERSION` is still 1 (src/evo/assetlocktx.h:31), and `CheckAssetLockTx` returns `bad-assetlocktx-version` for `getVersion() > CURRENT_VERSION` (src/evo/assetlocktx.cpp:56). `MemPoolAccept::PreChecks` calls `CheckSpecialTx` unconditionally at validation.cpp:959 (not gated by `fRequireStandard`), and `ProcessSpecialTxsInBlock` calls the same path during block validation. As a result:
- `node_wallet.sendrawtransaction(lock_v2.serialize().hex())` in the new functional test (feature_asset_locks.py:794) will throw with `bad-assetlocktx-version` even on the permissive `-acceptnonstdtxn=1` node, so the test will fail.
- Even if the tx is force-injected into a block, `CheckBlock`/`ConnectBlock` paths will reject the block as consensus-invalid.
Either bump `CAssetLockPayload::CURRENT_VERSION` to 2 (and gate it on the appropriate deployment, e.g. v24), or remove the v2 standardness path from this PR until consensus actually accepts v2. As written, the PR cannot achieve its stated outcome and the functional test will not pass.
In `test/functional/feature_asset_locks.py`:
- [SUGGESTION] test/functional/feature_asset_locks.py:765-802: `assetlocktx-too-big` policy branch is not exercised
`test_non_standard` covers `assetlocktx-version-2` and `assetlocktx-too-many-inputs`, but never builds an asset lock with ≤100 inputs and `GetTotalSize() > 20480`. In `IsStandardSpecialTx`, the input-count check runs before the size check (specialtxman.cpp:1248 → 1254), so the 101-input fixture short-circuits on `assetlocktx-too-many-inputs` and the 20 KB branch is dead from the test's perspective. Add a case that crafts an asset lock with ≤100 inputs whose serialized size exceeds 20480 bytes (e.g. by padding `creditOutputs` count or scriptSigs) and asserts the reject reason is `assetlocktx-too-big`. Otherwise a future reorder of the checks or a change to the `max_tx_size_for_platform` constant can regress silently.
knst
force-pushed
the
fix-non-standard-asset-locks
branch
from
5f0e2fc to
c8801b2
Compare
3 participants
Issue being fixed or feature implemented
Platform ignores asset-locks with amount of inputs more than 100 or with size of transaction bigger than 20480 bytes.
If user will create too big asset-lock transaction that platform (L2) can not recognize it may cause to fund lost.
Issue is worsened once user will have ability to create asset-lock tx himself by Dash Core after #7294
It's an addition to this PR: https://github.com/dashpay/platform/pull/3491/files
What was done?
Asset locks that have more than 100 inputs are non-standard txes
Asset locks that have size bigger than 20480 bytes are non-standard txes
Asset locks that have v2 payload are non-standard txes. Once v24 is activated, platform may be not ready to process them. This non-standard limitation is a feature for soft enabling v2 asset-locks without 2nd fork.
How Has This Been Tested?
See updates in functional test.
Breaking Changes
Some special transactions are now valid for consensus if they are mined in block but they are non-standard and won't be relayed to network.
Checklist: