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

PoX 2 Unlock #3260

Merged
merged 20 commits into from
Sep 12, 2022
Merged

PoX 2 Unlock #3260

merged 20 commits into from
Sep 12, 2022

Conversation

kantai
Copy link
Member

@kantai kantai commented Aug 22, 2022

This PR implements automatic unlocks for PoX 2.

Stackers who do not qualify for a slot in a reward set when the reward set is selected will be automatically unlocked after the reward set is selected. The auto unlock occurs 1 bitcoin block after the auto-unlock.

This requires a lot of complexity in pox-2. This PR adds a test for the simple use case, but probably more testing is needed.

@kantai kantai requested a review from jcnelson August 22, 2022 15:29
@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #3260 (4270820) into next (ad30e13) will decrease coverage by 54.00%.
The diff coverage is 43.87%.

@@             Coverage Diff             @@
##             next    #3260       +/-   ##
===========================================
- Coverage   83.92%   29.91%   -54.01%     
===========================================
  Files         277      279        +2     
  Lines      227444   228223      +779     
===========================================
- Hits       190880    68282   -122598     
- Misses      36564   159941   +123377     
Impacted Files Coverage Δ
clarity/src/vm/clarity.rs 81.13% <ø> (-6.61%) ⬇️
clarity/src/vm/database/clarity_db.rs 70.22% <ø> (-19.95%) ⬇️
clarity/src/vm/database/structures.rs 67.17% <0.00%> (-21.10%) ⬇️
.../chainstate/burn/operations/leader_block_commit.rs 13.55% <ø> (-81.67%) ⬇️
src/chainstate/coordinator/tests.rs 0.00% <0.00%> (-95.68%) ⬇️
src/chainstate/stacks/boot/contract_tests.rs 0.00% <0.00%> (-94.45%) ⬇️
src/chainstate/stacks/boot/pox_2_tests.rs 0.00% <0.00%> (-94.29%) ⬇️
src/chainstate/stacks/boot/mod.rs 9.26% <49.24%> (-85.17%) ⬇️
clarity/src/vm/contexts.rs 53.47% <55.00%> (-40.47%) ⬇️
src/chainstate/coordinator/mod.rs 74.48% <75.00%> (-15.95%) ⬇️
... and 256 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

;; calculate the index into the reward-set-indexes that `cycle` is at
(moved-cycle-index (- cycle (get first-reward-cycle moved-state)))
(moved-reward-list (get reward-set-indexes moved-state))
;; reward-set-indexes[moved-cycle-index] = set-index via slice, append, concat.
Copy link
Member

Choose a reason for hiding this comment

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

A pity that there isn't a set-element-at keyword in Clarity for sequences. Maybe this could be a separate function that makes this clear? Then we could at least point it out to developers if they want to do the equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a private method in 266caf2.

It would be pretty easy to add a new Clarity built-in for this, though. I might try to include in the stacks-increase PR.

Copy link
Member

Choose a reason for hiding this comment

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

I would be supportive!

Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

mostly lgtm, just some minor comments

src/burnchains/burnchain.rs Outdated Show resolved Hide resolved
src/chainstate/stacks/boot/pox_2_tests.rs Outdated Show resolved Hide resolved
src/chainstate/stacks/boot/mod.rs Show resolved Hide resolved
src/chainstate/stacks/boot/mod.rs Outdated Show resolved Hide resolved
src/chainstate/stacks/boot/pox-2.clar Outdated Show resolved Hide resolved
);
for _i in 0..slots_taken {
test_debug!("Add to PoX reward set: {:?}", &address);
reward_set.push(address.clone());
}
// if stacker did not qualify for a slot *and* they have a stacker
// pointer set by the PoX contract, then add them to auto-unlock list
Copy link
Member

Choose a reason for hiding this comment

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

The case where they wouldn't have a stacker pointer is if they were in a pool, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep -- if they used delegation.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

This pretty much LGTM. Thanks @kantai!

I just had one remaining design question about how to handle unlocks across reward cycle boundaries, which is still detailed in the comments. Let's get that addressed before merging. Thanks!

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking the time to address this @kantai! Users are gonna love auto-unlocks :D

self.inner_execute_contract(contract, tx_name, args, read_only, true)
}

fn inner_execute_contract(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can add a note on this function directly that specifies that allow_private must always be false when doing user tx processing


#[derive(Debug, PartialEq, Serialize, Deserialize)]
pub struct PoxStartCycleInfo {
pub missed_reward_slots: Vec<(PrincipalData, u128)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add comment about what PrincipalData and u128 are referring to

// peak at the next address in the set, and see if we need to sum
while addresses.last().map(|x| &x.0) == Some(&address) {
let (_, additional_amt) = addresses
while addresses.last().map(|x| &x.reward_address) == Some(&address) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the logic here, but it could be useful to leave a comment for posterity: The outer while loop pops the last element of the addresses vector, here we call last()... and since the addresses are sorted...

Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

lgtm

for the future though, I think it would be desirable for the pox contract to have some meta documentation.

@kantai
Copy link
Member Author

kantai commented Sep 12, 2022

Fixed the CI tasks by switching to stable (thanks @lgalabru for pointing out that grcov works in stable now!), though it seems like the unit-tests tasks didn't successfully upload the coverage data. Will poke into that separately, but merging this PR now because the tests are passing, and its been approved.

@kantai kantai merged commit 7db7efd into next Sep 12, 2022
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants