NetUpgrade: 1 enum for NetUpgrade - #70
Conversation
This is continuation of #65 implement `TryFrom` for POWConfig, to generate this config from the upgrade version and the chain_type. simplify netupgrade. Moved to its own package in the chain package.
b-yap
changed the title
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct NetUpgrade(Vec<(BlockHeight, UpgradeVersion)>); |
There was a problem hiding this comment.
Shouldn't this be called NetUpgrades?
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Ord, PartialOrd)] | ||
| #[repr(u8)] | ||
| pub enum UpgradeVersion { |
There was a problem hiding this comment.
In the interest of making this testable and useful at the same time, this has to be passed to NetUpgrades as a generic. You then create an enum for testing, which is private for this crate, and the ChainConfig then uses one that's for real mainnet, testnet, etc.
fix uppercase names no more skipping numbers NetUpgrades is of generic <T> updated the `is_activated` function, to only those needed for checking. binary search for "is_activated" checking
c1d015b to
382cb72
Compare
| self.0 | ||
| } | ||
|
|
||
| pub fn checked_mul(&self, rhs: u64) -> Option<Self> { |
There was a problem hiding this comment.
Quite frankly I don't want to have all these operations with block height... do we need them all? I liked the one from Anton with just +1, because that's all we may need.
There was a problem hiding this comment.
Why is it called checked_ml when it does this self.0.checked_add(rhs).map(BlockHeight::new)?
And then checked_add below contains checked_mul... Is there something weird I'm missing or is this just transposed?
There was a problem hiding this comment.
Yep. You're 100% right. Lots of typos. But I want them all dead anyway. There's no need to do arithmetic with block heights other than +1.
There was a problem hiding this comment.
Adding amounts (other than 1) to block height is useful for time locks. Agreed multiplication does not make much sense.
At risk of over-engineering this, there could be two types
BlockHeight(unsigned quantity)BlockDistance(could be signed or unsigned quantity, both have some merit and different tradeoffs)
With operations:
BlockHeight.next() ->BlockHeightBlockHeight+BlockDistance->BlockHeight(orOption<BlockHeight>if distance is signed)BlockHeight-BlockHeight->BlockDistance(orOption<BlockDistance>if distance is unsigned)BlockHeight+BlockHeightis NOT definedBlockDistance+BlockDistance->BlockDistance- Both
BlockHeightandBlockDistancehaveEq,Ordand such From<u64> for BlockHeightFrom<u64/i64> for BlockDistance(depending on whether distance is signed or not)- Absolute time locks are represented as
BlockHeight - Relative time locks are represented as
BlockDistance
This is somewhat similar to the distinction between Instant and Duration in std::time.
There was a problem hiding this comment.
i have updated the checked_add (facepalm) and removed other functions.
@iljakuklic i added a TODO with the link of your comment here, just in case:
https://github.com/mintlayer/mintlayer-core/pull/70/files#diff-3ca7280eaab4d55cd3c25c5117f12fbbb434f06c8c013b3093cc91a5513cf7a0R10
There was a problem hiding this comment.
I don't mind at all to have a BlockDistance (which should be signed, btw), but all in good time.
What's the point of having BlockHeight if it's just another number where we can add, subtract, multiply and calculate its logarithm with base 1.335 and its gamma function? We want to limit the operations to exclusively what we need.
The diff I see, Carla, seems fine for now.
There was a problem hiding this comment.
I don't mind at all to have a BlockDistance (which should be signed, btw), but all in good time.
My initial instinct was also to have a signed BlockDistance but then, if relative time locks were to be represented as BlockDistance, it would allow for negative time locks and that's somewhat weird.
Just to clarify, I do not propose the unsigned block time would be an absolute value of distance of two blocks. BlockHeight(20) - BlockHeight(30) would give None, not 10.
|
The PR looks fine except for the few things I mentioned. Please fix them and let's merge this tomorrow. |
only `checked_add` function left; updated `.sort` to `.sort_unstable`.
5 participants
This is continuation of #65
implement
TryFromfor POWConfig, to generate this config from the upgrade version and the chain_type.simplify netupgrade. Moved to its own package in the chain package.