Update gas target immediately after block execution #39
Conversation
| clock.FastForwardTo( | ||
| b.BuildTime(), | ||
| pts.SubSecondBlockTime(b.Header()), | ||
| b.Time(), |
There was a problem hiding this comment.
I want to differentiate between the different times, much like how we do with state roots. Granted in this case it couldn't be execution (gas nor wall) time, but more generally I want to be more precise in this code base. Happy to change BuildTime() to something else if you'd like, but I'd prefer to not use raw Time() (which should probably be removed).
There was a problem hiding this comment.
BuildTime doesn't exist on the provided type. The function is passing in types.Block, not blocks.Block. Are you suggesting to revert the change and take in blocks.Block again?
hook/hook.go
Outdated
| // GasTarget returns the amount of gas per second that the chain should | ||
| // target to consume after executing the provided block. | ||
| GasTarget(*types.Header) gas.Gas |
There was a problem hiding this comment.
| // GasTarget returns the amount of gas per second that the chain should | |
| // target to consume after executing the provided block. | |
| GasTarget(*types.Header) gas.Gas | |
| // GasTargetAfter returns the amount of gas per second that the chain should | |
| // target to consume after executing the provided block. | |
| GasTargetAfter(*types.Header) gas.Gas |
Also (nit), something about the wording of "to consume after executing the provided block" feels off and it confused me for a split second. Perhaps "target to consume in subsequent blocks."? The current phrasing suggests immediacy, not long-term horizon.
There was a problem hiding this comment.
I reworded it, lmk what you think
hook/hook_test.go
Outdated
| assert.Equal(t, initialTarget, tm.Target(), "Target not changed by BeforeBlock()") | ||
|
|
||
| enforcedPrice := tm.Price() | ||
| assert.LessOrEqual(t, enforcedPrice, initialPrice, "Price should not increase in BeforeBlock()") |
There was a problem hiding this comment.
I think we should tighten this (and the AfterBlock() equivalent) to remove the equalities. It probably requires an accompanying comment for each, to explain that the strict inequality is synthetic, but it gives us slightly stronger guarantees of directionality in the test.
ACP176 expects the gas target to be updated immediately after the execution of the block. However, the prior code updates the target prior to executing the next block.