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: add support of block sidecars for bsc #45

Merged
merged 4 commits into from
Jul 8, 2024
Merged

Conversation

pythonberg1997
Copy link
Contributor

Description

This pr is to add support of block sidecars for bsc

Rationale

tell us why we need these changes...

Example

add an example CLI or API response...

Changes

Notable changes:

  • add sidecars field to Block and SealedBlock
  • add sidecars table to db

Potential Impacts

  • add potential impacts for other components here
  • ...

@pythonberg1997 pythonberg1997 force-pushed the roshan/parlia branch 6 times, most recently from 24c036b to 250248f Compare June 27, 2024 07:34
@@ -59,6 +59,7 @@ impl TryFrom<alloy_rpc_types::Block> for Block {
withdrawals: block.withdrawals.map(Into::into),
// todo(onbjerg): we don't know if this is added to rpc yet, so for now we leave it as
// empty.
sidecars: None, // TODO(roshan)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the TODO for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@@ -289,8 +289,12 @@ impl PendingBlockEnv {
requests_root,
};

// TODO: add sidecars
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will we fix this todo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@@ -297,6 +306,11 @@ impl<DB: Database, D: BodyDownloader> Stage<DB> for BodyStage<D> {
withdrawals_cursor.delete_current()?;
}

// Delete the sidecars entry if any
Copy link
Collaborator

@forcodedancing forcodedancing Jun 27, 2024

Choose a reason for hiding this comment

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

By the way, for unwind, did we unwind snapshots ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's unnecessary. The saved snapshots are always behind current height. So we will always get correct snapshot. And unwinding snapshots is somehow difficult to do.

@@ -12,16 +20,49 @@ use reth_stages_api::{
#[non_exhaustive]
pub struct FinishStage;

impl FinishStage {
/// prune sidecars older than `BLOCKS_KEPT` blocks
pub fn prune_sidecars<DB: Database>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we put prune here? It's kind of wired - for other pruning (header, body ...), they do not execute here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@@ -35,6 +35,7 @@ mod metrics;
mod provider;

pub use provider::{DatabaseProvider, DatabaseProviderRO, DatabaseProviderRW};
use reth_storage_api::SidecarsProvider;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about put the import along with other provides in line 5-9?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

};
use reth_prune_types::{PruneCheckpoint, PruneLimiter, PruneModes, PruneSegment};
use reth_stages_types::{StageCheckpoint, StageId};
use reth_storage_api::SidecarsProvider;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about put the import along with others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -85,6 +90,7 @@ impl EthStateCache {
provider,
full_block_cache: BlockLruCache::new(max_blocks, "blocks"),
receipts_cache: ReceiptsLruCache::new(max_receipts, "receipts"),
sidecars_cache: SidecarsLruCache::new(max_receipts, "sidecars"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

max_receipts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to max_blocks. I think we could reuse max_blocks to avoid a new param as there is 1 sidecars in 1 block.

let start_prune = if current_block > 1_000_000 { current_block - 1_000_000 } else { 0 };

let block_range = start_prune..current_block;
let mut limiter = PruneLimiter::default().set_time_limit(Duration::from_secs(3600));
Copy link
Collaborator

@unclezoro unclezoro Jun 27, 2024

Choose a reason for hiding this comment

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

so the blob share the same database with plainstate, hashstate? This seems unacceptable.

@@ -414,6 +414,9 @@ tables! {

/// Stores the parlia snapshot data by block hash.
table ParliaSnapshot<Key = BlockHash, Value = Snapshot>;

/// Stores block sidecars, indexed by block number.
table BlockSidecars<Key = BlockNumber, Value = BlobSidecars>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not we expected, blob storage size is huge, we can not store it in a same database with other kind of state, it will cause huge state explosion

@1nanosecond 1nanosecond added this pull request to the merge queue Jul 8, 2024
Merged via the queue into develop with commit 00259fa Jul 8, 2024
33 checks passed
pythonberg1997 added a commit that referenced this pull request Jul 10, 2024
forcodedancing added a commit to forcodedancing/reth that referenced this pull request Jul 10, 2024
forcodedancing added a commit to forcodedancing/reth that referenced this pull request Jul 10, 2024
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.

4 participants