Stacks 2.1: epoch initialization #2639
Conversation
src/burnchains/mod.rs
Outdated
| /// Returns the PoX contract that is "active" at the given burn block height | ||
| pub fn active_pox_contract(&self, burn_height: u64) -> &'static str { | ||
| if burn_height >= (self.v1_unlock_height as u64) { | ||
| "pox-2" |
There was a problem hiding this comment.
Do we want to label the PoX contract by the Stacks major/minor version, or by the contract version? i.e. pox-2 vs pox-2.1? Either one is fine with me, just as long as it's consistent.
There was a problem hiding this comment.
My preference is by the contract version -- it's not the case that future versions of the Stacks chain (if they exist) will necessarily create new PoX contracts, so it'd be kind of confusing that Stacks 2.9 uses pox-2.4 (or whatever the versioning mismatch becomes).
There was a problem hiding this comment.
However, I could be convinced otherwise if people believe one or the other will create more confusion.
There was a problem hiding this comment.
Up to you. I'm fine with it as-is as long as we believe users won't get confused.
| @@ -282,7 +283,7 @@ pub struct BurnchainBlockHeader { | |||
| pub timestamp: u64, | |||
| } | |||
|
|
|||
| #[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] | |||
| #[derive(Debug, PartialEq, Clone)] | |||
There was a problem hiding this comment.
how does the dropping of serialization fit into this change?
There was a problem hiding this comment.
Serialize/Deserialize were never used for this struct, and including them would require that the new Account enum implement serialize.
| /// The auto unlock height for PoX v1 lockups before transition to PoX v2. This | ||
| /// also defines the burn height at which PoX reward sets are calculated using | ||
| /// PoX v2 rather than v1 | ||
| pub v1_unlock_height: u32, |
There was a problem hiding this comment.
why is it called "v1_unlock"? isn't it "v2_unlock"?
There was a problem hiding this comment.
It's v1 balances that all unlock, so that's "the height at which all v1 locks expire"
| /// | ||
| #[test] | ||
| fn test_simple_pox_lockup_transition_pox_2() { | ||
| let AUTO_UNLOCK_HT = 12; |
There was a problem hiding this comment.
is HT for "height"? maybe spell it out.
| burnchain.pox_constants.reward_cycle_length = 5; | ||
| burnchain.pox_constants.prepare_length = 2; | ||
| burnchain.pox_constants.anchor_threshold = 1; | ||
| burnchain.pox_constants.v1_unlock_height = AUTO_UNLOCK_HT + 25; |
There was a problem hiding this comment.
The simmed environment for these tests produces 25 blocks with no sortitions, which don't iterate the "tenure" count.
| burnchain.pox_constants.anchor_threshold = 1; | ||
| burnchain.pox_constants.v1_unlock_height = AUTO_UNLOCK_HT + 25; | ||
|
|
||
| let first_v2_cycle = burnchain |
There was a problem hiding this comment.
maybe you can just hard-code this number, instead of getting it as a function of the code that is kind of under test.
maybe you can hard-code it, and then have a separate check that "block_height_to_reward_cycle" also gets you back to the same number.
|
|
||
| let mut alice_reward_cycle = 0; | ||
|
|
||
| for tenure_id in 0u32..num_blocks { |
There was a problem hiding this comment.
I think this test would be easier to understand if it were not written in a for loop. Instead, it could be written as an alternating series of 1) execute a tenure, 2) check the relevant invariants after that tenure.
E.g.:
// Do tenure 1 by running code "code for tenure 1"
// Test what is true after tenure 1
// Do tenure 1 by running code "code for tenure 2"
// Test what is true after tenure 2
... etc
If it were done this way, without a loop, and with a more declarative syntax, I think it would have the following benefits:
- Easier to read - locality: Right now, the logic that changes the state is not near the logic that checks the state. So, I have to scroll back and forth and try to figure out what is being checked after each test. I think it would be easier to read if you could organize the code as a sequence of statements, where what is being checked is checked right after the relevant change.
- Easier to read - no thinking: This test has a lot of logic to understand what is tested when. It's confusing to have to do so much logic to read a test. The whole point of the test is to test the logic of the code under test, so it's harder to know what the test result should be if the test also has a lot of logic.
2)More fool-proof test: With so much logic going into the test, we run the risk of the test-logic having a bug.
There was a problem hiding this comment.
Yes, I agree with that. I'll unroll the test.
There was a problem hiding this comment.
Unrolled in 6c69759 -- I think it improved the clarity of the test.
| assert_eq!(alice_balance, 0); | ||
| } | ||
|
|
||
| // now, try to use PoX 2 contract. this should _fail_. |
There was a problem hiding this comment.
I'm not sure which of the asserts shows that this "failed".. is there one? If so, can a stronger connection be made with comments, e.g., does "failing" mean Alice has a balance of 0?
| assert_eq!(alice_txs.len(), 3, "Alice should have 3 confirmed txs"); | ||
| assert_eq!(bob_txs.len(), 2, "Bob should have 2 confirmed txs"); | ||
|
|
||
| assert!( |
There was a problem hiding this comment.
For each assert in this section, where you are checking transactions, can you put a quick comment about why we expect these things to happen?
| 4, | ||
| tip.block_height, | ||
| ); | ||
| let tip_index_block = peer.tenure_with_txs(&[alice_lockup], &mut coinbase_nonce); |
There was a problem hiding this comment.
looks pretty clean the way that you were able to use "tenure_with_txs".. is it possible to indicate, at least on some more instances, where the "counter" is? It's easier to get lost as teh number of iterations goes up.
… costs-2 contract is initialized
|
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. |
This PR implements a basic structure for doing contract initialization at the "beginning" of Epoch 2.1. Importantly, "beginning" here isn't a one-time event: any Stacks block that is the first Stacks block in its fork after the epoch must call the initialization method. Right now, the initialization method only reinitializes the original PoX contract as "pox-2", but it would eventually need to do more than just initialize the new PoX contract (e.g., initialize analysis storage for extension functions). This PR also changes the #2659 behavior so that Clarity version 2 default check uses the epoch transition application rather than using a burn db lookup.
The other things implemented in this PR is the start of the PoX 1 -> PoX 2 transition. Right now, it implements:
#2610 - automatic unlock for PoX 1 lockups before the PoX 2 reward cycles begin.
#2611 - instantiation of PoX 2 at the epoch boundary, and the transition to PoX 2 reward cycles after the v1 automatic unlock.
This PR adds an end-to-end test case for the epoch and PoX transitions.