Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Account Query DB : Proposal to maintain get_(key|controlled)_accounts [2.0] #8899

Merged
merged 18 commits into from
May 18, 2020

Conversation

b1bart
Copy link
Contributor

@b1bart b1bart commented Mar 31, 2020

Change Description

This is a proposed optional addition to chain_plugin and chain_api_plugin that provides a superset of the deprecated history_api_plugin's get_key_accounts and get_controlled_accounts RPC methods. While we have many other solutions covering the history access that the history_api_plugin provided, none easily covered this use case for wallets and other end-users.

This solution builds the associated indices on start-up and, as a result, does not suffer from the brittle issues that the history_api_plugin had. It also means it can be added to or removed from any operating nodeos at any time. This does impose a penalty on start-up times.
Timings on a modest cloud VM* indicated that these indices can be built for a node synced with the EOS Public Network @ block 113060000 in about 15-20 seconds. Note: this penalty is only imposed if the Account Query DB is enabled (see below).

The solution will watch the chain as it processes and update the indices accordingly. This process has minimal overhead on the operating node.

*modest VM had 2x 2.0-2.2 gHz vCPUs

API Changes

  • API Changes

POST /v1/chain/get_accounts_by_authorizers

This is the single endpoint that produces a superset of the information provided by get_key_accounts and get_controlled_accounts.

Request format

The basic request payload is the following JSON:

{
   "accounts": [
      //<zero or more account name strings or {actor, permission} objects>
   ],
   "keys":[
      //<zero or more public key strings>
   ]
}

Account entries that are only account names have an implicit wildcard permission which matches any permission level from that account. This can be explicitly set to an empty string or a specific permission level. For instance:

{
   "accounts": [
      "eosio"
   ]
}

will list all accounts authorized in part or whole by eosio whereas:

{
   "accounts": [
      { "actor": "eosio", "permission": "eosio.code" }
   ]
}

will only list accounts authorized specifically by [email protected]

Omitting accounts or keys indicates that those arrays would be empty if explicit.

Response format

The basic response format is the following JSON:

{
   "accounts" : [
      // one entry per authorized {account, permission, authorizing account|key} triplet
      {
         "account_name" : "",            // the name of the authorized account
         "permission_name" : "",         // the permission level on that authorized account
         "authorizing_key" : "PUB_...",  // [optional] present when entry included due to a listed key
         "authorizing_account" : {       // [optional] present when entry included due to a listed account
            "actor" : "",                // the name of the authorizing account
            "permission" : "",           // the name of the permission required to authorize as this account 
         },
         "weight" : 1,                   // the weight this authorization has in this permission
         "threshold": 1                  // the threshold needed to satisfy this permission
      },
     // additional entries...
   ]
}

Porting Hints

get_key_accounts
$ curl http://127.0.0.1:8888/v1/history/get_key_accounts -d '{"public_key":"EOS..."}' | jq '.account_names[]'

becomes:

$ curl http://127.0.0.1:8888/v1/chain/get_accounts_by_authorizers -d '{"keys":["EOS..."]}' 2>/dev/null | jq '[.accounts[].account_name] | unique'
get_controlled_accounts
$ curl http://127.0.0.1:8888/v1/history/get_controlled_accounts -d '{"controlling_account":"alice"}' | jq '.controlled_accounts[]'

becomes:

$ curl http://127.0.0.1:8888/v1/chain/get_accounts_by_authorizers -d '{"accounts":["alice"]}' 2>/dev/null | jq '[.accounts[].account_name] | unique'
Batch fetches

In addition to single value queries, you can add as many account names or public keys to the request as needed. The result will be the union of all the individual responses saving time when determining a set of accounts from various authorization materials.

Consensus Changes

  • Consensus Changes

Documentation Additions

  • Documentation Additions

--enable-account-queries

default: false

Boolean that indicates whether the Account Query DB should be initialized at start-up and maintained for the life of this instance. if set to true then the RPC endpoint for /v1/chain/get_accounts_by_authorizers will be registered otherwise it will not be present and requests to that endpoint will return 404 errors.

@b1bart b1bart requested review from heifner and arhag March 31, 2020 15:52
@nsjames
Copy link
Contributor

nsjames commented Mar 31, 2020

This is an excellent addition, and couldn't wish for a better API structure. 👍

@b1bart b1bart marked this pull request as ready for review March 31, 2020 21:01
@matthewdarwin
Copy link

When providing a non-key to the API, you get back a 500 error. I was expecting a 200 with an empty list. You get 200 with empty list when providing a non-exist account, so the API is really not consistent here.

curl -d '{"keys": ["garbage"]}' http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   319  100   298  100    21    235     16  0:00:01  0:00:01 --:--:--   252
{
  "code": 500,
  "message": "Internal Service Error",
  "error": {
    "code": 10,
    "name": "assert_exception",
    "what": "Assert Exception",
    "details": [
      {
        "message": "pivot != std::string::npos: No delimiter in string, cannot determine data type: garbage",
        "file": "public_key.cpp",
        "line_number": 48,
        "method": "parse_base58"
      }
    ]
  }
}

@b1bart
Copy link
Contributor Author

b1bart commented Apr 15, 2020

@matthewdarwin I agree that is not desired behavior.

An account that does not exist is still a valid "account name". The key you provided is not a valid key. As such, I wondered if you would agree to the following behavior:

  • [Change] if you provide an invalid EOSIO public key, we return a 400 indicating that your request is malfomed
  • [Change] if you provide an invalid name (disallowed characters or longer than 13 characters etc), the API returns a 400 indicating that the request is malformed
  • [Keep] if you provide a valid key that is unknown to the chain, we return a 200 with an empty result array.
  • [Keep] if you provide a valid account that is unknown to the chain, we return a 200 with an empty result array.
  • [Change] reserve 500 errors for real internal errors that were unpredictable.

@matthewdarwin
Copy link

Hi Bart,

The proposal looks good to me. Much better than current.

Feedback from wallet owners would be good. I see you got a +1 from scatter already.

@matthewdarwin
Copy link

I should add that passing no parameters right now gets a 200. Maybe that should be a 400?

http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers

@b1bart
Copy link
Contributor Author

b1bart commented Apr 15, 2020

I agree that no inputs should also be a 400

@matthewdarwin
Copy link

I reached out to TokenPocket to see if they have any comments.

@heipacker
Copy link

heipacker commented Apr 16, 2020

this issue is great, really liberates all mankind

@aaroncox
Copy link
Contributor

Agreeing with all of the other commenters above, this would be a very welcome addition from the wallet developer point of view. This solution would be far better than multiple calls against the existing APIs and still maintain a great UX during the import process.

@b1bart
Copy link
Contributor Author

b1bart commented Apr 17, 2020

@matthewdarwin do you mind re-testing to check me?

@matthewdarwin
Copy link

matthewdarwin commented Apr 17, 2020

I deployed the new version. It does resolve the 500 error which was my main concern.

A few comments:

  1. I expected 400 here (empty json document)
curl -Ssd '{}' http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers | jq .
{
  "accounts": []
}
  1. I expected 400 here (invalid json using "xkeys" instead of "keys"):
curl -Ssd '{"xkeys": ["EOS4uP4oHhtVyZAQze1qMY7ZKqof3y26L6bvnr2FfpMJhhvAnEr1v"]}' http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers | jq .
{
  "accounts": []
}
  1. The error messages are very unfriendly (key is invalid on purpose)
curl -Ssd '{"keys": ["EOS5uP4oHhtVyZAQze1qMY7ZKqof3y26L6bvnr2FfpMJhhvAnEr1v"]}' http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers | jq .
{
  "code": 400,
  "message": "Invalid Request",
  "error": {
    "code": 3200006,
    "name": "invalid_http_request",
    "what": "invalid http request",
    "details": [
      {
        "message": "Unable to parse valid input from POST body",
        "file": "chain_api_plugin.cpp",
        "line_number": 47,
        "method": "parse_params"
      },
      {
        "message": "wrapper::calculate_checksum(wrapped.data) == wrapped.check: ",
        "file": "public_key.cpp",
        "line_number": 42,
        "method": "parse_base58"
      }
    ]
  }
}
  1. The error messages are very unfriendly (key is not a key at all)
$ curl -Ssd '{"keys": ["garbage"]}' http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers | jq .
{
  "code": 400,
  "message": "Invalid Request",
  "error": {
    "code": 3200006,
    "name": "invalid_http_request",
    "what": "invalid http request",
    "details": [
      {
        "message": "Unable to parse valid input from POST body",
        "file": "chain_api_plugin.cpp",
        "line_number": 47,
        "method": "parse_params"
      },
      {
        "message": "pivot != std::string::npos: No delimiter in string, cannot determine data type: garbage",
        "file": "public_key.cpp",
        "line_number": 48,
        "method": "parse_base58"
      }
    ]
  }
}
  1. The error messages are very unfriendly (key is not a key at all)
curl -Ssd '{"keys": ["EOSgarbage"]}' http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers | jq .
{
  "code": 400,
  "message": "Invalid Request",
  "error": {
    "code": 3200006,
    "name": "invalid_http_request",
    "what": "invalid http request",
    "details": [
      {
        "message": "Unable to parse valid input from POST body",
        "file": "chain_api_plugin.cpp",
        "line_number": 47,
        "method": "parse_params"
      },
      {
        "message": "bin.size() == sizeof(data_type) + sizeof(uint32_t): ",
        "file": "public_key.cpp",
        "line_number": 40,
        "method": "parse_base58"
      }
    ]
  }
}
  1. The error messages are very unfriendly (account name has uppercase letter)
