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

Backward compatibility with using entire g2 #482

Merged

Conversation

bxue-l2
Copy link
Contributor

@bxue-l2 bxue-l2 commented Apr 15, 2024

Why are these changes needed?

This is a correction for PR #472 which checks g2powerof2 file before starting node. However, #472 prevents a legacy usage that the operator might only have the entire g2 file.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@@ -70,21 +70,24 @@ func NewVerifier(config *kzg.KzgConfig, loadG2Points bool) (*Verifier, error) {
return nil, err
}
} else {
if len(config.G2PowerOf2Path) == 0 {
return nil, errors.New("G2PowerOf2Path is empty. However, object needs to load G2Points")
if len(config.G2PowerOf2Path) == 0 && len(config.G2Path) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

The config.G2Path isn't used below, although it's checking it here.
What if the control flow is if loadG2Points || len(config.G2PowerOf2Path) == 0 { ... } above? This entire block won't need to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to ensure backward compatibility for operators who uses the entire G2 points, though very unlikely. In the last PR #472 , node is required to read the powerOf2 file at start. However, it prevents the legacy usecase when the node is accessing the entire points.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node sets loadG2Points to be false.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my suggestion has the same logic, but simpler code?

@@ -21,7 +21,7 @@ import (

var (
bipMultiplier = big.NewInt(10000)
secondsTillExpiry = 90 * time.Second
secondsTillExpiry = 3600 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

Why extend to 3600s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

90sec is too stringent for some operators to opt in, especially smart contract operator. though it is already merged from #481

@bxue-l2 bxue-l2 merged commit f5e23d8 into Layr-Labs:master Apr 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants