-
Notifications
You must be signed in to change notification settings - Fork 818
ACP-226: add initial delay excess #4300
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
Conversation
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.
Pull Request Overview
Adds a named constant for the previously inlined default delay excess value (~2000ms) and updates the related test to use it.
- Introduces DefaultDelayExcess constant (7_970_124)
- Updates test case to reference the new constant and renames the test case to reflect default usage
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vms/evm/acp226/acp226.go | Adds exported DefaultDelayExcess constant |
| vms/evm/acp226/acp226_test.go | Replaces hardcoded numeric excess with DefaultDelayExcess and adjusts test name |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Ceyhun Onur <[email protected]>
Signed-off-by: Ceyhun Onur <[email protected]>
|
|
||
| // InitialDelayExcess represents the initial (≈2000ms) delay excess. | ||
| // Formula: ConversionRate (2^20) * ln(2000) + 1 | ||
| InitialDelayExcess = 7_970_124 |
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.
These feel like magic numbers even though we use a godoc to document them... could we define these through calculation in code? i.e
const conversionRate = 1 << 20 // 2^20 fixed-point scale
var InitialDelayExcess = int(float64(conversionRate)*math.Log(2000)) + 1
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.
These values are consensus critical, so I don't think it is reasonable to perform floating point operations to generate them. (The actual number is more important than the mechanism used to derive them)
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.
(same reason as what Stephen posted above while I writing this)
I think that would actually end up what we try to avoid with binary search. I think math.Log precision can differ from machine to machine and end-up having different initial values.
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 we need to make this configurable by minimum delay setting in the future, an approach @StephenButtolph recommended to me for ACP-224 is to use some rational approximation for ln(x), but that would basically be a magic number itself here anyways.
Why this should be merged
Adds default delay excess as 7_970_124 (~2000ms delay)
How this works
adds the default constant
How this was tested
changed the test with constant
Need to be documented in RELEASES.md?'
no