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

Add stack-aggregation-increase command #4652

Conversation

BowTiedRadone
Copy link
Contributor

This PR adds the stack-aggregation-increase command to the stateful property testing environment. It is part of #4548 and targets feat/pox-4-stateful-property-testing (#4550).

Adding the `StackAggregationIncreaseCommand` is currently causing the tests to fail.
To investigate:
- this may be a PoX-4 bug
- this may be a command implementation issue
@BowTiedRadone
Copy link
Contributor Author

@friedger @moodmosaic @setzeus adding the stack-aggregation-increase brought up some intermittent test failures. I have saved the error traces (that I'll continue investigating after the command is merged).

In case the command is correct, there may be some PoX-4 unexpected behavior.

Thank you in advance!

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.

If the operator uses only one pox-address, this looks good.

Do you reset the reward indices when a new cycle starts?

* Constraints for running this command include:
* - The Operator must have locked STX on behalf of at least one stacker.
* - The PoX address must have partial committed STX.
* - The Reward Cycle Index must be positive.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reward cycle index must be one of the returned indexes by the first aggregation commit tx for the pox address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I pick it from the list at the generator level, here

@@ -112,6 +112,7 @@ export class StackAggregationCommitAuthCommand implements PoxCommand {

const operatorWallet = model.wallets.get(this.operator.stxAddress)!;
operatorWallet.amountToCommit -= committedAmount;
operatorWallet.committedRewCycleIndexes.push(model.nextRewardSetIndex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the wallet is using more than one pox address the index storage should be a map of pox addresses to a list of indexes.

@moodmosaic
Copy link
Member

LGTM; should be ready to merge once we address @friedger's comments.

@moodmosaic
Copy link
Member

Merging into feat/pox-4-stateful-property-testing.

@moodmosaic moodmosaic merged commit ee51196 into feat/pox-4-stateful-property-testing Apr 8, 2024
1 of 2 checks passed
@moodmosaic moodmosaic deleted the feat/pox-4-stateful-stack-aggregation-increase branch April 8, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants