Stacks 2.1: PoX 2 `stack-extend`, `delegate-stack-extend` #2755

kantai · 2021-06-30T19:32:35Z

This implements #2532 and fixes #2522 in a new pox-2 contract.

Test coverage is supplied through two testing structures.

contract_tests contains "unit" tests for these new functions and fixes. This tests the contract purely using a Clarity VM -- so it can check how the contract itself is working, but not how it interacts with the SortitionDB / ChainsCoordinator when constructing reward sets.

test_pox_extend_transition_pox_2 in the pox_2_tests module does integration testing for extension across the epoch boundary -- testing that a user that locks via pox in Epoch 2.0 can stack-extend in pox-2 in Epoch 2.1.

jcnelson · 2021-07-02T04:13:15Z

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

+                (err ERR_STACKING_PERMISSION_DENIED))
+
+      ;; tx-sender principal must not be stacking
+      (asserts! (is-none (get-stacker-info tx-sender))


This code doesn't look any different from .pox -- in particular, the get-stacker-info code is the same, as are the checks here. Were any changes made in .pox-2 from .pox, or is the goal of this PR just to add stack-extend and delegate-stack-extend without e.g. implementing continuous stacking?

The only changes to PoX made in this PR are #2532 (stack-extend) and #2522 (fixing the contract allowance expiration).

stack-extend is how continuous stacking is implemented -- a caller that is currently locked can call extend to lengthen their lockup and "restack" without the current cooldown period.

@kantai would the following use case supported by this?

Arkadiko has a smart contract where users deposit their STX tokens to borrow our stablecoin against that STX position (called an Arkadiko vault)

As soon as the Arkadiko contract has the minimum amount of uSTX, we will call stack-stx with all the STX from vaults that signalled to stack in PoX (they all arrive in 1 address, so we're not really a pool but similar, we group STX tokens)

Whenever a new vault (i.e. new STX is deposited) is created or destroyed (i.e. STX gets withdrawn), we can call stack-extend to increase/decrease the amount of STX stacked in PoX through our contract.

At the end of each cycle, the Arkadiko address receives the btc

Any thoughts?

That use case is intended to be serviced via the stack-increase #2533, but it seems likely that the use case would also want to use stack-extend and (potentially) stack-decrease.

Nice, great to hear that this will be possible. In the current implementation, we always stack 1 cycle and then have a cool down cycle, so there's no optimal usage of PoX & bitcoin yield within Arkadiko yet.

Stacks 2.1 is going to be a big improvement on this then 👍

Back to Jude's question.. could this have been written as a module, that didn't copy-and-paste code, but instead just adds some new code?

Back to Jude's question.. could this have been written as a module, that didn't copy-and-paste code, but instead just adds some new code?

I think this could be interpreted in two ways. First, could this have been written as a contract that just adds the new code? The answer is "yes" but with a huge caveat: the new contract would need to share its data-space with the original PoX contract -- i.e., it would need direct access to all the underlying data-var, data-map, etc. structures of the original PoX contract. This is problematic for a couple reasons. Firstly, it only works if those structures don't need schema changes. This is true for stack-extend, but once we try introducing things like allowing unlocks or stacking increases, we either need to do schema changes, or introduce new auxiliary data-structures that would need to be updated/maintained through contract-call interposition, etc. Secondly, it would require special-casing the data-structure lookup code in the VM so that the new contract used the old contract's data-space. This is possible, but would introduce an otherwise unnecessary potential source for a pretty scary class of bugs (contracts that use prior contracts' data-spaces).

I think the second way to interpret this is as follows: could this have been written as a contract that just includes the code of the prior contract as a rust literal string include? The answer here is "probably", but I don't think it'd be the best way to implement the new contract. Doing that would obscure the contract implementation, and if the original functions end up being changed, it'd be hard to follow exactly how they're being changed (you'd need to mentally track the changes to the string inclusion).

Back to Jude's question.. could this have been written as a module, that didn't copy-and-paste code, but instead just adds some new code?

I was not asking this. I very much agree with @kantai that having .pox-2 be the same as .pox plus the new code is a lot less error-prone since you don't have to share data-spaces between contracts.

I was asking specifically if stack-stx needed to change to accommodate continuous stacking. The answer is "no."

jcnelson · 2021-07-02T04:13:55Z

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

+)
+
+
+(define-public (stack-extend (extend-count uint)


Can you add a method-level comment block to describe this new method, and indicate that it's a new change in 2.1?

It probably makes more sense to document the method in the docs module, but I can add a marker here to indicate that it is new in 2.1.

the other methods are documented in this file

Added documentation for these methods in bbd826d

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

jcnelson · 2021-07-02T04:18:38Z

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

+                (err ERR_STACKING_PERMISSION_DENIED))
+
+      ;; tx-sender must be locked
+      (asserts! (> amount-ustx u0)


Shouldn't we also be checking that this meets the minimum amount? I don't see a call to can-stack-stx, for example.

More generally -- do we want to permit someone to stack-extend their locked STX indefinitely, even if the quantity stacked becomes smaller than the minimum amount? I don't think it matters much either way from a technical standpoint, but it does affect how the user finds out that they won't clinch a reward address -- they can either get an error here, or they can wait until a reward cycle completes where they have no reward slots.

I'm not sure if it ultimately makes much difference in terms of alerting the user -- the can-stack-stx check is performed at the time of the extension, so if the liquid supply of STX increases during their stacking (e.g., they stack for 12 cycles, but the supply increases during cycle 6), they wouldn't be alerted either way.

The biggest difference is in how the user needs to deal with the consequences of this. In any event, to meet the new threshold, they will need to invoke stack-increase to increase their lock and stacking amount. However, if the can-stack-stx check is in place, then it imposes an ordering constraint: they would need to invoke stack-increase before extending.

I think it probably does make sense to add the check here, as it seems the least surprising behavior.

Added can-stack-stx and minimal-can-stack-stx checks to stack-extend and delegate-stack-extend respectively.

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

jcnelson · 2021-07-02T04:27:15Z

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

+         (first-extend-cycle (burn-height-to-reward-cycle unlock-height))
+         (last-extend-cycle  (- (+ first-extend-cycle extend-count) u1))
+         (cur-cycle (current-pox-reward-cycle))
+         (lock-period (- last-extend-cycle cur-cycle))


This can't underflow either, right?

I'm not so worried about an underflow -- it would just abort the tx with a runtime error. But we should check to make sure that extend-count is positive and first-extend-cycle is in the future.

That's fair.

Checks added in bbd826d

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

gregorycoppola

Hey Aaron.. looks good but had some misc comments/questions.

src/chainstate/stacks/boot/mod.rs

src/vm/docs/mod.rs

src/vm/functions/special.rs

src/chainstate/stacks/boot/contract_tests.rs

src/chainstate/stacks/boot/pox_2_tests.rs

gregorycoppola

Huge change. I think it looks pretty good.. trying to follow along with what is tested, and make sure each of the edge cases in the ".clar" file is tested.

src/chainstate/stacks/boot/pox_2_tests.rs

gregorycoppola · 2021-08-06T19:48:18Z

src/burnchains/burnchain.rs

@@ -504,13 +504,22 @@ impl Burnchain {
        self.first_block_height + reward_cycle * (self.pox_constants.reward_cycle_length as u64) + 1


Do the implementations of #2532 and #2522 have to be done in the same PR?

They don't absolutely have to be, but it seemed easy to address #2522 when pox-2 code was first introduced.

src/vm/functions/special.rs

gregorycoppola · 2021-08-06T21:12:54Z

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

+)
+
+
+(define-public (stack-extend (extend-count uint)


the other methods are documented in this file

gregorycoppola · 2021-08-06T21:13:50Z

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

+         (new-unlock-ht (reward-cycle-to-burn-height (+ u1 last-extend-cycle))))
+
+      ;; must be called directly by the tx-sender or by an allowed contract-caller
+      (asserts! (check-caller-allowed)


do these various asserts have corresponding tests?

Yes, check-caller-allowed is tested via unit tests in pox_2_contract_caller_units.

gregorycoppola · 2021-08-06T21:15:09Z

src/chainstate/stacks/mod.rs

@@ -228,6 +229,9 @@ impl fmt::Display for Error {
            Error::MemPoolError(ref s) => fmt::Display::fmt(s, f),
            Error::NoTransactionsToMine => write!(f, "No transactions to mine"),
            Error::PoxAlreadyLocked => write!(f, "Account has already locked STX for PoX"),
+            Error::PoxExtendNotLocked => {
+                write!(f, "Account has not already locked STX for PoX extend")


newbie question: is "stacking" == "locking"?

yes, in this context

src/vm/database/structures.rs

src/burnchains/burnchain.rs

src/chainstate/stacks/boot/pox_2_tests.rs

src/vm/functions/special.rs

src/chainstate/stacks/boot/pox_2_tests.rs

jcnelson · 2021-08-19T00:39:18Z

src/chainstate/stacks/boot/contract_tests.rs

+            .0
+            .to_string(),
+            "(err 2)".to_string()
+        );


Just be sure to add a test in here to make sure that you can't extend "into the past". stack-extend should only allow you to extend into the future.

Added a test in e0f6077 that covers the closest to this case that I think is possible -- it attempts to call stack-extend after the lock for a user expired (that would be the only way to extend into the past). This case is caught early in invocation, because the user has no locked up funds.

src/chainstate/stacks/boot/pox_2_tests.rs

…ck to delegate-stack-extend

kantai · 2021-08-25T15:35:12Z

Thanks for the reviews -- the existing comments have been addressed, and this PR is ready for review again.

gregorycoppola

Seems OK as far as I can tell at this point.

The notes I put about the unit tests can be ignored if not appropriate... I just tried to be exhaustive in listing them since we don't have the UI for it in GitHub anymore.

gregorycoppola · 2021-08-10T16:01:50Z

src/chainstate/stacks/db/accounts.rs

+        let mut snapshot = db.get_stx_balance_snapshot(principal);
+
+        if !snapshot.has_locked_tokens() {
+            return Err(Error::PoxExtendNotLocked);


this line isn't covered by unit tests

That's not surprising -- this line shouldn't be reachable because the PoX smart contract checks that the account has locked tokens before getting to the line. This check is just defensive.

gregorycoppola · 2021-08-10T16:04:35Z

src/vm/database/structures.rs

+
+        if !self.has_locked_tokens() {
+            // caller needs to have checked this
+            panic!("FATAL: account does not have locked tokens");


not covered by unit tests.. kind of a drawback of panicing instead of InternalError is that you can't test this case

I learned you actually can test panics in rust actually!.. not that you have to test this

https://stackoverflow.com/questions/26469715/how-do-i-write-a-rust-unit-test-that-ensures-that-a-panic-has-occurred

True, but in a lot of cases (like this one), we use the panic because this line should be unreachable.

gregorycoppola · 2021-08-10T16:05:45Z

src/vm/functions/special.rs

+                        ));
+                    }
+                }
+                Err(ChainstateError::DefunctPoxContract) => {


not tested in unit tests

I don't think this error case is reachable -- parse_lock_extend_v2 doesn't throw DefunctPoxContract.

gregorycoppola · 2021-08-10T16:06:18Z

src/vm/database/structures.rs

+
+        if unlock_burn_height <= self.burn_block_height {
+            // caller needs to have checked this
+            panic!("FATAL: cannot set a lock with expired unlock burn height");


not covered by unit tests.. kind of a drawback of panicing instead of InternalError is that you can't test this case

I learned you actually can test panics in rust actually!.. not that you have to test this

https://stackoverflow.com/questions/26469715/how-do-i-write-a-rust-unit-test-that-ensures-that-a-panic-has-occurred

True, but in a lot of cases (like this one), we use the panic because this line should be unreachable.

gregorycoppola · 2021-08-10T16:08:00Z

src/clarity_vm/clarity.rs

@@ -697,9 +705,9 @@ impl<'a> ClarityBlockConnection<'a> {
            // instantiate PoX 2 contract...

            let pox_2_code = if mainnet {
-                &*BOOT_CODE_POX_MAINNET
+                &*POX_2_MAINNET_CODE


this line isn't covered by unit tests.. could be worth testing.

Perhaps, but I think that we would need a separate PR to do this -- this is a general issue in the codebase that there is no testing infrastructure to exercise the mainnet conditions (because in general, when testing modes are enabled, every such branch is testnet).

gregorycoppola · 2021-08-26T02:04:25Z

src/vm/functions/special.rs

+                Err(ChainstateError::DefunctPoxContract) => {
+                    return Err(Error::Runtime(RuntimeErrorType::DefunctPoxContract, None))
+                }
+                Err(e) => {


can we put this answer in the comment?

gregorycoppola · 2021-08-26T02:04:36Z

src/vm/functions/special.rs

+
+            return Ok(());
+        } else {
+            // nothing to do -- the function failed


can we put this answer in the comment?

gregorycoppola · 2021-08-26T02:05:53Z

src/chainstate/stacks/boot/pox_2_tests.rs

@@ -486,3 +489,938 @@ fn test_simple_pox_lockup_transition_pox_2() {
        "Charlie tx0 should have committed okay"
    );
 }
+
+/// In this test case, two Stackers, Alice and Bob stack and interact with the


I like this explanation. Thanks.

gregorycoppola · 2021-08-26T02:19:51Z

src/chainstate/stacks/boot/pox_2_tests.rs

+        tip.block_height,
+    );
+
+    let tip_index_block = peer.tenure_with_txs(&[alice_lockup], &mut coinbase_nonce);


Where is the run-time error?

It's checked in the receipts at line 851 -- transactions with runtime errors still get included in the block, but they just fail with a runtime error. The transaction receipt doesn't capture the actual runtime error thrown, but that's a deficiency in our transaction receipt format.

gregorycoppola · 2021-08-26T02:35:38Z

Thanks for an ambitious PR by the way!

pavitthrap

lgtm, just had a few minor comments!

pavitthrap · 2021-08-26T17:35:33Z

src/chainstate/stacks/boot/contract_tests.rs

+            .0
+            .to_string(),
+            "(err 9)".to_string(),
+            "After revocation, stack-through shouldn't be an allowed caller for POX_ADDR[0] in the PoX2 contract",


POX_ADDR[0] -> USER_KEYS[0]?

src/chainstate/stacks/boot/contract_tests.rs

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

pavitthrap · 2021-08-27T18:50:06Z

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

+
+      ;; standard can-stack-stx checks
+      (try! (can-stack-stx pox-addr amount-ustx first-extend-cycle lock-period))
+


Do we want to call check-pox-lock-period in stack-extend?

It isn't necessary -- the lock-period is checked by can-stack-stx

kantai added 7 commits June 18, 2021 13:11

initial impl of stack-extend

5587e79

instantiate pox-2

bfc03cb

working pox-extend integration test, need more coverage though

c6efa4b

feat: add delegate-stack-extend, fix #2522

57de82c

test: unit tests for stack-extend

58c8397

test: fix for PoX contract-caller check

e814daa

test: coverage for delegate-stack-stx

ad5e8cc

jcnelson reviewed Jul 2, 2021

View reviewed changes

src/chainstate/stacks/boot/pox-2.clar Show resolved Hide resolved

jcnelson reviewed Jul 2, 2021

View reviewed changes

src/chainstate/stacks/boot/pox-2.clar Outdated Show resolved Hide resolved

jcnelson reviewed Jul 2, 2021

View reviewed changes

src/chainstate/stacks/boot/pox-2.clar Outdated Show resolved Hide resolved

jcnelson reviewed Jul 2, 2021

View reviewed changes

src/chainstate/stacks/boot/pox-2.clar Show resolved Hide resolved

jcnelson reviewed Jul 2, 2021

View reviewed changes

src/chainstate/stacks/boot/pox-2.clar Show resolved Hide resolved

Merge branch 'feat/stacks-2.1-initialization' into feat/pox-2-extend

caa1bfb

jcnelson requested review from pavitthrap, gregorycoppola, lgalabru and reedrosenbluth July 12, 2021 15:12

gregorycoppola reviewed Jul 12, 2021

View reviewed changes

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

ca8ae82

gregorycoppola reviewed Aug 6, 2021

View reviewed changes

pavitthrap reviewed Aug 16, 2021

View reviewed changes

jcnelson reviewed Aug 19, 2021

View reviewed changes

src/chainstate/stacks/boot/pox_2_tests.rs Show resolved Hide resolved

kantai changed the base branch from feat/stacks-2.1-initialization to next August 20, 2021 16:19

kantai added 9 commits August 20, 2021 11:21

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

e697d59

first pass of PR feedback

bbd826d

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

1759672

fix tests for changes from merge of origin/next

02bd93c

unroll lock extend test

4a5677c

test: add delegate-stack-extend across PoX1/2 boundary in pox_2_tests

20f156f

more PR feedback

199ce41

add test case for attempted extend on expired lock, can-stack-stx che…

e0f6077

…ck to delegate-stack-extend

address last PR feedback, upgrade code-cov gh action

1b97e6c

kantai requested review from jcnelson, pavitthrap and gregorycoppola August 25, 2021 15:34

gregorycoppola approved these changes Aug 26, 2021

View reviewed changes

kantai added 2 commits August 26, 2021 08:23

more PR feedback

a4fddd5

Merge branch 'next' into feat/pox-2-extend

ac2cb95

pavitthrap approved these changes Aug 27, 2021

View reviewed changes

PR feedback

0a6bf7a

kantai merged commit 7b7d652 into next Aug 27, 2021

This was referenced May 23, 2022

PoX Updates: stack-extend and delegate-stack-extend #2532

Closed

pox contract: parameter in allow-contract-caller for upper limit does not work #2522

Closed

This was referenced Aug 17, 2022

[Stacks 2.1] Support PoX-2 for account STX balance locked state hirosystems/stacks-blockchain-api#1277

Closed

[Stacks 2.1] Support PoX-2 in Rosetta stacking operations hirosystems/stacks-blockchain-api#1278

Closed

zone117x mentioned this pull request Aug 29, 2022

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

Closed

4 tasks

		@@ -504,13 +504,22 @@ impl Burnchain {
		self.first_block_height + reward_cycle * (self.pox_constants.reward_cycle_length as u64) + 1


		;; standard can-stack-stx checks
		(try! (can-stack-stx pox-addr amount-ustx first-extend-cycle lock-period))

Stacks 2.1: PoX 2 stack-extend, delegate-stack-extend #2755

Stacks 2.1: PoX 2 stack-extend, delegate-stack-extend #2755

Conversation

kantai commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philiphacks Jul 15, 2021 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorycoppola left a comment

Choose a reason for hiding this comment

gregorycoppola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantai Aug 20, 2021 • 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

kantai commented Aug 25, 2021

gregorycoppola left a 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

kantai Aug 26, 2021 • 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

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

gregorycoppola commented Aug 26, 2021

pavitthrap left a 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

Stacks 2.1: PoX 2 `stack-extend`, `delegate-stack-extend` #2755

Stacks 2.1: PoX 2 `stack-extend`, `delegate-stack-extend` #2755

philiphacks Jul 15, 2021 •

edited

Loading

kantai Aug 20, 2021 •

edited

Loading

kantai Aug 26, 2021 •

edited

Loading