ethereum / portal-network-specs

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

Conflict between empty receipts and unsuccessful RFC lookups #256

Open njgheorghita opened 10 months ago

njgheorghita commented 10 months ago

In the current jsonrpc spec, for portal_*recursiveFindContent & portal_*TraceRecursiveFindContent, if the lookup is not able to find the target information, a return value of "0x" is expected (If the content is not available, returns "0x"). However, "0x" is also a valid value for an empty, ssz-encoded Receipts type (reference).

Therefore, it is impossible for a user to distinguish between a failed lookup for a Receipts content value, or a successful lookup that returns an empty Receipts type. I also expect that there might be future content values where we want "0x" to be a valid value.

It seems unavoidable that we need to change the If the content is not available, return "0x" convention for RFC lookups.

Some possible solutions...

  1. If the content is not available, return "null"
    • null is a valid json type, so if the RFC lookup fails, then simply return null as the content value.

RFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": null,
    "utpTransfer": false
  }
}

TRFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": null,
    "utpTransfer": false,
    "trace": {...}
  }
}
  1. Return a json error if the RFC lookup fails.

RFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 32000,
    "message": "Unable to find content in RFC lookup."
  }
}

TRFC example

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 32000,
    "message": "Unable to find content in RFC lookup.",
    "data": {..trace_info..}
  }
}

Personally, I'm in favor of solution 1, because it's simpler. I'm not convinced that we should treat an "unsuccessful" RFC lookup as an error. But, I don't have any strong objections if that's the consensus. If there are any other possible solutions / thoughts / feedback, please leave them in a comment below.

kdeme commented 10 months ago

Personally, I'm in favor of solution 1, because it's simpler. I'm not convinced that we should treat an "unsuccessful" RFC lookup as an error. But, I don't have any strong objections if that's the consensus.

I'm of the same opinion, but also not strongly against the the other solution.

ogenev commented 10 months ago

I'm also in favor of option 1).

pipermerriam commented 10 months ago

I believe option 1 is probably a "foot gun" and that it's likely for implementations to get this wrong or for users to interpret it wrong...

Which suggest that option 2 is likely to be less likely to result in problems...

This isn't a strong opinion and you guys can go whichever way you like.

morph-dev commented 9 months ago

There is also option 3, that is similar to option 1 but has explicitness of option 2.

  1. Use Union type from SSZ specification. In practice, this would add 1 extra byte (e.g. 0 for absent, 1 for present), which would result in something like this:

RFC example - content not available

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": "0x00",
    "utpTransfer": false
  }
}

content is empty list

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": "0x01",
    "utpTransfer": false
  }
}

content is 0xabc...

{
  "jsonrpc": "2.0",
  "id": 1,
  "result": {
    "content": "0x01abc",
    "utpTransfer": false
  }
}

This also wouldn't need special (de)serialization as this is standard type.

njgheorghita commented 9 months ago

Personally, I don't love the idea of wrapping our responses in another level of ssz-encoding, it could just get confusing especially when none of our other endpoints behave this way.