-
Notifications
You must be signed in to change notification settings - Fork 106
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
Time based forking #1434
Time based forking #1434
Conversation
Replaced the ForkToBlockNumber array with a (more complicated) object called ForkTransitionTable that knows the difference between block-based forks, time-based forks, and the special cases like MergeFork and Shanghai (which IIUC needs to be doable via either block number or time, at least temporarily). Created a type called ForkDeterminationInfo to represent the information needed to determine which hard fork a particular block is on. In the past all we needed was a block number; now we need to pass in both a block number and timestamp (and if mergeForkBlock isn't specified, maybe TD as well), so I created a type for it (though I don't love the name). Several things are still unresolved. (See various places that say FIXME-Adam.) But I think this is progress.
tools/common/helpers.nim
Outdated
@@ -49,7 +49,9 @@ func getChainConfig*(network: string, c: ChainConfig) = | |||
c.grayGlacierBlock = number[HardFork.GrayGlacier] | |||
c.mergeForkBlock = number[HardFork.MergeFork] | |||
c.shanghaiBlock = number[HardFork.Shanghai] | |||
c.cancunBlock = number[HardFork.Cancun] | |||
# FIXME-Adam: I don't understand how these tests are supposed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, similar to TTD, they will specify timestamp
|
||
func forkDeterminationInfoForVMState*(vmState: BaseVMState): ForkDeterminationInfo = | ||
# FIXME-Adam: Is this timestamp right? Note that up above in blockNumber we add 1; | ||
# should timestamp be adding 12 or something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp increment depends on the consensus configuration, e.g. Clique
is using cliquePeriod
. But POS block timestamp get the timestamp from beacon client, see core/casper.nim
and core/sealer.nim
.
# How do we calculate "the timestamp for the block | ||
# after this one"? | ||
# | ||
# If this makes no sense, what should the callers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment above for evm/state.nim
nimbus/common/hardforks.nim
Outdated
else: | ||
result.toFork = forkFalse | ||
|
||
# FIXME: is BlockToForks even necessary anymore? I feel like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it's enough to keep only ForkTransitionTable
, then please do it.
# HardFork to a BlockNumber; now we have this ForkTransitionTable, which | ||
# contains a couple of arrays and also special cases for MergeBlock and | ||
# Shanghai. | ||
func isGTETransitionThreshold*(map: ForkTransitionTable, forkDeterminer: ForkDeterminationInfo, fork: HardFork): bool = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test for isGTETransitionThreshold
. test_blockchain_json
doesn't test every transition.
@@ -129,7 +129,10 @@ proc processTransaction*( | |||
{.gcsafe, raises: [Defect,CatchableError].} = | |||
## Variant of `processTransaction()` with `*fork* derived | |||
## from the `vmState` argument. | |||
let fork = vmState.com.toEVMFork(header.blockNumber) | |||
## | |||
## FIXME-Adam: Hmm, I'm getting the block number and timestamp from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a plan to remove this fork override in near future, see #1410.
Intending to implement the V2 versions next. (I need the bodies to be in separate procs so that multiple versions can use them.)
(We only use the new forkTransitionTable now.)
#1424
Replaced the ForkToBlockNumber array with a (more complicated) object called ForkTransitionTable that knows the difference between block-based forks, time-based forks, and the special cases like MergeFork and Shanghai (which IIUC needs to be doable via either block number or time, at least temporarily).
Created a type called ForkDeterminationInfo to represent the information needed to determine which hard fork a particular block is on. In the past all we needed was a block number; now we need to pass in both a block number and timestamp (and if mergeForkBlock isn't specified, maybe TD as well), so I created a type for it (though I don't love the name).
Several things are still unresolved. (See various places that say FIXME-Adam.) But I think this is progress.