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

Remove block contents from mempool #485

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Remove block contents from mempool #485

merged 2 commits into from
Oct 30, 2023

Conversation

zeegomo
Copy link
Contributor

@zeegomo zeegomo commented Oct 27, 2023

Let's signal to the mempool which items were included in a block already to avoid reinclusion.

Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines 5 to 11
pub trait Attestation {
type Blob: Blob;
type Hash: Hash + Eq + Clone;
fn blob(&self) -> <Self::Blob as Blob>::Hash;
fn hash(&self) -> <Self::Blob as Blob>::Hash;
fn hash(&self) -> Self::Hash;
fn as_bytes(&self) -> Bytes;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why adding the Hash associated type? Reasoning for using the blob one was to mark that the return hash is not the attestation/certificate/whatever but the related blob one. Which now that I think of it it makes sense at type level but maybe not so much as it cannot be enforced (neither I added documentation, completely wrong on my side).

Copy link
Contributor Author

@zeegomo zeegomo Oct 30, 2023

Choose a reason for hiding this comment

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

Reasoning for using the blob one was to mark that the return hash is not the attestation/certificate/whatever but the related blob one.

For that we have the blob method. Attestations and certificates could be for the same blob but still be different (e.g. produced by different nodes) and we need a way to differentiate those.

Why adding the Hash associated type?

In theory we could reuse the blob type to enforce that all the items for DA use the same hash type but I could not find a way to write a generic constraint like Attestation<Blob::Hash = T> (rust-lang/rust#52662), so I had to add this as a separate trait instead

@zeegomo zeegomo merged commit e505618 into master Oct 30, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants