Giugliano HF

[lucca30]

Description

PR to introduce a new HardFork in order to bring PIP-66 back. This HF is necessary to let the consensus be more flexible about early block announcements.

Changes

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)
• Changes only for a subset of nodes

Breaking changes

HF, it's a breaking change.

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

• I have added at least 2 reviewer or the whole pos-v1 team
• I have added sufficient documentation in code
• I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
• Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
• Includes RPC methods changes, and the Notion documentation has been updated

Cross repository changes

• This PR requires changes to heimdall
  • In case link the PR here:
• This PR requires changes to matic-cli
  • In case link the PR here:

Testing

• I have added unit tests
• I have added tests to CI
• I have tested this code manually on local environment
• I have tested this code manually on remote devnet using express-cli
• I have tested this code manually on amoy
• I have created new e2e tests into express-cli

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

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.49%. Comparing base (eab2a72) to head (358e88b).

Files with missing lines	Patch %	Lines
consensus/bor/bor.go	66.66%	2 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (66.66%) 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    #2099      +/-   ##
===========================================
- Coverage    51.51%   51.49%   -0.03%     
===========================================
  Files          882      882              
  Lines       154081   154082       +1     
===========================================
- Hits         79371    79339      -32     
- Misses       69537    69562      +25     
- Partials      5173     5181       +8     

Files with missing lines	Coverage Δ
consensus/bor/bor.go	83.69% <66.66%> (-0.30%)	⬇️

... and 18 files with indirect coverage changes

Files with missing lines	Coverage Δ
consensus/bor/bor.go	83.69% <66.66%> (-0.30%)	⬇️

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lucca30]

@claude Can you do an analysis in this PR. Checking about any potential bug introduced, security breach or edge cases?

[claude]

Claude finished @lucca30's task in 1m 58s —— View job

---

Analysis of Giugliano Hard Fork PR

• Read changed files and gather context
• Analyze params/config.go changes
• Analyze consensus/bor/bor.go changes
• Analyze consensus/bor/bor_test.go changes
• Check for bugs, security issues, and edge cases
• Post final analysis

---

Bug Found: Duplicated Upper-Bound Check

consensus/bor/bor.go:434-445 — The upper-bound timestamp check is duplicated identically. Lines 434-439 and 440-445 are exact copies:

// Upper-bound check: a block whose timestamp is more than maxAllowedFutureBlockTimeSeconds
// ahead of the local clock is rejected.
if header.Time > now+maxAllowedFutureBlockTimeSeconds {
    log.Error("Block timestamp too far in future post giugliano", "number", number, "headerTime", header.Time, "now", now)
    return consensus.ErrFutureBlock
}
// Upper-bound check: a block whose timestamp is more than maxAllowedFutureBlockTimeSeconds  ← DUPLICATE
// ahead of the local clock is rejected.
if header.Time > now+maxAllowedFutureBlockTimeSeconds {
    log.Error("Block timestamp too far in future post giugliano", "number", number, "headerTime", header.Time, "now", now)
    return consensus.ErrFutureBlock
}

This is a clear copy-paste error. The second block is dead code — it can never be reached because the first check would already return. Not a correctness bug (the logic is still correct), but it's code noise that should be removed.

Fix this →

---

Behavioral Regression: verifyHeader Now Uses IsGiugliano Instead of IsRio

The first if branch in verifyHeader was changed from c.config.IsRio(header.Number) to c.config.IsGiugliano(header.Number). This is the core change of the PR, but has an important implication:

Between the Rio and Giugliano fork blocks, the flexible blocktime validation (parent-time check + upper-bound) will no longer apply. Instead, the code falls through to the IsBhilai branch, which uses the stricter CalcProducerDelay check.

