Lisk-sdk: bad sorting when using balance:asc

[corsaro1]

Expected behavior

When retrieving voters for a delegate i'd expect the sort field to be applied properly so that low balance accounts are returned first and page 2 contains voters that have a higher balance than all entries in page 1 (when using balance:asc).

Actual behavior

Instead voters are queried without any apparent ordering and sorting is applied after the query has run

Steps to reproduce

please take a look at: https://github.com/LiskHQ/lisk/blob/development/modules/voters.js#L245-L252

the following 2 queries are just to see the problem:
/api/voters?sort=balance:asc&address=17670127987160191762L&limit=100&offset=0

/api/voters?sort=balance:asc&address=17670127987160191762L&limit=100&offset=100

Which version(s) does this affect? (Environment, OS, etc...)

core 1

[MaciejBaj]

Thanks, @corsaro1, we should move sort to the corresponding DB query, so the records are ordered in an expected way.

[yatki]

I think having sort functionality here makes sense (with balance).

We can basically extend get_voters.sql to be like below:

SELECT t2.address, t2.balance, encode(t2."publicKey", 'hex') AS "publicKey" FROM mem_accounts2delegates t1
INNER JOIN mem_accounts t2 ON t1."accountId" = t2.address
WHERE t1."dependentId" = ${publicKey}
ORDER BY "${sortField:raw}" ${sortMethod:raw}
LIMIT ${limit} OFFSET ${offset}

And then we can get rid of populatedVoters task in modules/voters.js.

getVoters function is used only in voters controller, so it should be safe to update? There are no unit tests written for it yet. So I will need to write unit tests and update some integration tests.

@nazarhussain I think it doesn't make sense to sort voters with fields other than balance. username is also not possible, since it can be empty. What do you think?

[nazarhussain]

@yatki Sorting on other field (publicKey) was a thought for determinist pagination, in case we remove sorting. So the solution you provided is perfect and good to go, so we keep the sorting for voters as well.

[vekexasia]

It's worth noticing that because no standard ordering is applied to the SQL the pagination is not really meaningful as postgres might decide to return some overlapping set of items for another page.

This is especially true if you use a pool of nodes trying to minimize the number of requests made to a single endpoint.

Pool operators might struggle a lot to fetch the his voters.

[corsaro1]

yes, it looks like this issue affects too pools voters list. At every query voters list is different from the precedent query and from the successive query. I am testing using this python code coming from dakk's lisk pool https://github.com/dakk/lisk-pool/blob/master/liskpool.py.

votes = requests.get (conf['node'] + '/api/voters?address=' + conf['address']).json ()['data']['votes']
        d = []

        for offset in range(0,votes,100):
            dpart = requests.get (conf['node'] + '/api/voters?address=' + conf['address'] + '&offset='+str(offset)+'&limit=100').json ()['data']['voters']
            d += dpart
        d = { "accounts": d }

[biolypl]

I confirm the issues with :desc sorting

[karek314]

Workaround - do not use sorting. Sort yourself.

[4miners]

Reopened. We decided to include the fix for this issue as part of 1.1.0 release but the change implemented is a breaking one (username parameter is removed from sort fields). We cannot do this as part of the minor release.