search

ethereum / portal-network-specs

Official repository for specifications for the Portal Network
313 stars 85 forks source link

Update `portal_*Offer` jsonrpc call to accept multiple key-value pairs #342

Closed KolbyML closed 1 month ago

KolbyML commented 1 month ago

I am trying to write a Hive test for https://github.com/ethereum/trin/issues/1484, and I am sending offer values of 64 items. The current issue I am facing is all Portal clients don't implement a JSON-RPC command that can send more than 1 piece of content per offer.

Trin already does, though, and we call it portal_historyWireOffer. I believe Nick made it for internal testing, but I think it would be useful for all clients to implement it for hive.

I am not adding this command to State or Beacon due to the way it works. If we want it to work for the State Network, we should include the content value in the parameters. I think Nick didn't do this because the message would could be very big, which might be a bad idea? I'm unsure if that is an issue, though, if the limit is only 64 items.

Recap

It is currently not possible to test whether Portal clients follow the spec in being able to send 64 pieces of content in one offer. I am open to discussing making this an RPC call in the debug namespace.

Update to PR 9/26/2024

After a suggestion from Scotty the current leading solution is to make our current portal_*Offer accept multiple items. I updated the PR to reflect this

KolbyML commented 1 month ago

The goal is to have a JSON-RPC command that can create an offer with multiple content keys https://github.com/ethereum/portal-network-specs/blame/35085002701e4ffad7fb95085c1211157bd32a49/portal-wire-protocol.md#L214-L223

To test if clients can properly handle the limit of 64 given in the spec

acolytec3 commented 1 month ago

Seems reasonable to me, though I'd opt for a name more like portal_historyOfferMultiple or something that is more indicative of what the message does. I don't see any reason to move it to debug since all of our RPC endpoints are basically debug endpoints anyway.

KolbyML commented 1 month ago

I general I have no issue with adding a call like this to Fluffy, but it's definition is currently not clear to me.

Regarding debug prefix, I am mostly in favor:

If the call is only supposed to do the offer/accept (without utp part) then I think this should get a debug_portal prefix (we probably should revise if more should then go in the debug prefix, but that's another issue).

If the call is supposed to do offer/accept/utp, then I'm not 100% sure, but it still feels like a debug call because of the fact that you already need to have stored the content locally.

Maybe we should just do scotty's suggestion and make it so portal*offer can be used to send multiple pieces of content. I think this makes the most sense as currently portal*offer returns a bitlist as you said so I think it would make sense for it to be able to take multiple pieces of content

kdeme commented 1 month ago

Maybe we should just do scotty's suggestion and make it so portal_*offer can be used to send multiple pieces of content.

Yeah, I'm fine with that too (in fact I've needed it in the past also for local tests). But there is a difference that in that call the content gets passed within the json-rpc method parameters, and this new call expects it to be there already.

KolbyML commented 1 month ago

Maybe we should just do scotty's suggestion and make it so portal_*offer can be used to send multiple pieces of content.

Yeah, I'm fine with that too (in fact I've needed it in the past also for local tests). But there is a difference that in that call the content gets passed within the json-rpc method parameters, and this new call expects it to be there already.

I am fine with content gets passed within the json-rpc method parameters. I open this PR mostly to start a discussion for how we should move forward. We already had this call in Trin so I used it as a starting place. I think Scotty's idea just makes sense. So I am happy to go with it

kdeme commented 1 month ago

I think Scotty's idea just makes sense. So I am happy to go with it

Sure, I'm fine with adjusting that existing call. It can also stay under the regular portal_ prefix then.

KolbyML commented 1 month ago

I think Scotty's idea just makes sense. So I am happy to go with it

Sure, I'm fine with adjusting that existing call. It can also stay under the regular portal_ prefix then.

Sounds like a win. I will update the PR when I wake up

morph-dev commented 1 month ago

+1 for modifying portal_*Offer.

njgheorghita commented 1 month ago

Currently, portal_*Offer accepts a single content key/value pair, which is beneficial because you don't have to store the data locally to send an offer (very helpful for testing & bridge stuff)

I think we want to retain both the ability to send an offer jsonrpc request with & without the accompanying value. So I'm not sure it's as simple as updating portal_*Offer to take multiple content keys.

I'm not sure I'd be in favor of updating portal_*Offer to accept multiple content keys with optional values (that's messy).

IIUC, this thread is leaning towards updating portal_*Offer to accept multiple content keys? Imo, we still want to retain some kind of ability to offer a single content key/value over jsonrpc. So, I'd be in favor of adding a new endpoint for multiple content keys.

Tbh, i don't have strong feelings about which kind ends up as portal_*Offer or portal_*OfferMultiple or portal_*OfferSingle ... but I do think we want two different jsonrpc endpoints...

I'd also suggest adding portal_*TraceOffer to the specs (already implemented in trin) which does wait for utp tx and returns the result (for both of the cases listed above)

KolbyML commented 1 month ago

I'm not sure I'd be in favor of updating portal_*Offer to accept multiple content keys with optional values (that's messy).

IIUC, this thread is leaning towards updating portal_*Offer to accept multiple content keys? Imo, we still want to retain some kind of ability to offer a single content key/value over jsonrpc. So, I'd be in favor of adding a new endpoint for multiple content keys.

If we can send multiple pairs of content, we would still maintain the ability to send ability to offer a single content key/value over jsonrpc

Sending a single pair of content is a subcase of multiple

njgheorghita commented 1 month ago

I think the difference here is between sending multiple content keys and multiple content key/value pairs

It seems messy to me to have a single endpoint that'll support both of these use cases.

KolbyML commented 1 month ago

I think the difference here is between sending multiple content keys and multiple content key/value pairs

  • we want to support offering multiple keys (in the case where the content is stored locally)

^ This doesn't work for the state network. It doesn't make sense for the beacon network. Personally I don't see a need for it

  • we want to support offering multiple key/value pairs (where the content doesn't need to be stored locally)

It seems messy to me to have a single endpoint that'll support both of these use cases.

^ We won't be supporting both usecases. We will only be supporting we want to support offering multiple key/value pairs (where the content doesn't need to be stored locally with portal_*Offer

njgheorghita commented 1 month ago

This doesn't work for the state network. It doesn't make sense for the beacon network. Personally I don't see a need for it

Ok I guess I can't think of any good reason to keep that functionality around just for the history network. Maybe simply the reason that this represents the actual wire protocol message, but there's no reason we need to expose jsonrpc endpoints for the raw protocol messages.

:+1:

KolbyML commented 1 month ago

@acolytec3 @kdeme @njgheorghita @ScottyPoi ready for another look. I updated the PR to follow Scotty's suggestion

KolbyML commented 1 month ago

@kdeme @njgheorghita @ScottyPoi ready for another look if nobody sees an issue we can merge it