Skip to content

refactor: Extract post-conditions from stackslib into separate crate - #7478

Open
jbencin-stacks wants to merge 7 commits into
stacks-network:mainfrom
jbencin-stacks:refactor/stacks-postconditions-crate
Open

refactor: Extract post-conditions from stackslib into separate crate#7478
jbencin-stacks wants to merge 7 commits into
stacks-network:mainfrom
jbencin-stacks:refactor/stacks-postconditions-crate

Conversation

@jbencin-stacks

Copy link
Copy Markdown
Contributor

Description

Extract post-conditions from stackslib into separate crate (stacks-postconditions), so they can be built for WASM targets

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • For new Clarity features or consensus changes, add property tests (see docs/property-testing.md)
  • Changelog fragment(s) or "no changelog" label added (see changelog.d/README.md)
  • Required documentation changes (e.g., rpc/openapi.yaml for RPC endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo

@jbencin-stacks
jbencin-stacks marked this pull request as draft July 31, 2026 21:12
@jbencin-stacks
jbencin-stacks force-pushed the refactor/stacks-postconditions-crate branch from e65c562 to 856cdd7 Compare August 3, 2026 17:50
jbencin-stacks and others added 5 commits August 3, 2026 15:45
…tion

Post-conditions constrain the assets a transaction may move. That check is a
static function of the declared post-conditions, an `AssetMap`, the origin
principal and the epoch — it needs no database or chainstate, and nothing about
it is specific to Clarity as a source language. Hosting it in `stackslib` made
it unreachable from wasm, and hosting it in `clarity` would tie it to one
contract language as we look at supporting others.

Move `check_transaction_postconditions` and its `HashableClarityValue` helper
into a new `stacks-postconditions` crate, leaving a thin adapter in `stackslib`
that projects `StacksAccount` onto the origin principal the check needs.

The crate still depends on `clarity` for `AssetMap` and `VmExecutionError`, so
this does not yet decouple from the Clarity VM; it isolates the logic so that
the remaining step is a change to one crate rather than a change spread across
`stackslib`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three epoch-activation rules for post-conditions (`Originator` mode and
NFT `MaybeSent` requiring Stacks 3.4, `Staking`/`Pox` requiring Stacks 4.0)
were duplicated verbatim between `process_transaction_precheck` and
`StacksBlock::validate_transaction_static_epoch`. Like the checker moved in the
previous commit they are pure functions of the post-conditions, the mode and
the epoch, with no chainstate dependency.

Add `check_post_conditions_supported_in_epoch`, returning a typed
`UnsupportedPostCondition` rather than a formatted message so both call sites
keep their existing error channel and log level. This collapses the two copies
into one predicate, and matters for wasm consumers: because
`check_transaction_postconditions` evaluates explicit post-conditions in every
epoch, a caller running only that function would return a pass/fail verdict for
a transaction mainnet rejects outright.

Behavior is unchanged, including the message text at both call sites. The
`Display` impl carries the "before Stacks X.Y" phrasing that
`process_transaction_precheck` puts in its error, and `subject()` exposes the
offending feature on its own so `validate_transaction_static_epoch` can keep
naming the current epoch in its log.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Move the tests that exercise the checker in isolation — a hand-built `AssetMap`
plus a list of post-conditions — alongside the code they cover. The
`process_transaction` pipeline tests that reach the checker through the node
(fee handling, receipts, rollback) stay in `stackslib`.

This is a pure move. The only changes to the test bodies are mechanical: the
`make_account` helper is gone, since the relocated check takes a
`&PrincipalData` rather than a `&StacksAccount`, so each case passes the origin
principal directly instead of fabricating a nonce and balance the check ignored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AssetMap::to_table()` excludes the stacking map and the pox-action set, so the
staking and PoX coverage checks are hand-rolled loops reachable only through
their own path. Several branches in them had no test:

- `Originator` mode for staking/PoX coverage. Every existing test used
  `Deny`/`Allow`, leaving the non-origin skip in
  `enforce_unchecked_assets_for_principal` unexercised.
- `PostConditionPrincipal::Standard` for `Staking`/`Pox`. All existing tests
  used `::Origin`, so non-origin resolution through `to_principal_data` was
  untested for these two variants.
- The `amount_staked == 0` skip in the staking coverage loop.
- What the epoch gate does *not* gate: explicit `Staking`/`Pox` post-conditions
  are evaluated in every epoch, and only the coverage requirement is gated.
  Existing pre-4.0 cases used empty post-condition lists.

Also cover `check_post_conditions_supported_in_epoch` on both sides of the
Stacks 3.4 and 4.0 boundaries, and add a test showing the admission and
asset-movement checks are independent, so callers need both.

Correct stale terminology in the PoX test comments, which referred to
"unstaking" / "MaybeUnstaked" from an earlier naming iteration; the condition
codes are `PoxConditionCode::{NotPerformed, MaybePerformed, Performed}` over
general PoX actions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbencin-stacks
jbencin-stacks force-pushed the refactor/stacks-postconditions-crate branch from 856cdd7 to cdbf6ca Compare August 3, 2026 19:45
@jbencin-stacks
jbencin-stacks marked this pull request as ready for review August 3, 2026 20:41

@brice-stacks brice-stacks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -0,0 +1 @@
Moved transaction post-condition verification out of `stackslib` into a new `stacks-postconditions` crate, so that it can be used in WASM environments and can eventaully be de-coupled from Clarity

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Moved transaction post-condition verification out of `stackslib` into a new `stacks-postconditions` crate, so that it can be used in WASM environments and can eventaully be de-coupled from Clarity
Moved transaction post-condition verification out of `stackslib` into a new `stacks-postconditions` crate, so that it can be used in WASM environments and can eventually be de-coupled from Clarity

@benjamin-stacks benjamin-stacks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, just a few smaller things.

//! Transaction post-condition verification for the Stacks blockchain.
//!
//! Post-conditions constrain the assets a transaction is allowed to move. Two
//! checks, both needed to match mainnet semantics:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you mean by "needed to match mainnet semantics"? These define mainnet semantics, don't they?

Comment on lines +85 to +109
impl UnsupportedPostCondition {
/// The offending feature on its own, for callers that phrase the epoch
/// requirement themselves instead of using [`Display`].
pub fn subject(&self) -> &'static str {
match self {
Self::OriginatorMode => "Originator post-condition mode",
Self::NftMaybeSent => "NFT MaybeSent post-condition",
Self::StakingOrPox => "Staking/Pox post-condition",
}
}
}

impl std::fmt::Display for UnsupportedPostCondition {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let version = match self {
Self::OriginatorMode | Self::NftMaybeSent => "3.4",
Self::StakingOrPox => "4.0",
};
write!(
f,
"{} is not supported before Stacks {version}",
self.subject()
)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels unnecessarily complex. Can we just have one message per post condition and be done with it? The fact that those error! messages are going to be slightly different doesn't seem like a problem to me.

Comment on lines +2649 to +2653
/// A post-condition rejected at admission is still evaluated by
/// [`check_transaction_postconditions`] in that epoch, which is why callers
/// need both checks.
#[test]
fn test_epoch_admission_is_independent_of_asset_check() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't feel to me that we should have unit tests asserting this behavior, but we should rather fix that behavior.

If a contract call transaction with an unsupported post-condition is actually executed, then that is a bug, not a feature, and the result should be an error, not a silent acceptance. Or am I missing/misunderstanding something?

(To be clear, I'm not saying you should fix that in this PR -- here I'm only saying we shouldn't have unit tests for broken behavior.)

origin_principal: &PrincipalData,
asset_map: &AssetMap,
epoch_id: StacksEpochId,
) -> Result<Option<String>, VmExecutionError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably change the return type of this as well, from stringly typed to actually typed, like you did for check_post_conditions_supported_in_epoch.

(Not in the PR to be clear, this is just a thought.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants