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.

I don't really mind one or the other either. I think what @njgheorghita refers to is e.g. the items iterator for dictionary in Python? But sure, that's rather language specific.

I prefer ContentPair's as it is more descriptive, I updated the PR to ContentItems, because I think getting the PR in takes priority over small nits I might have

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

I didn't know tuple was a type in openapi I will updated it

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

I couldn't find a tuple type so I ended up using an array to achieve a similar thing

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 am not sure how to do this, I tried a few times. Maybe someone else knows how?

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 · 2024-09-30T20:27:11Z

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

njgheorghita · 2024-10-02T18:33:28Z

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 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 or less than 1 content items are provided..",


nitpick there's 2 periods at the end of this sentence

njgheorghita · 2024-10-02T18:33:46Z

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 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 or less than 1 content items are provided..",


njgheorghita · 2024-10-02T18:39:25Z

jsonrpc/src/schemas/portal.json

+  },
+  "ContentItem": {
+    "title": "content_item",
+    "type": "array",


Why is this an array? Shouldn't it be an object?

@ScottyPoi said it should be a tupel? but that doesn't exist so I did an array.

I will let you guys discuss what we should use as I am impartial as long as I can use this RPC command to test the spec. I do think an object would be potentially cleaner though, but I haven't looked too into this yet

i'm OK with it either way

I implemented it with tuples and it feels good so I am on the tuple side now

I implemented it with tuples and it feels good so I am on the tuple side now

This statement is rather confusing to me. Because afaik tuples don't exist in JSON, and when they are translated from other languages I think they are typically translated to objects.

An object seems also more natural to me here, I think you don't need that "minItems": 2, "maxItems": 2, in that case.
This will do too, but I wanted to highlight it for others adjusting this rpc, as I was implementing it in fluffy with tuple/object, and then realized its an actually array.

and when they are translated from other languages I think they are typically translated to objects.

sorry for the confusion. i thought "tuple" was universally an array-like structure, but with constraints (i.e. length: 2)

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

tried to resolve pr concerns

f62f44f

fix ci

1b10f60

ScottyPoi approved these changes Sep 30, 2024

View reviewed changes

ScottyPoi mentioned this pull request Sep 30, 2024

RPC: update portal_*Offer ethereumjs/ultralight#636

Merged

kdeme approved these changes Oct 1, 2024

View reviewed changes

njgheorghita reviewed Oct 2, 2024

View reviewed changes

njgheorghita approved these changes Oct 2, 2024

View reviewed changes

KolbyML mentioned this pull request Oct 2, 2024

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

Merged

KolbyML added 2 commits October 2, 2024 15:52

fix: pr concerns

56c1757

fix: add limit to content items

16439d7

KolbyML merged commit 380dc84 into ethereum:master Oct 2, 2024
2 checks passed

kdeme mentioned this pull request Oct 3, 2024

Adapt portal_*Offer method to the latest specification status-im/nimbus-eth1#2689

Merged

	"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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Sep 30, 2024

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 Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

kdeme Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

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

KolbyML Oct 2, 2024 •

edited

Loading

kdeme Oct 3, 2024 •

edited

Loading