-
Notifications
You must be signed in to change notification settings - Fork 131
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: update portal_*Offer
to handle multiple pieces of content
#1507
Conversation
598241e
to
0371cac
Compare
0371cac
to
ad27680
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add test here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove WireOffer
then Offer
will take it's place for tests which should be sufficient for test coverage
rpc/src/beacon_rpc.rs
Outdated
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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably define 64
as a constant somewhere in the ethportal-api
.
let request = Request::PopulatedOffer(PopulatedOffer { | ||
content_items: vec![(content_key, content_value)], | ||
}); | ||
let request = Request::PopulatedOffer(PopulatedOffer { content_items }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the code to use that const. I think it is fine if we have a lower and higher level check
I will remove
I will remove WireOffer and use Offer directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -22,19 +22,12 @@ pub async fn test_unpopulated_offer(peertest: &Peertest, target: &Client) { | |||
info!("Testing Unpopulated OFFER/ACCEPT flow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
let request = Request::PopulatedOffer(PopulatedOffer { | ||
content_items: vec![(content_key, content_value)], | ||
}); | ||
let request = Request::PopulatedOffer(PopulatedOffer { content_items }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ethportal-api/src/types/portal.rs
Outdated
@@ -32,6 +32,8 @@ pub struct PongInfo { | |||
|
|||
pub type FindNodesInfo = Vec<Enr>; | |||
|
|||
pub const MAX_AMOUNT_OF_OFFERED_CONTENT_KEYS: usize = 64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit MAX_CONTENT_KEYS_PER_OFFER
} | ||
} | ||
} | ||
|
||
pub async fn test_populated_offer(peertest: &Peertest, target: &Client) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
} | ||
} | ||
|
||
pub async fn test_populated_offer(peertest: &Peertest, target: &Client) { | ||
info!("Testing Populated Offer/ACCEPT flow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit Testing OFFER/ACCEPT flow
/ Testing Offer/Accept flow
(& in other test's info!
logs)
let request = Request::PopulatedOffer(PopulatedOffer { | ||
content_items: vec![(content_key, content_value)], | ||
}); | ||
let request = Request::PopulatedOffer(PopulatedOffer { content_items }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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