fix(dpp): reject empty token pricing schedules to prevent a direct-purchase chain halt#3865
Conversation
…rchase chain halt
An empty `SetPrices(BTreeMap::new())` pricing schedule was a representable,
storable value that passed both structure and state validation for
`TokenSetPriceForDirectPurchase`. Once stored, any `TokenDirectPurchase` for that
token reached the direct-purchase transformer's `None` arm, where the original code
did `set_prices.keys().next().expect("Map is not empty")` — which panics on an empty
map.
That transformer runs during deterministic block execution on every validator
(process_proposal -> run_block_proposal -> process_raw_state_transitions ->
transform_into_action). Per-state-transition `Err`s are caught into an
`InternalError` execution result and the chain continues, but panics are not caught:
there is no `catch_unwind` anywhere on the path, and the global panic hook
(main.rs install_panic_hook) calls `cancel.cancel()`, shutting the node down. Because
block execution is deterministic, every validator panics on the same block, taking the
whole quorum down — a chain halt.
Fix (defense in depth):
- Reject an empty `SetPrices` map at set-price structure validation with a new
`TokenPricingScheduleEmptyError` consensus error (basic code 10461), so such a
schedule can never be written to state. Structure validation short-circuits the
batch before the transition is applied.
- The transformer already handles an empty schedule gracefully (matches on
`keys().next()` and returns `TokenNotForDirectSale` instead of `.expect()`); kept as
the consume-side guard.
Adds regression tests for both layers: structure validation rejects an empty
`SetPrices`, and the transformer's `None`-arm logic resolves an empty map to
not-for-sale without panicking. Wires the new error through `BasicError` (tail-
appended for bincode positional encoding), error codes, and the wasm-dpp bindings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete (commit 9eb600d) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR defines a new ChangesToken pricing schedule empty error validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/v0/transformer.rs (1)
563-591: 🏗️ Heavy liftExercise the transformer path, not just
BTreeMapbehavior.This test proves that an empty map yields
None, but it never invokesTokenDirectPurchaseTransitionActionV0::try_from_borrowed_token_direct_purchase_transition_with_contract_lookupor asserts the emittedTokenNotForDirectSaleerror. A future regression inside the transformer'sNonearm would still leave this test green. Please route the check through the real transformer flow, or extract theSetPricesresolution into a pure helper and test that directly.🤖 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 `@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/v0/transformer.rs` around lines 563 - 591, The test only inspects BTreeMap behavior instead of exercising the transformer path; change the test to call TokenDirectPurchaseTransitionActionV0::try_from_borrowed_token_direct_purchase_transition_with_contract_lookup (or, alternatively, extract the set_prices resolution into a pure helper like resolve_set_prices_tier and test that) so the transformer's None branch is executed; construct a minimal BorrowedTokenDirectPurchaseTransition (with an empty SetPrices schedule and token_count=5) or call the new helper, run the transformer, and assert it returns the TokenNotForDirectSale/appropriate consensus error rather than just checking BTreeMap::keys/next behavior.
🤖 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.
Nitpick comments:
In
`@packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/v0/transformer.rs`:
- Around line 563-591: The test only inspects BTreeMap behavior instead of
exercising the transformer path; change the test to call
TokenDirectPurchaseTransitionActionV0::try_from_borrowed_token_direct_purchase_transition_with_contract_lookup
(or, alternatively, extract the set_prices resolution into a pure helper like
resolve_set_prices_tier and test that) so the transformer's None branch is
executed; construct a minimal BorrowedTokenDirectPurchaseTransition (with an
empty SetPrices schedule and token_count=5) or call the new helper, run the
transformer, and assert it returns the TokenNotForDirectSale/appropriate
consensus error rather than just checking BTreeMap::keys/next behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 596428bc-a427-4418-97d0-66885ea484e9
📒 Files selected for processing (7)
packages/rs-dpp/src/errors/consensus/basic/basic_error.rspackages/rs-dpp/src/errors/consensus/basic/token/mod.rspackages/rs-dpp/src/errors/consensus/basic/token/token_pricing_schedule_empty_error.rspackages/rs-dpp/src/errors/consensus/codes.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/validate_structure/v0/mod.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/v0/transformer.rspackages/wasm-dpp/src/errors/consensus/consensus_error.rs
…testable helper Addresses review feedback (CodeRabbit): the empty-schedule regression test only inspected `BTreeMap` behavior, so a future regression in the transformer's `None` arm — e.g. reintroducing the chain-halting `.expect()` — would have left it green. Extract the `SetPrices` tier-resolution logic (tier lookup, overflow, under-minimum, price-too-low, and the empty-schedule -> `TokenNotForDirectSale` case) out of the transformer into a pure `resolve_set_prices_direct_purchase_price` helper. The transformer now routes through it with identical observable behavior (on `Err` it bumps the identity-contract nonce and surfaces the same consensus error as before). Replace the BTreeMap-replica regression test with five tests that drive the real helper across every branch, so a regression inside the `None` arm now fails a test instead of silently passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted, well-scoped fix for the direct-purchase chain-halt: empty SetPrices is now rejected at DPP structure validation with a new TokenPricingScheduleEmptyError (code 10461), appended at the tail of BasicError so bincode discriminants are preserved, and bridged through the wasm-dpp exhaustive converter. Plumbing, ordering, and gating are correct. The only in-scope concern is that the new transformer-side 'regression' test does not call the transformer — it asserts standard BTreeMap semantics on an empty map, so a reintroduction of the original .expect("Map is not empty") would not be caught.
🟡 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 `packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/v0/transformer.rs`:
- [SUGGESTION] packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_direct_purchase_transition_action/v0/transformer.rs:573-591: Regression test does not exercise the transformer — it tests `BTreeMap`, not the fixed code path
`set_prices_empty_map_does_not_panic_and_is_treated_as_not_for_sale` is documented as the regression test for the chain-halt bug, but it never invokes `try_from_borrowed_token_direct_purchase_transition_with_contract_lookup` (or any transformer entry point). It only asserts that `BTreeMap::range(..=k).next_back()` and `BTreeMap::keys().next()` return `None` on an empty map — properties of the standard library, not of the transformer. If a later refactor restored the original `*set_prices.keys().next().expect("Map is not empty")` on the `None` arm, this test would still pass green. Drive the transformer directly with a `TokenDirectPurchaseTransition` whose token state carries `Some(TokenPricingSchedule::SetPrices(BTreeMap::new()))` and assert the result is `BumpIdentityDataContractNonce` + `TokenNotForDirectSale` rather than a panic; that fails closed if the `.expect()` is ever reintroduced. Not blocking because the new structure-validation guard prevents an empty schedule from being stored on the happy path, but the consume-side defense-in-depth narrative in the PR description currently isn't backed by a real test of the consume-side branch.
|
@coderabbitai addressed in 9eb600d. Extracted the |
|
Tip For best results, initiate chat on the files or code changes.
(¯ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative re-review at head 9eb600d. The single prior finding — that the regression test only asserted BTreeMap stdlib semantics rather than exercising the fixed transformer code path — is FIXED. The latest delta (refactor commit 9eb600d) extracts the SetPrices resolution logic into a pure helper resolve_set_prices_direct_purchase_price (transformer.rs:254-308) that the transformer's SetPrices arm now invokes (transformer.rs:199-224). The replacement test resolve_set_prices_empty_map_returns_not_for_sale_without_panicking (transformer.rs:571-587) calls that helper directly with an empty BTreeMap and asserts Err(StateError::TokenNotForDirectSale), so reintroducing .expect("Map is not empty") in the helper's None arm would now fail the test instead of leaving it green. Four additional tests cover below-minimum, matched-tier, underpayment, and overflow branches through the helper. No new in-scope defects identified in the latest delta; the structure-validation guard, tail-appended BasicError variant (code 10461), and wasm-dpp bridge remain correct from the prior pass.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "d36fa20867de18b9277d8ca73e9400720e296473fc3db0edeb1d3718820395b8"
)Xcode manual integration:
|
…tch #3865) Per maintainer decision, drop the version-gating and add the byte-array encoding-stability check directly to validate_update_v0, consistent with the sibling chain-halt fix #3865 which added its rejection rule to validate_structure_v0 in place. The check runs before validate_schema so it cannot be bypassed by a JSON-schema-compatible widening. - revert validate_update_v1, the dispatcher arm, and the DPP_VALIDATION_VERSIONS_V3 feature-version bump - check + tests now live in validate_update/v0 (tests still exercise the public validate_update dispatcher); drops the pre-activation gating test - e2e error assertion keeps assert_matches! on the decode-failure variant Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3865 +/- ##
============================================
+ Coverage 70.73% 71.20% +0.47%
============================================
Files 20 20
Lines 2788 2837 +49
============================================
+ Hits 1972 2020 +48
- Misses 816 817 +1
🚀 New features to boost your workflow:
|
3 participants
Issue being fixed or feature implemented
A chain-halt (liveness) bug in the token direct-purchase path.
An empty
SetPrices(BTreeMap::new())pricing schedule is a representable, storable value:TokenPricingSchedule::SetPriceswraps aBTreeMap<TokenAmount, Credits>, and neither theset-price structure validation nor the set-price state validation rejected an empty map.
A token's pricing authority could therefore set
price = Some(SetPrices(empty map))via aTokenSetPriceForDirectPurchasetransition that passed all validation, and the empty scheduleflowed unchanged into GroveDB.
Afterwards, any identity submitting a
TokenDirectPurchasefor that token detonated a panic.In the transformer's
Nonearm, the original code built the error viaset_prices.keys().next().expect("Map is not empty"), which panics when the map is empty.This panic is a deterministic, quorum-wide chain halt — confirmed by tracing the call stack:
process_proposal→run_block_proposal→process_raw_state_transitions→process_state_transition→transform_into_action→ the token transformer.Errs are caught into anInternalErrorexecution result (chaincontinues) at
process_raw_state_transitions/v0/mod.rs, but panics are not — that catch is.unwrap_or_else(...)on aResult, which a panic unwinds straight through.catch_unwindanywhere on the ABCI path (workspace-wide).install_panic_hookinmain.rs) callscancel.cancel(), shutting thenode down on any panic.
Result: every validator panics on the same block during
process_proposal→ every node shutsdown → chain halt.
What was done?
Defense in depth across the produce and consume sides:
SetPricesat set-price structure validation with a newTokenPricingScheduleEmptyErrorconsensus error (basic code10461). Emptiness is a pureproperty of the transition's
pricefield — no state needed — and structure validationshort-circuits the batch before the transition is ever applied, so an empty schedule can never
reach state.
Nonearm matches onset_prices.keys().next()and returnsTokenNotForDirectSalefor an empty map instead of.expect()-ing. This PR keeps it as the consume-side guard and adds a regression test for it.Wiring for the new error: new error struct under
errors/consensus/basic/token/, re-exported inthat module, added to the
BasicErrorenum (tail-appended to preserve bincode positionaldiscriminants), assigned error code
10461incodes.rs, and bridged in thewasm-dppexhaustive consensus-error match.
How Has This Been Tested?
New regression tests:
should_return_error_when_set_prices_is_empty— set-price structure validation rejects an emptySetPriceswithTokenPricingScheduleEmptyError(plusshould_pass_with_non_empty_set_pricesand
should_pass_with_single_pricefor the happy paths).set_prices_empty_map_does_not_panic_and_is_treated_as_not_for_sale— the transformer'sNone-arm logic resolves an empty map to the not-for-sale branch without panicking.Verification run locally:
cargo test -p dpp --all-features(new validation tests pass)cargo test -p drive --lib set_prices_empty_map_does_not_panic(transformer regression passes)cargo check -p dpp --all-features --testscargo check -p wasm-dpp --target wasm32-unknown-unknowncargo fmt --allBreaking Changes
Consensus behavior changes: a
TokenSetPriceForDirectPurchasetransition carrying an emptySetPricesmap is now rejected at structure validation (new consensus error code10461),whereas it was previously accepted. This is a consensus-relevant hardening landing pre-release
(
4.0.0-rc.2) — the protocol version is not finalized/deployed, so there is no released networkto fork against and no migration concern. The new
BasicErrorvariant is appended at the enumtail to preserve bincode positional encoding of existing variants.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit