Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC updates for ed/x25519 keys #880

Open
wants to merge 3 commits into
base: dev
from

Conversation

@jagerman
Copy link

jagerman commented Oct 8, 2019

Edit: obsolete, see below.

  • Revamps the get_all_service_nodes_public_keys RPC call to support
    returning any of the three pubkey types, and make the base32z
    conversion an opt-in option rather than forced on.

  • Simplify the code by making the above RPC code just go through the
    full sninfo retrieval. Getting the list of all pubkeys doesn't seem
    to be super performance critical (it's only used for this rpc call),
    so this removes the specialized get_all_service_nodes_public_keys
    member functions from core/sn_list and just goes through the existing
    full path instead.

  • adds the ed25519 and x25519 pubkeys for the current node to the rpc
    call that returns the node's own SN pubkey.

@jagerman

This comment has been minimized.

Copy link
Author

jagerman commented Oct 9, 2019

Don't worry about this for release, nothing will depend on this for 5.x anyway.

@jagerman

This comment has been minimized.

Copy link
Author

jagerman commented Oct 9, 2019

Going to revisit this -- I think we can make lokinet use get_n_service_nodes instead and either add a flag to base38z the pubkey values, or else do the base38z conversion in lokinet. [Edit: Jeff has said the base38z conversion in lokinet is trivially easy]

Since lokinet hits that rpc fairly often (every 5s) I think we also want another response parameter "blockchain_height" for which the SN list applies, and a request parameter "if_height_changed" that takes the last height received and, if no different than the current height, just returns a mostly empty "not_change" response.

@jagerman jagerman changed the title RPC updates for ed/x25519 keys [WIP] RPC updates for ed/x25519 keys Oct 10, 2019
jagerman added 3 commits Nov 8, 2019
This allows a `"if_block_not_equal": "hash"` parameter to be given to
`get_n_service_nodes` which, if the given value matches the current
top block hash, skips building and returning a reply.

This is primarily aimed at lokinet which polls fairly frequenty for an
update but where the vast majority of those polls contain no new
information.

It also removes the get_all_service_nodes_public_keys RPC request
completely as lokinet was the only consumer of it (particularly unlikely
that anyone else was using this because it was returning the keys
base32z-encoded).
With the previous commit we no longer need this conversion (and lokinet
is perfectly happy just getting hex encoded values instead).
@jagerman jagerman force-pushed the jagerman:pk25519-rpc-interfaces branch from 0c1d946 to 4591e28 Nov 8, 2019
@jagerman jagerman changed the title [WIP] RPC updates for ed/x25519 keys RPC updates for ed/x25519 keys Nov 8, 2019
@jagerman

This comment has been minimized.

Copy link
Author

jagerman commented Nov 8, 2019

Rebased and updated this PR to do the things mentioned above.

In particular, get_n_service_nodes now accepts a "if_block_not_equal" parameter that can be given a block hash and then only returns results if the current block hash doesn't match the given one.

The main point here is to be able to give lokinet a "nothing has changed" response when it polls for new SN keys; it just needs to request the block_hash in fields on the first request, then on subsequent request can pass the currently stored block_hash in the if_block_not_equal parameter. If the block hash hasn't changed the response includes "unchanged": true and omits serializing and returning (or even calculating) all the data.

This PR also removes the base32z encoding code (Jeff is fine with just doing that in lokinet from hex values), and adds the missing ed25519/x25519 public keys to the rpc call to retrieve the SN's own pubkeys.

For example:

$ curl -X POST http://localhost:38157/json_rpc -d '
{"jsonrpc":"2.0","id":"0","method":"get_n_service_nodes",
"params":{
"if_block_not_equal":"9d13b6b0f18bfa7d965c2d0f784c7985cf45917f4e33449466b8950957f06ff9b",
"fields":{"service_node_pubkey":true}}}' -H 'Content-Type: application/json'
{
  "id": "0",
  "jsonrpc": "2.0",
  "result": {
    "block_hash": "d13b6b0f18bfa7d965c2d0f784c7985cf45917f4e33449466b8950957f06ff9b",
    "service_node_states": [{
      "service_node_pubkey": "9c09fcdb15b07ce15894b861cac270a30d190961031d6176dd51c314385d2ff5"
    },{
      "service_node_pubkey": "1c9cfead855a841a338800b77f9808f3aaae4c1bdb6a25b91ecac809506c143c"
    },{
      (etc.)
    }],
    "status": "OK",
    "unchanged": false
  }
}

but with the correct current block hash provided gives just:

$ curl -X POST http://localhost:38157/json_rpc -d '{"jsonrpc":"2.0","id":"0","method":"get_n_service_nodes", "params":{"if_block_not_equal":"d13b6b0f18bfa7d965c2d0f784c7985cf45917f4e33449466b8950957f06ff9b","fields":{"service_node_pubkey":true}}}' -H 'Content-Type: application/json'
{
  "id": "0",
  "jsonrpc": "2.0",
  "result": {
    "status": "OK",
    "unchanged": true
  }
}
@@ -2938,11 +2918,13 @@ namespace cryptonote
uint32_t limit;
bool active_only;
requested_fields_t fields;
std::string if_block_not_equal;

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Nov 20, 2019

Collaborator

Should annotate this field (and the above active_only, limit) using this field changes the behaviour quite abit and it's not obvious immediately in the docs anymore.

Something like // Optional: The given block hash is checked against the current service node list and avoids generating output if the hashes are the same (i.e. the list has not changed since the block hash)

Also suggestion for name to be expected_block_hash

@@ -3026,29 +3008,36 @@ namespace cryptonote
};

requested_fields_t fields;
bool gave_if_not_equal;

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Nov 20, 2019

Collaborator

For gave_if_not_equal and fields you can add the comment // @NoLokiRPCDocGen Internal use only, not serialized and it won't display in docs when I generate the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.