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

Feat: Create immutable builder for incremental Cardano DB #2223

Merged
merged 23 commits into from
Jan 17, 2025

Conversation

sfauvel
Copy link
Collaborator

@sfauvel sfauvel commented Jan 14, 2025

Content

The CardanoDatabaseArtifactBuilder can build and upload immutables files and digests file

Implement the artifact builder for the Incremental Cardano Database with the target JSON structure:

{
  "merkle_root": "0506e5e1f7df9314ebdbb532768a5addc2931004c5cd8fd8cd464090f95e46e4",
  "beacon": {
    "network": "mainnet",
    "epoch": 525,
    "immutable_file_number": 6550
  },
  "certificate_hash": "04219eb7a66c37f0d917160b8bdc617dc4ff911344f2e72bdb22c1316cbe8d71",
  "total_db_size_uncompressed": 52470081414,
  "created_at": "2024-12-02T04:36:40.811764301Z",
  "locations": {
    "digests": [
      {
        "type": "aggregator",
        "uri": "https://aggregator.release-mainnet.api.mithril.network/aggregator/artifact/cardano-database/digests"
      },
      {
        "type": "cloud_storage",
        "uri": "https://storage.googleapis.com/cdn.aggregator.release-mainnet.api.mithril.network/mainnet-e525-i6550.digests.tar.zst"
      }
    ],
    "immutables": [
      {
        "type": "cloud_storage",
        "uri": {
                "Template": "https://storage.googleapis.com/cdn.aggregator.release-mainnet.api.mithril.network/mainnet-e525-i6550.{immutable_file_number}.tar.zst"
        }
      },
      {
        "type": "bit_torrent",
        "uri": {
                "Template": "https://storage.googleapis.com/cdn.aggregator.release-mainnet.api.mithril.network/mainnet-e525-i6550.{immutable_file_number}.tar.zst"
        }
      }
    ],
    "ancillary": [
      {
        "type": "cloud_storage",
        "uri": "https://storage.googleapis.com/cdn.aggregator.release-mainnet.api.mithril.network/mainnet-e525.ancillary.tar.zst"
      }
    ]
  },
  "compression_algorithm": "zstandard",
  "cardano_node_version": "10.1.2"
}

And the target design:

Image

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Issue(s)

Closes #2151

Copy link

github-actions bot commented Jan 14, 2025

Test Results

    4 files  ± 0     52 suites  ±0   10m 18s ⏱️ -28s
1 524 tests +31  1 524 ✅ +31  0 💤 ±0  0 ❌ ±0 
1 780 runs  +31  1 780 ✅ +31  0 💤 ±0  0 ❌ ±0 

Results for commit 1b1913b. ± Comparison against base commit 90f44cd.

This pull request removes 6 and adds 37 tests. Note that renamed tests count towards both.
mithril-aggregator ‑ snapshotter::tests::test_dumb_snapshotter_snasphot_return_archive_name_with_size_0
mithril-common ‑ entities::cardano_database::tests::compute_hash_returns_different_hash_with_different_beacon
mithril-common ‑ entities::cardano_database::tests::compute_hash_returns_different_hash_with_different_merkle_root
mithril-common ‑ entities::cardano_database::tests::compute_hash_returns_different_hash_with_same_epoch_in_beacon
mithril-common ‑ entities::cardano_database::tests::compute_hash_returns_same_hash_with_same_cardano_database_snapshot
mithril-common ‑ entities::cardano_database::tests::test_cardano_database_snapshot_compute_hash
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::create_digest_file_should_create_json_file_with_all_digests
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::digest_artifact_builder_return_digests_route_on_aggregator
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::upload_digest_file_should_log_upload_errors
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::upload_digest_file_should_not_error_even_if_no_location_returned_from_uploaders
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::upload_digest_file_should_return_all_uploaders_returned_locations
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::upload_digest_file_should_return_location_even_with_uploaders_errors
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::digest::tests::upload_should_call_upload_with_created_digest_file_and_delete_the_file
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::immutable::tests::batch_upload::extract_archive_name_to_deduce_template_location
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::immutable::tests::batch_upload::returns_error_when_uploaded_filename_not_templatable_without_5_digits
mithril-aggregator ‑ artifact_builder::cardano_database_artifacts::immutable::tests::create_archive::return_all_archives_but_not_rebuild_archives_already_compressed
…

♻️ This comment has been updated with latest results.

@sfauvel sfauvel changed the title create immutable builder Create immutable builder for incremental Cardano DB Jan 15, 2025
@sfauvel sfauvel force-pushed the ensemble/2151/create-immutable-builder branch from ea49807 to 8d7a22a Compare January 15, 2025 08:53
@sfauvel sfauvel temporarily deployed to testing-preview January 15, 2025 09:02 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet January 15, 2025 09:02 — with GitHub Actions Inactive
@sfauvel sfauvel force-pushed the ensemble/2151/create-immutable-builder branch from 563b5bd to 14d1067 Compare January 16, 2025 08:38
@sfauvel sfauvel changed the title Create immutable builder for incremental Cardano DB Feat: Create immutable builder for incremental Cardano DB Jan 16, 2025
@sfauvel sfauvel temporarily deployed to testing-sanchonet January 16, 2025 10:36 — with GitHub Actions Inactive
@sfauvel sfauvel marked this pull request as ready for review January 16, 2025 10:37
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sfauvel sfauvel requested review from Alenar and dlachaume January 16, 2025 13:27
@sfauvel sfauvel force-pushed the ensemble/2151/create-immutable-builder branch from 290570c to 7a38984 Compare January 16, 2025 15:24
@sfauvel sfauvel temporarily deployed to testing-preview January 16, 2025 15:33 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet January 16, 2025 15:33 — with GitHub Actions Inactive
@sfauvel sfauvel force-pushed the ensemble/2151/create-immutable-builder branch from 7a38984 to fc43569 Compare January 16, 2025 15:35
@sfauvel sfauvel temporarily deployed to testing-preview January 16, 2025 15:47 — with GitHub Actions Inactive
@sfauvel sfauvel temporarily deployed to testing-sanchonet January 16, 2025 15:47 — with GitHub Actions Inactive
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Just a quick note, I think the location examples in the openapi.yaml file should be updated with the example provided in the description of this PR.
The examples for CardanoDatabaseArtifactsLocationsMessagePart and CardanoDatabaseSnapshotMessage should be updated with the values from the JSON above.

Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

I've some doubt on the new methods added to the snapshotter trait but this can be addressed in another PR.

/// Define the ability to create snapshots.
pub trait Snapshotter: Sync + Send {
/// Create a new snapshot with the given filepath.
fn snapshot_all(&self, filepath: &Path) -> StdResult<OngoingSnapshot>;

/// Create a new snapshot with the given filepath from a subset of directories and files.
fn snapshot_subset(&self, filepath: &Path, files: Vec<PathBuf>) -> StdResult<OngoingSnapshot>;

/// Check if the snapshot exists.
fn is_snapshot_exist(&self, filepath: &Path) -> bool;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not wrong there's a little grammar error here, it should be does instead of is

Suggested change
fn is_snapshot_exist(&self, filepath: &Path) -> bool;
fn does_snapshot_exist(&self, filepath: &Path) -> bool;

Comment on lines 28 to 32
/// Check if the snapshot exists.
fn is_snapshot_exist(&self, filepath: &Path) -> bool;

/// Give the full target path for the filepath.
fn get_file_path(&self, filepath: &Path) -> PathBuf;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those new methods are difficult to understand and use.

They operate using relative path from a cardano database directory:

  • This provide no abstraction for callers as they need to know exactly the structure of a cardano database meaning that the knowledge of it is spread out on all callers.
  • This allow incorrect use such as giving an absolute path.

We could refactor the whole trait the following way:

  • make public the TarAppender trait (maybe rename it) and change it's implementers to be domain based:
    • ImmutableFileFullAppender instead of AppenderDirAll
    • AncilaryAppender and ImmutableTrioAppender instead of AppenderEntries
  • replace the snapshot_all and snapshot_subset methods of the snapshotter with a unique snapshot method that take those TarAppender implementation as a parameter.

Note

An alternative would be to introduce a new type instead, maybe an enum, and keep the existing appender system private with a mechanism that construct the appropriate appender for a given new type.

This refactor would allow to remove the needs for snapshotter user to have knowledge of the cardano database inner structure. Simplified usage example:

  • before:
if !snapshot_all.is_snapshot_exist(Path::from('immutables_trio-i4378.tar.zst')) {
  snapshotter.snapshot_subset(
    vec![
      PathBuff::from(IMMUTABLE_DIR).join("04378.chunk"),
      PathBuff::from(IMMUTABLE_DIR).join("04378.primary"),
      PathBuff::from(IMMUTABLE_DIR).join("04378.secondary")
    ]
  );
}
  • after:
let appender = ImmutableTrioAppender::immutable(4378);
if !snapshot_all.is_snapshot_exist(&appender) {
  snapshotter.snapshot(appender);
}

dlachaume and others added 4 commits January 17, 2025 12:15
@dlachaume dlachaume force-pushed the ensemble/2151/create-immutable-builder branch from 453246b to a110e44 Compare January 17, 2025 11:16
* mithril-aggregator from `0.6.12` to `0.6.13`
* mithril-common from `0.4.104` to `0.4.105`
* openapi.yaml from `0.1.41` to `0.1.42`
@dlachaume dlachaume force-pushed the ensemble/2151/create-immutable-builder branch from a110e44 to 1b1913b Compare January 17, 2025 11:20
@dlachaume dlachaume temporarily deployed to testing-sanchonet January 17, 2025 11:37 — with GitHub Actions Inactive
@dlachaume dlachaume merged commit f236320 into main Jan 17, 2025
43 checks passed
@dlachaume dlachaume deleted the ensemble/2151/create-immutable-builder branch January 17, 2025 11:39
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.

Implement artifacts builder for Incremental Cardano DB
4 participants