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

Add epoch 3.0 #3954

Closed
wants to merge 3 commits into from
Closed

Add epoch 3.0 #3954

wants to merge 3 commits into from

Conversation

cylewitruk
Copy link
Member

@cylewitruk cylewitruk commented Sep 20, 2023

This PR aims to add the boilerplate code around Epoch 3.0 and Clarity 3.0 so that teams can start building feature gating as needed based on these.

I've made some assumptions, and I've added some TODO's and even todo!()'s in areas where it was either unclear, or obvious that something would need to be done (e.g. in chainstate where I know there's big changes coming).

If we can get this PR in an "OK" state for next, and make sure we have Github issues for each todo (ideally that we even add the issue number in a comment next to the todo, give me the issue number and I can do this), then I hope the chances of forgetting about any of those will be slim :)

So, please have a look through the todos and comment those which you can/will take and a link to the Github issue to track it. If I've missed any, please also comment where I've missed (or if you would like me to add a todo somewhere for you) and I'll add them :)

Keeping this as a draft for now.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #3954 (83de74f) into next (e17f094) will decrease coverage by 0.01%.
Report is 171 commits behind head on next.
The diff coverage is 0.00%.

❗ Current head 83de74f differs from pull request most recent head 4bc3b70. Consider uploading reports for the commit 4bc3b70 to get more accurate results

@@            Coverage Diff             @@
##             next    #3954      +/-   ##
==========================================
- Coverage    0.16%    0.16%   -0.01%     
==========================================
  Files         333      333              
  Lines      291078   291244     +166     
==========================================
  Hits          469      469              
- Misses     290609   290775     +166     
Files Coverage Δ
clarity/src/vm/analysis/mod.rs 0.00% <ø> (ø)
...c/vm/analysis/type_checker/v2_1/tests/contracts.rs 0.00% <ø> (ø)
testnet/stacks-node/src/run_loop/neon.rs 0.00% <ø> (ø)
clarity/src/vm/analysis/type_checker/mod.rs 0.00% <0.00%> (ø)
...rity/src/vm/analysis/type_checker/v2_1/contexts.rs 0.00% <0.00%> (ø)
clarity/src/vm/costs/mod.rs 0.00% <0.00%> (ø)
clarity/src/vm/functions/mod.rs 0.00% <0.00%> (ø)
stackslib/src/chainstate/stacks/db/mod.rs 0.00% <0.00%> (ø)
stackslib/src/chainstate/stacks/db/transactions.rs 0.00% <0.00%> (ø)
stackslib/src/chainstate/stacks/index/storage.rs 0.00% <0.00%> (ø)
... and 13 more

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@cylewitruk
Copy link
Member Author

After some thought, I wonder if we shouldn't go for Clarity 2.1 instead of 3 here? From a language perspective, there isn't all that much that's changing (with the Wasm runtime), that change will mostly be an implementation detail.
Thoughts?

@obycode
Copy link
Contributor

obycode commented Sep 22, 2023

You're right that the runtime change should be an implementation detail, and can be epoch-gated rather than clarity version gated. We have discussed before that we'd like to push any clarity changes out after Nakamoto, unless absolutely necessary, just to avoid adding more complexity into an already very large change. So I'd say, if we can get away with it, we don't bump the clarity version at all yet.

@cylewitruk
Copy link
Member Author

You're right that the runtime change should be an implementation detail, and can be epoch-gated rather than clarity version gated. We have discussed before that we'd like to push any clarity changes out after Nakamoto, unless absolutely necessary, just to avoid adding more complexity into an already very large change. So I'd say, if we can get away with it, we don't bump the clarity version at all yet.

Won't we have to bump the version due to changes in block-height and addition of tenure-height ?

@obycode
Copy link
Contributor

obycode commented Sep 28, 2023

Won't we have to bump the version due to changes in block-height and addition of tenure-height ?

I guess that's still an open question - #3943

Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

Thanks for this! This looks good to me, left a couple of comments.

clarity/src/vm/functions/mod.rs Outdated Show resolved Hide resolved
clarity/src/vm/costs/mod.rs Outdated Show resolved Hide resolved
Comment on lines 538 to 540
// TODO: Afaik we're not making any changes which should change this behavior?
// Maybe add a "Note for future epochs, Epochs >= 2.1 should use `admits_type_v2_1` function."
| StacksEpochId::Epoch30 => self.admits_type_v2_1(other),
Copy link
Member

Choose a reason for hiding this comment

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

Yep, agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed todo and added note

clarity/src/vm/types/signatures.rs Outdated Show resolved Hide resolved
stacks-common/src/types/mod.rs Outdated Show resolved Hide resolved
@@ -3052,6 +3053,7 @@ impl<
return Ok(Some(pox_anchor));
}
}
StacksEpochId::Epoch30 => todo!(), // Need input here
Copy link
Member

Choose a reason for hiding this comment

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

This should have the same behavior as Epochs 2.1 and 2.4 (for now: this will likely be changed in nakamoto, but in this PR, it should just share that behavior with 2.1-2.4).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, moved into their match arm

@@ -5291,6 +5335,7 @@ impl StacksChainState {
cur_epoch.start_height,
)
}
StacksEpochId::Epoch30 => todo!(), // Need input here
Copy link
Member

Choose a reason for hiding this comment

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

This should share the behavior of 2.1-2.4.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, moved into their match arm

@@ -5371,6 +5416,7 @@ impl StacksChainState {
pox_reward_cycle,
pox_start_cycle_info,
),
StacksEpochId::Epoch30 => todo!(), // Need input here
Copy link
Member

Choose a reason for hiding this comment

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

This should share the behavior of 2.4.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, moved 3.0 into 2.4's match arm.

@@ -225,6 +225,7 @@ impl DBConfig {
StacksEpochId::Epoch22 => self.version == "3" || self.version == "4",
StacksEpochId::Epoch23 => self.version == "3" || self.version == "4",
StacksEpochId::Epoch24 => self.version == "3" || self.version == "4",
StacksEpochId::Epoch30 => todo!(), // Need input from Jude here
Copy link
Member

Choose a reason for hiding this comment

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

This should be the same as 2.1-2.4 for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oki, copied

@@ -1276,6 +1276,10 @@ impl<'a, 'b> ClarityBlockConnection<'a, 'b> {
})
}

pub fn initialize_epoch_3_0(&mut self) -> Result<Vec<StacksTransactionReceipt>, Error> {
todo!()
Copy link
Member

Choose a reason for hiding this comment

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

This should just be Ok(vec![]) for now: once there's actual changes necessary for epoch initialization, they can be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, updated!

@kantai kantai mentioned this pull request Oct 6, 2023
@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

@jcnelson
Copy link
Member

next now has both Epoch25 and Epoch30. Can I close this?

@cylewitruk
Copy link
Member Author

next now has both Epoch25 and Epoch30. Can I close this?

Yup 🙂

@jcnelson
Copy link
Member

Thanks for understanding 🙏

@jcnelson jcnelson closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants