Skip to content

Polygon Bhilai Fork Updates#15261

Merged
yperbasis merged 19 commits into
mainfrom
bhilai_fork_updates
May 29, 2025
Merged

Polygon Bhilai Fork Updates#15261
yperbasis merged 19 commits into
mainfrom
bhilai_fork_updates

Conversation

@mh0lt

@mh0lt mh0lt commented May 26, 2025

Copy link
Copy Markdown
Contributor

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need to update (evm *EVM) precompile(addr common.Address) for Bhilai

Comment thread polygon/bor/bor.go Outdated
Comment thread core/vm/contracts.go
Comment thread core/vm/interpreter.go Outdated

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isEoaCodeAllowed in filterBadTransactions should be extended to Bhilai

@yperbasis

Copy link
Copy Markdown
Member

If maticnetwork/bor includes Bhilai into fork ID, then we need to do that in GatherForks (similar to Napoli).

@mh0lt mh0lt added this to the 3.0.5 milestone May 26, 2025
@mh0lt

mh0lt commented May 26, 2025

Copy link
Copy Markdown
Contributor Author

isEoaCodeAllowed in filterBadTransactions should be extended to Bhilai

Fixed by: 9581c31

@mh0lt

mh0lt commented May 26, 2025

Copy link
Copy Markdown
Contributor Author

If maticnetwork/bor includes Bhilai into fork ID, then we need to do that in GatherForks (similar to Napoli).

Fixed by: 9581c31

@mh0lt

mh0lt commented May 26, 2025

Copy link
Copy Markdown
Contributor Author

Need to update (evm *EVM) precompile(addr common.Address) for Bhilai

Fixed by: 9581c31

Comment thread txnprovider/txpool/pool.go Outdated
}
if bhilaiBlock != nil {
if !bhilaiBlock.IsUint64() {
return nil, errors.New("agraBlock overflow")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

small nit: it should be bhilaiBlock overflow

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed by: 1f5f126

Comment thread polygon/bor/bor.go
// from non-primary producer. Such blocks will be rejected later when we know the succession
// number of the signer in the current sprint.
if header.Time-config.CalculatePeriod(header.Number.Uint64()) > uint64(time.Now().Unix()) {
return fmt.Errorf("%w: expected: %s(%s), got: %s", consensus.ErrFutureBlock, time.Unix(now.Unix(), 0), now, time.Unix(int64(header.Time), 0))

@manav2401 manav2401 May 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a suggestion from previous experience -- we have found errors with unix based timestamps much easy to debug and understand instead of printing full time stamps.
Reference: 0xPolygon/bor@f735312

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You will notice that it actually prints both. This is because we have discovered an issue - particularly on amoy where we are receiving 'early packets', however these are actually withing a few milliseconds of the clock time.

time.Unix just truncates the nanosecond part of the timestamp meaning that you need to receive at a second's granularity if your clock is slow. The information is included in the error so it is clear what is going on.

I have not changed what is being checked as we're about to allow early headers which will make this go away.

Its part of a bigger occasional problem we see in erigon where we occasionally see gaps in processing headers. We have traced it down to these missing headers getting dropped and then on certain occasions not being recovered - we're still trying work out what is happening.

@yperbasis yperbasis May 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see that for Ethereum we have a very generous tolerance of 15 sec - see allowedFutureBlockTimeSeconds

Comment thread core/vm/contracts.go Outdated
Comment thread cmd/utils/flags.go Outdated
cfg.GasLimit, err = flags.GetUint64(MinerGasLimitFlag.Name)
if err != nil {
gasLimit, err := flags.GetUint64(MinerGasLimitFlag.Name)
if _, ok := err.(*strconv.NumError); ok || err == nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When can strconv.NumError happen?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the flag is not set then it will return a parse error, Because the value is "".

Ideally we should make this an optional flag and check for optionality - but I couldn't see an obvious way of doing that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually re-reviewing the doe I can just use is set. I'll change it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in: a8d32b9

@mh0lt mh0lt requested review from manav2401 and yperbasis May 28, 2025 11:53
Comment thread polygon/bor/bor.go

var delay time.Duration
// Sweet, the protocol permits us to sign the block, wait for our time
delay := time.Until(time.Unix(int64(header.Time), 0))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[nitpicking] To avoid duplication we could keep this line, but override it if IsBhilai && successionNumber == 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed by: e8a60ea

Comment thread polygon/bor/bor.go Outdated
// early block announcements. Note that this is a loose check and would allow early blocks
// from non-primary producer. Such blocks will be rejected later when we know the succession
// number of the signer in the current sprint.
if header.Time-config.CalculatePeriod(header.Number.Uint64()) > uint64(time.Now().Unix()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ValidateHeaderTime should use its now argument instead of time.Now()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed by: e8a60ea

@yperbasis yperbasis enabled auto-merge (squash) May 29, 2025 08:34
@yperbasis yperbasis merged commit 589d81f into main May 29, 2025
11 checks passed
@yperbasis yperbasis deleted the bhilai_fork_updates branch May 29, 2025 08:40
somnergy pushed a commit that referenced this pull request May 29, 2025
This is a fix to PR #15261. Before this PR
[EIP-7623](https://eips.ethereum.org/EIPS/eip-7623) and
[EIP-7702](https://eips.ethereum.org/EIPS/eip-7702) were not counted as
activated in `(p *TxPool) validateTx` when
[Bhilai](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-63.md)
was activated.
mh0lt pushed a commit that referenced this pull request May 29, 2025
Perhaps it doesn't matter much in practice, but this PR hardens the
[implementation](#15261) of
[PIP-66](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-66.md)
to avoid potential `uint64` overflows.
mh0lt added a commit that referenced this pull request May 29, 2025
Perhaps it doesn't matter much in practice, but this PR hardens the
[implementation](#15261) of
[PIP-66](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-66.md)
to avoid potential `uint64` overflows.

(cherry picked from commit bc4cb61)
mh0lt added a commit that referenced this pull request May 29, 2025
This is a fix to PR #15261. Before this PR
[EIP-7623](https://eips.ethereum.org/EIPS/eip-7623) and
[EIP-7702](https://eips.ethereum.org/EIPS/eip-7702) were not counted as
activated in `(p *TxPool) validateTx` when
[Bhilai](https://github.com/maticnetwork/Polygon-Improvement-Proposals/blob/main/PIPs/PIP-63.md)
was activated.

(cherry picked from commit fc4f264)
@yperbasis yperbasis removed this from the 3.0.5 milestone May 30, 2025
mh0lt added a commit that referenced this pull request May 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants