Add consensus amplification for SIP-45#20668
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
mwtian
left a comment
There was a problem hiding this comment.
Looks good overall. A few comments.
| // TODO: Make this configurable. | ||
| let k = 5; | ||
| let multipler = gas_price / epoch_store.reference_gas_price(); | ||
| let amplification_factor = if multipler >= k { multipler } else { 0 }; |
There was a problem hiding this comment.
I think in the original proposal this is if multipler >= k { multipler + 1 } else { 0 }, but I like the simpler version here.
https://github.com/sui-foundation/sips/pull/45/files#diff-75580faeea2effa3b32a39975085e3ae86fb79e86f9f7208f3a20aafd002f504R33
|
|
Hi, the PR is currently based on main. Could it be related to permission? |
I see. There is a recent issue with Sui CI config. The fix was just merged: #20707. We can pick up this change to fix the CI issue. |
Cool, I have rebased the PR |
mwtian
left a comment
There was a problem hiding this comment.
If @akichidis or @arun-koshy can take a look before merging, it will be great.
| // TODO: Make this configurable. | ||
| let k = 5; | ||
| let multipler = gas_price / epoch_store.reference_gas_price(); | ||
| let amplification_factor = if multipler >= k { multipler } else { 0 }; |
There was a problem hiding this comment.
In this or the next PR, how about adding a histogram metric to record amplification_factor, using SEQUENCING_CERTIFICATE_POSITION_BUCKETS bucket?
There was a problem hiding this comment.
Sounds good. Will do it in the next PR.
Thanks @mwtian for reviewing this. I’ll take a look today |
Description
This is one of the change that is proposed from SIP-45.
Planning to make another separated PR for protocol config, that is:
Test plan
CI