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

KolbyML · 2024-09-25T19:05:40Z

I am trying to write a Hive test for ethereum/trin#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 · 2024-09-25T19:10:42Z

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 · 2024-09-25T19:57:36Z

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.

ScottyPoi

I think that this is how portal_historyOffer itself ought to work, rather than a separate endpoint

kdeme

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.

kdeme · 2024-09-26T07:04:24Z

jsonrpc/src/methods/history.json

@@ -115,6 +115,21 @@
      "$ref": "#/components/contentDescriptors/OfferResult"
    }
  },
+  {
+    "name": "portal_historyWireOffer",
+    "summary": "Send an OFFER request with given ContentKeys, to the designated peer and wait for a response. Requires the content keys to be stored locally. Returns the content keys bitlist upon successful content transmission or empty bitlist receive.",


It is not fully clear to me from the explanation whether the call should also do the uTP transfers or not (and wait for their result or not).

The result type is just OfferResult, which is practically the bitlist, so the result of the offer (the returned accept).

But it does say Requires the content keys to be stored locally. I assume this should be: Requires the content to be stored locally. And that would mean that it needs the content because it also needs to send it afterwards.

So please clarify what this call is supposed to do.

KolbyML · 2024-09-26T07:15:04Z

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 · 2024-09-26T07:18:08Z

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 · 2024-09-26T07:21:52Z

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 · 2024-09-26T07:50:27Z

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 · 2024-09-26T07:53:12Z

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 · 2024-09-26T14:22:24Z

+1 for modifying portal_*Offer.

njgheorghita · 2024-09-26T15:03:06Z

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...

supports multiple content keys
supports a single content key/value pair

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 · 2024-09-26T15:05:17Z

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 · 2024-09-26T15:10:07Z

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)
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.

KolbyML · 2024-09-26T15:12:22Z

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 · 2024-09-26T15:28:05Z

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

fair enough, it's not useful in beacon/state
is it useful in history?
- we use it in our tests (though, after a quick skim it seems that these could be updated to use the new endpoint)
- are there user stories where this is useful...
  - ummm i can't think of any right now, but i haven't thought about it too much
  - it's maybe not a great ux to have an endpoint where the user is expected to maintain knowledge of what content is stored locally

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.

👍

KolbyML · 2024-09-26T16:54:25Z

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

njgheorghita · 2024-09-26T19:51:20Z

jsonrpc/src/content/params.json

@@ -43,6 +43,17 @@
      }
    }
  },
+  "ContentPairs": {


ContentItems? imo (at least from python land) this is the usual term used to identify a key/value piar

Personally ambivalent about ContentPair vs ContentItem. For me, ContentPair feels more descriptive (KeyValuePair even more so), but if ContentItem is more conventional in a way that the rest of you appreciate, then I support the choice.

njgheorghita · 2024-09-26T19:52:56Z

jsonrpc/src/methods/history.json

@@ -99,16 +99,13 @@
  },
  {
    "name": "portal_historyOffer",
-    "summary": "Send an OFFER request with given ContentKey, to the designated peer and wait for a response.",
+    "summary": "Send an OFFER request with given array of Content Pairs (Keys & Values), to the designated peer and wait for a response. `portal_historyOffer` should only accept 1 to 64 content pairs else the client will throw an out of bounds error.",


nitpick ... content pairs or else the ....

kdeme · 2024-09-27T08:07:14Z

jsonrpc/src/schemas/portal.json

+      "contentKey": {
+        "title": "Content key",
+        "description": "The encoded Portal content key",
+        "$ref": "#/components/schemas/hexString"
+      },
+      "contentValue": {
+        "title": "Content value",
+        "description": "The encoded Portal content value",
+        "$ref": "#/components/schemas/hexString"


ContentKey and ContentValue already exist here: https://github.com/ethereum/portal-network-specs/blob/master/jsonrpc/src/content/params.json#L2-L16

I guess they could be re-used here inside the ContentPair (or ContentItem).

I'm curious about the choice for ContentPair to be an object, rather than a tuple [key, value].

kdeme · 2024-09-27T08:07:59Z

jsonrpc/src/methods/beacon.json

@@ -99,16 +99,13 @@
  },
  {
    "name": "portal_beaconOffer",
-    "summary": "Send an OFFER request with given ContentKey, to the designated peer and wait for a response.",
+    "summary": "Send an OFFER request with given array of Content Pairs (Keys & Values), to the designated peer and wait for a response. `portal_historyOffer` should only accept 1 to 64 content pairs else the client will throw an out of bounds error.",


The text is quite ambiguous right now regarding the limits/failure case:

It might not be completely clear right now if it is allowed to fail on 2 content items (it is not, only when > 64)

should is not sufficient, must be must

it must be made clear that it is the rpc client that will fail in case > 64 items, not the receiving client on the p2p network.

Suggested change

"summary": "Send an OFFER request with given array of Content Pairs (Keys & Values), to the designated peer and wait for a response. `portal_historyOffer` should only accept 1 to 64 content pairs else the client will throw an out of bounds error.",

"summary": "Send an OFFER request with given array of content items (keys & values), to the designated peer and wait for a response. The client MUST return an error if more than 64 content items are provided.",

ScottyPoi

LGTM. I think this is important. it will give us a new way to stress-test uTP in the hive suites.

I left a couple comments, but generally I would be happy with merging as it is

ScottyPoi · 2024-09-27T21:04:38Z

jsonrpc/src/schemas/portal.json

+      "contentKey": {
+        "title": "Content key",
+        "description": "The encoded Portal content key",
+        "$ref": "#/components/schemas/hexString"
+      },
+      "contentValue": {
+        "title": "Content value",
+        "description": "The encoded Portal content value",
+        "$ref": "#/components/schemas/hexString"


I'm curious about the choice for ContentPair to be an object, rather than a tuple [key, value].

ScottyPoi · 2024-09-27T21:05:28Z

jsonrpc/src/content/params.json

@@ -43,6 +43,17 @@
      }
    }
  },
