Skip to content
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

feat(protocol): Scale up damping factor and flatten curve #13809

Merged
merged 4 commits into from
May 25, 2023

Conversation

adaki2004
Copy link
Contributor

@adaki2004 adaki2004 commented May 24, 2023

The current adjustment quotient (aka. damping factor) was based on Vitalik's calculation (low number like 8, 16, etc) but did not take into consideration that our exp() calculation uses 1e18 fixed point arithmetic, therefore 8, 16, 30, 40, whatever low numbers flattening the curve only just a tiny bit.

WIth the current settings, plotted 2 scenarios with which this works pretty 'smooth'.

  1. We overestimate almost by double the proof time target (than the average):
    image

  2. We underestimating the proof time by half:

image

This way the provers have more time to adjust their behavior - if ever on testnet :) - and even in case they don't, we only slowly converging towards 0 or infinity in the meantime.

Should be going into A3 testnet!

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this pull request looks good. I have a few comments and suggestions:

  • The adjustmentQuotient has been changed from 8 to 16 in multiple files. This might be an oversight, as the value of 16000 should be used instead.
  • The INITIAL_PROOF_TIME_TARGET has been changed from 375 to 70 in TaikoL1.sim.sol. It is not clear why this has been changed and it should be verified that this is the desired value.
  • The nextBlockTime and minDiffToBlockPropTime have been changed from 12 to 8 seconds in TaikoL1.sim.sol. This should also be verified to ensure that it is the desired value.
  • The startBlockProposeTime and upperDevToBlockProveTime have been changed from 1600 and 800 to 100 and 40 seconds respectively in TaikoL1.sim.sol. Again, this should be verified to ensure that these are the desired values.
  • The depositTaikoToken function in TaikoL1Simulation has been changed from 1e6 to 1e9. This should be verified to make sure that this is the correct value for the test scenario being simulated.

@adaki2004
Copy link
Contributor Author

adaki2004 commented May 24, 2023

Note: I'd not make this updateable by the setProofParams setter bc. it is meant to be a slow-down constant. If things still look ugly, we can have 2 choices:

  1. Set the proofTimeTarget (+ proofTimeIssued) to a lower value (closer to actual average)
  2. Reset this value with an upgrade

Brechtpd
Brechtpd previously approved these changes May 24, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this pull request makes changes to the Taiko protocol. It increases the damping factor and flattens the curve.

The following changes have been made:

  • In TaikoConfig.sol, the adjustmentQuotient has been increased from 16 to 16000.
  • In TaikoData.sol, the parameter adjustmentQuotient has been changed from uint8 to uint16.
  • In DeployOnL1.s.sol, uint8 has been changed to uint16 in the calculation of initProofTimeIssued.
  • In DetermineNewProofTimeIssued.s.sol, the constant ADJUSTMENT_QUOTIENT has been increased from 16000 to 32000 and DESIRED_PROOF_TIME_TARGET has been decreased from 500 to 160.
  • In LibLn.sol, uint8 has been changed to uint16 in the calculation of initProofTimeIssued.
  • In TaikoL1TestBase.t.sol, the constant ADJUSTMENT_QUOTIENT has been increased from 16000 to 32000.

I suggest that you add comments in the code explaining why these changes were made and why these values were chosen for each parameter, as this will make it easier for other developers to understand what is happening when they review your code in the future. I also suggest that you add a comment above each function that explains what it does and how it works (e.g., a brief description

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this pull request looks good. Here are some points to consider:

  • It looks like the adjustmentQuotient in TaikoConfig.sol and TaikoData.sol has been increased by a factor of 1000. Is this intentional?
  • The proofCooldownPeriod in TaikoL1.sim.sol has been increased from 1 minute to 5 minutes. Is this intentional?
  • The INITIAL_PROOF_TIME_TARGET in TaikoL1.sim.sol has been decreased from 375 seconds to 70 seconds, and the startBlockProposeTime and upperDevToBlockProveTime have also been adjusted accordingly. Is this intentional?
  • The ADJUSTMENT_QUOTIENT in TaikoL1TestBase.t.sol has been increased from 16000 to 32000, is this intentional?

@davidtaikocha davidtaikocha added this pull request to the merge queue May 25, 2023
Merged via the queue into main with commit b1dcb59 May 25, 2023
@davidtaikocha davidtaikocha deleted the scale_up_damping_factor branch May 25, 2023 04:55
@github-actions github-actions bot mentioned this pull request May 25, 2023
davidtaikocha added a commit that referenced this pull request May 25, 2023
Co-authored-by: adaki2004 <adaki2004@users.noreply.github.com>
Co-authored-by: David <david@taiko.xyz>
dantaik pushed a commit that referenced this pull request May 25, 2023
…13814)

Co-authored-by: D <51912515+adaki2004@users.noreply.github.com>
Co-authored-by: adaki2004 <adaki2004@users.noreply.github.com>
RogerLamTd pushed a commit that referenced this pull request May 25, 2023
Co-authored-by: adaki2004 <adaki2004@users.noreply.github.com>
Co-authored-by: David <david@taiko.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants