Go: add "Accounts for Signer" endpoint

[tomerweller]

An endpoint for getting all accounts associated with a specific key.
For example: HTTP GET /signers/ed25519/<address>/accounts

Rationale (multisig):

Currently, most user facing clients assume a one-to-one mapping of keys to accounts. This gets in the way of making multi-sig more accessible to the common user. Such an endpoint will make it easier for Stellar clients to manage all accounts associated with a specific key without having the user explicitly adding them.

Airdrops (spam?):

For better or for worse, this will also allow visibility into airdrops in which the airdropper adds existing accounts as signers for escrow style accounts. However, these airdrops come with a price of XLM minimal balance, so I don't see it becoming a problem

[tomerweller]

Request

HTTP GET /signers/{key_type}/{public_key}/accounts{?cursor,limit,order}

Response

A list of account details, of which the given signer key is part of. Each one similar to the Account Details endpoint response, plus paging information.

Implementation

All required account information is already indexed in stellar-core's database. However, the current "Account Details" endpoint supports only 1 account and makes 4 return trips to stellar-core's DB, which doesn't scale well for multiple accounts, so that code can't be used as is.

Spam

Given that any account can add a signer without permission, the list of accounts can grow unbound without involving the signer. This risk is mitigated by the high cost of creating accounts (2xBASE_RESERVE). Also, it is up to the client apps to provide a good user experience. For example: only display accounts with enough privilege to submit transactions, and/or facilitate account hiding.

[nullstyle]

The design of the endpoint looks good to me, and the discussion on spam seems reasonable. IMO we should keep an eye on usage, and perhaps explore some sort of authorization system for adding a signer similar to AllowTrust with trustlines.

As mentioned in the Implementation section, the cost of loading from the stellar-core database is going to be cost prohibitive. What are your thoughts on solving this?

What order do you plan to impose upon the collection of accounts?

[tomerweller]

Order: lexicographic order of account ids, as that index already exists on the accounts table.

Implementation: I suggest moving the sub-entry aggregation to the sql query. This would save sequential round-trips and give the postgres query planner a chance to parallelize.

The following query seems to do the job though without paging or limit on sub-entry counts yet. I also need to analyze on a larger dataset to ensure it makes proper use of indexes. (a bit verbose, for the sake of readability)

-- aggregated accounts for signers
WITH
  matching_account_ids as (
    -- get account ids by signer
    SELECT accountid
    FROM signers
    WHERE publickey = 'GD3E7HKMRNT6HGBGHBT6I6JE4N2S4W5KZ246TGJ4KQSXJ2P4BXCUPQMP'
    UNION
    -- including master signer account
    SELECT 'GD3E7HKMRNT6HGBGHBT6I6JE4N2S4W5KZ246TGJ4KQSXJ2P4BXCUPQMP'
  ),
  matching_accounts as (
    -- get matching account rows
    SELECT accounts.*
    FROM accounts NATURAL JOIN matching_account_ids
  ),
  matching_signers_agg as (
    -- group matching signers as [account_id, array_of_signers]
    SELECT accountid, json_agg(signers) as signers
    FROM matching_account_ids NATURAL JOIN signers
    GROUP BY accountid
  ),
  matching_data_agg as (
    -- group matching data entries as [account_id, array_of_data_entries]
    SELECT accountid, json_agg(accountdata) as data_entries
    FROM matching_account_ids NATURAL JOIN accountdata
    GROUP BY accountid
  ),
  matching_trustlines_agg as (
    -- group matching trustlines as [account_id, array_of_trustlines]
    SELECT accountid, json_agg(trustlines) as trustlines
    FROM matching_account_ids NATURAL JOIN trustlines
    GROUP BY accountid
  )
SELECT *
FROM matching_accounts
  NATURAL LEFT JOIN matching_signers_agg
  NATURAL LEFT JOIN matching_data_agg
  NATURAL LEFT JOIN matching_trustlines_agg;

Example result (single account with one additional signer)

accountid, balance, sequnum, subentries, inflationdest, homedomain, thresholds, flags, lastmodified, signers, date_entries,trustlines
--GDXFAGJCSCI4CK2YHK6YRLA6TKEXFRX7BMGVMQOBMLIEUJRJ5YQNLMIB,9999999600,17179869188,1,,"",AgICAg==,0,6,"[{""accountid"":""GDXFAGJCSCI4CK2YHK6YRLA6TKEXFRX7BMGVMQOBMLIEUJRJ5YQNLMIB"",""publickey"":""GD3E7HKMRNT6HGBGHBT6I6JE4N2S4W5KZ246TGJ4KQSXJ2P4BXCUPQMP"",""weight"":1}]",[],[]