+  "ContentPairs": {


Personally ambivalent about ContentPair vs ContentItem. For me, ContentPair feels more descriptive (KeyValuePair even more so), but if ContentItem is more conventional in a way that the rest of you appreciate, then I support the choice.

KolbyML requested review from carver, pipermerriam, kdeme, njgheorghita, acolytec3, ScottyPoi and morph-dev September 25, 2024 19:05

ScottyPoi reviewed Sep 25, 2024

View reviewed changes

kdeme reviewed Sep 26, 2024

View reviewed changes

KolbyML changed the title ~~Add historyWireOffer to jsonrpc spec~~ Update portal_*Offer jsonrpc call to accept multiple key-value pairs Sep 26, 2024

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

2714aee

KolbyML force-pushed the add-historyWireOffer branch from f8a44fd to 2714aee Compare September 26, 2024 16:52

update summary

b85fcc1

KolbyML requested review from ScottyPoi and kdeme September 26, 2024 17:06

njgheorghita approved these changes Sep 26, 2024

View reviewed changes

kdeme reviewed Sep 27, 2024

View reviewed changes

ScottyPoi approved these changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

KolbyML commented Sep 25, 2024 •

edited

Loading

KolbyML commented Sep 25, 2024 •

edited

Loading

acolytec3 commented Sep 25, 2024

ScottyPoi left a comment

kdeme left a comment •

edited

Loading

kdeme Sep 26, 2024 •

edited

Loading

KolbyML commented Sep 26, 2024

kdeme commented Sep 26, 2024

KolbyML commented Sep 26, 2024

kdeme commented Sep 26, 2024

KolbyML commented Sep 26, 2024

morph-dev commented Sep 26, 2024

njgheorghita commented Sep 26, 2024

KolbyML commented Sep 26, 2024

njgheorghita commented Sep 26, 2024 •

edited

Loading

KolbyML commented Sep 26, 2024 •

edited

Loading

njgheorghita commented Sep 26, 2024

KolbyML commented Sep 26, 2024

njgheorghita Sep 26, 2024

ScottyPoi Sep 27, 2024

njgheorghita Sep 26, 2024

kdeme Sep 27, 2024

ScottyPoi Sep 27, 2024

kdeme Sep 27, 2024 •

edited

Loading

ScottyPoi left a comment

ScottyPoi Sep 27, 2024

ScottyPoi Sep 27, 2024

	"summary": "Send an OFFER request with given array of Content Pairs (Keys & Values), to the designated peer and wait for a response. `portal_historyOffer` should only accept 1 to 64 content pairs else the client will throw an out of bounds error.",
	"summary": "Send an OFFER request with given array of content items (keys & values), to the designated peer and wait for a response. The client MUST return an error if more than 64 content items are provided.",

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

Are you sure you want to change the base?

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

Conversation

KolbyML commented Sep 25, 2024 • edited Loading

Recap

Update to PR 9/26/2024

KolbyML commented Sep 25, 2024 • edited Loading

acolytec3 commented Sep 25, 2024

ScottyPoi left a comment

Choose a reason for hiding this comment

kdeme left a comment • edited Loading

Choose a reason for hiding this comment

kdeme Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

KolbyML commented Sep 26, 2024

kdeme commented Sep 26, 2024

KolbyML commented Sep 26, 2024

kdeme commented Sep 26, 2024

KolbyML commented Sep 26, 2024

morph-dev commented Sep 26, 2024

njgheorghita commented Sep 26, 2024

KolbyML commented Sep 26, 2024

njgheorghita commented Sep 26, 2024 • edited Loading

KolbyML commented Sep 26, 2024 • edited Loading

njgheorghita commented Sep 26, 2024

KolbyML commented Sep 26, 2024

njgheorghita Sep 26, 2024

Choose a reason for hiding this comment

ScottyPoi Sep 27, 2024

Choose a reason for hiding this comment

njgheorghita Sep 26, 2024

Choose a reason for hiding this comment

kdeme Sep 27, 2024

Choose a reason for hiding this comment

ScottyPoi Sep 27, 2024

Choose a reason for hiding this comment

kdeme Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

ScottyPoi left a comment

Choose a reason for hiding this comment

ScottyPoi Sep 27, 2024

Choose a reason for hiding this comment

ScottyPoi Sep 27, 2024

Choose a reason for hiding this comment

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

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

KolbyML commented Sep 25, 2024 •

edited

Loading

KolbyML commented Sep 25, 2024 •

edited

Loading

kdeme left a comment •

edited

Loading

kdeme Sep 26, 2024 •

edited

Loading

njgheorghita commented Sep 26, 2024 •

edited

Loading

KolbyML commented Sep 26, 2024 •

edited

Loading

kdeme Sep 27, 2024 •

edited

Loading