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

support non-clean shutdown #1039

Merged
merged 28 commits into from
Sep 1, 2022
Merged

support non-clean shutdown #1039

merged 28 commits into from
Sep 1, 2022

Conversation

tsahee
Copy link
Contributor

@tsahee tsahee commented Aug 29, 2022

uses geth shutdown markers (requires stopping blockchain before backend)

logs time for block creation fo blockchain will write trie cache

Pulls in OffchainLabs/go-ethereum#155

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Aug 29, 2022
joshuacolvin0
joshuacolvin0 previously approved these changes Aug 29, 2022
Copy link
Member

@joshuacolvin0 joshuacolvin0 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #1039 (f1a7d09) into master (f68775b) will decrease coverage by 0.27%.
The diff coverage is 42.42%.

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
- Coverage   48.03%   47.75%   -0.28%     
==========================================
  Files         218      218              
  Lines       25180    25207      +27     
  Branches      523      523              
==========================================
- Hits        12094    12037      -57     
- Misses      11477    11538      +61     
- Partials     1609     1632      +23     

@tsahee tsahee marked this pull request as draft August 30, 2022 05:05
@tsahee tsahee marked this pull request as ready for review August 30, 2022 16:21
@PlasmaPower PlasmaPower mentioned this pull request Aug 30, 2022
PlasmaPower
PlasmaPower previously approved these changes Aug 30, 2022
Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM

@PlasmaPower PlasmaPower dismissed their stale review August 30, 2022 17:04

Dismissing approval as Harry has some suggestions for the geth side

arbnode/node.go Outdated
Archive: false,
BlockCount: 128,
BlockAge: 30 * time.Minute,
TrieTimeLimit: 5 * time.Minute,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's update this to an hour and then LGTM :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've pushed up a commit changing this to an hour

Copy link
Member

@joshuacolvin0 joshuacolvin0 left a comment

Choose a reason for hiding this comment

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

LGTM

@PlasmaPower PlasmaPower merged commit a30288d into master Sep 1, 2022
@PlasmaPower PlasmaPower deleted the shutdown-markers branch September 1, 2022 20:54
@hdiass
Copy link

hdiass commented Sep 5, 2022

This only works if node works for more than 1 hours.. not an amazing solution as it can be in sync for less than a hour thus if restarted in less than 1 hour state gets bricked

@PlasmaPower
Copy link
Collaborator

@hdiass this is configurable with --node.caching.trie-time-limit=30m for instance to make it 30 minutes, but be aware that lowering the time to sync after an unclean shutdown increases the db size. We follow upstream geth behavior here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants