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

Gossip pruned #1341

Merged
merged 2 commits into from
Aug 7, 2024
Merged

Gossip pruned #1341

merged 2 commits into from
Aug 7, 2024

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Jul 15, 2024

What was wrong?

fixes #981

How was it fixed?

  • add content that's dropped from local storage to propagation content vector
  • limit offer message in propagation to 64 content keys (according to spec)

To-Do

@njgheorghita njgheorghita force-pushed the gossip-pruned branch 3 times, most recently from beb4284 to 6329f89 Compare July 15, 2024 20:11
@njgheorghita njgheorghita mentioned this pull request Jul 17, 2024
1 task
@njgheorghita njgheorghita force-pushed the gossip-pruned branch 3 times, most recently from 956e625 to 16101da Compare July 19, 2024 16:23
@njgheorghita njgheorghita force-pushed the gossip-pruned branch 2 times, most recently from 8e14c13 to afdfae0 Compare July 29, 2024 18:55
@njgheorghita njgheorghita force-pushed the gossip-pruned branch 18 times, most recently from 9485061 to a54de6e Compare August 5, 2024 20:01
@njgheorghita njgheorghita marked this pull request as ready for review August 5, 2024 20:13
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit:

@njgheorghita njgheorghita requested a review from ogenev August 6, 2024 18:16
Copy link
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

Nicely done. Couple of suggestion, nothing major (except maybe calculating distance correctly).

interested_content.len() - 64
);
// sort content keys by distance to the node
interested_content.sort_by(|a, b| {
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 could be simplified to something like this:

interested_content.sort_by_cached_key(|(key, value)| {
  let key_as_array: [u8; 32] = std::array::from_fn(|i| key[i]);
  XorMetric::distance(&key_as_array, &enr.node_id().raw());
});

While writing this, I realized that we are calculating distance from content key, not content id, which I think is wrong. Is that intentional?
I would say that enrs_and_content should most likely be HashMap<String, Vec<(TContentKey, Vec<u8>)>>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, yup my mistake, we should be calculating on the id.

@@ -124,16 +128,22 @@ impl StateStorage {
let trie_node = TrieNode {
node: last_trie_node.clone(),
};
self.store
match self
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 can be simplified to:
self.store.insert(...).and(Ok(vec![]))

Optionally and in addition, you can let these functions return dropped content and handle it once in a put function above.

key: Self::Key,
value: V,
) -> Result<Vec<(Self::Key, Vec<u8>)>, ContentStoreError> {
match self.store(&key, &value.as_ref().to_vec()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as state store, this could be:
self.store(...).and(Ok(vec![])).

Or if you prefer this:
self.store(...).map(|_| vec![]).

@@ -52,7 +52,15 @@ pub trait ContentStore {
fn get(&self, key: &Self::Key) -> Result<Option<Vec<u8>>, ContentStoreError>;

/// Puts a piece of content into the store.
fn put<V: AsRef<[u8]>>(&mut self, key: Self::Key, value: V) -> Result<(), ContentStoreError>;
/// Returns a list of keys that were evicted from the store, which are gossiped into the
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe this

Suggested change
/// Returns a list of keys that were evicted from the store, which are gossiped into the
/// Returns a list of keys that were evicted from the store, which should be gossiped into the

@@ -75,7 +75,7 @@ pub fn delete_farthest(content_type: &ContentType) -> String {
ORDER BY distance_short DESC
LIMIT :limit
)
RETURNING content_size",
RETURNING content_key, content_value",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I would still return content_size as well. This should save us some calculations and make sure we are using the right value.

@@ -241,7 +242,7 @@ impl<TContentKey: OverlayContentKey> IdIndexedV1Store<TContentKey> {
&mut self,
content_key: &TContentKey,
content_value: Vec<u8>,
) -> Result<(), ContentStoreError> {
) -> Result<Vec<(TContentKey, Vec<u8>)>, ContentStoreError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Update function documentation.

@@ -275,12 +276,13 @@ impl<TContentKey: OverlayContentKey> IdIndexedV1Store<TContentKey> {
self.usage_stats.total_entry_size_bytes += content_size as u64;
self.usage_stats.report_metrics(&self.metrics);

let mut dropped_content: Vec<(TContentKey, Vec<u8>)> = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I don't think that you need to declare type here, so simple let mut dropped_content = vec![]; should be enough.

Also, you can do something like this:

let mut dropped_content = if self.pruning_strategy.should_prune(&self.usage_stats) {
    self.prune()?
} else {
    vec![]
}

@njgheorghita njgheorghita merged commit 5a763d1 into ethereum:master Aug 7, 2024
8 checks passed
@njgheorghita njgheorghita deleted the gossip-pruned branch August 7, 2024 16:14
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.

Trin should gossip content before it drops it from storage
3 participants