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

ordering of get-public-keys changes #1429

Closed
jkrauska opened this issue Jan 11, 2019 · 2 comments · Fixed by #1594
Closed

ordering of get-public-keys changes #1429

jkrauska opened this issue Jan 11, 2019 · 2 comments · Fixed by #1594
Assignees

Comments

@jkrauska
Copy link
Contributor

Not a bug, per-se -- mostly an observation.

The numerical ordering of get-public-keys now changes as accounts are added or modified.

Pre-tfc merge, I THINK the ordering was static - and new accounts were appended on to the END of the list.

Now I'm observing that recently changed accounts are added to the TOP of the list.

eg. (after adding a few new accounts and transferring 50 units, the public key list looks like this:

 1, 9999770, KImusi2ZkuWGPXN0YoOiOYRwIVLTqj91h1lYCbosEBDza7AEtmgBAAAB
 2, 50, KASB0MXDyktqZ9dANwXQ1lyNja5CSqrEFr+iPO71hpEWnsyNRI4DAAAB
 3, 50, KF6905x0RLEkzm3eakW6tbppEmpSNjrlCd35TnwgYsIyOQy4G1UDAAAA
 4, 50, KNGPOc1Iax5pOlvgI6wpK+o8DZauEB++lApr7ogYk7UZtQtNcV8DAAAA
 5, 50, KEexREoBMWEyl2yvP6HeTEfEDVoNSA/m84bhrlG6GRvEw05jlt4BAAAB
 6, 50, KFDmexywDzaErx+sqJA2CfGsCwXHYgbBerhJKlxlIfn1IlDxMqsAAAAA
 7, 50, KFMV0arCCMhg0RcQO+hR+AV5GvVHYi/4H6wI4FaxI4rHnclYEpkBAAAB
 8, 50, KFgokYYfJSa1maqsKggP/QSDPLsQcJpwy4JdIxxL+nlP0aNCJ/UAAAAB
 9, 50, KJdSzprEs5eomYM+4cBO+wzlUC8fvo+P5uCQbkgH4jUQGqInIcgCAAAB
10, 50, KHr+GWGlaI0mxh6CN2GJn878wxMycO9iHLpisAofTAt9yi4RJYMDAAAB
11, 50, KJRXW0Brt/Tde6Cz0A+vMEbJb+txaEBfl52kyURI0INey+BVRA8AAAAA
12, 50, KLo3BGRsLEtxj3db3DRkd3u2EUPCyrU61XVr86vqu3PmZcfuQfgAAAAB
13, 50, KMk3A7+Nroevyz0Zh0psF5ZmGyYXFgRGyuZXkONWAH8MzeTquPACAAAB
14, 50, KGPvdSmfhUKD2mib7bjnm+VTip6GFTC4oJ/bjRMLkbafF4U9GOECAAAB
15, 1050, KKjyGpCXQZzIMX71qIXrJi0plpvhTb77DBMnZBZOMospQ+7Ssh8BAAAA
16, 1000, KF/gCh9fJ7rsTA72qygPI+BW1hLOGmNHIj+jXsv30WJpIkrVxzsBAAAA
17, 1000, KPmGh0Hcd+reZhLFrPOhWcYMkSi4qvfkGNkO2g0i1Sz9M8IPakABAAAA
18, 1000, KF5Lzhu3HEKR20EaK8bwKBbZKKVBRRowuYOLFIqye5MgMi4DqLADAAAA
19, 1000, KPwcHBWoR9QzBsm/g46deD7eGFTId0n1xxBV4VoCcDBB3ik4/ekCAAAA
20, 1000, KF4DPWVIdbXOLus9wM/HZS0rIFnX+YpGO9jnAdFt7xLR2QNB66EBAAAB
21, 1000, KEYVC4/00VIjid9Sk7xGzZ46dQ7mWlZ5eFeSSnD7OLvw+NKq25IAAAAA
22, 1000, KKtOMsn5QagJty8uN/9uJeA18V/4ZHWAZl2tqR391DcOBCSiPlYBAAAB
23, 1000, KB8X1InBWdbUBnknkjz33c2MaCvzxSZqqJugHwK5cCQysb87v+AAAAAB
24, 1000, KFhYqUSIKJHkk8ELghLu27TMwUw4zEwwdIwVn06y/z5BebexrPgBAAAA
25, 1000, KHYImaHvbNz3wCv5w2TpJLrZmLKP5y6YcYk6grOzpdXhF4cFcxIDAAAA
26, 1000, KGmEwVccJY/A9dYFzR16F5bmSS7r3HgArxhVLhpmyhClWeEPWoACAAAB
27, 1000, KKZnIR0wa7JnCOEOe+0MbPLrMNc8I9Hu22fDD/iU5tIiYS+NwnwCAAAA
28, 1000, KK5afuhiG4JzEMGtV0Id4oe7hZKta46imNd6UHynVzhskFsDYqkAAAAA
29, 1000, KHvST2E7CskzVqg8ZL0DuWZyRGkJrEBTN8Qif2xhrLvCifowKO8BAAAB
30, 1000, KM7BRDKfDn7HJQZ7vtCMLvL9oBSr5rAGM0ObeHMJUQySWwY4Tw0DAAAA
31, 1000, KNJqKTfF6H2/Z0jqKa947fdjOj0q7GkfnCW5OI6Gf6/cmdAk0b0BAAAB

Lines 2-14 are all NEW accounts.

@emberian
Copy link
Contributor

This is from the new mask-based ledgers. Previously Ledger.to_list would just give the list in merkle-tree-index order. The underlying database still has this behavior. But with masks, it first lists all the accounts in the mask (in some arbitrary order, we get it from a hash map), then all the accounts from the parent.

We should return (index * Account.t) list from to_list and sort the output by the index.

@jkrauska jkrauska changed the title ordering of get-public-keys chainges ordering of get-public-keys changes Jan 18, 2019
@jkrauska
Copy link
Contributor Author

@johnwu93 does this make sense?

I'd actually advocate removing the index entirely and changing the order

from:
1, 10000119, KImusi2ZkuWGPXN0YoOiOYRwIVLTqj91h1lYCbosEBDza7AEtmgBAAAB
2, 65, KGJVFo/Cs6VP5/6NfEfDgCbsm/ETPXIHxsEaaQDCgxTJ2mAwyJgBAAAA
3, 1026, KGmEwVccJY/A9dYFzR16F5bmSS7r3HgArxhVLhpmyhClWeEPWoACAAAB

to
KImusi2ZkuWGPXN0YoOiOYRwIVLTqj91h1lYCbosEBDza7AEtmgBAAAB, 10000119
KGJVFo/Cs6VP5/6NfEfDgCbsm/ETPXIHxsEaaQDCgxTJ2mAwyJgBAAAA, 65
KGmEwVccJY/A9dYFzR16F5bmSS7r3HgArxhVLhpmyhClWeEPWoACAAAB, 1026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants