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

Improve the BOLD Challenge Cache #2459

Merged
merged 12 commits into from
Jul 16, 2024
Merged

Improve the BOLD Challenge Cache #2459

merged 12 commits into from
Jul 16, 2024

Conversation

rauljordan
Copy link
Contributor

This PR improves the BOLD challenge cache with feedback from @tsahee on https://github.com/OffchainLabs/nitro/pull/2391/files. It contains the following improvements:

  • Add a separate Init() method to the cache such that New has no side-effects
  • Creates a temp dir inside of the base dir on cache Init(). All temporary writes when putting to the cache will be made to temp files in this directory. This means when we Put(), we can just do os.CreateTemp within that dir. Once the writing completes, we do an atomic os.Rename of that file from the temp dir to the actual path it needs to live in
  • In the file hierarchy path name, we include message number as the first element so we can sort the files by time
  • Minor spelling mistakes
  • Adds a prune function that removes all items from the cache that has a message number <= N with tests

@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 Jul 8, 2024
@rauljordan rauljordan requested review from tsahee and magicxyyz July 8, 2024 14:42
@rauljordan rauljordan marked this pull request as draft July 8, 2024 14:42
@rauljordan rauljordan marked this pull request as ready for review July 8, 2024 14:42
staker/challenge-cache/cache.go Show resolved Hide resolved
staker/challenge-cache/cache.go Outdated Show resolved Hide resolved
staker/challenge-cache/cache_test.go Outdated Show resolved Hide resolved
staker/challenge-cache/cache_test.go Outdated Show resolved Hide resolved
staker/challenge-cache/cache_test.go Outdated Show resolved Hide resolved
staker/challenge-cache/cache_test.go Outdated Show resolved Hide resolved
}
t.Run("pruning non-existent dirs does nothing", func(t *testing.T) {
if err = cache.Prune(ctx, key.MessageHeight); err != nil {
t.Error(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it t.Error and not t.Fatal on urpose? (It's probably o.k. either way.. just making sure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so that it does not stop other runs

@rauljordan
Copy link
Contributor Author

Ready again @tsahee

@rauljordan rauljordan requested a review from tsahee July 15, 2024 16:12
Copy link
Collaborator

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

LGTM

@joshuacolvin0 joshuacolvin0 enabled auto-merge July 16, 2024 06:19
@joshuacolvin0 joshuacolvin0 merged commit ce6cedb into master Jul 16, 2024
13 checks passed
@joshuacolvin0 joshuacolvin0 deleted the cache-feedback-bold branch July 16, 2024 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved 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.

3 participants