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

Bucket cleanup #4093

Merged
merged 4 commits into from
Feb 14, 2024
Merged

Bucket cleanup #4093

merged 4 commits into from
Feb 14, 2024

Conversation

SirTyson
Copy link
Contributor

Description

Resolves #4031

This PR includes cleanups and hardening of Bucket related code.

  • loadPoolShareTrustlinesByAccountAndAsset: Extended a test to make sure this function does not return outdated entries. This was working properly, but did not have an explicit test. I've also added an explicit seenKeys set instead of relying on the fact that unordered_map::emplace will not update a key if it already exists for more defensive programming. Finally, there was an edge case where if a Bucket only contained offers and did not contain a METAENTRY, the scan would return early. This should never happen in practice, but it has been fixed.
  • XDRStream audit: I took a deep dive into ifstream and ran several experiments such as disabling stream buffering. I've concluded that ifstream with buffering is still the optimal file IO interface for BucketsDB. I've also checked that we're handling EOF and bad bits properly, and could not find any dependencies on out-of-spec behavior. The only code change was a minor scoping issue on a tracy metric.
  • Added separate stream for BucketDB scans and eviction scans.
  • Warn more aggressively if eviction scans get stuck. Previously, there was only a single warning per stuck bucket, and there we certain edge cases where the warning would not be triggered, such as if a large bucket was almost entirely TEMPORARY entries. Now, we warn on every ledger and check much more aggressively.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

src/bucket/Bucket.cpp Outdated Show resolved Hide resolved
src/bucket/Bucket.cpp Outdated Show resolved Hide resolved
src/bucket/Bucket.h Show resolved Hide resolved
src/bucket/BucketList.cpp Show resolved Hide resolved
src/bucket/Bucket.cpp Outdated Show resolved Hide resolved
src/util/XDRStream.h Outdated Show resolved Hide resolved
src/bucket/Bucket.cpp Show resolved Hide resolved
src/bucket/BucketList.cpp Show resolved Hide resolved
@SirTyson SirTyson force-pushed the bucket-cleanup branch 2 times, most recently from 32784b6 to 888b313 Compare February 13, 2024 19:41
@marta-lokhova
Copy link
Contributor

r+ 4f0789d

@latobarita latobarita merged commit 7d17f27 into stellar:master Feb 14, 2024
15 checks passed
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.

Harden Bucket File IO
4 participants