This appears intentional based on the comments ("Using strict CalcProducerDelay for early block announcement (introduced back in Giugliano) would reject valid blocks"), but it's worth confirming: Are there any nodes currently relying on the Rio-era flexible validation between RioBlock and GiuglianoBlock? If GiuglianoBlock has not been set yet for Amoy/Mainnet (which it hasn't — the network configs don't define it), then the Giugliano path will never activate until the fork block is configured.

This means after this code is deployed but before Giugliano is scheduled, the old Rio behavior (flexible blocktime check) will be lost, and nodes will fall back to the Bhilai-era check. This is a breaking behavioral change during the window between code deployment and Giugliano activation.

Recommendation: Consider keeping IsRio as a fallback, e.g.:

if c.config.IsGiugliano(header.Number) || c.config.IsRio(header.Number) {

Or ensure Giugliano is activated at the same block as Rio for existing networks.

---

Behavioral Regression: Prepare Wait and Seal Delay=0

Similarly, two other changes gate behavior on IsGiugliano instead of IsBhilai:

1. bor.go:1129: c.config.IsGiugliano(header.Number) && waitOnPrepare — The wait-in-Prepare for primary producers was previously gated on IsBhilai. Between Bhilai and Giugliano activation, primary producers will stop waiting in Prepare and revert to waiting in Seal.

2. bor.go:1408: c.config.IsGiugliano(header.Number) && successionNumber == 0 — The delay=0 in Seal for primary producers was gated on IsBhilai. Between Bhilai and Giugliano, primary producers will get a non-zero delay in Seal again (double-waiting effectively reverted).

These are consistent with each other (both the Prepare-wait and Seal-delay=0 were a pair), but it means there's a temporary regression between code deployment and Giugliano activation where block production timing reverts to pre-Bhilai behavior.

---

Cache TTL Change: Potential Timing Edge Case

bor.go:527: The extra cache TTL for future-timestamped blocks is now gated on IsGiugliano:

if headerTime.After(nowTime) && c.config.IsGiugliano(header.Number) {

Before this change, this applied unconditionally. Now, pre-Giugliano blocks with future timestamps won't get extended cache TTL. This could cause premature cache eviction for early-announced blocks on pre-Giugliano forks, leading to redundant re-verification. Not a correctness bug, but a performance regression for the Rio-to-Giugliano window.

---

Edge Case: now < parent.Time Check

bor.go:430: if parent == nil || now < parent.Time — This rejects if the current time is less than parent's time. This handles clock skew, but note that it uses strict <. If a node's clock is exactly equal to the parent time (now == parent.Time), the block is accepted. This seems correct.

However, if the parent is nil (line 430), we return ErrFutureBlock with a log message saying "Block announced too early". A nil parent more likely indicates a missing parent header (DB issue or sync gap), not a timing problem. The error message is misleading, but functionally it correctly rejects the block.

---

Missing Network Config Definitions

GiuglianoBlock is not yet defined in any of the built-in chain configs (Amoy at line ~340, Mainnet at line ~425, AllBorChainConfig at line ~730). This is presumably intentional (fork block TBD), but it means all the new Giugliano-gated code paths are unreachable until network configs are updated. The tests all use GiuglianoBlock: big.NewInt(0) to test the logic, which is good.

---

Security Assessment

No security vulnerabilities identified. The changes:

• Don't introduce new external inputs or attack surfaces
• Don't modify signature verification or validator validation
• The maxAllowedFutureBlockTimeSeconds (30s) upper bound is maintained
• Fork activation is properly gated on block number

---

Summary

Issue	Severity	Type
Duplicated upper-bound check (lines 434-445)	Low	Bug (dead code)
Rio→Giugliano gate change in verifyHeader may break existing behavior before Giugliano activation	Medium	Behavioral regression
Bhilai→Giugliano gate change in Prepare/Seal may revert block timing before Giugliano activation	Medium	Behavioral regression
Cache TTL no longer extended for pre-Giugliano future blocks	Low	Performance regression
No security vulnerabilities found	—	Security

The most important item to address is the duplicated code block and to confirm the intended behavior during the window between code deployment and Giugliano fork activation. If nodes are already running with Rio active, changing the gate from IsRio to IsGiugliano will alter validation behavior before Giugliano is scheduled.

---

[lucca30]

@claude Do a review again, but now see my response for it.

• The duplicate timestamp check will be removed.
• The Giugliano gating is intentional.
• The behaviors mentioned were never active on production networks, so this PR does not introduce a regression.
• The fork block will later define when these behaviors become active.

[claude]

Claude finished @lucca30's task in 1m 4s —— View job

---

Re-review of Giugliano Hard Fork PR

Taking into account your clarifications:

• Duplicate timestamp check → already removed in 2dd555f

• Giugliano gating is intentional (not a regression since Rio/Bhilai behaviors were never active on production)

• Fork block will be defined later for Amoy/Mainnet

• Read changed files and gather context

• Review params/config.go changes

• Review consensus/bor/bor.go changes

• Review consensus/bor/bor_test.go changes

• Post final review

---

Updated Review

With the clarification that Rio and Bhilai block-timing behaviors were never activated on production networks, the previous "regression" concerns are resolved. The Giugliano gating effectively consolidates these behaviors under a single new fork, which is clean.

Code quality: Good. The duplicate block from the previous review has been addressed in commit 2dd555f.

Minor comment wording nit

consensus/bor/bor.go:419-421:

// Rio introduced flexible blocktime (can be set larger than consensus without approval).
// Using strict CalcProducerDelay for early block announcement (introduced back in Giugliano)
// would reject valid blocks...

The comment says CalcProducerDelay was "introduced back in Giugliano" — but CalcProducerDelay was introduced in Bhilai, not Giugliano. Since Giugliano is the new fork being added in this PR, this reads confusingly. Suggest rewording to something like:

// Using strict CalcProducerDelay (from the Bhilai path) for early block announcement
// would reject valid blocks...

Fix this →

Tests

The new tests are well-structured and cover the key boundary scenarios:

• TestVerifyHeader_Giugliano_Boundary — correctly tests pre/post Giugliano behavior for timestamp validation
• TestPrepare_WaitGate_GiuglianoOnly — validates that Prepare wait only activates with Giugliano
• TestSeal_PrimaryProducerDelay_GiuglianoBoundary — confirms delay=0 gating

The existing TestBorPrepare_WaitOnPrepareFlag was properly updated from Bhilai to Giugliano config.

Bhilai TODO comment

consensus/bor/bor.go:441: The TODO comment is helpful for future cleanup:

// TODO: Once Amoy and Mainnet supports Giugliano HF, we are safe to remove this check

Minor grammar: "supports" → "support" (plural subject).

Summary

Area	Status
Duplicate code block	Fixed in 2dd555f
Giugliano gating (intentional)	Confirmed, no issues
Config changes (params/config.go)	Clean — follows existing fork pattern
Tests	Good coverage of boundary conditions
Security	No vulnerabilities introduced
Nits	Comment wording (CalcProducerDelay attribution, grammar)

Overall this looks good to merge once the comment nits are addressed (or ignored if preferred). The approach of consolidating under a single Giugliano fork gate is clean and well-tested.

---

View job

[lucca30]

@claude Regarding this comment: // Using strict CalcProducerDelay for early block announcement (introduced back in Giugliano)

• It was done on purpose, the early block announcement feature was introduced back in Giugliano. I wan not referring to calcproducerdelay func

[claude]

Claude finished @lucca30's task in 15s —— View job

---

Acknowledgment: Comment Wording Clarification

Thanks for the clarification! I understand now — "introduced back in Giugliano" refers to the early block announcement feature being introduced in the Giugliano hard fork, not to the CalcProducerDelay function itself. The comment reads correctly as intended. My previous suggestion to change it was based on a misreading — no changes needed there.

---

View job

[sonarqubecloud]

Quality Gate failed

Failed conditions
14.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud