Add content validation for accepted content #376

njgheorghita · 2022-07-11T19:30:52Z

What was wrong?

Content validation needed for accepted content.
Fixes #340

How was it fixed?

Spawned a non-blocking task to validate content once it's been accepted.
Refactored overlay_protocol.store_overlay_content() -> storage.store_if_should()
Removed self ref for propagate_gossip() so that it can be used inside tokio task

To-Do

Add entry to the release notes (may forgo for trivial changes)
Clean up commit history

ogenev

Looks good but I think we should try to pass a vector of content keys/values to propagate_gossip as an argument or take a different refactor direction with the idea to be able to propagate all the content with one OFFER message to the interested node.

ogenev · 2022-07-25T12:21:25Z

trin-core/src/portalnet/storage.rs

+    ) -> Result<bool, PortalStorageError> {
+        match self.should_store(key)? {
+            true => {
+                let _ = self.store(key, value)?;


Suggested change

let _ = self.store(key, value)?;

self.store(key, value)?;

newsfragments/376.added.md

ogenev · 2022-07-25T12:43:43Z

trin-core/src/portalnet/overlay.rs

+                            .store_if_should(&key, &content_value.to_vec());
+
+                        // Propagate all validated content, whether or not it was stored.
+                        Self::propagate_gossip((key, content_value), kbuckets, request_tx);


I understand the reason for propagating only one key/content_value at a time, but this prevents us from propagating multiple content items to an interested node with one OFFER message.

I definitely agree one offer message with many key/value > many messages with one key/value.

It seems we have 3 goals here, and one of them will need to sacrificed to achieve the other two.

Only OFFER content that's been validated

Dropping this requirement = potentially rebroadcasting invalid / malicious content into the network

Validate content in a non-blocking manner

Dropping this requirement = not possible, we need to validate content in a non-blocking manner, since content validation might require dropping back into the same overlay network to fetch another piece of data

Collect all validated content and offer it in a single message

Dropping this requirement = increases network traffic / congestion

imo, the last one is the best option to drop. It's possible I'm missing something here. Let me know what you think? Is there an alternative workaround here that can satisfy all three requirements?

I get that it's more complex, but I don't understand why it would be impossible, to collect all the validated content into a batch. When all of the content has been found valid or invalid, then it could all be sent all together, right?

FWIW, I think it would be fine to punt this into an issue for performance improvement.

I think it is fine to block the thread that is validating the content until all content is validated (we can use different threads in parallel for validating one content item and wait for all threads to finish) and then pass only the valid content items in batch to propagate_gossip as Jason mentioned.

ogenev · 2022-07-25T12:51:57Z

trin-core/src/portalnet/overlay.rs

+            enrs_and_content
+                .entry(enr.to_base64())
+                .or_default()
+                .push(content_key_value.clone().0.into());


This logic becomes obsolete if propagate_gossip accepts only
one content item argument, because we will never have more than one item per node.

ogenev · 2022-07-25T12:57:41Z

trin-core/src/portalnet/overlay.rs

@@ -297,37 +316,44 @@ where
        let mut enrs_and_content: HashMap<String, Vec<RawContentKey>> = HashMap::new();

        // Filter all nodes from overlay routing table where XOR_distance(content_id, nodeId) < node radius
-        for content_key_value in content {


The reason for this iteration and the hashmap enrs_and_content above is that we want to accomplish two things:

Have all interested nodes as a key for the hashmap.

Have all content items a node is interested in as hashmap values.

This allows us to send one OFFER message per node with multiple content items instead of multiple OFFER messages per node.

ogenev

Looks good to me. I like the new comments and the JoinHandle approach 👍

ogenev · 2022-08-02T12:22:06Z

trin-core/src/portalnet/overlay.rs

+        content: Vec<(TContentKey, ByteList)>,
+        kbuckets: Arc<RwLock<KBucketsTable<NodeId, Node>>>,
+        command_tx: UnboundedSender<OverlayCommand<TContentKey>>,
+    ) -> anyhow::Result<()> {


I think we don't return errors here anymore, so we can remove this Result?

carver · 2022-08-03T01:31:41Z

It's possible that you'll get more information about what is panicking if you cherry-pick this change:
bbc325c

I suspect there's missing information that's making it difficult to debug the crash.

carver · 2022-08-05T22:39:25Z

You should be able to remove the increase poolsize commit now, after rebase.

njgheorghita force-pushed the validate-accept branch from b75fc0f to 0e361d0 Compare July 11, 2022 19:31

njgheorghita mentioned this pull request Jul 11, 2022

Remove blocking async from overlay service loop #373

Merged

2 tasks

njgheorghita force-pushed the validate-accept branch from 0e361d0 to 63e9ea8 Compare July 12, 2022 12:17

njgheorghita force-pushed the validate-accept branch 4 times, most recently from 311daa3 to 8612661 Compare July 20, 2022 16:59

njgheorghita marked this pull request as ready for review July 20, 2022 17:06

njgheorghita force-pushed the validate-accept branch 2 times, most recently from acb19e0 to 674e911 Compare July 20, 2022 17:08

njgheorghita requested review from ogenev, jacobkaufmann and carver July 20, 2022 17:12

njgheorghita force-pushed the validate-accept branch from 674e911 to 0a1c882 Compare July 22, 2022 15:29

ogenev requested changes Jul 25, 2022

View reviewed changes

njgheorghita force-pushed the validate-accept branch 5 times, most recently from c5a22e3 to 07402ea Compare August 1, 2022 19:47

njgheorghita requested a review from ogenev August 1, 2022 22:52

ogenev approved these changes Aug 2, 2022

View reviewed changes

njgheorghita force-pushed the validate-accept branch from 07402ea to 5b3bae1 Compare August 2, 2022 12:57

njgheorghita force-pushed the validate-accept branch from 2cb9eb0 to 045ef11 Compare August 3, 2022 19:13

njgheorghita mentioned this pull request Aug 4, 2022

Increase the default number of rpc serving threads #400

Merged

1 task

njgheorghita force-pushed the validate-accept branch from 045ef11 to 5b3bae1 Compare August 5, 2022 23:06

Add content validation for accepted content

4fda56f

njgheorghita force-pushed the validate-accept branch from 5b3bae1 to 4fda56f Compare August 5, 2022 23:08

njgheorghita merged commit 94e3165 into ethereum:master Aug 5, 2022

njgheorghita deleted the validate-accept branch August 5, 2022 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add content validation for accepted content #376

Add content validation for accepted content #376

njgheorghita commented Jul 11, 2022 •

edited

Loading

ogenev left a comment

ogenev Jul 25, 2022

ogenev Jul 25, 2022

njgheorghita Jul 25, 2022

carver Jul 25, 2022

ogenev Jul 26, 2022 •

edited

Loading

ogenev Jul 25, 2022

ogenev Jul 25, 2022

ogenev left a comment

ogenev Aug 2, 2022

carver commented Aug 3, 2022

carver commented Aug 5, 2022 •

edited

Loading

Add content validation for accepted content #376

Add content validation for accepted content #376

Conversation

njgheorghita commented Jul 11, 2022 • edited Loading

What was wrong?

How was it fixed?

To-Do

ogenev left a comment

Choose a reason for hiding this comment

ogenev Jul 25, 2022

Choose a reason for hiding this comment

ogenev Jul 25, 2022

Choose a reason for hiding this comment

njgheorghita Jul 25, 2022

Choose a reason for hiding this comment

carver Jul 25, 2022

Choose a reason for hiding this comment

ogenev Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

ogenev Jul 25, 2022

Choose a reason for hiding this comment

ogenev Jul 25, 2022

Choose a reason for hiding this comment

ogenev left a comment

Choose a reason for hiding this comment

ogenev Aug 2, 2022

Choose a reason for hiding this comment

carver commented Aug 3, 2022

carver commented Aug 5, 2022 • edited Loading

njgheorghita commented Jul 11, 2022 •

edited

Loading

ogenev Jul 26, 2022 •

edited

Loading

carver commented Aug 5, 2022 •

edited

Loading