Skip to content
This repository has been archived by the owner on May 11, 2024. It is now read-only.

feat(prover): improve proof submission delay calculation #249

Merged
merged 9 commits into from
May 29, 2023

Conversation

davidtaikocha
Copy link
Member

use:

if basefee != 0 {
  target_delay = expected_reward/basefee * target;
  if target_delay < target * 0.25 {
    target_delay = target * 0.25;
  } else if target_delay > target * 4 {
    target_delay = target * 4;
  }
} else {
  target_delay = target * 4;
}

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. There are a few minor things I would recommend to improve the code:

  • The variable names could be more descriptive and consistent, e.g. targetDelay and stateVar.
  • The calculation of the proof submission delay could be more concise.
  • Consider using constants for the proofTimeTarget and blockFee values.

@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #249 (6133b2c) into main (f64a762) will decrease coverage by 0.03%.
The diff coverage is 20.68%.

@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
- Coverage   53.11%   53.09%   -0.03%     
==========================================
  Files          36       36              
  Lines        3579     3586       +7     
==========================================
+ Hits         1901     1904       +3     
- Misses       1430     1434       +4     
  Partials      248      248              
Impacted Files Coverage Δ
driver/state/state.go 62.98% <0.00%> (+2.02%) ⬆️
prover/proof_producer/zkevm_cmd_producer.go 23.46% <0.00%> (ø)
prover/proof_producer/zkevm_rpcd_producer.go 27.55% <0.00%> (ø)
prover/prover.go 51.68% <ø> (+0.15%) ⬆️
prover/proof_submitter/util.go 32.96% <25.00%> (-2.10%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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. The code changes appear to be well thought out and the commit messages provide enough context for the changes.

Some points to consider:

  • The variable startAt is used in multiple places. It would be better to use a more descriptive variable name such as proofSubmissionStartTime.
  • The logic for calculating the delay could be improved. It might be worth considering using a different approach such as exponential backoff.
  • There are some typos in the commit messages that should be corrected.

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 comments:

  • This pull request looks to be improving the proof submission delay calculation in the prover.
  • The code changes appear to be well written and easy to read.

Specific comments:

  • It might be worth considering adding some additional documentation to explain the purpose of this change and how it works.
  • In Patch 1/3, Line 97, it looks like there is a typo with the variable needNewProof.
  • In Patch 2/3, Line 4, it looks like the improve-prover-wait-time-calculation branch is not included in the list of branches that will trigger this workflow.

@davidtaikocha davidtaikocha enabled auto-merge (squash) May 29, 2023 11:52
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, the code changes in this pull request look good. However, there are a few things I would like to point out:

  • The variables startAt, targetDelay, and expectedReward are used multiple times in the code. It would be better to use more descriptive variable names to make the code more readable.
  • The comment on line 111 should be updated to include the block ID.
  • The log message on line 122 should include the amount of time left to wait for the transaction to submit.

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 my comments:

  • The variable startAt appears to be used in multiple places. It would be better to use more descriptive names for the variables, such as proofSubmissionStartTime or proofSubmissionStartAt.
  • The logs could be improved with more descriptive messages and relevant data such as the blockID.
  • The code could be refactored to reduce the duplication of similar code blocks.

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 I noticed:

  • In Patch 1/6, the variable startAt is declared twice and could be combined to one.
  • In Patch 2/6, the branch name should be consistent with other branch names.
  • In Patch 4/6, there is an unnecessary blockID log in Cancel function.
  • In Patch 5/6, the log message in Cancel function can be improved for more clarity.
  • In Patch 6/6, the branch name should be consistent with other branch names.

Copy link
Contributor

@RogerLamTd RogerLamTd left a comment

Choose a reason for hiding this comment

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

code matches the intended logic, LGTM

@davidtaikocha davidtaikocha merged commit 7cc5d54 into main May 29, 2023
@davidtaikocha davidtaikocha deleted the improve-prover-wait-time-calculation branch May 29, 2023 20:00
vhjiang pushed a commit to taikoverse/taiko-degen-client that referenced this pull request Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants