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

Implemented raw block storage and RPC endpoint for retrieving raw block by number #340

Merged
merged 13 commits into from
May 4, 2024

Conversation

polydez
Copy link
Contributor

@polydez polydez commented May 2, 2024

Resolves #316

@polydez polydez requested review from hackaugusto and bobbinth May 2, 2024 15:50
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few comments inline, the main one is about making sure the store always stays in a consistent state (even if block writing fails).

crates/store/src/config.rs Outdated Show resolved Hide resolved
crates/store/src/server/api.rs Outdated Show resolved Hide resolved
crates/store/src/blocks.rs Outdated Show resolved Hide resolved
crates/store/src/blocks.rs Outdated Show resolved Hide resolved
bin/node/miden-node.toml Outdated Show resolved Hide resolved
crates/store/src/blocks.rs Outdated Show resolved Hide resolved
crates/proto/proto/rpc.proto Outdated Show resolved Hide resolved
crates/proto/proto/store.proto Outdated Show resolved Hide resolved
@polydez polydez requested a review from bobbinth May 2, 2024 18:23
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of non-blocking comments inline.

Also:

  • Let's create an issue to figure out how to do streaming.
  • The number of block we'll create per year is probably going to be over 10 million and I'm not sure much of a problem it would be. Regardless of this, we may want to figure out some strategy for storing blocks in sub-directories (e.g., define an epoch and store blocks for each epoch in a separate sub-directory). Let's create an issue for this.

crates/store/src/state.rs Outdated Show resolved Hide resolved
crates/store/src/state.rs Show resolved Hide resolved
@polydez
Copy link
Contributor Author

polydez commented May 3, 2024

@bobbinth:

  • The number of block we'll create per year is probably going to be over 10 million and I'm not sure much of a problem it would be. Regardless of this, we may want to figure out some strategy for storing blocks in sub-directories (e.g., define an epoch and store blocks for each epoch in a separate sub-directory). Let's create an issue for this.

Yes, I agree with you, such a big number of files in a single directory might be inconvenient to manage. I introduced epochs in this PR, please take a look! The main idea here is that we get first 16 bits of block number as an epoch, it means, each epoch will store 65536 blocks, it's about 7.5 days per epoch with current block issuing frequency (one block per 10 seconds).

@polydez polydez requested a review from bobbinth May 3, 2024 02:41
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! I left a few more comments inline.

crates/store/src/state.rs Outdated Show resolved Hide resolved
crates/store/src/server/mod.rs Outdated Show resolved Hide resolved
crates/store/src/state.rs Outdated Show resolved Hide resolved
Comment on lines +37 to +41
fn block_path(&self, block_num: u32) -> PathBuf {
let epoch = block_num >> 16;
let epoch_dir = self.store_dir.join(format!("{epoch:04x}"));
epoch_dir.join(format!("block_{block_num:08x}.dat"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine for now, but I would still create an issue to discuss what should be the length of an epoch. For example, our plan is to reduce block times to 3 - 4 seconds, in which case, the current approach would mean that an epoch is just 2 - 3 days. This may be OK but also may be too short.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use epoch only for limiting file count in the directory, I think, current epoch size is good. Even if we want to use it for something else, 2-3 days is still good (for example, in Solana each epoch lasts for about 2 days).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! I guess we can always introduce a higher-level periods that would be something like top 14 bits.

One potential change here: should epoch be formatted as: {epoch:08x} rather than {epoch:04x}?

@polydez polydez requested a review from bobbinth May 3, 2024 15:52
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of minor comments inline. Once these are addressed, let's fix the merge conflicts, and then merge.

Also, let's create an issue for refactoring of the apply_block() process as we discussed in the comments above.

Comment on lines 56 to 57
/// The underlying database.
db: Arc<Db>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would update the comment to something like: "The database which stores block headers, nullifiers, notes, and the latests states of accounts."

Comment on lines 59 to 60
/// The underlying block store.
block_store: Arc<BlockStore>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the comment to something like: "The block store which stores full block contents for all blocks."

polydez added 3 commits May 4, 2024 08:48
@polydez polydez merged commit 0549ebe into next May 4, 2024
5 checks passed
@polydez polydez deleted the polydez-block-storage branch May 4, 2024 04:06
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.

2 participants