Repository navigation
fix(drive): read stored group actions without the proof decoding budget - #5006
Merged
Merged
Conversation
GroupAction's `limit = 100000` (#4629) was sized as if bincode counted bytes read. It counts memory claimed: a map claims len * size_of::<T>() at its length prefix, and a conventions localization entry claims 80 bytes against as few as 13 encoded. The derive also applies the limit to the trusted decode, so a valid stored conventions change with 1,250 localizations (16,325 bytes) could not be read back by Drive: its co-signature ended in an internal error and the group actions query and proof verification failed on any page holding it. Drive now reads its own stored actions with deserialize_from_bytes_trusted_no_limit, as every v4.1 binary does at every protocol version it runs. The untrusted proof budget is sized from the worst memory claim a 20,480-byte transition can produce (the localizations map, about 125,600 bytes) and set to 262,144. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedNext included review available in 17 seconds. 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 |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit 9cb905e) · triage: normal |
Merged
4 of 12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
#4629 (first released in v4.2.0-beta.1, in no v4.1.x) gave
GroupAction#[platform_serialize(limit = 100000)]to bound decoding of group actions from proofs. Its comment reasoned that no valid stored action can exceed the limit, because a state transition is capped at the same 100,000 bytes. That does not hold, for two reasons:len * size_of::<T>()from its length prefix before it reads an element; strings and byte vectors claim their length. A token conventions localization entry claims 80 bytes and encodes in as few as 13.A conventions change with 1,250 valid localizations encodes in 16,325 bytes but claims just over 100,000 at the map's length prefix. Encoding does not check the limit, so the proposal is accepted and the action is stored. Every read of it then fails:
fetch_action_infos(thegetGroupActionsquery) andverify_action_infos_in_contract(client proof verification) fail for any page that includes it.v4.1.x binaries decode these actions without any limit, so they and the 4.2 binaries also treated such a co-signature differently.
What was done?
Drive reads its own stored actions without a budget, as v4.1.x did.
fetch_active_action_infov0 (both functions) andfetch_action_infosv0 now calldeserialize_from_bytes_trusted_no_limit. These are the onlyGroupActiondecodes outside tests apart from the client proof path. Stored actions were validated when written and are bounded by the transition that proposed them (max_state_transition_size, 20,480 bytes).The proof budget is sized from memory claims. The untrusted decode keeps a budget, raised from 100,000 to 262,144, and the attribute comment now gives the real reasoning. Per container reachable from a storable
GroupActionEvent, on a 64-bit target, packed into 20,480 encoded bytes:BTreeMap<String, TokenConfigurationLocalization>SetPrices,BTreeMap<u64, u64>Stepwise,BTreeMap<u64, u64>[u8; 32]No event a group can store holds an identifier list or any other container. The fixed claims outside the container (the action's two ids and the like) come to under 100 bytes, so the worst storable action claims about 125,600 bytes, and 262,144 (256 KiB) leaves more than twice that.
Why the number is a literal and not in
SystemLimits: the derive parseslimitas an integer literal and passes it towith_limit::<N>(), a const generic, so it cannot come from a version table, and after this change no consensus path reads it. A test packs each container shape to the largestmax_state_transition_sizeinPLATFORM_VERSIONS, so raising that limit without raising this budget fails the test.New shared fixture
get_token_conventions_with_localizations_fixture(n)in dpp'stests::fixtures.Not done: rewording the limit error. The derive reports
MaxEncodedBytesReachedError { max_size_kbytes: <limit>, size_hit: <input length> }, displayed as "Payload reached a 100000KB limit", which reads as a byte count. That display text also reachesSerializedObjectParsingErrormessages on the state transition decode path, so changing it is not local to this fix; it is left for a separate change.Before / after
A token whose conventions change needs both members of a two-member group. Member 1 proposes conventions with 1,250 localizations (
"en"plus two-letter codes, each with forms"abc"/"abc", all accepted byvalidate_localizations). The proposal is about 16.5 KB, it is accepted, and the stored action is 16,325 bytes.Before:
After:
In-place changes to shipped generations
Drive::fetch_active_action_info_v0andfetch_active_action_info_and_add_operations_v0DRIVE_GROUP_METHOD_VERSIONS_V1is the only group method table)Drive::fetch_action_infos_v0andfetch_action_infos_and_add_operations_v0getGroupActionsquery; block execution does not call it.The
GroupActionbudget itself is unversioned and, after this change, only client proof verification reads it.Networks: testnet runs drive 4.1.0 at protocol version 13 (seeds 1 to 5 checked 2026-09-26), which has no budget here. The moutai devnet runs 4.2.0-beta.4 at protocol version 14. It holds 28 contracts and none defines a group, so it stores no group action and nothing there is waiting on this fix.
Breaking Changes
None, and no
!in the title.!is for changes nodes could disagree on if rolled out unevenly. This change makes 4.2 decode stored group actions exactly as every released v4.1.x binary does at every protocol version they run, so a network mixing those binaries with this one agrees on every co-signature. The only binaries it differs from are the 4.2 betas, which run protocol version 14 only on devnets that are re-cut each release. The client budget only grows: any proof that verified before still verifies.Other decode limits (checked, not changed)
platform_serialize(... limit ...)attribute is unchanged except the two fix(dpp): bound untrusted length prefixes on the proof-verification decode path #4629 added:GroupAction(this PR) andAssetLockValue(15,000).AssetLockValueis also decoded under its budget during block execution (fetch_asset_lock_outpoint_info). That budget holds, because its containers claim about what they encode: a byte vector for the script, and a vector of 32-byte tags capped bymax_asset_lock_usage_attempts(16). The existinglargest_valid_value_round_trips_under_limittest covers a 10,000-byte script with the maximum tags. The directwith_limit::<N>()calls added since v4.1.2 are all client side (proof verifier, SDK, wallet), and the contract decode limit (CONTRACT_DESERIALIZATION_LIMIT, 15,000) is unchanged and used only in tests.platform_serializeattribute, so thelimit = 100000onStateTransition,VotePollandContestedDocumentResourceVotePollhas never applied: each carries#[platform_serialize(unversioned)]first. That limit counts memory in the same way. The 16,465-byte co-signing transition in the new drive-abci test claims more than 100,000 bytes and is refused (LimitExceeded) when decoded under a 100,000StateTransitionbudget. Applying that limit as written would refuse valid transitions well under 20 KiB, such as a contract create or a token config update with many localizations, so it needs sizing from memory claims first, as done here forGroupAction.How Has This Been Tested?
group::group_action::deserialize_limit_tests:should_decode_a_conventions_change_with_1250_localizations: valid localizations, 16,325 bytes, refused under the previous budget; decodes untrusted, trusted, and trusted without a limit.should_decode_the_largest_storable_action_of_each_container_shape: localizations,SetPrices,Stepwiseand a note, each packed tomax_state_transition_size, decode with every decoder. Of these, only the localizations shape was refused under the previous budget.should_refuse_a_localizations_length_prefix_beyond_the_budget: a map length prefix of 1,000,000 with no entries behind it is refused by the untrusted decode. The existing 8 GB note prefix test still passes.should_fetch_a_stored_conventions_change_with_1250_localizations(at protocol version 13 and at the latest),should_list_a_stored_conventions_change_with_1250_localizations,should_verify_a_proof_holding_a_conventions_change_with_1250_localizations.should_let_a_group_co_sign_a_conventions_change_with_1250_localizations. Member 1 proposes and the stored bytes equal the proposed action's encoding; member 2's co-signature succeeds and the token's conventions are the 1,250 localizations.MaxEncodedBytesReachedError { max_size_kbytes: 100000, size_hit: 16325 }, and drive-abci withInternalError("storage: protocol: Payload reached a 100000KB limit")on the co-signature.cargo test -p dpp --all-features --lib group::(32 passed),cargo test -p drive --lib -- drive::group verify::group(111 passed),cargo test -p drive-abci --lib -- token_config_update_tests group_queries(98 passed).cargo clippy -p dpp -p drive -p drive-abci --all-features --all-targets -- -D warningsandcargo fmt --all -- --checkare clean.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 ·
9cb905e/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.