Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/check pox 2 params #3409

Merged
merged 66 commits into from
Dec 13, 2022
Merged

Feat/check pox 2 params #3409

merged 66 commits into from
Dec 13, 2022

Conversation

jcnelson
Copy link
Member

@jcnelson jcnelson commented Nov 18, 2022

This PR adds a config method that will validate the PoX 2 and epoch parameters. Specifically:

  • v1_unlock_height must be strictly after the epoch 2.1 start height, and cannot fall on a reward cycle start boundary if it's in the subsequent reward cycle from the epoch 2.1 start.
  • If v1_unlock_height falls in the same reward cycle as the epoch 2.1 start height, then the epoch 2.1 start height must be in the reward phase -- ideally, the very first block.

The reasons for this are that the v1_unlock_height value is used to determine which PoX contract to query to determine a reward cycle's reward set. So, pox-2 must be instantiated before the PoX anchor block for its first reward cycle is mined. This also means that v1_unlock_height must not be at the very beginning of a reward cycle -- reward cycles start at the first block, not the zeroth block, of the reward cycle, so the reward cycle of a v1_unlock_height that fell on a sortition height such that sortition_height % reward_cycle_len == 0 would be in the preceding reward cycle.

This PR makes the both the mocknet and Neon nodes panic early if these conditions are not met.

I'm going to see if all integration tests continue to pass with the settings we've given them. But in the mean time, feel free to try this out. This is meant to address #3407.

EDIT: this PR now also fixes #3367.

Copy link
Contributor

@wileyj wileyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shipit

Copy link
Contributor

@deantchi deantchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Nov 19, 2022

Codecov Report

Merging #3409 (b2d886a) into next (ce61513) will increase coverage by 23.04%.
The diff coverage is 51.71%.

@@             Coverage Diff             @@
##             next    #3409       +/-   ##
===========================================
+ Coverage   28.58%   51.63%   +23.04%     
===========================================
  Files         284      229       -55     
  Lines      260447   121772   -138675     
===========================================
- Hits        74439    62872    -11567     
+ Misses     186008    58900   -127108     
Impacted Files Coverage Δ
clarity/src/vm/errors.rs 29.68% <ø> (-9.21%) ⬇️
src/chainstate/stacks/db/mod.rs 83.81% <0.00%> (+14.19%) ⬆️
src/clarity_vm/special.rs 22.82% <0.00%> (-24.89%) ⬇️
src/net/mod.rs 43.82% <ø> (+25.59%) ⬆️
testnet/stacks-node/src/tests/epoch_205.rs 12.03% <0.00%> (-55.79%) ⬇️
testnet/stacks-node/src/tests/epoch_21.rs 0.00% <0.00%> (-68.03%) ⬇️
testnet/stacks-node/src/tests/integrations.rs 0.00% <0.00%> (-41.33%) ⬇️
testnet/stacks-node/src/tests/mod.rs 44.93% <18.18%> (-9.77%) ⬇️
src/burnchains/db.rs 71.35% <23.52%> (-1.42%) ⬇️
src/net/relay.rs 60.55% <37.03%> (+38.77%) ⬆️
... and 251 more

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

@jcnelson
Copy link
Member Author

jcnelson commented Dec 2, 2022

This PR now addresses #3367. It does so by pushing the boot receipts immediately after instantiating the chainstate, but before any bitcoin block header synchronization happens. To do this, it pushes the events attached to the true genesis block -- the one with height 0.

@zone117x
Copy link
Member

zone117x commented Dec 6, 2022

@jcnelson can you add a check to one of the TestEventObserver tests to ensure the pox-2 tx is emitted? I'm now only seeing the costs-3 tx in the epoch2.1 /new_block payload. I've tested with several different block height toml configurations, and with & without performing a stacks-node restart.

The pox-2 contract does exist when querying the /v2/data_var/ST000000000000000000002AMW42H/pox-2/configured endpoint, and it can be called from contract-call txs. It's just not showing up in the event stream.

@jcnelson
Copy link
Member Author

jcnelson commented Dec 6, 2022

@zone117x Try now. There was a bug in the epoch initialization code in which it didn't return the tx receipts for both costs-3 and pox-2.

@zone117x
Copy link
Member

zone117x commented Dec 6, 2022

Thanks for the quick fix @jcnelson, the pox-2 tx is being emitted now. I'm working on ingesting the new "block 0" data to ensure there are no breaking changes and will get back to you here when done.

Copy link
Member

@zone117x zone117x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the following changes from this PR and looks good:

@diwakergupta diwakergupta linked an issue Dec 7, 2022 that may be closed by this pull request
@jcnelson jcnelson merged commit 1a0883b into next Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants