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

feat: Implement ReorgDetector V2 #34

Merged
merged 45 commits into from
Aug 27, 2024
Merged

feat: Implement ReorgDetector V2 #34

merged 45 commits into from
Aug 27, 2024

Conversation

begmaroman
Copy link
Contributor

No description provided.

@begmaroman begmaroman self-assigned this Aug 7, 2024
@begmaroman begmaroman changed the title Added e2e test for reorg detection feat: Implement ReorgMonitor to replace ReorgDetector Aug 13, 2024
@begmaroman begmaroman changed the title feat: Implement ReorgMonitor to replace ReorgDetector feat: Implement ReorgDetector V2 Aug 20, 2024
Copy link
Contributor

@arnaubennassar arnaubennassar left a comment

Choose a reason for hiding this comment

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

A part from the coments, missing the starting logic were blocks that were added by subscribers are checked before start adding blocks (those blocks may be old and conisdered final, but have wrong hashes). This is reasonable to happen when restarting the node

reorgmonitor/monitor.go Outdated Show resolved Hide resolved
cmd/run.go Outdated Show resolved Hide resolved
cmd/run.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetectorv2.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetectorv2.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetectorv2.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetectorv2.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetectorv2.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetectorv2.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetectorv2.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetector.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetector.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetector_sub.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetector_sub.go Show resolved Hide resolved
reorgdetector/reorgdetector_sub.go Show resolved Hide resolved
reorgdetector/reorgdetector_db.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetector_sub.go Outdated Show resolved Hide resolved
"github.com/stretchr/testify/require"
)

func TestBlockMap(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func TestBlockMap(t *testing.T) {
func TestHeadersList(t *testing.T) {

reorgdetector/config.go Outdated Show resolved Hide resolved
reorgdetector/reorgdetector.go Outdated Show resolved Hide resolved
trackedBlocks[string(k)] = newBlockMap(blocks...)
}
// Update the tracked blocks in the DB
if err := rd.updateTrackedBlocksDB(ctx, id, hdrs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this is needed? Is it possible that is a left over from refactoring?

Update: understood, that the update is meant to delete in case the hdrs map removed some entities. It's a bit counter intuitive that this doesn't happen while doing the deletes, but being able to do it on the deletion would complicate the DB handling. LGTM, maybe in the future we move to SQLite and can improve this

reorgdetector/reorgdetector_test.go Outdated Show resolved Hide resolved
l1bridge2infoindexsync/e2e_test.go Outdated Show resolved Hide resolved
l1infotreesync/e2e_test.go Show resolved Hide resolved
reorgdetector/reorgdetector_sub.go Outdated Show resolved Hide resolved
reorgdetector/types.go Show resolved Hide resolved
reorgdetector/reorgdetector.go Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Aug 27, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
27.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@begmaroman begmaroman merged commit 7c419d5 into develop Aug 27, 2024
7 of 9 checks passed
@begmaroman begmaroman deleted the feature/CDK-385 branch August 27, 2024 09:24
Stefan-Ethernal pushed a commit that referenced this pull request Sep 17, 2024
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.

4 participants