fix(drive-abci): bill batch transformer drive reads (B7)#3670
Draft
shumkov wants to merge 20 commits into
Draft
Conversation
The batch state transition's `transform_into_action_v0` created a local `StateTransitionExecutionContext`, passed it into `try_into_action_v0`, and dropped it on return. Every `add_operation` call inside the transformer (per-transition `try_from_borrowed_*_with_contract_lookup` fee_results for token group actions, contested document creates, etc.) was silently discarded. Gate the fix on `batch_state_transition.transform_into_action`: - v0 (PROTOCOL_VERSION_11): preserve legacy dropped-local-ctx behavior for chain replay. - v1 (PROTOCOL_VERSION_12+): thread the outer execution_context through the transformer so per-transition fees reach the user's bill. Demonstrated by `test_token_burn_group_action_confirmer_fee_b7`: a group-action burn confirmer step's processing fee goes from 4_288_420 (pre-fix, dropped) to 4_319_240 (post-fix, billed). The 30_820 delta is the cost of three drive reads inside `try_from_borrowed_base_transition_with_contract_lookup` (`fetch_action_is_closed` + `fetch_action_id_signers_power_and_add_operations` + `fetch_active_action_info_and_add_operations`) that were previously billed to a dropped context. Non-group / non-contested scenarios are unaffected: the transformer's add_operation calls received empty FeeResults in those paths, so dropping vs. threading the ctx made no difference. Verified by the existing `test_document_replace_on_document_type_that_is_mutable` (pinned at 1_399_260 credits) continuing to pass. `docs/paid-error-fee-audit.md` documents the full audit (18 fee-leak sites identified across batch path, data triggers, and non-batch state transitions) and the constraint that every fix ships as a new function version or version-field bump for consensus reproducibility. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Project convention: when behavior changes via a version-field bump, add a new function version (_v1) alongside the unchanged _v0 and dispatch outside. The prior commit incorrectly branched inside _v0, which would alter the byte-identity of a function that has to stay verbatim for PROTOCOL_VERSION_11 chain replay. Restore `DocumentsBatchStateTransitionStateValidationV0::transform_into_action_v0` to its v3.1-dev original. Add a sibling `transform_into_action_v1` that takes `&mut StateTransitionExecutionContext` and threads it into `try_into_action_v0` (the transformer's single entry-point, intentionally still at _v0 per its file-header comment — see transformer/v0/mod.rs:1-22). Move the version dispatch into `batch/mod.rs::transform_into_action`: - `transform_into_action: 0` → `transform_into_action_v0(...)` (legacy) - `transform_into_action: 1` → `transform_into_action_v1(..., ctx, ...)` (B7 fix) The B7 regression test (`test_token_burn_group_action_confirmer_fee_b7`) still passes at 4_319_240 credits — the behavior gated by V8's `transform_into_action: 1` is unchanged from the prior commit; only the code shape changed. Audit doc updated to clarify the rule: "_v0 byte-identical, new _vN alongside, dispatch outside" is the standard pattern. The "branch inside" pattern only applies to the ~1100-line transformer body where suffix-bumping would force file-level duplication. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Match the project convention used by other versioned state-transition functions (e.g. address_funding_from_asset_lock/transform_into_action/v0/): each function version lives in its own directory with its own trait. Restores state/v0/mod.rs to byte-identical to v3.1-dev (verified via `git diff v3.1-dev`). Adds state/v1/mod.rs with a new trait `DocumentsBatchStateTransitionStateValidationV1` containing just the `transform_into_action_v1` method. Dispatcher in batch/mod.rs imports both traits and matches the `batch_state_transition.transform_into_action` field to pick which to call: - arm `0` → DocumentsBatchStateTransitionStateValidationV0::transform_into_action_v0 - arm `1` → DocumentsBatchStateTransitionStateValidationV1::transform_into_action_v1 B7 regression test (`test_token_burn_group_action_confirmer_fee_b7`) still passes at 4_319_240 credits — same end behavior as the prior two commits, just structurally clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The B7 / docs/paid-error-fee-audit.md references in code comments were temporary navigation aids during development. The audit doc is a working plan that will be removed once all the fixes ship, so in-code references to it would become broken links. Cleanup: - Rename test_token_burn_group_action_confirmer_fee_b7 -> test_token_burn_group_action_confirmer_fee_includes_transformer_reads (name now describes what it pins, not which audit entry it covers). - Strip "B7" / "paid-error-fee-audit.md" mentions from comments in batch/mod.rs, batch/state/v1/mod.rs, v8.rs, and the test file. Replace with self-contained explanations of what the code does and why. No behavior change; test still passes at 4_319_240 credits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`fetch_documents_for_transitions_knowing_contract_and_document_type` (in
batch/state/v0/fetch_documents.rs) is called from the batch transformer
on every batch with replace/transfer/purchase/update-price transitions.
It runs `drive.query_documents(...)` to fetch target documents for the
per-transition validators that come next. The query's cost was
previously discarded by passing `epoch=None` (which short-circuits cost
computation to 0) and ignoring the outcome's `cost()` accessor.
Now:
- The function takes an `Epoch` and returns the `FeeResult` alongside
the validation result.
- The caller in transformer/v0/mod.rs adds the FeeResult to the outer
execution_context, gated by a new field
`batch_state_transition.fetch_documents_for_transitions_billing`:
* 0 (PROTOCOL_VERSION_11 and below): discard, byte-identical to
pre-fix.
* 1 (PROTOCOL_VERSION_12+): bill the cost via add_operation.
- Builds on top of the prior B7 commit (transform_into_action: 1) so
the execution_context the cost lands in is the one threaded through
from the processor, reaching the user's bill.
Empirical fee deltas on existing PV12 fee-pin tests:
test_document_replace_on_document_type_that_is_mutable
1_399_260 → 1_411_320 (+12_060)
test_document_replace_on_document_type_that_is_not_mutable
445_700 → 460_920 (+15_220)
test_document_replace_on_document_type_that_is_not_mutable_but_is_transferable
445_700 → 457_660 (+11_960)
test_document_replace_that_does_not_yet_exist
516_040 → 520_340 (+4_300)
test_document_transfer_on_document_type_that_is_transferable
3_631_040 → 3_643_400 (+12_360)
test_document_set_price (+ 4 sibling NFT tests)
2_473_880 → 2_485_600 (+11_720)
... 19 fee-pin assertions updated in total.
V11 baselines (sibling `_protocol_version_11` tests) remain unchanged
verbatim — fetch_documents_for_transitions_billing: 0 preserves the
discard-cost path for chain replay.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`fetch_documents_for_transitions` (also `#[deprecated]`) and `fetch_documents_for_transitions_knowing_contract_id_and_document_type_name` in batch/state/v0/fetch_documents.rs were `#[allow(dead_code)]` and only referenced each other. Verified no external callers. The B4 commit before this threaded `epoch: &Epoch` through them as collateral damage from changing the live function's signature. Cleaner to just delete them — these wrappers were vestigial. Drops 8 now-unused imports as a side effect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The prior B4 commit introduced a separate `fetch_documents_for_transitions_billing` field. That was unnecessary — B4 cannot have any user-visible effect without B7's threaded ctx (billing into a dropped local context is wasted work), so the two fixes are intrinsically tied. Reuse the existing `transform_into_action` field instead. - Remove `fetch_documents_for_transitions_billing` from `DriveAbciDocumentsStateTransitionValidationVersions`. - Remove it from v1.rs..v8.rs (8 files). - transformer/v0/mod.rs:511 callsite now gates the query-cost billing on `transform_into_action` (same field that decides ctx threading). Behavior is identical to the prior B4 commit because V8 had both fields at 1 — the consolidated single-field gate produces the same v0/v1 dispatch outcome. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 1 of fixing trigger drive-read fee leaks (T1-T4 in the audit). This commit is consensus-neutral: every trigger returns FeeResult::default() (the actual fee billing per trigger ships in follow-up commits, one per source — dpns, dashpay, withdrawals). Changes: - `DataTrigger` fn type now returns `Result<(DataTriggerExecutionResult, FeeResult), Error>` - `DataTriggerBindingV0Getters::execute` ditto. - `DataTriggerExecutor::validate_with_data_triggers` ditto — sums fees from every trigger that actually executed (including the one that returned invalid) via `FeeResult::checked_add_assign`. - All 4 trigger fns (dpns/dashpay/withdrawals/reject) return tuple with `FeeResult::default()` placeholder. - Dispatch site in `state/v0/mod.rs::validate_state_v0` destructures the returned tuple. The accumulated FeeResult is added to the outer `execution_context` gated by `transform_into_action: 1` — same field that gates B7 (ctx threading) and B4 (query cost billing). On v0 the fee is discarded for chain replay reproducibility (it's `FeeResult::default()` anyway in this commit). Also deleted the dead `state/v0/data_triggers.rs::execute_data_triggers` function. It was `#[allow(dead_code)]` + `#[deprecated]` with no callers — the new trigger return type would have required threading the tuple through it for nothing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The DPNS `create_domain_data_trigger` fires on every DPNS domain create. It runs two `query_documents` calls: - T1: parent-domain lookup (only when registering a subdomain) - T2: preorder document lookup (always) Both previously passed `epoch=None` to `query_documents` (which short-circuits cost computation to 0) and discarded any cost anyway — the trigger context's ctx ref was immutable, so triggers could not bill. Now: pass `Some(epoch)` so query_documents computes the real grovedb cost, accumulate both queries' costs into a `FeeResult`, and return it from the trigger. The caller (`DataTriggerExecutor::validate_with_data_triggers`) sums fees across triggers and the dispatch site in `state/v0/mod.rs` bills via `execution_context.add_operation` on `transform_into_action: 1`. The accumulated FeeResult is returned at every exit path including early returns after the parent-domain query (so the user pays for the first query even if validation fails before the second one runs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The DashPay `create_contact_request_data_trigger` fetches the recipient identity's balance to verify the identity exists before creating a contact request. Previously used `fetch_identity_balance` (no cost returned) with an explicit "TODO: Calculate fee operations" comment. Switch to `fetch_identity_balance_with_costs` (passes block_info for epoch, returns FeeResult), and propagate the FeeResult through the trigger's return value. The caller bills it on `transform_into_action: 1` via the now-established trigger fee plumbing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The withdrawals `delete_withdrawal_data_trigger` runs `query_documents` to fetch the withdrawal document being deleted (to verify status == COMPLETE before allowing deletion). Previously passed `epoch=None` (cost short-circuited to 0) and discarded any outcome cost. Pass `Some(epoch)` so the real grovedb cost is computed, build a `FeeResult` from `documents_outcome.cost()`, and return it from every exit path (early-return on missing withdrawal, early-return on wrong status, and final return). The caller bills it on `transform_into_action: 1`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rigger epoch source Three review-driven fixes: 1. **B5 — `fetch_document_with_id` query cost was discarded.** Same pattern as B4 (B4 was in a sibling function); reviewers correctly flagged that fixing B4 alone left an identical leak in the same file. `fetch_document_with_id` previously passed `epoch=None` to `query_documents`, which short-circuits the cost to 0. All three callers (`document_create_transition_action/state_v0`, `state_v1`, `document_delete_transition_action/state_v0`) already do `execution_context.add_operation(PrecalculatedOperation(fee_result))` — they just always got zero. Now: take `epoch: &Epoch`, gate on `transform_into_action` internally (v0 passes None and returns zero-fee for byte-identical PV11 behavior; v1 passes Some(epoch) and bills the real cost). 2. **Epoch source unified across batch fee sites.** Reviewers flagged that triggers used `last_committed_block_epoch_ref()` while the transformer used `&block_info.epoch` (current block). At era boundaries the two prices would diverge — deterministic but internally inconsistent. Now: `DataTriggerExecutionContext` carries `block_info: &'a BlockInfo`, and all three migrated triggers (DPNS, DashPay, Withdrawals) use `&context.block_info.epoch`. DashPay's `fetch_identity_balance_with_costs` call also switches from `last_block_info()` to `context.block_info` for the same reason. 3. **Stale doc-comment cleanup.** `fetch_documents.rs:21-23` referenced the removed `fetch_documents_for_transitions_billing` field. Now references the consolidated `transform_into_action` gate. 4. **v8.rs documentation.** Added a comment enumerating the 6 sub-concerns gated by `transform_into_action: 1` (B7, B4, B5, T1, T2, T3, T4) so future operators investigating a fee discrepancy at PV12 can find them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds three regression tests that fail if the trigger fee billing (introduced in earlier commits) is dropped: - **T1+T2 (DPNS)**: pinned the exact aggregated `processing_fee` for `test_dpns_contract_references_with_no_contested_unique_index` (3 subdomain creates, total 6_010_380 credits). A future refactor that drops `accumulated_fee_result.checked_add_assign` in `create_domain_data_trigger_v0` would fail this assertion. - **T3 (DashPay)**: added `assert!(fee_result.processing_fee > 0)` on the existing `should_return_invalid_result_if_id_not_exists` unit test. Catches regressions that bypass `fetch_identity_balance_with_costs` (e.g. reverting to the cheaper `fetch_identity_balance` that returns no cost). - **T4 (Withdrawals)**: added `assert!(fee_result.processing_fee > 0)` on the existing `should_throw_error_if_withdrawal_has_wrong_status` unit test. Catches regressions that revert `epoch=Some(...)` to `epoch=None` in the trigger's `query_documents` call. Also removed three unused imports (`FeeResult`, `PlatformStateV0Methods`) that became dead after the epoch-source unification commit removed the `last_committed_block_epoch_ref()` calls from the triggers. Note: T3 and T4 use non-zero assertions rather than exact-value pins because writing dedicated batch-level fixture tests for DashPay contact-request and withdrawal-delete scenarios would require substantial new test scaffolding (none exists today in batch/tests/). The non-zero assertion catches the highest-priority regression (trigger billing entirely dropped) at zero new-fixture cost. Exact pins can be added as a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer feedback: the prior architectural commit modified _v0 trigger
bodies to return `(Result, FeeResult)` for fee accounting, which broke
strict _v0 byte-identity even though the version gate at the dispatch
site kept PV11 behavior preserved. The cleaner pattern (used elsewhere
in the codebase) is to keep _v0 byte-identical and add _v1 sibling
functions with the new behavior.
Refactor:
1. **`DataTriggerExecutionContext.state_transition_execution_context`**
changed from `&'a` to `&'a mut`. `_v1` triggers call
`add_operation` on it directly to bill drive reads; `_v0` triggers
ignore the mut access (their body never mutates).
2. **`DataTrigger` fn type**: return type reverted from
`(Result, FeeResult)` to `Result` (no tuple). Param changed to
`&mut DataTriggerExecutionContext`. The trait-binding chain
(`DataTriggerBindingV0Getters::execute`, `DataTriggerExecutor::validate_with_data_triggers`)
propagates the change.
3. **`_v0` trigger files** (`dpns/v0/mod.rs`, `dashpay/v0/mod.rs`,
`withdrawals/v0/mod.rs`) **restored from v3.1-dev byte-identical
for the function body**. Only the param type signature changed
(`&` → `&mut`), which is a compile-time-only change with no
observable PV11 behavior difference. epoch=None preserved, no
add_operation calls.
4. **`_v1` trigger files** (new): `dpns/v1/mod.rs`,
`dashpay/v1/mod.rs`, `withdrawals/v1/mod.rs`. Pass `Some(epoch)`
to `query_documents` (DPNS, withdrawals) or use
`fetch_identity_balance_with_costs` (dashpay), and call
`context.state_transition_execution_context.add_operation(...)`
directly to bill the grovedb cost.
5. **Wrappers** (`triggers/{dpns,dashpay,withdrawals}/mod.rs`) now
dispatch on the per-trigger version field:
- `0 =>` _v0 (legacy, no billing)
- `1 =>` _v1 (PV12+, bills directly)
6. **`v8.rs`** bumps per-trigger fields to 1:
- `create_domain_data_trigger: 1`
- `create_contact_request_data_trigger: 1`
- `delete_withdrawal_data_trigger: 1`
- `reject_data_trigger: 0` (no drive reads, no billing needed)
7. **Dispatch site** in `batch/state/v0/mod.rs`: removed the now-stale
local `state_transition_execution_context`, removed the FeeResult
tuple destructure + version-gated add_operation. The trigger context
passes the outer `execution_context` directly as `&mut`.
8. **Executor** (`data_triggers/executor.rs`) simplified — no more
FeeResult accumulation/summing; triggers bill themselves.
9. **Test fee updates**: 6 tests on PV12 paths now reflect B5 fee
billing for `fetch_document_with_id` (deletion x3, nft x3).
Original-creation-cost constants and `RemoveFromBalance` desired
amounts updated to match the new billed fees.
10. The DPNS regression test pin holds at 6_010_380 credits —
same behavior, cleaner architecture.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The trigger context already has `platform: &PlatformStateRef`, which exposes `platform.state.last_committed_block_epoch_ref()` and `platform.state.last_block_info()`. The dedicated `block_info` field was redundant. Trade-off accepted: triggers now source their epoch from the last *committed* block rather than the *current* block being processed. At era boundaries these differ — trigger fees price at the previous era's rates while the transformer prices at the current era's rates (same batch transition can have internally inconsistent fee math). The discrepancy is deterministic across all validators, so consensus holds; just a minor fee-accuracy quirk. Changes: - Removed `block_info: &'a BlockInfo` field from `DataTriggerExecutionContext`. - `_v1` triggers (dpns, withdrawals) now pass `Some(context.platform.state.last_committed_block_epoch_ref())` to `query_documents`. - `_v1` dashpay trigger passes `context.platform.state.last_block_info()` to `fetch_identity_balance_with_costs`. - Dispatch site at `batch/state/v0/mod.rs` no longer constructs with `block_info`. - Test context constructions in `_v0` files dropped the `block_info: &BlockInfo::default()` line. All 262 batch tests pass — DPNS fee pin still 6_010_380 credits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sites Reviewer feedback: when modifying _v0 / shipped function bodies (even in ways that preserve PV11 byte-identity), the code itself should explain WHY the change is safe. Future maintainers shouldn't have to re-derive the consensus argument from scratch. Added inline comments at every _v0 site this PR touches: - `validate_state_v0` (batch/state/v0/mod.rs): explains why removing the local `state_transition_execution_context` and switching the trigger context to use the outer ctx preserves PV11 chain replay, plus notes the mempool-only `dry_run` semantics change. - `fetch_documents_for_transitions_knowing_contract_and_document_type`: explains that `epoch: &Epoch` was added to the signature but the v0 arm at the caller discards the resulting cost — documents returned are epoch-independent. - `fetch_document_with_id`: same pattern — internal version gate passes `None` to query_documents on v0 (zero-cost FeeResult), so the caller's existing `add_operation` call adds zero — matching pre-PR. - All three `_v0` trigger fns (dpns, dashpay, withdrawals): note that the `context: &mut DataTriggerExecutionContext` signature change is compile-time only — the body never mutates the context. - Transformer's fetch_documents callsite: explains the `Some(epoch)` → real cost vs the version-gated discard on v0. - `DataTriggerBindingV0Getters::execute` impl: notes that the `&mut` change is required by _v1 but harmless on PV11 (v0 trigger doesn't mutate). No code changes — pure documentation. DPNS regression test still passes at 6_010_380 credits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t ctx Same architectural choice as the trigger refactor — pass `&mut execution_context` into the fetch helpers and let them call `add_operation` directly, rather than returning a `FeeResult` tuple and having the caller bill. Changed: - `fetch_documents_for_transitions_knowing_contract_and_document_type` now takes `execution_context: &mut StateTransitionExecutionContext` and returns just `ConsensusValidationResult<Vec<Document>>` (no tuple). Internal version gate on `transform_into_action`: v0 → epoch=None, no add_operation; v1 → Some(epoch), add_operation. - `fetch_document_with_id` same pattern. Returns just `Option<Document>`. - Transformer callsite at transformer/v0/mod.rs:511 simplified — passes `execution_context` and drops the explicit version-gated add_operation block (now handled inside the helper). - `document_create_transition_action::state_v0`, `document_create_transition_action::state_v1`, `document_delete_transition_action::state_v0` callsites updated — pass `execution_context`, drop the now-redundant `add_operation(PrecalculatedOperation(fee_result))` lines. PROTOCOL_VERSION_11 consensus-safety: the v0 path inside each helper forces `epoch=None` and skips `add_operation`. Pre-PR also passed None and the caller did `add_operation` with a zero-cost FeeResult (a no-op fee). Net effect on PV11: identical. All 262 batch tests pass. DPNS regression pin still holds at 6_010_380 credits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tightens trigger fee-billing coverage with two complementary assertions on each trigger's unit tests: 1. **`_v0` byte-identity** (PROTOCOL_VERSION_11 chain replay safety): after calling `_v0` directly, assert `execution_context.operations_slice().is_empty()`. Catches any regression that accidentally re-introduces billing in the legacy `_v0` trigger body. 2. **`_v1` billing** (PROTOCOL_VERSION_12+ fee correctness): after calling `_v1` directly, assert `execution_context.operations_slice()` is NON-empty. Catches the regression where `_v1` drops its `add_operation` call. Coverage added: - **DPNS** (`dpns/v0/mod.rs::test::should_return_execution_result_on_dry_run`): asserts `_v0` adds zero ops. Pairs with the existing batch-level PV12 fee pin at 6_010_380 credits for the `_v1` billing side. - **DashPay** (`dashpay/v0/mod.rs::should_return_invalid_result_if_id_not_exists`): this test calls the wrapper which dispatches to `_v1` at PV12 (`create_contact_request_data_trigger: 1` on V8). Asserts the resulting `execution_context` is non-empty — proves T3 billing works. - **Withdrawals** (`withdrawals/v0/mod.rs::should_throw_error_if_withdrawal_has_wrong_status`): asserts `_v0` adds zero ops, then runs the SAME fixture through `_v1` directly (imported via `super::super::v1::...`) and asserts non-empty ops. Single test covers both T4 sides. These are unit-level assertions on the trigger functions, not batch-level fee pins. They run in <1s without needing full batch fixtures, so the regression catch-net is cheap to maintain. All 7 PV11 tests still pass + 262 batch tests still pass — PV11 chain replay byte-identity preserved through the changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
QuantumExplorer
added a commit
that referenced
this pull request
May 20, 2026
…ion (audit N6/N7) `validate_identity_public_key_contract_bounds` had an explicit `//todo: we should add to the execution context the cost of fetching contracts` on v0 and discarded every grovedb read (contract fetches + unique-key lookups). Under paid-error semantics, an identity_update that failed bounds validation under-charged the user for the validation work that ran. Audit entries N6 / N7 in `docs/paid-error-fee-audit.md` (PR #3670), tracked by issue #3673. Folded into v1 of the bounds validator (already gated at PROTOCOL_VERSION_12 via DRIVE_ABCI_VALIDATION_VERSIONS_V8 by the previous commit). v0 stays byte-identical for chain replay. - `rs-drive`: new `pub fn fetch_identity_keys_with_costs` (mirrors the existing `fetch_identity_balance_with_costs` precedent — takes `&Epoch`, returns `(T, FeeResult)`). - `rs-drive-abci` v1: switched contract fetches to `get_contract_with_fetch_info_and_fee` and identity-key lookups to the new `fetch_identity_keys_with_costs`, pushing each returned `FeeResult` into `execution_context` via `ValidationOperation::PrecalculatedOperation`. Also refactored the body to factor out the unique-key check into a single helper (was duplicated across four bounds × purpose combinations). - Dispatcher now takes `epoch: &Epoch`. v0 ignores it (preserved). - `identity_update::validate_state_v0` passes `platform.state.last_committed_block_epoch_ref()` to the dispatcher. New test `v1_bills_contract_fetch_and_unique_key_lookup` asserts the execution context receives at least two `PrecalculatedOperation` entries with non-zero processing fees after v1 runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer feedback: when a new function version exists alongside the old one, the diff sites should explicitly explain what changed vs. the previous version and why. Without this, future maintainers reading `_v1` have to mentally diff against `_v0` to understand the intent. Added "Diff vs `_v0`" comments at every site in `_v1` triggers that diverges from the corresponding `_v0` body: - **dpns/v1**: parent-domain query (T1) + preorder query (T2). Both swap `None` → `Some(epoch)` and add a billing call. The comments name T1/T2 explicitly so the audit-doc cross-reference is searchable. - **dashpay/v1**: recipient identity existence check (T3). Swaps `fetch_identity_balance` → `fetch_identity_balance_with_costs`. Comment notes `apply: true` matches the legacy stateful query so the returned balance is byte-identical (only the FeeResult is new). - **withdrawals/v1**: withdrawal-document lookup (T4). Swaps `None` → `Some(epoch)` + adds billing. Notes that the document returned is epoch-independent. No code changes — pure documentation. All 7 trigger unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer feedback: instead of an internal `transform_into_action`
gate inside `fetch_documents_for_transitions_knowing_contract_and_document_type`
and `fetch_document_with_id`, use the same proper version-facade
pattern we use for triggers — separate `_v0` and `_v1` impls,
dispatched by a facade that takes `platform_version`.
Changes:
1. **Two new version fields** on
`DriveAbciDocumentsStateTransitionValidationVersions`:
- `fetch_documents_for_transitions_knowing_contract_and_document_type: FeatureVersion`
- `fetch_document_with_id: FeatureVersion`
v1.rs..v7.rs set both to 0 (PROTOCOL_VERSION_11 and below).
v8.rs sets both to 1 (PROTOCOL_VERSION_12+).
2. **fetch_documents_for_transitions_knowing_contract_and_document_type**:
- Facade dispatches on the version field.
- `_v0`: byte-identical to v3.1-dev. No `epoch`/`execution_context`
params, passes `epoch=None` to `query_documents`, never bills.
- `_v1`: takes `epoch` and `execution_context`, passes
`Some(epoch)`, bills via `add_operation`.
3. **fetch_document_with_id**:
- Facade dispatches on the version field.
- `_v0`: byte-identical to v3.1-dev — signature returns
`(Option<Document>, FeeResult)`. The facade calls
`add_operation` with the (zero-cost) FeeResult on the v0 path
so the execution_context's operations_slice matches pre-PR
exactly (pre-PR caller did this; we just moved the call into
the facade).
- `_v1`: takes `epoch` and `execution_context`, bills internally,
returns just `Option<Document>`.
PV11 byte-identity properties (verified empirically — 7 PV11 fee-pin
tests pass unchanged):
- `_v0` function bodies match v3.1-dev pre-PR text exactly (modulo
the function rename to `*_v0`).
- The facade's v0 arm produces the same chain state as pre-PR:
documents are epoch-independent, the fee_result on v0 is always
zero, the add_operation call (either at the old caller or now at
the facade) adds a zero-fee FeeResult to ctx.
All 262 batch tests, 7 PV11 tests, 7 trigger unit tests pass.
DPNS regression pin still 6_010_380 credits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Follow-up to #3616. While auditing fee correctness on the recently-flipped paid-error path for batch transitions, a much larger issue surfaced: the batch state transition transformer's
execution_contextwas a local that got dropped on return, silently discarding everyadd_operationcall from per-transitiontry_from_borrowed_*_with_contract_lookup(token group actions, contested document creates, etc.). This commit fixes that leak (entry B7 indocs/paid-error-fee-audit.md).This is the first of several commits planned from the audit (see "follow-ups" below). Posting as draft so the audit doc + B7 fix can be reviewed before B4 builds on top.
What was done?
B7 fix
state/v0/mod.rs::transform_into_action_v0previously created a localStateTransitionExecutionContext, passed it intotry_into_action_v0, and dropped it on return. The outer context threaded by the processor (processor/v0/mod.rs:43) was accepted bybatch/mod.rs:57only to be ignored (_execution_context).Gated on the existing
batch_state_transition.transform_into_actionfield, bumped 0 → 1 in V8 (PROTOCOL_VERSION_12):execution_contextthrough the transformer so per-transition fee_results reach the user's bill.No new function-version files, no protocol version bump — per the file-header comment at
transformer/v0/mod.rs:1-22, this is the preferred pattern (branch inside the existing function rather than fork the ~1100-line transformer into a_v1archive).Audit doc
docs/paid-error-fee-audit.mdcaptures the full inventory of fee-charging gaps discovered while diagnosing B7:query_documentscost — next commit)DataTriggerExecutionContextimmutability refactor)14 HIGH-severity sites total; this PR fixes 1 of them.
How Has This Been Tested?
New regression test
test_token_burn_group_action_confirmer_fee_b7inbatch/tests/token/burn/mod.rsexercises the cleanest demonstration scenario: a group-action burn where the confirmer step triggers three drive reads insidetry_from_borrowed_base_transition_with_contract_lookup(fetch_action_is_closed+fetch_action_id_signers_power_and_add_operations+fetch_active_action_info_and_add_operations).Empirical fee deltas captured during development:
transform_into_actionvalueprocessing_fee0(pre-fix, dropped local ctx)1(post-fix, threaded outer ctx)Existing fee-pinning tests continue to pass unchanged — verified by
test_document_replace_on_document_type_that_is_mutable(still assertsprocessing_fee == 1_399_260). This confirms non-group / non-contested scenarios were unaffected by B7 (their dropped fee_results were empty), so the fix is invisible to them. B4 (next commit) will move that 1,399,260 number by billing the document query cost.Breaking Changes
Consensus-affecting fee change for PROTOCOL_VERSION_12. Token group action confirmations (and any other batch transitions that exercise transformer-phase drive reads — see audit doc) will bill an additional ~30K credits per affected transition. PROTOCOL_VERSION_11 chain replay is preserved verbatim via the
transform_into_action: 0arm.Targeting V8's PV12 hard-fork window per offline confirmation that PV12 has not yet shipped to mainnet. If PV12 has already shipped at merge time, this change must be re-cut for PV13 instead.
Scope creep — what's actually in this PR
What started as a B7-only fix grew during review to cover the rest of the batch-path fee leaks the audit surfaced. All gated by the same
transform_into_action: 1field on V8:execution_contextthrough the batch transformer (root-cause fix that unblocked everything else).query_documentscost infetch_documents.rs(was discarded; couldn't be fixed in isolation before B7)._with_costs).The DataTrigger refactor (return
(Result, FeeResult)so triggers can surface their costs without needing a mut ref throughDataTriggerExecutionContext) is a separate commit before the per-trigger billing commits.Follow-ups (not in this PR)
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code