feat: update `portal_*Offer` to handle multiple pieces of content #1507

KolbyML · 2024-10-02T21:46:46Z

What was wrong?

updates Trin to match proposed spec changes ethereum/portal-network-specs#342

How was it fixed?

updating our code to match the spec

morph-dev

It looks good in general, but there are few things I want to discuss.

The *TraceOffer is not defined in spec. Should we make it accept multiple items as well?

The *WireOffer is also not defined in spec, and if I'm not wrong, it was introduced because there was no way to test sending multiple content items using portal network OFFER request. Do we still need it? Can we remove it?

If you feel this can be left for future PR, I'm fine with that as well (but in that case, we should have an issue for it).

morph-dev · 2024-10-04T08:32:25Z

ethportal-peertest/src/scenarios/offer_accept.rs

Should we add test here as well?

I will remove WireOffer then Offer will take it's place for tests which should be sufficient for test coverage

morph-dev · 2024-10-04T08:38:55Z

rpc/src/beacon_rpc.rs

-        let content_value = BeaconContentValue::decode(&content_key, &content_value)
-            .map_err(RpcServeError::from)?;
-        let endpoint = BeaconEndpoint::Offer(enr, content_key, content_value);
+        if !(1..=64).contains(&content_items.len()) {


We should probably define 64 as a constant somewhere in the ethportal-api.

morph-dev · 2024-10-04T08:43:53Z

portalnet/src/overlay/protocol.rs

-        let request = Request::PopulatedOffer(PopulatedOffer {
-            content_items: vec![(content_key, content_value)],
-        });
+        let request = Request::PopulatedOffer(PopulatedOffer { content_items });


In my opinion, check that number of content items is 1..=64 should happen here (in which case it should return Err(OverlayRequestError::InvalidRequest(..))).

In that case, it's probably not needed in *_rpc.rs.

I don't think we should move the request check to portalnet/src/overlay/protocol.rs. Because then we would be sending possibly over 64 key-value pairs through the *_rx channel which I think should be avoided. Yes this suggestion would reduce code duplication, but I don't think we should push validating length checks after a channel. We should do the range check as soon as possible to avoid that send. Why waste extra resources when it can be avoided.

The reason why I think it should be there is not because I want to avoid code duplication, but because this type of check should be at lower level. Reason being is that what if there is alternative call to this function (or is added in the future).

Now that I think about it, we might want it at even lower level (e.g. right before sending offer request over network). Because poke, and gossip on deletion (and maybe some other mechanics) wouldn't have this check.

But, regarding your point, it's fine to have it on upper level as well (and not make calls that we know would fail).

We do have a check at the lower level https://github.com/ethereum/trin/blob/master/portalnet/src/gossip.rs#L103-L115 ... but i'd include an update in this pr to use the MAX_AMOUNT_OF_OFFERED_CONTENT_KEYS const in that code

I will update the code to use that const. I think it is fine if we have a lower and higher level check

KolbyML · 2024-10-06T03:15:54Z

It looks good in general, but there are few things I want to discuss.

The *TraceOffer is not defined in spec. Should we make it accept multiple items as well?

I will remove WireOffer and replace it with Offer. I think TraceOffer is a custom endpoint that will likely be removed, assuming trin-bridge stops using the JSON-RPC and integrates Trin instead of running Trin as a binary. Because TraceOffer is only used for bridging/internal use, I don't think we need to change it unless something requires it.

The *WireOffer is also not defined in spec, and if I'm not wrong, it was introduced because there was no way to test sending multiple content items using portal network OFFER request. Do we still need it? Can we remove it?

If you feel this can be left for future PR, I'm fine with that as well (but in that case, we should have an issue for it).

I will remove WireOffer and use Offer directly

morph-dev

Look good from my point of view. But I will let @njgheorghita to give approval as I think he should take a look as well.

I still think that TraceOffer should behave the same as Offer but with trace. But I'm not going to insist on it.

morph-dev · 2024-10-07T07:10:56Z

ethportal-peertest/src/scenarios/offer_accept.rs

@@ -22,19 +22,12 @@ pub async fn test_unpopulated_offer(peertest: &Peertest, target: &Client) {
    info!("Testing Unpopulated OFFER/ACCEPT flow");


This is no longer "unpopulated" flow. If I'm not mistaken, we no longer have "unpopulated" flow.

Either rename/update current test, or remove it if you think it's redundant.

morph-dev · 2024-10-07T07:29:06Z

portalnet/src/overlay/protocol.rs

-        let request = Request::PopulatedOffer(PopulatedOffer {
-            content_items: vec![(content_key, content_value)],
-        });
+        let request = Request::PopulatedOffer(PopulatedOffer { content_items });


The reason why I think it should be there is not because I want to avoid code duplication, but because this type of check should be at lower level. Reason being is that what if there is alternative call to this function (or is added in the future).

Now that I think about it, we might want it at even lower level (e.g. right before sending offer request over network). Because poke, and gossip on deletion (and maybe some other mechanics) wouldn't have this check.

But, regarding your point, it's fine to have it on upper level as well (and not make calls that we know would fail).

njgheorghita

I still think that TraceOffer should behave the same as Offer but with trace. But I'm not going to insist

Since the bridges are only offering a single content item at a time, there isn't any immediate benefit to this change. So I'm not gonna insist either. But it might end up being useful in the state bridge fairly soon assuming that we'll want to gossip more than 1 content item -> peer per content type in each diff (or when gossiping an era2 file) which seems likely imo. Though this can all be handled in a future pr

njgheorghita · 2024-10-08T18:06:38Z

ethportal-api/src/types/portal.rs

@@ -32,6 +32,8 @@ pub struct PongInfo {

 pub type FindNodesInfo = Vec<Enr>;

+pub const MAX_AMOUNT_OF_OFFERED_CONTENT_KEYS: usize = 64;


nit MAX_CONTENT_KEYS_PER_OFFER

njgheorghita · 2024-10-08T18:08:43Z

ethportal-peertest/src/scenarios/offer_accept.rs

-        }
-    }
-}
-
 pub async fn test_populated_offer(peertest: &Peertest, target: &Client) {


Along with @morph-dev comment above, you can update the name of this test & log to just test_offer ... and remove any reference to unpopulated/populated in other test names / logs / comments in this file

njgheorghita · 2024-10-08T18:09:48Z

ethportal-peertest/src/scenarios/offer_accept.rs

-        }
-    }
-}
-
 pub async fn test_populated_offer(peertest: &Peertest, target: &Client) {
    info!("Testing Populated Offer/ACCEPT flow");


nit Testing OFFER/ACCEPT flow / Testing Offer/Accept flow (& in other test's info! logs)

njgheorghita · 2024-10-08T18:18:13Z

portalnet/src/overlay/protocol.rs

-        let request = Request::PopulatedOffer(PopulatedOffer {
-            content_items: vec![(content_key, content_value)],
-        });
+        let request = Request::PopulatedOffer(PopulatedOffer { content_items });


We do have a check at the lower level https://github.com/ethereum/trin/blob/master/portalnet/src/gossip.rs#L103-L115 ... but i'd include an update in this pr to use the MAX_AMOUNT_OF_OFFERED_CONTENT_KEYS const in that code

KolbyML force-pushed the support-offer-with-items branch from 598241e to 0371cac Compare October 2, 2024 22:02

feat: update portal_*Offer to handle multiple pieces of content

ad27680

KolbyML force-pushed the support-offer-with-items branch from 0371cac to ad27680 Compare October 3, 2024 15:00

KolbyML marked this pull request as ready for review October 3, 2024 15:09

KolbyML requested review from carver, njgheorghita, ogenev and morph-dev October 3, 2024 15:10

KolbyML self-assigned this Oct 3, 2024

morph-dev reviewed Oct 4, 2024

View reviewed changes

KolbyML added 2 commits October 5, 2024 21:26

fix: remove wire_offer and use offer

6c60229

fix: use const for max amount of offered content keys

805f946

KolbyML requested a review from morph-dev October 6, 2024 08:10

morph-dev reviewed Oct 7, 2024

View reviewed changes

njgheorghita approved these changes Oct 8, 2024

View reviewed changes

fix: pr concerns

5b50eaf

KolbyML merged commit 627189e into ethereum:master Oct 8, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update `portal_*Offer` to handle multiple pieces of content #1507

feat: update `portal_*Offer` to handle multiple pieces of content #1507

KolbyML commented Oct 2, 2024

morph-dev left a comment

morph-dev Oct 4, 2024

KolbyML Oct 6, 2024

morph-dev Oct 4, 2024

morph-dev Oct 4, 2024

KolbyML Oct 6, 2024

morph-dev Oct 7, 2024

njgheorghita Oct 8, 2024

KolbyML Oct 8, 2024

KolbyML commented Oct 6, 2024

morph-dev left a comment

morph-dev Oct 7, 2024

morph-dev Oct 7, 2024

njgheorghita left a comment

njgheorghita Oct 8, 2024

njgheorghita Oct 8, 2024

njgheorghita Oct 8, 2024

njgheorghita Oct 8, 2024

		@@ -22,19 +22,12 @@ pub async fn test_unpopulated_offer(peertest: &Peertest, target: &Client) {
		info!("Testing Unpopulated OFFER/ACCEPT flow");

		@@ -32,6 +32,8 @@ pub struct PongInfo {

		pub type FindNodesInfo = Vec<Enr>;

		pub const MAX_AMOUNT_OF_OFFERED_CONTENT_KEYS: usize = 64;

feat: update portal_*Offer to handle multiple pieces of content #1507

feat: update portal_*Offer to handle multiple pieces of content #1507

Conversation

KolbyML commented Oct 2, 2024

What was wrong?

How was it fixed?

morph-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Oct 6, 2024

morph-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: update `portal_*Offer` to handle multiple pieces of content #1507

feat: update `portal_*Offer` to handle multiple pieces of content #1507