Extend ForkHeight to manage Fork numbers #20
Conversation
edmundnoble
left a comment
edmundnoble
left a comment
There was a problem hiding this comment.
Overall this seems to be an excellent idea that extends the existing machinery in a good way.
| succByHeight (ForkAtBlockHeight x) = ForkAtBlockHeight $ succ x | ||
| succByHeight ForkNever = ForkNever | ||
| succByHeight _ = error "Only a Blockheight defined fork can be succ'ed" | ||
|
|
There was a problem hiding this comment.
Note to self: This being somewhat hacky is expected because the mechanism it's working on is also somewhat hacky. In future, ForkNumber-based forks will make the definition of e.g. MaxBlockGasLimit much cleaner regardless.
Co-authored-by: Edmund Noble <edmundnoble@users.noreply.github.com>
|
There's one more thing I'd like: I'd like you to add an assertion to |
Yes 100% agree |
| case minimumBlockHeaderHistory cwVersion latestHeader of | ||
| -- Note, this behaviour may be dangerous in case of changes on the miniumum block history. | ||
| -- | ||
| -- TODO = Option to prune headers history to the minimum should be enabled by flag. |
There was a problem hiding this comment.
I'd rather have a TODO for renaming the current minimumBlockHeaderHistory to versionSpvProofRootValidWindow and adding a new minimumBlockHeaderHistory which is set to Nothing (i.e. all history) for all existing versions for this to use.
| chainweb219Pact' = guardCtx chainweb219Pact txCtx | ||
| chainweb223Pact' = guardCtx chainweb223Pact txCtx | ||
| allVerifiers = verifiersAt v cid currHeight | ||
| allVerifiers = verifiersAt v cid pact4ForkNumber currHeight |
There was a problem hiding this comment.
This is fine with me, using ctxParentForkNumber here would also have been fine.
| validateNoHeightAfterChainweb31' :: ChainwebVersion -> ForkHeight -> Either String () | ||
| validateNoHeightAfterChainweb31' v fh = | ||
| case (fork31Height, fh) of | ||
| (Just (ForkAtBlockHeight refAth), ForkAtBlockHeight ath) -> | ||
| if ath > (refAth + 1) | ||
| then Left ("validateVersion: Forking rule must only defined by Fork numbers after Chainweb31: " ++ show ath ++ " > " ++ show refAth) | ||
| else Right () | ||
| _ -> Right () | ||
| where | ||
| fork31Height = v ^? versionForks . at Chainweb31 . _Just . atChain (unsafeChainId 0) |
There was a problem hiding this comment.
This needs to work properly if Chainweb31Fork is activated at genesis as well so here's a more pithy version that handles that:
| validateNoHeightAfterChainweb31' :: ChainwebVersion -> ForkHeight -> Either String () | |
| validateNoHeightAfterChainweb31' v fh = | |
| case (fork31Height, fh) of | |
| (Just (ForkAtBlockHeight refAth), ForkAtBlockHeight ath) -> | |
| if ath > (refAth + 1) | |
| then Left ("validateVersion: Forking rule must only defined by Fork numbers after Chainweb31: " ++ show ath ++ " > " ++ show refAth) | |
| else Right () | |
| _ -> Right () | |
| where | |
| fork31Height = v ^? versionForks . at Chainweb31 . _Just . atChain (unsafeChainId 0) | |
| validateNoBlockHeightForkAfterChainweb31' :: ChainwebVersion -> ForkHeight -> Either String () | |
| validateNoBlockHeightForkAfterChainweb31' v fh = | |
| if fh `atOrAfter` fork31Height | |
| then Left $ "validateVersion: Forking rule must only defined by Fork numbers after Chainweb31: " ++ show ath ++ " > " ++ show refAth | |
| else Right () | |
| where | |
| fork31Height = v ^? versionForks . at Chainweb31 . _Just . atChain (unsafeChainId 0) |
|
|
||
| -- We consider the following ordering for Forks: | ||
| -- - ForkAtGenesis | ||
| -- - ForkNumber = 0 (unusual case) |
There was a problem hiding this comment.
This is a good point. Please make ForkNumber 0 illegal for forks in validateVersion. I don't mind if this is in a followup PR.
Co-authored-by: Edmund Noble <edmundnoble@users.noreply.github.com>
The objective of this PR is to make a standardized and homogeneous way to manage forks and rules by Height, and fork numbers.
The object ForkHeight as been extended to manage fork numbers as well.
Compare rules have been moved to the data definition from the predicates (after, atOrAfter, ...).
The same object can now be used for Rules definition too.
A new checkFork' function has been added to manage forks by numbers, while the old checkFork function still manages forks by heights.
Postulate: starting with fork number 1, no forks or rules change will use heights anymore.
Note : upgrades don't support fork numbers for now, as it requires a more complex mechanism.