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

Add check to not overwrite an existing key when migrating after downgrade #5709

Closed
wants to merge 3 commits into from

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Mar 15, 2024

Motivation

Downgrading to v1.3.x after upgrading to v1.4.x will generate a new key that when the node is updated again to v1.4.x will overwrite the existing key.

This adds a check that prevents the node from overwriting a possibly already existing key.

Description

When migrating the key the code now checks if there is already a local.key before copying the old key there.

Test Plan

  • added test for this specific occurance

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed
  • Update changelog as needed

@fasmat fasmat self-assigned this Mar 15, 2024
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 79.8%. Comparing base (d8fa47c) to head (251c7be).

Files Patch % Lines
node/node_identities.go 71.4% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #5709   +/-   ##
=======================================
  Coverage     79.7%   79.8%           
=======================================
  Files          279     279           
  Lines        28527   28541   +14     
=======================================
+ Hits         22755   22782   +27     
+ Misses        4202    4185   -17     
- Partials      1570    1574    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fasmat fasmat changed the title Add check to not overwrite an existing key Add check to not overwrite an existing key when migrating Mar 15, 2024
@fasmat fasmat changed the title Add check to not overwrite an existing key when migrating Add check to not overwrite an existing key when migrating after downgrade Mar 15, 2024
@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Mar 15, 2024
…rade (#5709)

## Motivation

Downgrading to v1.3.x after upgrading to v1.4.x will generate a new key that when the node is updated again to v1.4.x will overwrite the existing key.

This adds a check that prevents the node from overwriting a possibly already existing key.
@fasmat fasmat force-pushed the prevent-users-overwriting-keys branch from e57bcfa to 26f4227 Compare March 15, 2024 13:18
@spacemesh-bors
Copy link

Canceled.

@fasmat fasmat force-pushed the prevent-users-overwriting-keys branch from 26f4227 to 5ebe3c8 Compare March 15, 2024 13:21
@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

bors merge

@fasmat fasmat force-pushed the prevent-users-overwriting-keys branch from 5ebe3c8 to 6f090dc Compare March 15, 2024 15:08
@spacemesh-bors
Copy link

Canceled.

@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Mar 15, 2024
…rade (#5709)

## Motivation

Downgrading to v1.3.x after upgrading to v1.4.x will generate a new key that when the node is updated again to v1.4.x will overwrite the existing key.

This adds a check that prevents the node from overwriting a possibly already existing key.
@spacemesh-bors
Copy link

Build failed (retrying...):

spacemesh-bors bot pushed a commit that referenced this pull request Mar 15, 2024
…rade (#5709)

## Motivation

Downgrading to v1.3.x after upgrading to v1.4.x will generate a new key that when the node is updated again to v1.4.x will overwrite the existing key.

This adds a check that prevents the node from overwriting a possibly already existing key.
@spacemesh-bors
Copy link

Build failed:

  • systest-status

@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

@dshulyak looks unrelated to the change of this PR and the other PR that was included in the bors batch

@fasmat fasmat force-pushed the prevent-users-overwriting-keys branch from 6f090dc to 251c7be Compare March 15, 2024 18:50
@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Mar 15, 2024
…rade (#5709)

## Motivation

Downgrading to v1.3.x after upgrading to v1.4.x will generate a new key that when the node is updated again to v1.4.x will overwrite the existing key.

This adds a check that prevents the node from overwriting a possibly already existing key.
@spacemesh-bors
Copy link

Build failed:

@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

rpc error: code = DeadlineExceeded

bors merge

@dshulyak
Copy link
Contributor

last job failure. all changes from march 8 were merged without single systest retry, i think there is either env fault or something else that makes it unusable

image

@dshulyak
Copy link
Contributor

based on merge history repeated retries started from 14th, i would check what in 12-13 changes could cause them, otherwise look at env

spacemesh-bors bot pushed a commit that referenced this pull request Mar 15, 2024
…rade (#5709)

## Motivation

Downgrading to v1.3.x after upgrading to v1.4.x will generate a new key that when the node is updated again to v1.4.x will overwrite the existing key.

This adds a check that prevents the node from overwriting a possibly already existing key.
@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

I would not say that they started march 14, we had failing merges every day for the last week or so. Most of them failed due to flaky tests which should now have been fixed 🤷

@fasmat
Copy link
Member Author

fasmat commented Mar 15, 2024

I don't immediately see any PR that was merged recently that could have caused system tests to be more flaky.

In the last 3 days we had merges of:

On March 11th we had a long string of failing merges because of flaky tests that should be fixed now. I am not sure a recent merge caused the issues we see 🤔

@pigmej
Copy link
Member

pigmej commented Mar 15, 2024

Env is the same, no changes on env side at all.

@spacemesh-bors
Copy link

Build failed:

@fasmat
Copy link
Member Author

fasmat commented Mar 16, 2024

Manually merged.

@fasmat fasmat closed this Mar 16, 2024
@fasmat fasmat deleted the prevent-users-overwriting-keys branch March 16, 2024 11:23
@fasmat
Copy link
Member Author

fasmat commented Mar 16, 2024

@dshulyak The node probably didn't publish an ATX because of this error which happened in epoch 5 and persisted for the rest of the run:

{"L":"WARN","T":"2024-03-15T21:22:15.583Z","N":"node.sync","M":"background atx sync failed","sessionId":"b4540da2","epoch_id":5,"errmsg":"failed to persist state for epoch 5: insert synced atx id 5/f34cac63b5 failed: prepare insert into atx_sync_state \n\t\t(epoch, id, requests) values (?1, ?2, ?3)\n\t\ton conflict(epoch, id) do update set requests = ?3;: sqlite.Stmt.ClearBindings: SQLITE_INTERRUPT (insert into atx_sync_state \n\t\t(epoch, id, requests) values (?1, ?2, ?3)\n\t\ton conflict(epoch, id) do update set requests = ?3;)","name":"sync"}

{"L":"WARN","T":"2024-03-15T21:23:15.606Z","N":"node.sync","M":"background atx sync failed","sessionId":"0d233121","epoch_id":6,"errmsg":"failed to persist state for epoch 6: begin: sqlite.Stmt.Step: SQLITE_BUSY: database is locked (BEGIN IMMEDIATE;)","name":"sync"}  
{"L":"WARN","T":"2024-03-15T21:24:15.621Z","N":"node.sync","M":"background atx sync failed","sessionId":"a2b64219","epoch_id":7,"errmsg":"failed to persist state for epoch 7: begin: sqlite.Stmt.Step: SQLITE_BUSY: database is locked (BEGIN IMMEDIATE;)","name":"sync"}
{"L":"WARN","T":"2024-03-15T21:25:15.640Z","N":"node.sync","M":"background atx sync failed","sessionId":"e9039bf3","epoch_id":8,"errmsg":"failed to persist state for epoch 8: begin: sqlite.Stmt.Step: SQLITE_BUSY: database is locked (BEGIN IMMEDIATE;)","name":"sync"}

https://grafana.spacemesh.dev/explore?panes=%7B%22_J_%22:%7B%22datasource%22:%22loki-gke-dev%22,%22queries%22:%5B%7B%22refId%22:%22A%22,%22expr%22:%22%7Bnamespace%3D%5C%22test-dvrv%5C%22,%20pod%3D%5C%22smesher-4-7469ccd99d-8rzpr%5C%22%7D%20%7C~%20%5C%22ERROR%7CWARN%5C%22%22,%22queryType%22:%22range%22,%22datasource%22:%7B%22type%22:%22loki%22,%22uid%22:%22loki-gke-dev%22%7D,%22editorMode%22:%22code%22%7D%5D,%22range%22:%7B%22from%22:%221710536160000%22,%22to%22:%221710538860000%22%7D%7D%7D&schemaVersion=1&orgId=1

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.

3 participants