curl -sSd '{"accounts": ["Eosnationftw"]}' http://jungle.eosn.io/v1/chain/get_accounts_by_authorizers | jq .
{
  "code": 400,
  "message": "Invalid Request",
  "error": {
    "code": 3200006,
    "name": "invalid_http_request",
    "what": "invalid http request",
    "details": [
      {
        "message": "Unable to parse valid input from POST body",
        "file": "chain_api_plugin.cpp",
        "line_number": 47,
        "method": "parse_params"
      },
      {
        "message": "Name not properly normalized (name: Eosnationftw, normalized: .osnationftw) ",
        "file": "name.cpp",
        "line_number": 15,
        "method": "set"
      }
    ]
  }
}

@matthewdarwin
Copy link

I should add that I didn't test the other nodeos APIs, so maybe the errors here are no worse than the rest of nodoes. But still, seems it could be better. Just use something "does not seem like a valid key" rather then getting code back in the error message.

Also not good practice to echo back the provided input (see "garbage" above test). Could possibly be used to generate some cross-site scripting issue.

@b1bart
Copy link
Contributor Author

b1bart commented Apr 17, 2020

@matthewdarwin Agreed on the un-helpfulness of the messages. I can confirm, those are the "standard" messages from the rest of the APIs but I think its worth cleaning them up in this PR as an example of what we'd like to see. I will ping you again once I have that fixed up.

Also not good practice to echo back the provided input (see "garbage" above test). Could possibly be used to generate some cross-site scripting issue.

I suspect you have verbose-http-errors set to true can you confirm? With that set there are several avenues for "strange" messages in responses. Is it your expectation that these would be safe even with that enabled?

@aaroncox
Copy link
Contributor

For most situations, wallets depend on verbose-http-errors = true to get any sort of useful error reporting back to the end user.

@jnordberg
Copy link

Would be better if the response format was not mixing types (to match the request format and avoid clients having to do typechecking on responses). E.g.

{
   "accounts" : [
      {
         "account_name" : "",
         "permission_name" : "",
         "authorizer_key": "PUB_...",
         "authorizer_auth": null.
         "weight" : 1,
         "threshold": 1
      }
   ]
}

@b1bart
Copy link
Contributor Author

b1bart commented Apr 21, 2020

Would be better if the response format was not mixing types (to match the request format and avoid clients having to do typechecking on responses).

My assumption is that you are asking for the "authorizer" field, which is currently either a string or an object depending on why that entry is present, to be changed into two mutually exclusive fields where the type is no longer varying.

I think that is a fine change. Can others on this thread weigh in with a 👍 or 👎 ?

@gituser
Copy link

gituser commented Apr 22, 2020

Guys, this is a great feature, must have for wallet developers!

Especially get_key_accounts method to get all associated accounts with a specific public key without history plugin or state_history_plugin.

…unt` so that the type of the field can be inferred by the name of the field
@b1bart
Copy link
Contributor Author

b1bart commented Apr 30, 2020

Updated to include the response suggestion from @jnordberg

/**
* Allow moving the account query DB (including by assignment)
*/
account_query_db(account_query_db&&);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not implemented. Also I doubt the operator= is used, I think you should remove them.

…required. Consider moving this to an atomic high-watermark
@heifner heifner requested a review from brianjohnson5972 May 4, 2020 14:55
Copy link
Contributor

@brianjohnson5972 brianjohnson5972 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One or two required changes and several suggestions.

plugins/chain_plugin/chain_plugin.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/account_query_db.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/account_query_db.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/account_query_db.cpp Outdated Show resolved Hide resolved
@b1bart b1bart removed the request for review from arhag May 5, 2020 17:08
… concrete alias pattern, added extra error checking to parsing accounts that ensures no extraneous keys are allowed and that the actor must be present if it is an object, fixed other typos etc
@b1bart b1bart requested a review from brianjohnson5972 May 5, 2020 20:44
@b1bart b1bart merged commit b40015f into release/2.0.x May 18, 2020
nksanthosh added a commit that referenced this pull request Aug 10, 2020
Account Query DB : maintain get_(key|controlled)_accounts develop version ported from #8899
nksanthosh added a commit that referenced this pull request Aug 10, 2020
Account Query DB : maintain get_(key|controlled)_accounts port to develop from #8899
nksanthosh added a commit that referenced this pull request Aug 10, 2020
Account Query DB maintain get_(key|controlled)_accounts port to develop from #8899
nksanthosh added a commit that referenced this pull request Aug 10, 2020
Account Query DB : maintain get_(key|controlled)_accounts port to develop from #8899
@brianjohnson5972 brianjohnson5972 deleted the feature/account_query_db branch November 21, 2020 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants