feat: Make minPrice and excessScalingFactor Configurable#80
feat: Make minPrice and excessScalingFactor Configurable#80
Conversation
…abs/strevm into ceyonur/acp-224-gastime-parameters
ceyonur
changed the title
ceyonur
changed the title
ARR4N
left a comment
ARR4N
left a comment
There was a problem hiding this comment.
This is an elegant approach, thank you. The devil is in the details, but I'm generally very keen to merge this.
Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>
Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>
…abs/strevm into ceyonur/acp-224-gastime-parameters
alarso16
left a comment
alarso16
left a comment
There was a problem hiding this comment.
I didn't finish reviewing, but this was as much overflow math as I could deal with today :)
gastime/testdata/fuzz/FuzzPriceInvarianceAfterBlock/036482440c1ec875
Outdated
Show resolved
Hide resolved
alarso16
left a comment
alarso16
left a comment
There was a problem hiding this comment.
A few nits, got distracted before I finished
…abs/strevm into ceyonur/acp-224-gastime-parameters
hook/hook.go
Outdated
| // GasTargetAfter returns the gas target that should go into effect | ||
| // immediately after the provided block. | ||
| GasTargetAfter(*types.Header) gas.Gas | ||
| // GasConfigAfter returns the gas configuration that should go into effect | ||
| // immediately after the provided block. | ||
| GasConfigAfter(*types.Header) GasConfig |
There was a problem hiding this comment.
Thoughts on merging these two functions?
| // GasTargetAfter returns the gas target that should go into effect | |
| // immediately after the provided block. | |
| GasTargetAfter(*types.Header) gas.Gas | |
| // GasConfigAfter returns the gas configuration that should go into effect | |
| // immediately after the provided block. | |
| GasConfigAfter(*types.Header) GasConfig | |
| // GasConfigAfter returns the gas target and configuration that should go | |
| // into effect immediately after the provided block. | |
| GasConfigAfter(*types.Header) (target gas.Gas, c GasConfig) |
We always call them together (except in tests) and externally from sae, the target does seem like a gas configuration.
Alternatively (or perhaps additionally), we could rename GasConfig to GasPriceConfig, because then there is a clear separation between the target and the rest of the params
There was a problem hiding this comment.
I was not %100 sure that these both will be handled in the same hook (After vs Before). I think if we want to merge them we should probably merge them in the canoto as well right?
There was a problem hiding this comment.
This is the only call-site makes me doubt merging these is a clean approach as we don't need the price config here. https://github.com/ava-labs/strevm/blob/ceyonur/acp-224-gastime-parameters/blocks/time.go#L34-L36
gastime/gastime.go
Outdated
| k := tm.excessScalingFactor() | ||
|
|
||
| // Binary search over [0, maxExcessSearchCap(k)] for smallest excess where price >= targetPrice. | ||
| lo, hi := gas.Gas(0), maxExcessSearchCap(k) |
There was a problem hiding this comment.
Similar comment w.r.t. if this is an optimization or not. I believe it is an optimization (or perhaps inspired from avalanchego's maxTargetExcess usage)... Should we be benchmarking this function?
In avalanchego I mainly calculated the cap so that I could use sort.Search (which works over int rather than uint64)... But here we could feasibly hit uint64 values, so we need to implement our own binary search anyways.
There was a problem hiding this comment.
This was from a first attempt to cap it for int type as well, but then I kept it for optimization. There is a ~2x in smaller prices and small/default K. I think we can just remove it though.
| if err := tm.SetTarget(target); err != nil { | ||
| return fmt.Errorf("%T.SetTarget() after block: %w", tm, err) | ||
| } |
There was a problem hiding this comment.
I don't know that SetTarget is really safe anymore. It errors if excess would exceed MaxUint64, which would be crazy on the C-chain, but could definitely happen with the right sequence of config modifications.
In other parts of the code, we just silently cap excess at MaxUint64.
There was a problem hiding this comment.
This PR already has become a beast, let's handle it in #164
|
|
||
| R, T := tm.Rate(), tm.Target() | ||
| quo, _, _ := intmath.MulDiv(g, R-T, R) // overflow is impossible as (R-T)/R < 1 | ||
| tm.excess += quo |
There was a problem hiding this comment.
This (adding to excess) can overflow now right?
…abs/strevm into ceyonur/acp-224-gastime-parameters
4 participants
This PR makes minPrice and excessScalingFactor Configurable, also adds a static pricing mode. These parameters will be required by ACP-224, which intends to set these via a system contract.
Closes: #77