[tomerweller]

Alternatively, we can serve more compact accounts for this endpoint (without all the subentries) and let the clients decide if/when to fetch the aggregated accounts. However, assuming they'll go and fetch that data for every account right after, it will result in more load on the system.

[tomerweller]

@nikhilsaraf suggests a more compact version of that:

WITH
  matching_account_ids as (
    -- get account ids by signer
    SELECT accountid
    FROM signers
    WHERE publickey = 'GAFEES4MDE5Z7Q6JBB2BYMLS7YWEHTPNR7ICANZA7TAOLMSRELE4H4S2'
    UNION
    -- including master signer account
    SELECT 'GAFEES4MDE5Z7Q6JBB2BYMLS7YWEHTPNR7ICANZA7TAOLMSRELE4H4S2'
  ),
  account_subentries_agg as (
    -- aggregate
    SELECT accountid,
      json_agg(signers) as signers,
      json_agg(accountdata) as data_entries,
      json_agg(trustlines) as trustlines
    FROM matching_account_ids
      NATURAL LEFT JOIN signers
      NATURAL LEFT JOIN accountdata
      NATURAL LEFT JOIN trustlines
    GROUP BY accountid
  )
SELECT *
FROM accounts NATURAL JOIN account_subentries_agg;

While @MonsieurNicolas is dreaded of the thought of any of these queries making it to stellar-core. His exact reaction: :man_facepalming:

[johansten]

@nullstyle

Making adding a signer require authorization would break one of the best features -- being able to sign with a private key that doesn't have any account registered.

[nullstyle]

Making adding a signer require authorization would break one of the best features -- being able to sign with a private key that doesn't have any account registered.

For sure, I wouldn't want to jeopardize that ability @johansten. I need to think some more about the areas around this feature since I didn't consider that problem.

[MonsieurNicolas]

my reaction was purely based on the performance characteristics of such queries: you're looking at a query that will basically lock the entire database. So at best we'll have something really slow in a production environment (that can be part of an amplification attack), at worst a bunch of deadlocks. I think we need to figure out how to first fix the Horizon <> core interface before moving to more complex queries.

[nikhilsaraf]

Alternatively, we can package these queries as being enabled with a special flag/config-value when starting up horizon. This will ensure that people can run this in their own internal horizon instances and we don't have to run it on public horizon. The advantage is that the code and logic will be out there and efficient enough to run for non-SDF entities.

[tomerweller]

@nikhilsaraf there's a separation of concerns issue here. While you can make this a configurable flag in horizon, it affects the performance of the db owned by stellar-core, not horizon.

It seems that the root cause of this problem, pointed by @MonsieurNicolas, is the tight coupling between horizon and stellar-core's db. But, can we still move forward with features like this, or are we waiting to fix the interface between horizon and stellar-core?

[johansten]

If it's not on by default, the better option is to just query stellar-core directly w/ your own api server, unless you need a public facing horizon already. I'd probably go that route anyway, as having people tied to a specific horizon server isn't a great idea either.

[johansten]

@tomerweller

why the complex sql if you're just looking for account signers?

[tomerweller]

@johansten I'm not looking for account signers. I'm looking for all accounts that match a specific signer.
The complexity is driven by the aggregation of subentries per matched account, (trustlines, data entries and signers), which are located in different tables.

[johansten]

Ah, so why not just return the account ID's? As a consumer of this API, I probably have the account info for most of these cached already, and am only interested in new accounts.

I mean, that's what I'd expect.

SELECT accountid from signers where publickey = '{signer}';

[tomerweller]

@johansten you can't design an API based on assumed caching policies. Not to mention that accounts are mutable and their sub-entries can change quite often. Maintaining and invalidating such a cache is not trivial. More likely than not, this type of API will lead to clients making a call to this endpoint and then a series of calls to the Account Details endpoint. Then, instead of just putting stress on the db you also overload the networking i/o.

Also, your query ignores the account to which the signer is the master signer (if it exists).

[johansten]

You can design an API based on what people are using.

I'd assume most wallets do exactly like I do, and cache the hell out of things, and listen to streams for changes.

An API that gives me the account details for all the accounts I'm a signer for would be a waste of networking resources, since I'd most likely throw away everything but the IDs.

Just giving you the other point-of-view.

[bartekn]

Thanks for a discussion everyone. Let's track this in #1472 that will likely be a part of Horizon 0.19.0.