2.1: Feature: `stack-increase` for PoX-2 #3282

kantai · 2022-09-07T19:09:33Z

This PR adds two new methods to pox-2:

stack-increase: this method increases the lockup for the stacker, and adds the new amount to the reward sets of the remaining cycles for the user
delegate-stack-increase: this method increases the lockup for the supplied stacker, adding the new amount to the partially-stacked state

As part of this work, this also removes the total-ustx field from the stacking-state map. The rationale for this is to normalize some of the state in the PoX-2 contract: this field is not used for any checks, but it would be "replicated" in several other states: that's at least the stx-account lock amount and the reward-set entries for the stacker in each cycle. Because of the "increase" methods, the total-ustx field would either be out of sync with the increased amount or the previous cycle entries. To keep this field meaningful, it would need to be a list of total-ustx for each cycle, but because the field isn't used in the contract anywhere then it seemed better to just remove the state.

…ease

codecov · 2022-09-07T19:11:42Z

Codecov Report

Merging #3282 (ad1ffc1) into feat/pox-2-unlock (4270820) will decrease coverage by 0.07%.
The diff coverage is 0.33%.

@@                  Coverage Diff                  @@
##           feat/pox-2-unlock    #3282      +/-   ##
=====================================================
- Coverage              29.91%   29.84%   -0.08%     
=====================================================
  Files                    279      279              
  Lines                 228223   228818     +595     
=====================================================
+ Hits                   68282    68287       +5     
- Misses                159941   160531     +590

Impacted Files	Coverage Δ
clarity/src/vm/database/structures.rs	`62.72% <0.00%> (-4.46%)`	⬇️
src/chainstate/stacks/boot/mod.rs	`9.25% <0.00%> (-0.02%)`	⬇️
src/chainstate/stacks/boot/pox_2_tests.rs	`0.00% <0.00%> (ø)`
src/chainstate/stacks/db/accounts.rs	`43.90% <0.00%> (-1.36%)`	⬇️
src/chainstate/stacks/mod.rs	`8.02% <0.00%> (-0.07%)`	⬇️
src/clarity_vm/special.rs	`48.23% <1.81%> (-12.77%)`	⬇️
src/clarity_vm/database/mod.rs	`73.28% <100.00%> (-0.26%)`	⬇️
src/burnchains/bitcoin/network.rs	`77.77% <0.00%> (-3.71%)`	⬇️
src/chainstate/stacks/index/cache.rs	`13.37% <0.00%> (-0.51%)`	⬇️
... and 15 more

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

zone117x · 2022-09-09T14:29:54Z

Closes #2533

(referencing the issue for anyone following along, it won't let me link the issue from the sidebar)

jcnelson · 2022-09-09T18:16:44Z

src/chainstate/stacks/boot/pox-2.clar

@@ -63,8 +64,6 @@
 (define-map stacking-state
    { stacker: principal }
    {
-        ;; how many uSTX locked?
-        amount-ustx: uint,


Do you think you could provide a read-only function that returns the number of locked tokens for a principal, via stx-account? Asking because I'm pretty sure stacking.club depends on this information (and there could be others).

jcnelson · 2022-09-09T18:22:49Z

src/chainstate/stacks/boot/pox-2.clar

+        (let ((existing-entry (unwrap-panic (map-get? reward-cycle-pox-address-list { reward-cycle: reward-cycle, index: reward-cycle-index })))
+              (existing-total (unwrap-panic (map-get? reward-cycle-total-stacked { reward-cycle: reward-cycle }))))
+            ;; stacker must match
+            (asserts! (is-eq (get stacker existing-entry) (some (get stacker data))) none)


If (is-some (get stacker data)) is true, then wouldn't this assertion's failure be a Really Bad Thing, i.e. indicative that reward-cycle-pox-address-list was corrupted somehow, since the reward-cycle-index didn't refer to a PoX address from this stacker?

I can see why (get stacker data) could be none -- e.g. when the stacker had stacked through a delegate. But, should this function even be called when this is the case (or, can we not tell prior to calling that this is the case)?

Yes, this would be a Really Bad Thing, but Clarity cannot assert a node panic, so instead, we just panic the transaction.

jcnelson · 2022-09-09T18:29:54Z

src/chainstate/stacks/boot/pox-2.clar

+                    reward-cycle: (get first-reward-cycle stacker-state),
+                    stacker: tx-sender,
+                    add-amount: increase-by })))
+            (err ERR_STACKING_UNREACHABLE))


So if this (fold increase-reward-cycle-entry) is always supposed to be (some ..), then doesn't that mean that internally, tx-sender must not have stacked through a delegate? Can we check this as part of the checks above this line?

Yep, will add a check.

jcnelson · 2022-09-09T18:38:48Z

src/chainstate/stacks/db/accounts.rs

+
+        let mut snapshot = db.get_stx_balance_snapshot(principal);
+
+        if !snapshot.has_locked_tokens() {


I find myself wondering whether or not these Err(..) paths are ever reachable. If the Clarity code for pox-2 is correct, then these should never be hit, right?

Ah, I see we panic() later in the special case handler.

jcnelson

This overall LGTM, modulo the failing unit test. Just had a few questions about how defensive we can be in the stack-increase.

jcnelson · 2022-09-12T15:33:12Z

N.B from blockchain meeting:

Increase code coverage in pox-2 to verify that the bi-directional map between stackers and the reward set state remains consistent. Pertaining to this, it's stack-increase, delegate-stack-increase, auto-unlock, and stack-extend. We should spend some time convincing ourselves that the structure of this map is preserved in all cases in which these are called.

PoX 2 Unlock

pavitthrap

lgtm, just had some suggestions wrt comments & readability

pavitthrap · 2022-09-12T18:19:08Z

src/chainstate/stacks/boot/pox-2.clar

+                (err ERR_STACK_EXTEND_NOT_LOCKED))
+      (asserts! (>= increase-by u1)
+                (err ERR_STACKING_INVALID_AMOUNT))
+      (asserts! (>= (get unlocked stacker-info) increase-by)


could define amount_unlocked in the let and use that instead of (get unlocked stacker-info)

pavitthrap · 2022-09-12T18:25:51Z

src/chainstate/stacks/boot/pox-2.clar

+            (map-set reward-cycle-pox-address-list
+                     { reward-cycle: reward-cycle, index: reward-cycle-index }
+                     { pox-addr: (get pox-addr existing-entry),
+                       total-ustx: (+ (get total-ustx existing-entry)


can compute total-ustx in the let (it's computed twice here)

pavitthrap · 2022-09-12T21:00:24Z

src/chainstate/stacks/boot/pox-2.clar

+      ;; stacker must have delegated to the caller
+      (let ((delegation-info (unwrap! (get-check-delegation stacker) (err ERR_STACKING_PERMISSION_DENIED))))
+        ;; must have delegated to tx-sender
+        (asserts! (is-eq (get delegated-to delegation-info) tx-sender)


these asserts would be easier to read if you moved the (get _ delegation-info) to the let

pavitthrap · 2022-09-12T21:33:17Z

src/chainstate/stacks/boot/pox_2_tests.rs

+            cycle_number,
+            &bob_principal,
+        );
+        assert_eq!(partial_stacked, 512 * POX_THRESHOLD_STEPS_USTX);


Can use alice_first_lock_amount here

pavitthrap · 2022-09-12T21:34:24Z

src/chainstate/stacks/boot/pox_2_tests.rs

+    assert_eq!(alice_txs.len() as u64, 2);
+    assert_eq!(bob_txs.len() as u64, 5);
+
+    assert_eq!(


Would be helpful to possibly include the name of the associated error (from the pox contract) as part of the comments here

pavitthrap · 2022-09-13T17:07:45Z

src/chainstate/stacks/boot/pox_2_tests.rs

+
+    // submit an increase: this should fail, because Alice is not yet locked
+    let fail_no_lock_tx = alice_nonce;
+    let alice_increase = make_pox_2_increase(&alice, alice_nonce, 512 * POX_THRESHOLD_STEPS_USTX);


could you define 512 * POX_THRESHOLD_STEPS_USTX as a variable in this test too?

pavitthrap · 2022-09-13T17:11:14Z

src/chainstate/stacks/boot/pox_2_tests.rs

+    assert_eq!(alice_txs.len() as u64, alice_nonce);
+
+    // transaction should fail because lock isn't applied
+    assert_eq!(&alice_txs[&fail_no_lock_tx].result.to_string(), "(err 26)");


could also include error "name" in the comments

kantai added 6 commits August 30, 2022 11:20

initial implementation of stack-increase

1aca820

Merge branch 'feat/pox-2-unlock' into feat/pox-2-increase

3c83d5e

simple stack-increase test

d150a39

remove amount-ustx from stacking-state, implement delegate-stack-incr…

7d42acf

…ease

tests for stack-increase and delegate-stack-increase

984ba7d

Merge branch 'feat/pox-2-unlock' into feat/pox-2-increase

3af2e7a

jcnelson added the stacks-2.1 label Sep 8, 2022

zone117x mentioned this pull request Sep 9, 2022

[Stacks 2.1] Support Stacks 2.1 in @stacks/stacking (PoX-2) hirosystems/stacks.js#1347

Closed

4 tasks

kantai linked an issue Sep 9, 2022 that may be closed by this pull request

PoX Updates: stack-increase #2533

Closed

jcnelson reviewed Sep 9, 2022

View reviewed changes

jcnelson approved these changes Sep 9, 2022

View reviewed changes

add check to ensure a delegated stacker cannot invoke stack-increase

5541494

kantai added 2 commits September 12, 2022 13:55

Merge pull request #3260 from stacks-network/feat/pox-2-unlock

7db7efd

PoX 2 Unlock

Merge remote-tracking branch 'origin/next' into feat/pox-2-increase

6956f00

pavitthrap approved these changes Sep 13, 2022

View reviewed changes

address PR review feedback

ad1ffc1

kantai merged commit ef63920 into feat/pox-2-unlock Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.1: Feature: `stack-increase` for PoX-2 #3282

2.1: Feature: `stack-increase` for PoX-2 #3282

kantai commented Sep 7, 2022

codecov bot commented Sep 7, 2022 •

edited

Loading

zone117x commented Sep 9, 2022 •

edited

Loading

jcnelson Sep 9, 2022

jcnelson Sep 9, 2022

kantai Sep 9, 2022

jcnelson Sep 9, 2022

kantai Sep 9, 2022

jcnelson Sep 9, 2022

jcnelson Sep 9, 2022

jcnelson left a comment •

edited

Loading

jcnelson commented Sep 12, 2022

pavitthrap left a comment •

edited

Loading

pavitthrap Sep 12, 2022

pavitthrap Sep 12, 2022

pavitthrap Sep 12, 2022

pavitthrap Sep 12, 2022

pavitthrap Sep 12, 2022

pavitthrap Sep 13, 2022

pavitthrap Sep 13, 2022


		let mut snapshot = db.get_stx_balance_snapshot(principal);

		if !snapshot.has_locked_tokens() {

2.1: Feature: stack-increase for PoX-2 #3282

2.1: Feature: stack-increase for PoX-2 #3282

Conversation

kantai commented Sep 7, 2022

codecov bot commented Sep 7, 2022 • edited Loading

Codecov Report

zone117x commented Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson left a comment • edited Loading

Choose a reason for hiding this comment

jcnelson commented Sep 12, 2022

pavitthrap left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2.1: Feature: `stack-increase` for PoX-2 #3282

2.1: Feature: `stack-increase` for PoX-2 #3282

codecov bot commented Sep 7, 2022 •

edited

Loading

zone117x commented Sep 9, 2022 •

edited

Loading

jcnelson left a comment •

edited

Loading

pavitthrap left a comment •

edited

Loading