DRAFT: Epoch 2.2, address PoX stack-increase issue (SIP-022) #3674
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3674 +/- ##
===========================================
+ Coverage 0.18% 31.54% +31.35%
===========================================
Files 298 299 +1
Lines 275558 277118 +1560
===========================================
+ Hits 512 87409 +86897
+ Misses 275046 189709 -85337
... and 192 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -105,6 +106,7 @@ pub const BITCOIN_MAINNET_FIRST_BLOCK_HASH: &str = | |||
| pub const BITCOIN_MAINNET_INITIAL_REWARD_START_BLOCK: u64 = 651389; | |||
| pub const BITCOIN_MAINNET_STACKS_2_05_BURN_HEIGHT: u64 = 713_000; | |||
| pub const BITCOIN_MAINNET_STACKS_21_BURN_HEIGHT: u64 = 781_551; | |||
| pub const BITCOIN_MAINNET_STACKS_22_BURN_HEIGHT: u64 = 787_700; | |||
There was a problem hiding this comment.
The SIP mentions block 787551. Either seems okay to me, but we should make sure they match.
There was a problem hiding this comment.
Yeah, I agree. I'll leave this thread open / unresolved until we have a conclusion there. I just wanted to put down something approximating the latest possible block in the PR.
kantai
force-pushed
the
feat/address-pox-increase
branch
from
b138c48 to
c6339e1
Compare
|
Re Solo Lock == Stacked Amount Assumption After the locking period, the locked amount is still the amount of stacked tokens of the previous cycle for 100 blocks. While the stacked amount is 0 for the cycle, assuming the user did not extend. |
Right -- the I'm interested in hearing more about the "different pox reward address" case -- how do users trigger this? is it via EDIT: I see (https://app.sigle.io/friedger.id/5zj_niUL0z0qBEl9HO2Ac) -- that answers my question. Yes,
The codebase considers the 100 prepare phase blocks as part of the previous cycle, so the "current cycle amount" for the user would still have the stacked amount = locked amount. |
|
Can't we have pox-3 that resets the total? |
I think it's worth exploring what that design would look like. One downside is that I believe this would require cycle 58 to be an all burn cycle. Unlike activation of 2.1, it wouldn't be safe to use the reward set computed by pox-2 (due to the runtime assertion issue raised in SIP-022). |
|
Can we initialize pox-3 with the locked amount from the txs, not from the reward sets? |
| exec_env | ||
| .global_context | ||
| .database | ||
| .set_entry_unknown_descriptor( | ||
| &boot_code_id(POX_2_NAME, mainnet), | ||
| "reward-cycle-pox-address-list", | ||
| key.into(), | ||
| value.into(), | ||
| ) | ||
| .expect("FATAL: Failed to update chainstate db"); |
There was a problem hiding this comment.
Will you do the same for my imaginary contract if I'll ask you? What if its logic depends on pox2 data, and this change will make it impossible to withdraw imaginary tokens from this contract?
Are you willing to take the same measure in the future? Are you willing to do the same if someone exploit big project like ALEX or Arkadiko?
What if I don't like someone but that person have a lot of STX? Can you use this approach to transfer let's say half of that person balance to my account - it wouldn't be difficult, right? Pleaseee!!
In short: What justify such radical approach? Why you want to mess with on-chain data and make it impossible to regenerate it by re-playing all transactions from the genesis?
Who and based on what defines the terms on which this "method" can or can't be used? If you'll do it once, what's gonna stop you from doing it again?
Why you're not upfront about what this PR propose and explain in simple words (so that everyone could understand it) that you want to alter the on-chain data in such way that it will be impossible to trace when or how it happened, because there won't be any trace of it in any block or transaction?
In other words it will be like transferring STX from one account to another without transaction - simply hard-coded in node code.
Stacks - A Bitcoin* layer for smart contracts.
*Secured by Bitcoin. Yes, we changed the blockchain data with github PR, but we did it only once. Pinky promise.
Jokes and sarcasm aside - deploy PoX-3 contract, enable burn cycle if it is necessary and move on.
Once you tamper on-chain data with PR you'll open a Pandora's box and you'll ruin all the trust you've been trying to build.
Next time do better, don't rush, test everything thoroughly (I can't find a single test for PoX-2 contract) and postpone release if that's necessary to triple check everything.
There was a problem hiding this comment.
Why you're not upfront about what this PR propose and explain in simple words (so that everyone could understand it) that you want to alter the on-chain data in such way that it will be impossible to trace when or how it happened, because there won't be any trace of it in any block or transaction?
In other words it will be like transferring STX from one account to another without transaction - simply hard-coded in node code.
I believe this PR is pretty up-front about what it is doing. But, this PR is probably not the right forum to discuss the hard-fork resolution. That would be the SIP, where, rather than having to talk about what the code is doing, the discussion can be about the spec for the code. This PR is simply trying to implement what is proposed in that SIP. If the SIP process goes a different route, then this PR will either be closed or changed to follow what is proposed there.
Secondly, none of the changes in this PR are impossible to trace -- the changes here are applied using state read directly out of the node's chain state. Anyone can run a node that verifies this state. pox-2 data is already modified by the stacks-node without corresponding transactions: auto-unlocks alter the contract map state, and interacts with the rest of the node in other special ways (i.e. the special module). I do agree, though, that other options would avoid modifying the contract state beyond what is already done. I would suggest again the SIP process.
Deploy PoX-3 contract, enable burn cycle if it is necessary and move on.
Ultimately, any hard fork performs unusual modification of blockchain state. Enabling and switching to pox-3 would still require this. There's key questions about the feasibility of different approaches, and I'd encourage open dialogue about the approaches. However, this PR is probably a less visible location than others, and it's easier for more people to weigh in on the proposal.
There was a problem hiding this comment.
@LNow as kantai mentioned, stacksgov/sips#127 is a better place to discuss the why. we should leave this PR for the work related to that proposal.
|
Note for reviewers: besides testing, this PR is also functionally incomplete -- it needs to also disable PoX auto-unlock (context on Discord). For reference, "auto unlock" refers new behavior introduced in Stacks 2.1 where any locked STX that don't qualify for a reward cycle (say due to the threshold changing), they would automatically unlock. |
| @@ -178,6 +178,7 @@ use stacks::codec::StacksMessageCodec; | |||
| use stacks::core::mempool::MemPoolDB; | |||
| use stacks::core::FIRST_BURNCHAIN_CONSENSUS_HASH; | |||
| use stacks::core::STACKS_EPOCH_2_1_MARKER; | |||
There was a problem hiding this comment.
This line can be deleted to fix a warning.
|
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. |
Description
This PR addresses the PoX stack-increase issue (SIP-022) by introducing a new 2.2 epoch. The changes are:
pox-2.stacks-increaseruntime aborts in Epoch 2.2This PR is a draft as it still needs test coverage:
Unit test to ensure that the epoch transition catches and correctly fixes PoX reward sets
pox_2_tests::epoch_2_2_stack_increasewhich checks a simple case of two stackers, where one invokes stacks increase.Unit test to ensure that the epoch transition correctly fixes all PoX reward sets which could be used after the transition (i.e., if the PoX anchor block selected is exactly EPOCH_2_2 transition height)
pox_2_tests::epoch_2_2_stack_increasechecks reward cycles at exactly the transition height, which means if that block was chosen as the anchor, the reward cycle data would correspond to the checked data in that method.End-to-end (bitcoin regtest) test case that applies the epoch transition and fixes the PoX reward set
tests::epoch_22::pox_2_stack_increase_epoch22_fixwhich transitions through epochs 2.1 and 2.2. It executes stack-increase in 2.1 in order to exhibit the buggy behavior, and then asserts that after 2.2 begins, the reward set distribution is corrected.End-to-end test case that applies the epoch transition and demonstrates that stacks-increase always runtime aborts
tests::epoch_22::pox_2_stack_increase_epoch22_fixtestsstack-increasein epoch 2.2 and asserts that it aborts.Unit test to ensure that a SortitionDB is correctly "migrated" (the schema doesn't change, but the epochs table must be refreshed)
Unit (?) or E2E test case that ensures the PoX anchor block selection cannot select an Epoch 2.1 block after Epoch 2.2 activates
Epoch 2.2 height
This PR also selects an Epoch 2.2 height:
This is 50 blocks before the start of the next reward cycle's prepare phase. In order for these changes to be applied before the next reward cycle, I think this is close to the latest block height.
Open Testing Questions
Here I'm listing some potential concerns that I have some uncertainty about the best way to test. I am interested in input and help here!
Solo Lock == Stacked Amount Assumption
One of the assumptions that this fix makes is: for every solo stacker, their current account lock should equal their "stacked" amount in the reward set list. This is not true when looking at current reward cycle: when you invoke
stack-stx, you are immediately locked, but not added to the current cycle. The first cycle you are added to is the next cycle.The commit d0c281b addressed this specific case by changing the epoch transition behavior such that it starts corrections at the next cycle (it also panics if Epoch 2.2's start height is a reward cycle boundary to avoid an off-by-one issue here).
This is the only case where this assumption breaks that is obvious to me, but I'd like some help in thinking about other ways this assumption (which becomes an assertion at the transition) could fail.