Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stateful property-tests for the error/failure paths in PoX-4 #4842

Merged
merged 73 commits into from
Jun 25, 2024

Conversation

BowTiedRadone
Copy link
Contributor

@BowTiedRadone BowTiedRadone commented Jun 3, 2024

This PR implements a strategy to test malicious user interactions with the PoX-4 contract, covering specific scenarios to ensure correct handling.

Structure
A new file with unhappy path scenarios has been added. See contrib/boot-contracts-stateful-prop-tests/tests/pox-4/err_Commands.ts, where each scenario has a generator simulating malicious behavior and specifying the expected PoX-4 error code.

The rest of the files in this PR are infrastructure.


Statistics
To facilitate the review of post-execution test-run statistics, tree logging has been added:

├─ AllowContractCallerCommand: 35
├─ DelegateStackExtendCommand: 3
├─ DelegateStackIncreaseCommand: 4
│   ├─ Err_Stacking_Insufficient_Funds: 5
│   ├─ Err_Stacking_Invalid_Amount: 3
│   ├─ Err_Stacking_Not_Delegated: 14
│   └─ Err_Stacking_Permission_Denied: 1
├─ DelegateStackStxCommand: 3
│   ├─ Err_Delegation_Too_Much_Locked: 2
│   └─ Err_Stacking_Permission_Denied_2: 2
├─ DelegateStxCommand: 22
│   └─ Err_Stacking_Already_Delegated: 16
├─ DisallowContractCallerCommand: 6
│   └─ Err: 39
├─ GetStackingMinimumCommand: 57
├─ GetStxAccountCommand: 30
├─ RevokeDelegateStxCommand: 14
│   └─ Err_Delegation_Already_Revoked: 26
├─ StackAggregationCommitAuthCommand: 1
│   ├─ Err_Stacking_No_Such_Principal_1: 12
│   └─ Err_Stacking_No_Such_Principal_2: 28
├─ StackAggregationCommitIndexedAuthCommand: 1
│   ├─ Err_Stacking_No_Such_Principal_1: 7
│   ├─ Err_Stacking_No_Such_Principal_2: 22
│   └─ Err_Stacking_Threshold_Not_Met: 1
├─ StackAggregationCommitIndexedSigCommand: 0
│   ├─ Err_Stacking_No_Such_Principal_1: 12
│   ├─ Err_Stacking_No_Such_Principal_2: 29
│   └─ Err_Stacking_Threshold_Not_Met: 1
├─ StackAggregationCommitSigCommand: 1
│   ├─ Err_Stacking_No_Such_Principal_1: 13
│   ├─ Err_Stacking_No_Such_Principal_2: 34
│   └─ Err_Stacking_Threshold_Not_Met: 2
├─ StackAggregationIncreaseCommand: 2
│   └─ Err_Stacking_No_Such_Principal: 7
├─ StackExtendAuthCommand: 6
│   ├─ Err_Stacking_Invalid_Lock_Period: 7
│   └─ Err_Stacking_Is_Delegated_1: 1
├─ StackExtendSigCommand: 2
│   ├─ Err_Stack_Extend_Not_Locked: 2
│   └─ Err_Stacking_Invalid_Lock_Period: 5
├─ StackIncreaseAuthCommand: 11
│   ├─ Err_Stacking_Insufficient_Funds: 24
│   ├─ Err_Stacking_Invalid_Amount: 10
│   └─ Err_Stacking_Is_Delegated: 3
├─ StackIncreaseSigCommand: 12
│   ├─ Err_Stacking_Insufficient_Funds: 13
│   ├─ Err_Stacking_Invalid_Amount: 13
│   └─ Err_Stacking_Is_Delegated: 3
├─ StackStxAuthCommand: 1
│   ├─ Err_Stacking_Already_Stacked_1: 21
│   └─ Err_Stacking_Already_Stacked_2: 9
└─ StackStxSigCommand: 5
    ├─ Err_Stacking_Already_Stacked_1: 14
    └─ Err_Stacking_Already_Stacked_2: 14

moodmosaic and others added 30 commits May 22, 2024 00:11
…types

- Added import for StackStxSigCommand_Err and StackStxAuthCommand_Err
- Added StackStxAuthCommand_Err with a custom check function delegate to
  PoxCommands
- Added StackStxSigCommand_Err with a custom check function delegate to
  PoxCommands

This allows the check function to be parameterized, reducing the need for
copy-pasting classes.

Note: This is a very work in progress.
…types

- Separate success paths from failure paths to keep pox_Commands.ts focused
  on success cases only. This prevents the file from growing with
  out-of-scope cases.

Note: This is a work in progress.
The command run tracking will be added to the command's `check` method.
If not passed incremented, the call will result in an `ERR_INVALID_START_BURN_HEIGHT` when being sent at the limit between 2 cycles.
This commit:
- adds 6 unhappy path cases for the `stack-stx` PoX-4 method, 3 for each signing method (authorization or signature)
- adds a dictionary that contains the PoX-4 error names and the error codes
- adds the command run tracking inside the `check` method, resulting in displaying all the paths hit and the number of times.
They needed to be excluded as we have removed the command run tracking from the run method.
The added unhappy path tries to call revoke-delegate-stx with an address that is not delegating.
The command run tracking was moved inside the command's check function. No need to report the run using the file name anymore.
The added unhappy path tries to call delegate-stx with an address that is already delegating.
The command run tracking was moved inside the command's check function. No need to report the run using the file name anymore.
This commit:
- includes the authorization and the function call in the same block. It is needed because otherwise, it can result in issuing the authorization for the wrong reward cycle.
- updates the passed start-burn-ht param, different from the StackStxSigCommand. If not doing it like this, the test fails when the command is called at the limit between 2 reward cycles.
- removes unnecessary operations: retrieving the reward cycle, retrieving the unlockBurnHeight.
This commit:
- adds 3 unhappy path cases for the `stack-aggregation-commit` PoX-4 method, called using a signature.
- adds the command run tracking inside the `check` method.
- adds the expected `stack-aggregation-commit` PoX-4 errors to the POX_4_ERRORS dictionary.
The command run tracking was moved inside the command's check function. No need to report the run using the file name anymore.
This commit:
- adds 3 unhappy path cases for the `stack-aggregation-commit` PoX-4 method, called using an authorization.
- adds the command run tracking inside the `check` method.
The command run tracking was moved inside the command's check function. No need to report the run using the file name anymore.
This commit improves the unhappy paths execution visibility after the test suite run is complete.
This commit:
- adds 3 unhappy path cases for the `stack-aggregation-commit-indexed` PoX-4 method, called using a signature.
- adds the command run tracking inside the `check` method.
This commit:
- adds 3 unhappy path cases for the `stack-aggregation-commit-indexed` PoX-4 method, called using an authorization.
- adds the command run tracking inside the `check` method.
The command run tracking for the unhappy paths was moved inside the commands' check function. No need to report the run using the file name anymore.
This commit:
- adds one unhappy path case for the `stack-aggregation-increase` PoX-4 method, called using an authorization.
- adds the command run tracking inside the `check` method.
This commit:
- adds 3 unhappy path cases for the `delegate-stack-stx` PoX-4 method.
- adds the command run tracking inside the `check` method.
- adds the expected `delegate-stack-stx` PoX-4 errors to the `POX_4_ERRORS` dictionary.
- exports the `nextCycleFirstBlock` method from pox_commands, as it is used inside err_Commands.
This commit:
- adds 3 unhappy path cases for the `stack-increase` PoX-4 method, called using a signature.
- adds the command run tracking inside the `check` method.
- adds the expected `stack-increase` PoX-4 errors to the `POX_4_ERRORS` dictionary.
This commit:
- adds 3 unhappy path cases for the `stack-increase` PoX-4 method, called using an authorization.
- adds the command run tracking inside the `check` method.
This commit:
- adds 5 unhappy path cases for the `stack-increase` PoX-4 method, called using a signature.
- adds the command run tracking inside the `check` method.
- adds the expected `stack-extend` PoX-4 errors to the `POX_4_ERRORS` dictionary.
This commit:
- adds 5 unhappy path cases for the `stack-extend` PoX-4 method, called using an authorization.
- adds the command run tracking inside the `check` method.
This commit:
- adds 4 unhappy path cases for the `delegate-stack-extend` PoX-4 method.
- adds the command run tracking inside the `check` method.
- adds the expected `delegate-stack-extend` PoX-4 error to the `POX_4_ERRORS` dictionary.
…types

