stacks-network / stacks-core

The Stacks blockchain implementation
https://docs.stacks.co
GNU General Public License v3.0
3.01k stars 667 forks source link

Update pox_4_tests to run in 2.5 and 3.0 #5072

Closed hstove closed 1 month ago

hstove commented 1 month ago

To help catch edge cases related to Nakamoto, this PR updates the pox_4_tests test suite to run tests in both epoch 2.5 and 3.0. Previously, all tests only ran in 2.5.

This PR also includes the work from @kantai to add consistent behavior to mod 0 blocks in Nakamoto, which was the cause of a previous reward cycle calculation bug. That work is included in https://github.com/stacks-network/stacks-core/pull/5072/commits/1d836175730eff88eaec9440c608267eb6303494.

This PR adds a nakamoto_cases rstest template, so that tests will run twice, and they're provided a use_nakamoto parameter. Certain setup functions have been updated (such as prepare_pox4_test) to utilize this argument. When use_nakamoto is true, the setup function utilizes the NakamotoBootPlan struct to boot into Nakamoto. Using NakamotoBootPlan adds things like automatically stacking a single signer and uses the Nakamoto VRF proof when mining blocks.

For the most part, the tests themselves are not updated. There are certain instances, such as expecting a reward set with 1 signer, which are updated to expect 2 signers when in epoch 3.0.

I've also added a helper function, tenure_with_txs, which mimicks the peer.tenure_with_txs function. In 3.0, this function creates a tenure with a Nakamoto block that includes the transactions.

hstove commented 1 month ago

Ok @obycode and @jcnelson , this is ready for review. There are really two separate things to review here:

hstove commented 1 month ago

@jcnelson regarding your points:

I think you forgot to update getpoxinfo.rs. It always uses reward_cycle_to_block_height(), but should probably use nakamoto_first_block_of_cycle if the current burnchain height is in epoch 3.0.

reward_cycle_to_block_height considers the modulo 0 block to be the start of the reward cycle, so I think this is already consistent. nakamoto_first_block_of_cycle, is the "reverse" operation, but it also uses the modulo 0 block height.

Similarly, I see that getstackers.rs continues to use reward_cycle_to_block_height in epoch 3.0

I'm a little unsure of how this should be handled. This function is only used to verify that pox-4 is the current active PoX contract, and it returns an error otherwise. From 2.5 onwards, this will simply pass.

If the current burn height is 40, and 2.5 activates at 41, what is the expected behavior here (assuming 20 block cycles)? Should it error?

jcnelson commented 1 month ago

reward_cycle_to_block_height considers the modulo 0 block to be the start of the reward cycle, so I think this is already consistent. nakamoto_first_block_of_cycle, is the "reverse" operation, but it also uses the modulo 0 block height.

Yes, you're right. I confused those two in my head.

I'm a little unsure of how this should be handled. This function is only used to verify that pox-4 is the current active PoX contract, and it returns an error otherwise. From 2.5 onwards, this will simply pass.

If the current burn height is 40, and 2.5 activates at 41, what is the expected behavior here (assuming 20 block cycles)? Should it error?

I think for getstackers.rs, we'd want to use nakamoto_first_block_of_cycle because that's when the signers' tenure starts. However, you're also right that it doesn't really matter since the PoX 4 contract has been active for multiple cycles already.

jferrant commented 1 month ago

I am not 100% positive where specifically but the very first commit of this branch introduces a database lock (at least I get a ton of logs about this) that seems to break the end_of_tenure test.

jcnelson commented 1 month ago

This tentatively has my approval but let's find out why there's a failing CI test.

jferrant commented 1 month ago

I am not 100% positive where specifically but the very first commit of this branch introduces a database lock (at least I get a ton of logs about this) that seems to break the end_of_tenure test.

I can confirm it has nothing to do with the above ^