Closed mynameisdaniil closed 1 year ago
@njgheorghita can you also take a look at glados PR? (https://github.com/ethereum/glados/pull/177) I can't add anyone to the list of reviewers there. Also, this change will require some changes in trin. How can we arrange that? I have an issue opened in trin repo for that (https://github.com/ethereum/trin/issues/987).
@mynameisdaniil Well, I'd say it looks like the glados
pr is blocked until we get the changes through on trin
, I'll pick that issue (987) up and once it gets merged / included in the next release then we'll move on to getting the glados pr through.
@mynameisdaniil In the specs you have the content field name as "contentValue"
but in the example list in the pr description you have just "content"
. Also in the example, it looks like it should be "content": "0x"
not "content": ""
. Just wondering since you say "Here's an example of the correct JSON response:"
. Personally, I'd prefer to see the spec updated to just "content"
rather than "contentValue"
- but just wondering what your thoughts about it are? How did you generate the "correct JSON response"?
Personally, I'd prefer to see the spec updated to just
"content"
rather than"contentValue"
- but just wondering what your thoughts about it are? How did you generate the "correct JSON response"?
I have no preference between contentValue
vs content
. My only request is that it should be consistent for both portal_historyTraceRecursiveFindContent
and portal_historyRecursiveFindContent
.
My only request is that it should be consistent for both portal_historyTraceRecursiveFindContent and portal_historyRecursiveFindContent.
Yeah, agreed. I only just now noticed that the spec for portal_historyRecursiveFindContent
uses "contentValue"
. In trin
we use "content"
for that endpoint, and it looks to me like fluffy does as well? As well as ultralight. In which case I'd recommend changing "contentValue"
-> "content"
in this pr for both portal_historyRecursiveFindContent
& portal_historyTraceRecursiveFindContent
Also worth mentioning here (from https://github.com/ethereum/trin/issues/987#issuecomment-1766838403) that we should leave in the startedAt
field inside the metadata since there are plans to use that field in glados eventually. Sorry for all the change requests post-approval :sweat_smile:
Looks good, only thing is that I agree that startedAt
should be added back in because while the caller knows when they sent the jsonrpc command, they don't know that the query started at exactly that moment and that it wasn't queued up to be sent some time later.
@mrferris Agree, but I propose we make it just integer in milliseconds, so it will fit into puny 53 bit of js integers. And I don't think we really need nanoseconds precision anyway.
@njgheorghita yeah, I was looking into the spec and not into the code, that's why it was 'contentValue'. I made it just content
now, and also changed for LocalContentResult
(https://github.com/ethereum/portal-network-specs/pull/236/files#diff-c2dedab1970d2849f29aa317f197e32966e0aaa38fab13e02dd24b62cb0f8482L186) which is not implemented according to spec in fluffy (we return just a string https://github.com/status-im/nimbus-eth1/blob/master/fluffy/rpc/rpc_portal_api.nim#L188C37-L188C37) and I guess the situation is the same for other clients.
I was going to quickly create issues for other clients to fix this. But I now see that it isn't trivial and I will need some time to figure this out. There is a mess of inconsistencies around returning content.
I guess the situation is the same for other clients. I was going to quickly create issues for other clients to fix this. But I now see that it isn't trivial and I will need some time to figure this out. There is a mess of inconsistencies around returning content.
In this case, I think it makes more sense to just adjust the specs for portal_*LocalContent
to not return an object but just the content in hexstring. IIRC this is already used in Hive and works for the clients there, you might want to verify that though.
The contentValue -> content
change must be made consistent for portal_historyRecursiveFindContent
though.
@njgheorghita
Also in the example, it looks like it should be
"content": "0x"
not"content": ""
.
That's because I just di"
content in vim. Of course it should be 0x...
How did you generate the "correct JSON response"?
With the current implementation in Fluffy. I did spec and code update at the same time, trying to keep it consistent.
@mynameisdaniil Thanks for making those changes! Everything looks good to me
@mrferris what do you think about https://github.com/ethereum/portal-network-specs/pull/236/files#diff-7c7eba9e61ee0bdf859040ec9b558b0d81838316425ec96ee231b6b06bf5c204R11 ?
I feel like it's better to keep all time measurements in same unit i.e. milliseconds. If we need nanosecond precision, let's make it seconds + nanoseconds remainder everywhere i.e. for both startedAt
and duration
.
PREFACE: There's a
portal_historyTraceRecursiveFindContent
call thattrin
andglados
support and use. I've been trying to implement it forfluffy
and found few inconsistencies (https://github.com/ethereum/trin/issues/987). This PR is an attempt to fix shortcomings that I've found and make this improved API implementation standardized.Here's a spec for
portal_historyTraceRecursiveFindContent
request, that is implemented by trin and is suported by glados. This spc differs from original idea described in https://github.com/ethereum/trin/pull/482 and also differs from current implementation. Main differences being: removed unused fields, removed extra fields (i.e. we dont need port and ip when we have enr), all fields arecamelCase
. Also, there is support for tracing cancelled requests e.g. we received data from somewhere else, so we cancel all other requests. Here's an example of the correct JSON response:origin
- originator of the request i.e. local node.targetId
- target content IDreceivedFrom
- node the data was received fromresponses
- map from the nodes which don't have content to the nodes that probably have this content or closer to it in therespondedWith
field. Except for the node that has the content, this node'srespondedWith
must be empty list.durationMs
is the time from the beginning of the original request up to receiving data from this node.metadata
- map from the node id to metadata,enr
anddistance
.metadata
must include all the nodes mentioned in any other fields in the response.cancelled
- list of requests that were cancelled before receiving data. Optional.Here's glados update to support trace request in this current form: https://github.com/ethereum/glados/pull/177
UPD: I've updated this post to reflect latest changes