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 on disk blob pool #5389

Merged
merged 8 commits into from
Nov 14, 2023
Merged

feat: add on disk blob pool #5389

merged 8 commits into from
Nov 14, 2023

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Nov 11, 2023

adds a simple file based on disk blob store

blobs are stored rlp encoded in individual files, file derived from hash

@DoTheBestToGetTheBest
Copy link
Contributor

Hey, this is amazing. I wrote some method, i want to push the code but i can't

impl BlobStore for DiskFileBlobStore {
    fn insert(&self, tx: B256, data: BlobTransactionSidecar) -> Result<(), BlobStoreError> {
        todo!()
    }

    fn insert_all(&self, txs: Vec<(B256, BlobTransactionSidecar)>) -> Result<(), BlobStoreError> {
        todo!()
    }

    fn delete(&self, tx: B256) -> Result<(), BlobStoreError> {
        let file_path = self.inner.blob_dir.join(tx.to_string());
        if file_path.exists() {
            let file_size = file_path.metadata().unwrap().len();
            fs::remove_file(&file_path).unwrap();
            // Decrement the size tracker by the size of the removed file
            self.inner.size_tracker.sub_size(file_size as usize);
            // Decrement the blob count by 1
            self.inner.size_tracker.update_len(self.inner.size_tracker.blobs_len() - 1);
            Ok(())
        } else {
            Err(BlobStoreError::MissingSidecar(tx))
        }
    }

    fn delete_all(&self, txs: Vec<B256>) -> Result<(), BlobStoreError> {
        if txs.is_empty() {
            return Ok(())
        }

        let mut total_sub = 0;
        let mut count_removed = 0;

        for tx in txs {
            let file_path = self.inner.blob_dir.join(tx.to_string());
            if file_path.exists() {
                let file_size = file_path.metadata().unwrap().len() as usize;
                fs::remove_file(&file_path).unwrap();
                total_sub += file_size;
                count_removed += 1;
            }
        }

        self.inner.size_tracker.sub_size(total_sub);
        self.inner.size_tracker.update_len(self.inner.size_tracker.blobs_len() - count_removed);
        Ok(())
    }

    fn get(&self, tx: B256) -> Result<Option<BlobTransactionSidecar>, BlobStoreError> {
        let file_path = self.inner.blob_dir.join(tx.to_string());
        if file_path.exists() {
            let mut data = fs::read(&file_path).unwrap();
            let sidecar = BlobTransactionSidecar::decode_inner(&mut data.as_slice())
                .map_err(|e| BlobStoreError::DecodeError(e))
                .unwrap();

            Ok(Some(sidecar))
        } else {
            Ok(None)
        }
    }

    fn get_all(
        &self,
        txs: Vec<B256>,
    ) -> Result<Vec<(B256, BlobTransactionSidecar)>, BlobStoreError> {
        let mut items = Vec::with_capacity(txs.len());

        for tx in txs {
            let file_path = self.inner.blob_dir.join(tx.to_string());
            if file_path.exists() {
                let mut data = fs::read(&file_path).unwrap();
                let sidecar = BlobTransactionSidecar::decode_inner(&mut data.as_slice())
                    .map_err(|e| BlobStoreError::DecodeError(e))?;

                items.push((tx, sidecar));
            }
        }

        Ok(items)
    }

    fn get_exact(&self, txs: Vec<B256>) -> Result<Vec<BlobTransactionSidecar>, BlobStoreError> {
        let mut items = Vec::with_capacity(txs.len());

        for tx in txs {
            let file_path = self.inner.blob_dir.join(tx.to_string());
            if file_path.exists() {
                let mut data = fs::read(&file_path).unwrap();
                let sidecar = BlobTransactionSidecar::decode_inner(&mut data.as_slice())
                    .map_err(|e| BlobStoreError::DecodeError(e))
                    .unwrap();

                items.push(sidecar);
            } else {
                return Err(BlobStoreError::MissingSidecar(tx))
            }
        }

        Ok(items)
    }

    fn data_size_hint(&self) -> Option<usize> {
        Some(self.inner.size_tracker.data_size())
    }

    fn blobs_len(&self) -> usize {
        self.inner.size_tracker.blobs_len()
    }
}

@DoTheBestToGetTheBest
Copy link
Contributor

I didn't know how to write the insert since i can't convert data to bytes so if any hint on this i can continue for it

@DoTheBestToGetTheBest
Copy link
Contributor

Changed also decode and encode method to public since it was private method

@mattsse
Copy link
Collaborator Author

mattsse commented Nov 11, 2023

amazing, do you want to branch off this PR, make changes and then open a new PR against this branch, so I can review?

@DoTheBestToGetTheBest
Copy link
Contributor

amazing, do you want to branch off this PR, make changes and then open a new PR against this branch, so I can review?

it is not better to just push to your branch ? what ever you want, i do it, no problem for me boss :)

@rkrasiuk rkrasiuk added C-enhancement New feature or request E-cancun Related to the Cancun network upgrade labels Nov 13, 2023
@mattsse mattsse marked this pull request as ready for review November 14, 2023 13:43
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

LGTM so far, pretty simple and should work

crates/transaction-pool/src/blobstore/disk.rs Outdated Show resolved Hide resolved
Comment on lines +393 to +399
pub enum OpenDiskFileBlobStore {
/// Clear everything in the blob store.
#[default]
Clear,
/// Keep the existing blob store and index
ReIndex,
}
Copy link
Member

Choose a reason for hiding this comment

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

guessing this part will be implemented later because this part of the config is ignored when the store is created

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, since we don't retire pooled txs to disk, this is redundant atm, just a placeholder

Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com>
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

LGTM

@mattsse mattsse added this pull request to the merge queue Nov 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 14, 2023
@mattsse mattsse added this pull request to the merge queue Nov 14, 2023
Merged via the queue into main with commit 100b6b8 Nov 14, 2023
26 checks passed
@mattsse mattsse deleted the matt/add-disk-blob-pool branch November 14, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request E-cancun Related to the Cancun network upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants