Skip to content

refactor: remove memory storage and indexes#1219

Merged
msbrogli merged 1 commit intomasterfrom
chore/remove-memory
Mar 14, 2025
Merged

refactor: remove memory storage and indexes#1219
msbrogli merged 1 commit intomasterfrom
chore/remove-memory

Conversation

@glevco
Copy link
Contributor

@glevco glevco commented Feb 5, 2025

Depends on #1218

Motivation

The exclusively in-memory storage and indexes haven't been very useful for a while, and generate maintenance overhead. Its implementation has to be fully compatible with its RocksDB counterpart, which is not true for some cases. For this and for simplicity, this PR completely removes support for in-memory storage/indexes, and updates all tests to run using RocksDB instead. This also allows us to have better guarantees that we're testing the same behavior that we observe in production.

A new CLI option --temp-data is provided to be used instead of --memory-storage. It requires no arguments and creates a temporary directory that is automatically cleaned up when the full node exits.

Acceptance Criteria

  • Changes in CLI args:
    • Deprecate --memory-storage and --memory-indexes.
    • Remove --rocksdb-storage and --x-rocksdb-indexes (because they're now the only possibility).
  • Remove all classes that were exclusively being used by memory storage or indexes.
  • Update Builder and CLIBuilder accordingly.
  • Refactor settings management on indexes so they all use injected settings.
  • Remove TimestampIndex.get_hashes_and_next_idx() as it's unused (this was a Sync-v1 only method).
  • All tests are changed to use RocksDB instead of memory.
    • This surfaced a few bugs/incompatible implementations that were also fixed in this PR. See the comments in the respective test files.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco self-assigned this Feb 5, 2025
@glevco glevco changed the title wip chore: remove memory storage and indexes Feb 5, 2025
@glevco glevco changed the title chore: remove memory storage and indexes refactor: remove memory storage and indexes Feb 5, 2025
@glevco glevco force-pushed the chore/remove-memory branch 4 times, most recently from 14cddd1 to c2d2754 Compare February 5, 2025 21:44
@codecov
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 88.46154% with 15 lines in your changes missing coverage. Please review.

Project coverage is 84.14%. Comparing base (6f3124a) to head (80c5ae3).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
hathor/builder/builder.py 63.15% 7 Missing and 7 partials ⚠️
hathor/indexes/rocksdb_mempool_tips_index.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1219      +/-   ##
==========================================
- Coverage   84.36%   84.14%   -0.23%     
==========================================
  Files         320      311       -9     
  Lines       24471    23868     -603     
  Branches     3748     3651      -97     
==========================================
- Hits        20646    20083     -563     
+ Misses       3098     3071      -27     
+ Partials      727      714      -13     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@glevco glevco force-pushed the chore/remove-memory branch from c2d2754 to 47c30c2 Compare February 6, 2025 00:50
@glevco glevco marked this pull request as ready for review February 6, 2025 01:11
@glevco glevco force-pushed the refactor/cleanup-full-verification branch from 7ca1e19 to 40650fb Compare February 11, 2025 20:56
@glevco glevco force-pushed the chore/remove-memory branch from 47c30c2 to 7038204 Compare February 11, 2025 20:59
@glevco glevco force-pushed the refactor/cleanup-full-verification branch from 40650fb to 57a9c4c Compare February 18, 2025 15:31
@glevco glevco force-pushed the chore/remove-memory branch from 7038204 to b52a3d4 Compare February 18, 2025 16:25
Base automatically changed from refactor/cleanup-full-verification to master February 18, 2025 20:53
jansegre
jansegre previously approved these changes Feb 19, 2025
Copy link
Member

@jansegre jansegre left a comment

Choose a reason for hiding this comment

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

The proposal looks good to me. I'm only a bit worried about deploys that still use --memory-indexes, but I guess they can wait to update when it works well with rocksdb indexes, which we might have to prioritize for the next releases.

@glevco glevco force-pushed the chore/remove-memory branch 2 times, most recently from abbc32e to b52a3d4 Compare March 14, 2025 17:35
@glevco glevco force-pushed the chore/remove-memory branch 2 times, most recently from a02a394 to 1aec649 Compare March 14, 2025 17:45
@github-actions
Copy link

🐰 Bencher Report

Branchchore/remove-memory
Testbedubuntu-22.04

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results
BenchmarkLatencyminutes (m)
sync-v2 (up to 20000 blocks)📈 view plot
⚠️ NO THRESHOLD
1.64 m
🐰 View full continuous benchmarking report in Bencher

@glevco glevco force-pushed the chore/remove-memory branch from 1aec649 to 80c5ae3 Compare March 14, 2025 17:55
@msbrogli msbrogli merged commit 80c5ae3 into master Mar 14, 2025
11 of 18 checks passed
@msbrogli msbrogli deleted the chore/remove-memory branch March 14, 2025 18:51
@github-project-automation github-project-automation bot moved this from In Review (Done) to Waiting to be deployed in Hathor Network Mar 14, 2025
@jansegre jansegre mentioned this pull request Jul 22, 2025
2 tasks
@jansegre jansegre moved this from Waiting to be deployed to Done in Hathor Network Jul 22, 2025
@jansegre jansegre mentioned this pull request Aug 7, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants