-
Notifications
You must be signed in to change notification settings - Fork 577
Enabling pip-66 back #2034
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
base: develop
Are you sure you want to change the base?
Enabling pip-66 back #2034
Conversation
Code ReviewBug: Missing empty signer guard at consensus/bor/bor.go:1094 The call to When Suggested fix: Wrap lines 1094-1101 with the same guard pattern used earlier: if currentSigner.signer != (common.Address{}) {
successionNumber, err := snap.GetSignerSuccessionNumber(currentSigner.signer)
if err != nil {
return err
}
// Wait before start the block production if needed (previously this wait was on Seal)
if c.config.IsBhilai(header.Number) && successionNumber == 0 && waitOnPrepare {
<-time.After(delay)
}
}Checked for bugs and CLAUDE.md compliance. |
cffls
left a comment
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.
Looks good to me, thanks! Could you pull in the latest CI fix from develop?
|
Also, looks like this integration test failed: https://github.com/0xPolygon/bor/actions/runs/21642843860/job/62387000004?pr=2034#step:7:4364 |
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (65.30%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #2034 +/- ##
===========================================
+ Coverage 49.59% 49.65% +0.06%
===========================================
Files 873 873
Lines 150507 150537 +30
===========================================
+ Hits 74639 74755 +116
+ Misses 70831 70721 -110
- Partials 5037 5061 +24
... and 18 files with indirect coverage changes
🚀 New features to boost your workflow:
|
| for { | ||
| block := nodes[0].BlockChain().CurrentBlock() | ||
| if block.Number.Uint64() >= 14 { | ||
| if block.Number.Uint64() >= 15 { |
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.
Thanks for fixing the test! However, I failed to understand why it is required to make this change. We haven't changed anything regarding the Rio HF number in the test, right?



Description
Isolating the feature from original PR: #2031
Changes
Breaking changes
Please complete this section if any breaking changes have been made, otherwise delete it
Nodes audience
In case this PR includes changes that must be applied only to a subset of nodes, please specify how you handled it (e.g. by adding a flag with a default value...)
Checklist
Cross repository changes
Testing
Manual tests
Please complete this section with the steps you performed if you ran manual tests for this functionality, otherwise delete it
Additional comments
Please post additional comments in this section if you have them, otherwise delete it