Repository navigation
fix(drive-abci)!: refuse IdentityCreate identity id and key proof failures before they consume the asset lock - #5431
Conversation
…lures unpaid (PV14) An IdentityCreate's asset lock signature covers the transition's signable bytes, which leave out the identity id and every key's proof-of-possession signature. identity_create advanced_structure v0 nonetheless charged the asset lock a penalty (a PartiallyUseAssetLockAction recording the signable bytes as used) for an identity id the asset lock does not derive and for a failed proof of possession, so a transition carrying a wrong value in one of those fields consumed part of the lock and left the correctly signed transition a replay. identity_create advanced_structure v1, selected only by DRIVE_ABCI_VALIDATION_VERSIONS_V10 (protocol version 14), refuses both failures unpaid: no action, no penalty, no used tag, the asset lock is left as it was. A proof-of-possession failure includes a signature on a hash-type key. Key structure failures stay paid, since the asset lock signature covers the keys. Protocol versions 1-13 keep v0, so their history replays unchanged. Protocol version 14 is neither in a stable release nor activated on testnet or mainnet, so its table is amended in place; no shipped generation is edited. The check_tx test identity_can_create_with_semi_used_outpoint built its paid partial use from an empty master key signature, which is now unpaid at the latest version; it now uses an identity without a master key (a key structure failure, still paid) and replays those same signed bytes. Tests would have caught this in CI (red with PV14's table on advanced_structure v0, green with v1): - should_refuse_unpaid_an_identity_create_whose_identity_id_does_not_match_its_asset_lock ✖ → ✔ - should_refuse_unpaid_an_identity_create_with_an_invalid_key_proof_of_possession ✖ → ✔ - should_refuse_unpaid_an_identity_create_with_a_signature_on_a_hash_key ✖ → ✔ - strategy should_reject_a_proposal_holding_an_identity_create_whose_identity_id_does_not_match_its_asset_lock ✖ (Accept) → ✔ (Reject); its control, the same proposal with the derived identity id, is accepted Guards, green before and after by design: - should_charge_the_asset_lock_for_an_identity_id_or_key_proof_fault_before_protocol_version_14 (PV1, PV13) - should_charge_the_asset_lock_for_an_identity_create_with_an_invalid_key_structure (latest) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ures at check_tx An IdentityCreate's identity id and each key's proof-of-possession signature are left out of the signable bytes the asset lock key signs, so a transition whose only fault is in one of them must not enter the mempool. check_tx did not run IdentityCreate's advanced structure validation, so it admitted such a transition. check_tx now: - checks, right after basic structure and before the asset lock proof is verified, that the identity id is the one the asset lock's outpoint derives. The id is also the transition's mempool sender, so a transition can only take the sender of the identity its own asset lock creates. A wrong id is refused in about 6 µs, where it used to be admitted after the ~0.6 ms instant lock check. - runs IdentityCreate's advanced structure validation (identity id, key structure, every key's proof of possession), as it already does for IdentityCreateFromAddresses and MasternodeVote. - keeps the action it built for advanced structure validation when it does not validate state, instead of transforming the transition a second time. That second transform verified an asset lock proof twice (the instant lock signature, or Core's copy of a chain-locked transaction) and counted its operations twice in the fee estimate. Batch and IdentityCreateFromAddresses, already on the list, were transformed twice too and now are not. Admission is not consensus, so every protocol version gets this; block execution is unchanged. Measured New check_tx cost (release build, instant lock verification included): a valid transition with one ECDSA key 0.68 -> 0.74 ms; the worst case, six BLS keys, about 4.6 ms, which matches what IdentityCreateFromAddresses already costs at check_tx. Recheck is unchanged at about 7 µs. Tests would have caught this in CI (red with this commit's check_tx and predicate changes reverted, where check_tx admitted the faulty transition at protocol version 1; green with them): - should_refuse_at_check_tx_an_identity_create_whose_identity_id_does_not_match_its_asset_lock ✖ → ✔ - should_refuse_at_check_tx_an_identity_create_with_an_invalid_key_proof_of_possession ✖ → ✔ - should_refuse_at_check_tx_an_identity_create_with_a_signature_on_a_hash_key ✖ → ✔ - should_refuse_at_check_tx_an_identity_create_with_an_invalid_key_structure ✖ → ✔ - requires_advanced_structure_validation_with_state_on_check_tx::should_return_true_only_for_batch_identity_creates_and_masternode_vote ✖ → ✔ - should_verify_the_asset_lock_once_at_check_tx ✖ → ✔ (with the action dropped again, Core is asked for the chain-locked transaction twice) With only the early identity id check reverted, the identity id test still fails: once the asset lock is spent, check_tx reports IdentityAssetLockTransactionOutPointAlreadyConsumedError instead of the identity id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 5
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit e897147. Normal review starts when eligible; priority review starts as soon as a slot is available. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
No actionable in-scope issues were found. The new validator confines unpaid identity-ID and proof-of-possession failures to protocol version 14 while preserving historical paid failures; check_tx hardening remains admission-only, and regression tests cover asset-lock preservation, legacy behavior, single-transform reuse, and proposal rejection. Verification was static only: no builds or tests were run locally, and the supplied CI snapshot still showed build/test prerequisites queued or pending.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate diff that changes consensus validation, asset-lock fee consumption, identity authorization, and proof-of-possession signature handling in the IdentityCreate advanced-structure validation and check_tx pipeline. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-15T10:01:08.418Z |
|
Bots are done — your move: post |
check_tx kept the action it built for advanced structure validation for every transition whose state it does not validate, instead of transforming the transition a second time. That avoided verifying an IdentityCreate's asset lock twice, but it also changed check_tx for Batch: at protocol versions 12 to 14 its fee estimate no longer counted the transform's operations twice, so check_tx admitted some unsponsored batches it refused before. Only an IdentityCreate keeps its action now; every other transition is transformed at check_tx exactly as before this change set. check_tx decides mempool admission only, so this is inert for consensus at every protocol version; the comment at the edit says so. The comments on the identity id check, on the list of transitions check_tx validates the advanced structure of, and on identity create advanced structure v1 now state the rule and why it holds. Tests: should_verify_the_asset_lock_once_at_check_tx still pins that an IdentityCreate's asset lock is verified once. No test is added for Batch: this restores the code path the suite already ran before this change set, and a test of the old fee estimate would pin a double count rather than an intended behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The comment on the protocol versions the IdentityCreate check_tx tests run at claimed versions 1 to 13 validate an identity create the same way; protocol version 11 raises its minimum asset lock balance and 14 also changes its state validation. The v14.rs summary claimed that, without the ranked or time-range grammar, everything but four fixes matched protocol version 13; the table changes listed below it, identity creation's among them, apply regardless. Comment-only change, no behaviour change, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…reate advanced structure v1 The v1 doc said every failure it refuses unpaid is in a field the asset lock signature does not cover. A key whose data does not parse also fails its proof of possession check and is refused unpaid like any other proof failure, although its data is signed: only the holder of the asset lock key can sign such data, and check_tx refuses it. The doc now says so. Comment-only change, no behaviour change, so no test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
Issue being fixed or feature implemented
An
IdentityCreateis authorized by its asset lock's one-time key, which signs the transition's signable bytes. Two fields are left out of those bytes: theidentity_id, and the proof-of-possession signature of each key. Each key signs those same bytes, so the asset lock signature cannot cover the key signatures.Until now, the identity create advanced structure validation charged the asset lock when one of those two fields was wrong. That covers an
identity_idother than the one the asset lock's outpoint derives, and a key whose proof of possession fails. Such a failure was a paid failure charged to the asset lock. The asset lock signature does not cover those values, so a failure in them must not consume the asset lock. A transition carrying such a value also must not enter the mempool. check_tx did not run this validation forIdentityCreate, so it admitted such a transition.#4968 made the same change for
IdentityCreateFromAddresses.What was done?
The work is split into two commits.
1. Consensus at protocol version 14: refuse without a fee (
fix(drive-abci)!: refuse IdentityCreate identity id and key proof failures unpaid (PV14))identity_createadvanced_structurev1. OnlyDRIVE_ABCI_VALIDATION_VERSIONS_V10(protocol version 14) selects it.prepare_proposaldrops such a transition andprocess_proposalrejects a block that holds one.2. Admission at every protocol version: refuse at check_tx (
fix(drive-abci): refuse IdentityCreate identity id and key proof failures at check_tx)IdentityCreatejoinsrequires_advanced_structure_validation_with_state_on_check_tx, whereIdentityCreateFromAddressesandMasternodeVotealready are. check_tx then runs the transform followed by advanced structure validation: identity id, key structure, and every key's proof of possession.IdentityCreate, check_tx keeps the action it built for advanced structure validation instead of transforming the transition a second time, so the asset lock proof (the instant lock signature, or the chain-locked transaction fetched from Core) and the asset lock signature are verified once, and their operations are counted once in the fee estimate. Every other transition is transformed at check_tx exactly as before.v14.rs, which claimed that apart from four fixes everything matched protocol version 13; and the doc of identity create advanced structure v1, which now covers key data that does not parse.check_tx cost
Method:
Platform::check_txon a fresh platform at the latest protocol version. Each figure is the median of up to 2,000 calls.max_public_keys_in_creation). The 6-BLS-key worst case matches whatIdentityCreateFromAddressesalready costs at check_tx (3.9 ms with 6 BLS keys; it has no instant lock).In-place changes to shipped generations
DRIVE_ABCI_VALIDATION_VERSIONS_V10identity_create_state_transition.advanced_structuregoes fromSome(0)toSome(1).PLATFORM_V14only.advanced_structurev0 is untouched.check_tx_verificationv0 (state_transition_to_execution_event_for_check_txv0) andrequires_advanced_structure_validation_with_state_on_check_txstate_transition_to_execution_event_for_check_txis 0 in every validation table.process_raw_state_transitions, whichprepare_proposalandprocess_proposalrun.IdentityCreate, the action check_tx now keeps is the one a second transform rebuilt from the same inputs. Every other transition is transformed exactly as before, so its admission and fee estimate are unchanged.How Has This Been Tested?
Tests in
identity_create/mod.rs, modulefields_outside_the_asset_lock_signature. They run throughPlatform::check_txand throughprocess_raw_state_transitions, whichprepare_proposalandprocess_proposalrun.check_tx tests, at protocol versions 1, 13 and latest. Each also checks that the correct transition is still admitted.
should_refuse_at_check_tx_an_identity_create_whose_identity_id_does_not_match_its_asset_lock. It also checks again after the asset lock is spent, which pins that the identity id is checked before the asset lock proof.should_refuse_at_check_tx_an_identity_create_with_an_invalid_key_proof_of_possessionshould_refuse_at_check_tx_an_identity_create_with_a_signature_on_a_hash_keyshould_refuse_at_check_tx_an_identity_create_with_an_invalid_key_structureshould_verify_the_asset_lock_once_at_check_tx: an identity create with a chain asset lock proof is admitted, and Core is asked for the transaction exactly once.Consensus tests:
should_refuse_unpaid_an_identity_create_whose_identity_id_does_not_match_its_asset_lock,..._with_an_invalid_key_proof_of_possessionand..._with_a_signature_on_a_hash_key, at the latest version. Each checks that:should_charge_the_asset_lock_for_an_identity_create_with_an_invalid_key_structure, at the latest version: the failure is still paid.should_charge_the_asset_lock_for_an_identity_id_or_key_proof_fault_before_protocol_version_14, at protocol versions 1 and 13: v0's charged refusal is unchanged.Other tests:
identity_create_proposal_tests::should_reject_a_proposal_holding_an_identity_create_whose_identity_id_does_not_match_its_asset_lock. Through the ABCI handler on a test chain,process_proposalrejects a proposed block that holds such a transition. Its control proposal, which carries the derived identity id, is accepted.check_tx::v0::tests::identity_can_create_with_semi_used_outpointused to build its paid partial use from an empty master key signature, which is unpaid from protocol version 14. It now uses an identity without a master key, a key structure failure that is still paid.requires_advanced_structure_validation_with_state_on_check_txnow includeIdentityCreate.Red → green:
process_proposalreturnsAccept. With v1 they pass. The two charge tests pass both ways, by design.IdentityAssetLockTransactionOutPointAlreadyConsumedErrorinstead of the identity id.should_verify_the_asset_lock_once_at_check_txfails, because Core is asked for the transaction twice.Runs, on this branch rebased on 5.0.0-rc.2:
cargo test -p drive-abci --lib -- check_tx asset_lock identity_create: 496 passed. 1 ignored:verify_check_tx_on_data_contract_create, which is already ignored onv5.0-dev.RUST_MIN_STACK=4194304 cargo test -p drive-abci --test strategy_tests -- identit top_up: 37 passed.cargo clippy -p drive-abci --all-targets --all-features --locked -- --no-deps -D warnings: clean.cargo fmt --all -- --check: clean.Breaking Changes
IdentityCreatewhose identity id is not the one its asset lock derives, or whose key proof of possession fails. Before, it charged the asset lock. Consensus at protocol versions 1–13 is unchanged.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
PR Hygiene ·
e897147/skip-botsproceeds without the ones not yet reported/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.