Adjust discv5_updateNodeInfo JSON-RPC according to its summary

[kdeme]

The existing summary was stating that it can add, update, or remove a key-value pair from the local ENR. However, the specification only allows for the socket address.
This change allows to touch any key/value except for those with keys id and secp256k1.

This is more aligned with how to call is specified here: https://ddht.readthedocs.io/en/latest/jsonrpc.html#discv5-updatenodeinfo

The schema basically allows something like this:

[
  {
    "key": "0xabcd",
    "value": "0xffff"
  },
  {
    "key": "0xffff",
    "value": "0xffff"
  }
]

[pipermerriam]

I think this is still ambiguous in behavior. Does the client replace the ENR with the provided set of key/value pairs? Or does it delete all keys that aren't provided in the call? Is this a merge operation or a replace operation? If it's a merge operation, we need some defined way to specify that a key should be deleted. If it's a replace operation, we need to specify very clearly in the description that this fully replaces all the ENR k/v pairs.

[kdeme]

It was my intention of this to be a merge operation. Because replace would require/allow you to replace all values including the secp256k1 public key, which means you also have to provide the private key to the node somehow, which doesn't sound right.

You are correct that it is ambiguous now, because it also mentions the remove action. If we remove the remove action in the description, it should be more clear. Could have the remove action as a seperate JSON-RPC. Perhaps not the clearest API still..?

[kdeme]

Perhaps I should change this to two calls:
discv5_updateEnrKeys
discv5_removeEnrKeys

or the singular versions:
discv5_updateEnrKey
discv5_removeEnrKey

[pipermerriam]

I think things should stay plural, as it would be mildly annoying to end up cycling through multiple new sequence numbers in order to update multiple fields....

I think it might be nice to explicitely blacklist which keys cannot be set using this method (seq, sepk???).

If there's a clean way to have them be a single endpoint, I think that also would be good for the same reason as not wanting to go towards singular key updates. It would be nice to do full atomic updates of the ENR fields. But this means we need a reliable/ergonomic way to specify keys for deletion (such as maybe omitting the value field?)

[pipermerriam]

I think maybe the way to do key deletion is just to submit two lists. One for updates that contains key/value pairs, and one for deletes that only contains keys:

{
    update: [[k, v], ...],
    delete: [k1, k2, ...],
}

The only thing that bugs me about this API is that you could have a key be present in both lists.... but that is currently already true of the current API so not a big deal.

[kdeme]

It would be nice to do full atomic updates of the ENR fields.

Yeah, that's a good point. As ENR updates can "immediately" trigger underlying calls depending on discv5 implementations.

I think maybe the way to do key deletion is just to submit two lists.

Yeah, I think I like this suggestion the most. I will adjust it in the PR.

The only thing that bugs me about this API is that you could have a key be present in both lists.... but that is currently already true of the current API so not a big deal.

Indeed, I'll add a remark that adding a key twice, whether in the update or the delete list or in both combined is an invalid query.

[pipermerriam]

Still relevant? What should happen with this PR?

[kdeme]

Still relevant? What should happen with this PR?

I paused on this PR as I paused improving the implementation of it in Fluffy considering there are much higher priorities (This call isn't even used in any of the testing frameworks afaik).

I can cleanup this PR by updating it to split in the two calls:

• discv5_updateEnrKeyValues
• discv5_removeEnrKeyValues

That sounds simplest.

Or I adjust the PR where we just remove this call all together.