Skip to content

eth, internal/cli: validate correctness of forks without whitelisted entry#1764

Merged
manav2401 merged 19 commits intov2.2.11-candidatefrom
manav/fork-correctness-check
Sep 16, 2025
Merged

eth, internal/cli: validate correctness of forks without whitelisted entry#1764
manav2401 merged 19 commits intov2.2.11-candidatefrom
manav/fork-correctness-check

Conversation

@manav2401
Copy link
Copy Markdown
Member

@manav2401 manav2401 commented Sep 13, 2025

Description

This PR adds an additional safety check to validate if chains being imported belong to correct fork or not.

Bor whitelists checkpoints and milestones at regular intervals as soon as they're produced by heimdall. This info is later used in validating chains while importing so that wrong forks can be rejected (which do not honour the whitelisted entries). Although for forks which doesn't overlap with the whitelist entries, we blindly accept them as there's no way for us to validate if that fork is correct. Only when next milestone or checkpoint is proposed, we can surely comment on the correctness of a fork.

This PR adds a way to iterate backwards using the parent hash of the incoming chain until we can validate it against the last whitelisted entry.

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

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

  • 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

@manav2401 manav2401 changed the title Manav/fork correctness check eth/downloader/whitelist: validate correctness of forks without milestone Sep 13, 2025
manav2401
manav2401 commented Sep 13, 2025
View reviewed changes
eth/downloader/whitelist/finality.go
return false, nil
}

f.RLock()
Copy link
Copy Markdown
Member Author

@manav2401 manav2401 Sep 13, 2025

Choose a reason for hiding this comment

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

This was added in #1757 but we already acquire a lock in the respective checkpoint and milestone services so this is redundant and not needed.

lucca30
lucca30 reviewed Sep 15, 2025
View reviewed changes
cffls
cffls reviewed Sep 15, 2025
View reviewed changes
@cffls
Copy link
Copy Markdown
Contributor

cffls commented Sep 15, 2025

make test-integration seems to fail when running local for some reason:

--- FAIL: TestAPIs (0.21s)
    bor_api_test.go:95:
        	Error Trace:	/home/ubuntu/bor/tests/bor/bor_api_test.go:95
        	            				/home/ubuntu/bor/tests/bor/bor_api_test.go:251
        	Error:      	Not equal:
        	            	expected: 2
        	            	actual  : 1
        	Test:       	TestAPIs
panic: runtime error: index out of range [1] with length 1 [recovered]
	panic: runtime error: index out of range [1] with length 1

goroutine 13 [running]:
testing.tRunner.func1.2({0x203f500, 0xc00082adc8})
	/home/ubuntu/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1734 +0x21c
testing.tRunner.func1()
	/home/ubuntu/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1737 +0x35e
panic({0x203f500?, 0xc00082adc8?})
	/home/ubuntu/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/runtime/panic.go:792 +0x132
github.com/ethereum/go-ethereum/tests/bor.testGetTransactionReceiptsByBlock(0xc000585500, 0xc000892350)
	/home/ubuntu/bor/tests/bor/bor_api_test.go:106 +0xcbd
github.com/ethereum/go-ethereum/tests/bor.TestAPIs(0xc000585500)
	/home/ubuntu/bor/tests/bor/bor_api_test.go:251 +0x5f4
testing.tRunner(0xc000585500, 0x2465bb0)
	/home/ubuntu/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1792 +0xf4
created by testing.(*T).Run in goroutine 1
	/home/ubuntu/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1851 +0x413
FAIL	github.com/ethereum/go-ethereum/tests/bor	0.251s

@manav2401
Copy link
Copy Markdown
Member Author

manav2401 commented Sep 16, 2025 •
edited
Loading

make test-integration seems to fail when running local for some reason:

Strange. It passed the first two times locally and then encountered the same panic. Maybe it's non deterministic. But this test even fails on master so probably it's unrelated.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Sep 16, 2025

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@manav2401 manav2401 changed the title eth/downloader/whitelist: validate correctness of forks without milestone Validate correctness of forks without whitelisted entry Sep 16, 2025
@manav2401 manav2401 changed the title Validate correctness of forks without whitelisted entry eth, internal/cli: validate correctness of forks without whitelisted entry Sep 16, 2025
@manav2401 manav2401 merged commit a3096f2 into v2.2.11-candidate Sep 16, 2025
7 checks passed
@manav2401 manav2401 deleted the manav/fork-correctness-check branch September 16, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lucca30 lucca30 lucca30 left review comments

@cffls cffls cffls approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@manav2401 @cffls @lucca30