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

Fix: allow epochs to transition in a single block, and boot cleanup #229

Merged
merged 4 commits into from
Mar 9, 2023

Conversation

kantai
Copy link
Contributor

@kantai kantai commented Mar 8, 2023

This PR does a couple of things related to epoch transitions and boot code in the subnet node:

  1. Removes boot contracts that definitely don't apply to a subnet (PoX, lockups, PoX-2, genesis)
  2. Sets the default epoch transition heights such that Epoch2.1 starts in the genesis block of a subnet node.
  3. Updates the epoch transition logic to allow nodes to immediately transition to Epoch2.1 in the first block (i.e., go straight from Epoch2 to Epoch2.1)

The integration tests all test this behavior now, because they used the default regtest epoch configurations.

@kantai kantai requested a review from obycode March 8, 2023 17:41
Copy link
Member

@obycode obycode left a comment

Choose a reason for hiding this comment

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

This is great. Thanks for this @kantai! This can close #196, and we can break out the remaining discussion on BNS into a separate issue. Before removing the genesis contract, I wonder if marketing has any fun ideas to include in the Hiro subnet.

const BOOT_CODE_POX_BODY: &'static str = std::include_str!("pox.clar");
const BOOT_CODE_POX_TESTNET_CONSTS: &'static str = std::include_str!("pox-testnet.clar");
const BOOT_CODE_POX_MAINNET_CONSTS: &'static str = std::include_str!("pox-mainnet.clar");
pub const BOOT_CODE_LOCKUP: &'static str = std::include_str!("lockup.clar");
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for doing this cleanup!!

("subnet", &BOOT_CODE_SUBNET),
("pox", &BOOT_CODE_POX_MAINNET),
("lockup", BOOT_CODE_LOCKUP),
("costs", BOOT_CODE_COSTS),
("cost-voting", BOOT_CODE_COST_VOTING_MAINNET),
("bns", &BOOT_CODE_BNS),
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... BNS is a bit weird within a subnet. Could we have a BNS contract in here that just implements transfers and the subnet-asset trait, so that BNS names could be traded in a subnet? Wallets should probably use L1 BNS to lookup addresses when interacting on a subnet. I'd like to think about that some more, and either way, it doesn't need to be part of this PR, just noting some thoughts here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was my thought -- do you think it makes sense to delete the BNS contract in this PR as well for now, and leave the subnet-BNS contract as a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that sounds good. Whatever we end up with, we definitely don't want this BNS contract.

Copy link
Member

Choose a reason for hiding this comment

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

I'll open an issue for it.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2023

Codecov Report

Merging #229 (ae60247) into master (54c653c) will increase coverage by 1.06%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   91.19%   92.26%   +1.06%     
==========================================
  Files           6        6              
  Lines         284      336      +52     
==========================================
+ Hits          259      310      +51     
- Misses         25       26       +1     
Impacted Files Coverage Δ
core-contracts/contracts/helper/simple-ft.clar 75.00% <ø> (-1.48%) ⬇️
...contracts/contracts/helper/simple-nft-no-mint.clar 66.66% <ø> (ø)
core-contracts/contracts/helper/simple-nft.clar 73.33% <ø> (ø)
core-contracts/contracts/multi-miner.clar 97.05% <ø> (ø)
core-contracts/contracts/subnet.clar 94.96% <ø> (+0.81%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kantai kantai force-pushed the feat/same-block-epochs+boot-cleanup branch from 0eea315 to ae60247 Compare March 9, 2023 01:05
@kantai kantai merged commit 582f524 into master Mar 9, 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.

3 participants