Repository navigation
fix(drive-abci)!: refuse bytes after a state transition (PV14) - #5011
Conversation
decode_raw_state_transitions v0 decodes with StateTransition::deserialize_from_bytes_untrusted_in_version, which ignores bytes left over after the value, so a transition with bytes appended executed as the original under another transaction hash. v1 decodes with the new StateTransition::deserialize_from_bytes_untrusted_exact_in_version: left over bytes are an InvalidEncoding (SerializedObjectParsingError), refused unpaid in check_tx and block processing. It also reports a transition whose version is not active as StateTransitionNotActiveError (the arm #4993 adds to v0). Only DRIVE_ABCI_METHOD_VERSIONS_V10 (protocol version 14) selects v1; v0's tests are pinned to 13. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
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 |
|
🕓 Queued for automated review — 2nd in line, estimated start in ~25 min (commit 9e29b15)
|
Issue being fixed or feature implemented
check_txand block processing decode every raw state transition withdecode_raw_state_transitions. Generation 0 of that method callsStateTransition::deserialize_from_bytes_untrusted_in_version, which ignores bytes left over after the value. So a transition with any bytes after it decodes, validates and executes exactly like the transition alone.Identity nonces still stop the same transition from executing twice. But every such variant has its own Tenderdash transaction hash. The mempool's duplicate check compares hashes, so it treats each variant as a different transaction. Clients that track a transition by the hash of its bytes can also see a transaction hash they never sent.
#4931 added exact decoders to dpp, but no consensus path called them yet.
What was done?
StateTransition::deserialize_from_bytes_untrusted_exact_in_versionnext todeserialize_from_bytes_untrusted_in_version. Both now go through one private helper that applies the value depth limit and the active version check. The exact one decodes with feat(dpp): exact and untagged untrusted state transition decode via the derive's consumed byte count #4931'sdeserialize_from_bytes_untrusted_exact, so leftover bytes are aPlatformDeserializationError("N bytes left over after the value"). The existing function behaves exactly as before.decode_raw_state_transitions. It is a copy of generation 0 that decodes exactly. Leftover bytes are handled like any other parse failure: anInvalidEncodingcarryingSerializedObjectParsingError(10002).check_txreturns that as its error, and block processing records it as an unpaid consensus error. No new error code.StateTransitionIsNotActiveErrorinto anInvalidEncodingcarryingStateTransitionNotActiveError(10603). That is the same arm #4993 adds to generation 0; see "Dependency on fix(drive-abci): a transition whose version is not active is not a decode failure #4993" below.DRIVE_ABCI_METHOD_VERSIONS_V10(protocol version 14, unreleased, amended in place) now selects generation 1. The dispatcher'sknown_versionsis[0, 1]. Protocol versions 1 to 13 keep generation 0.packages/rs-platform-version/src/version/v14.rs.PlatformVersion::get(13). Its code is unchanged.Before / after
Take a signed DashPay contract create
rawand append one byte (raw ++ [0x00]):check_txSerializedObjectParsingError(10002), "Parsing of serialized object failed due to: unable to deserialize dpp::state_transition::StateTransition: 1 bytes left over after the value"SuccessfulExecution: contract created, fee chargedUnpaidConsensusError(SerializedObjectParsingError): nothing written, nothing chargedThe exact bytes
raware accepted at both protocol versions.Honest clients are unaffected
No client encoder adds bytes after a transition:
encode_to_vec.serialize_to_bytes()intoBroadcastStateTransitionRequest.toBuffer()) use the same encoding.Dependency on #4993
#4993 adds the not-active arm to generation 0. Generation 1 already has it, so behaviour is the same whichever PR merges first. Both PRs edit the tests of
decode_raw_state_transitions/v0/mod.rsclose together (this one pins them to protocol version 13), so the second one to merge will have a small text conflict there.How Has This Been Tested?
decode_raw_state_transitionsv1 unit tests: exact bytes decode; 1 and 100 leftover bytes are refused; a transition followed by a second one is refused; the size limit is still checked first; a transition that is not active is reported asStateTransitionNotActiveError.check_txandprocess_raw_state_transitions:SerializedObjectParsingError, the check result has no fee, and the identity's balance is unchanged.Run locally:
cargo test -p drive-abci --lib -- execution::check_tx state_transition_processing: 60 passed, 1 ignored (already#[ignore]on the base branch)cargo test -p drive-abci --lib -- abci::handler::check_tx: 5 passedcargo test -p dpp --all-features --lib -- serialization state_transition::tests: 306 passed;cargo test -p dpp --all-features --test untrusted_decode: 8 passedcargo test -p platform-version: passedcargo clippy -p dpp -p drive-abci --all-features --all-targets -- -D warningsandcargo fmt --all -- --check: cleanIn-place changes to shipped generations
None that change behaviour.
StateTransition::deserialize_from_bytes_untrusted_in_version, which generation 0 calls at every shipped protocol version, now delegates to a helper shared with the exact decoder. It runs the same decoder, depth limit and active version check in the same order, so its output is identical. Generation 0 ofdecode_raw_state_transitionshas only its tests changed, pinned to protocol version 13.Breaking Changes
Consensus change at protocol version 14 only: a raw state transition with bytes after the encoded transition is now refused, unpaid, instead of executed. Protocol versions 13 and earlier are 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
PR Hygiene ·
9e29b15/skip-botsproceeds without the ones not yet reported/self-reviewedonce the bots are doneWhen every box is checked the
PR Hygienecheck passes and this can merge.