- Added import for StackStxSigCommand_Err and StackStxAuthCommand_Err
- Added StackStxAuthCommand_Err with a custom check function delegate to
  PoxCommands
- Added StackStxSigCommand_Err with a custom check function delegate to
  PoxCommands

This allows the check function to be parameterized, reducing the need for
copy-pasting classes.

Note: This is a very work in progress.
…types

- Separate success paths from failure paths to keep pox_Commands.ts focused
  on success cases only. This prevents the file from growing with
  out-of-scope cases.

Note: This is a work in progress.
@BowTiedRadone BowTiedRadone self-assigned this Jun 3, 2024
@BowTiedRadone BowTiedRadone requested a review from a team as a code owner June 3, 2024 11:11
Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

Thank you @BowTiedRadone 🙌

@moodmosaic moodmosaic changed the title Unhappy Path PoX-4 Stateful Property Testing Stateful property-tests for the error/failure paths in PoX-4 Jun 3, 2024
Copy link
Collaborator

@friedger friedger left a comment

Choose a reason for hiding this comment

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

That is a lot of new code.
The comments on the commands do not describe the functions.

This commit brings typo fixes for all the comments inside the PoX-4 stateful property tests.
@moodmosaic
Copy link
Member

moodmosaic commented Jun 10, 2024

@friedger, yes, those classes are templates. From one template, multiple unhappy paths can be derived. It's a good point that the comments should be updated.


For your information, in sBTC invariant testing, the boilerplate/setup will be significantly reduced. We now have a new syntax for bundling the randomly-generated inputs with the commands. For example, consider this Clarity code snippet from a demo:

(define-data-var counter uint u0)

(define-constant ERR_COUNTER_MUST_BE_POSITIVE (err u401))

(define-public (decrement)
  (let ((current-counter (var-get counter)))
    (asserts! (> current-counter u0) ERR_COUNTER_MUST_BE_POSITIVE)
    (ok (var-set counter (- current-counter u1)))
  )
)

The invariant in Clarinet/TypeScript is defined as:

import { tx } from "@hirosystems/clarinet-sdk";
import { Cl } from "@stacks/transactions";
import fc from "fast-check";

const accounts = simnet.getAccounts();

export const CounterDecrement = fc
  .record({
    sender: fc.constantFrom(...accounts.values()),
  })
  .map((r) => ({
    check: (model: Readonly<Model>) => {
      return model.counter > 0;
    },
    run: (model: Model, real: Real) => {
      const block = real.simnet.mineBlock([tx.callPublicFn("counter", "decrement", [], r.sender)]);
      expect(block[0].result).toBeOk(Cl.bool(true));
      model.counter = model.counter - 1;
    },
    toString: () => `decrement`,
  }));

In the current setup, this logic is split into two places, with one part being a class (constructor, readonly fields, etc.). In sBTC, everything will be declared in one place. /cc @hugocaillard @setzeus

Copy link
Collaborator

@hugocaillard hugocaillard left a comment

Choose a reason for hiding this comment

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

Some refactor suggestions.

Considering the size of the code base, I think you would greatly benefit from using a code formatter and a linter (prob in a different PR)

This update simplifies the err_Commands generators file.
The added condition ensures that an active lock does not expire at the end of the current cycle, and avoids the PoX-4 Clarity error 2 - ERR_STACKING_INVALID_LOCK_PERIOD
This commit refactors all the unhappy path check functions, returning early the false and removing the else branch.
This commit:
- simplifies the way firstRewardCycle is calculated, replacing the ternary operator with the max Math method.
- standardizes the const name of the stacker retrieved from the model.
Copy link
Collaborator

@hugocaillard hugocaillard left a comment

Choose a reason for hiding this comment

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

LGTM!
Impressive work

Copy link
Collaborator

@friedger friedger left a comment

Choose a reason for hiding this comment

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

LGTM

@BowTiedRadone BowTiedRadone added this pull request to the merge queue Jun 25, 2024
Merged via the queue into develop with commit 6586d94 Jun 25, 2024
1 check passed
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.

4 participants