refactor: build delegate list from memory #1134

faustbrian · 2018-10-17T03:49:03Z

Proposed changes

Resolves #850

Types of changes

Refactoring (improve a current implementation without adding a new feature or fixing a bug)

Checklist

I have read the CONTRIBUTING documentation
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective

codecov-io · 2018-10-17T03:53:50Z

Codecov Report

Merging #1134 into develop will increase coverage by 0.04%.
The diff coverage is 89.28%.

@@             Coverage Diff             @@
##           develop    #1134      +/-   ##
===========================================
+ Coverage    78.08%   78.13%   +0.04%     
===========================================
  Files          425      425              
  Lines         6923     6925       +2     
  Branches       891      895       +4     
===========================================
+ Hits          5406     5411       +5     
+ Misses        1343     1341       -2     
+ Partials       174      173       -1

Impacted Files	Coverage Δ
...ackages/core-database-postgres/lib/models/round.js	`100% <ø> (ø)`	⬆️
...ckages/core-database-postgres/lib/queries/index.js	`100% <ø> (ø)`	⬆️
.../core-database-postgres/lib/repositories/rounds.js	`100% <ø> (+20%)`	⬆️
packages/core-database-postgres/lib/connection.js	`74.39% <ø> (+0.3%)`	⬆️
packages/core-utils/lib/round-calculator.js	`100% <100%> (ø)`	⬆️
packages/core-database/lib/interface.js	`74.24% <100%> (-0.2%)`	⬇️
packages/crypto/lib/utils/sort-transactions.js	`72.72% <100%> (ø)`	⬆️
packages/core-database/lib/wallet-manager.js	`74.83% <87.5%> (+1.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82f6f4f...037eb2a. Read the comment docs.

faustbrian · 2018-10-17T04:27:12Z

178 and 441 now pass on devnet but 866 blows up because 2 delegates have the same balance.

Haven't applied any public key sorting to the final list yet.

fix · 2018-10-17T08:02:30Z

packages/core-database/lib/wallet-manager.js

+    // NOTE: At the launch of the blockchain we may not have enough delegates.
+    // In order to have enough forging delegates we complete the list in a
+    // deterministic way (alphabetical order of publicKey).
+    if (delegates.length < maxDelegates) {


it should throw if there is not enough delegates (ie genesisBlock should register at least maxDelegates)

Should we shutdown the node in case there are not enough delegates? Doesn't seem to make sense to keep it running without enough delegates.

yes, shut down is required, you could say genesis block is illformed with not enough registered delegates

fix · 2018-10-17T08:04:40Z

packages/core-database/lib/wallet-manager.js

+
+    return delegates.sort((a, b) => {
+      const aBalance = +a.voteBalance.toFixed()
+      const bBalance = +b.voteBalance.toFixed()


mmm toFixed gives you a string, this aBalance - bBalance is alphabetical order

is supposed to cast to integer then, true

The unary plus (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Arithmetic_Operators#Unary_plus_()) turns the toFixed result into an integer.

i expect however a performance hit, that might be an issue in the future (lots of registered delegates)

fix · 2018-10-17T08:09:33Z

packages/core-database/lib/wallet-manager.js

+      delegates = delegates.concat(fillerWallets.slice(0, maxDelegates - delegates.length))
+    }
+
+    return delegates.sort((a, b) => {


before doing that i would make a function that recalculate all vote balance:

this.allByPublicKeys().forEach((w)=> { if(w.vote) this.byPublicKey[w.vote].voteBalance += w.balance //missing initialisation })

fix · 2018-10-17T08:11:31Z

packages/core-database/lib/wallet-manager.js

+   * @param  {Transaction} transaction
+   * @return {void}
+   */
+  __updateVoteBalance (sender, transaction) {


that is quite complex operation.

this is different if the tx is applied or reverted

the votes change for sender AND receiver

fix · 2018-10-17T09:21:46Z

On the overall,

we need to be sure that the call to allActiveDelegates is done ONLY at turn round. Otherwise the active delegate list is NOT the right one.
as first iteration we can use recompute the voteBalance from scratch at each turn round. This has the property to stay the same at each call of listActive delegates:
applyRound -> if round change then walletManager.updateDelegates and store this list in walletManager on a different storage
revertRound -> if round change this is complex, needs to get the list from database (table rounds).
as second iteration, the voteBalance is updated at each block being applied. This will cut the performance issue of recomputing everything at turn round that will only backup the list for the next round.

faustbrian · 2018-10-18T03:28:29Z

Syncing on devnet seems to work without any issues now but mainnet is still having some issues so will continue to look into it.

fix · 2018-10-17T09:10:20Z

packages/core-database/lib/wallet-manager.js

+
+    return delegates.sort((a, b) => {
+      const aBalance = +a.voteBalance.toFixed()
+      const bBalance = +b.voteBalance.toFixed()


fix · 2018-10-17T09:11:26Z

packages/core-database/lib/wallet-manager.js

+    // NOTE: At the launch of the blockchain we may not have enough delegates.
+    // In order to have enough forging delegates we complete the list in a
+    // deterministic way (alphabetical order of publicKey).
+    if (delegates.length < maxDelegates) {


yes, shut down is required, you could say genesis block is illformed with not enough registered delegates

fix · 2018-10-17T09:11:38Z

packages/core-database/lib/wallet-manager.js

+   * @param  {Number} maxDelegates
+   * @return {Array}
+   */
+  allActiveDelegates (maxDelegates) {


The list is FIXED for 1 round

fix · 2018-10-17T09:13:40Z

packages/core-database/lib/wallet-manager.js

+    Object
+      .values(this.byUsername)
+      .forEach(delegate => (delegate.voteBalance = Bignum.ZERO))
+
    Object.values(this.byPublicKey)
      .filter(voter => !!voter.vote)


this is slower than using forEach directly and then testing if(voter.vote)

fix · 2018-10-17T09:23:17Z

packages/core-database/lib/wallet-manager.js

+
+    return delegates.sort((a, b) => {
+      const aBalance = +a.voteBalance.toFixed()
+      const bBalance = +b.voteBalance.toFixed()


i expect however a performance hit, that might be an issue in the future (lots of registered delegates)

fix · 2018-10-17T09:26:50Z

packages/core-database/lib/wallet-manager.js

      delete this.byUsername[data.asset.delegate.username]
    }

    if (recipient && type === TRANSACTION_TYPES.TRANSFER) {
      recipient.revertTransactionForRecipient(data)
    }

+    this.__updateVoteBalance('revert', sender, transaction)


what about having 2 functions:
__applyVoteBalance(sender, transaction)
__revertVoteBalance(receiver, transaction)

well this is a bit more complex than that actually

the logic is:

apply block reward to delegate -> add this vote reward to his vote delegate he is voting

apply transaction to receiver -> add the tx.amount to the vote if receiver is voting

apply transaction to sender -> remove the tx.fees to the vote if sender is voting

apply unvote transaction to sender -> remove sender.balance to the vote

apply vote transaction to sender -> add sender.balance to the new vote

Then same 5 logic in case of revert

fix · 2018-10-17T14:47:51Z

packages/core-database/lib/wallet-manager.js

    Object.values(this.byPublicKey)
      .filter(voter => !!voter.vote)
      .forEach(voter => {
        const delegate = this.byPublicKey[voter.vote]
        delegate.voteBalance = delegate.voteBalance.plus(voter.balance)
      })
+
    Object.values(this.byUsername)
      .sort((a, b) => +(b.voteBalance.minus(a.voteBalance)).toFixed())


why b.voteBalance.minus(a.voteBalance) is not enough or even b.voteBalance - a.voteBalance?

Just to make sure we are actually sorting by the numerical value instead of string that is returned by toFixed, and consistency.

fix · 2018-10-18T08:31:48Z

packages/core-database/lib/wallet-manager.js

+   * @param  {Number} maxDelegates
+   * @return {Array}
+   */
+  activeDelegation (maxDelegates, height) {


what about loadActiveDelegateList as a function name?

fix · 2018-10-18T10:26:25Z

packages/core-database-postgres/lib/connection.js

@@ -185,7 +183,10 @@ module.exports = class PostgresConnection extends ConnectionInterface {
      currentSeed = crypto.createHash('sha256').update(currentSeed).digest()
    }

-    this.activeDelegates = data
+    this.activeDelegates = data.map(delegate => {
+      delegate.round = +round


what is the rationale for this?

There is a comparison somewhere that was always false because it compared string === integer, this fixes the comparison to integer === integer.

i mean why adding delegate.round property?

ah you mean you override the round property of delegates?

@supaiku0 added it for here https://github.com/ArkEcosystem/core/blob/develop/packages/core-database-postgres/lib/connection.js#L167

@fix See #1143 and #1152

fix · 2018-10-18T10:39:43Z

packages/core-database/lib/wallet-manager.js

+          logger.error(`The balance and public key of both delegates are identical! Delegate "${a.username}" appears twice in the list.`)
+        }
+
+        return a.publicKey.localeCompare(b.publicKey, 'en-US-u-kf-lower')


why this 'en-US-u-kf-lower' ? 'en' should be ok as there is only '0123456789abcdef' as letters

Was mainly to make sure the string is treated as a lowercase string in case there ever is a mixed-case public key for whatever reason but changed it to en.

…core into memory-delegates-2

fix · 2018-10-18T12:31:08Z

packages/core-database/lib/wallet-manager.js

+    }
+
+    delegates = delegates.sort((a, b) => {
+      const aBalance = +a.voteBalance.toFixed()


by the way what is the point of this? i guess keeping a.voteBalance is enough.

they are numbers

if there is a rounding issue, since they are all integers, it won't change the order

a.voteBalance is a bignumber object inside a wallet object.

We could use https://mikemcl.github.io/bignumber.js/#cmp

ah true that, i should keep that in mind now. Is there some kind of comparison stuff in bignumber library, sounds weird to cast down to normal number in order to compare again

http://mikemcl.github.io/bignumber.js/#cmp

fix · 2018-10-18T12:32:03Z

packages/core-database/lib/wallet-manager.js

+        // logger.warn(`Delegate ${a.username} (${a.publicKey}) and ${b.username} (${b.publicKey}) have a matching vote balance of ${a.voteBalance.dividedBy(Math.pow(10, 8)).toLocaleString()}.`)
+
+        if (a.publicKey === b.publicKey) {
+          logger.error(`The balance and public key of both delegates are identical! Delegate "${a.username}" appears twice in the list.`)


i think we should throw here, because it means something even wronger happened

fix · 2018-10-18T12:32:14Z

packages/core-database/lib/wallet-manager.js

+      const bBalance = +b.voteBalance.toFixed()
+
+      if (aBalance === bBalance) {
+        // logger.warn(`Delegate ${a.username} (${a.publicKey}) and ${b.username} (${b.publicKey}) have a matching vote balance of ${a.voteBalance.dividedBy(Math.pow(10, 8)).toLocaleString()}.`)


we can keep this imo

I changed it because it issues warnings for all standby delegates as well unless we make it more strict.

fix · 2018-10-18T12:51:01Z

ok sounds good me. If it rebuilds the first 1000s blocks on mainnet/devnet, we can merge

faustbrian · 2018-10-18T12:52:48Z

@supaiku0 you still had a thing you would like to add in this PR right?

spkjp · 2018-10-18T12:59:14Z

Will open a separate PR.

faustbrian added 4 commits October 16, 2018 16:51

chore: remove unused round queries

8d40c23

refactor: load delegates from memory

82991ea

docs: update types

d4dd3bb

refactor: update vote balance for transfers

f032d1a

faustbrian requested a review from spkjp October 17, 2018 03:49

faustbrian mentioned this pull request Oct 17, 2018

[WIP] refactor: build delegate list from memory #1128

Closed

3 tasks

faustbrian and others added 6 commits October 17, 2018 06:54

fix: remove non-existent query files

ff8cadc

fix: compare numbers, not objects

38e71e5

Merge branch 'develop' into memory-delegates-2

42d4b72

refactor: handle filler wallets

2158326

chore: update yarn.lock

72954ff

fix: handle voteBalance as BigNumber

733bfd9

faustbrian and others added 6 commits October 17, 2018 07:48

refactor: add public key sorting for testing

0e4efb0

misc: add pubkey sort fixme

4ed5105

fix: change sort direction to previous (b-a vs. a-b)

3dfe02a

refactor: log equal balances as warnings

d34160f

misc: wording

365c30d

Merge branch 'develop' into memory-delegates-2

b92664b

fix reviewed Oct 17, 2018

View reviewed changes

refactor: throw if not enough delegates are found

5936fce

fix reviewed Oct 17, 2018

View reviewed changes

faustbrian and others added 3 commits October 17, 2018 11:41

refactor: handle apply/revert in __updateVoteBalance

732a594

Merge branch 'develop' into memory-delegates-2

27bbfd6

docs: wording

8603d40

refactor: make buildDelegates sync

feb0077

spkjp and others added 4 commits October 18, 2018 03:41

refactor: map voteBalance in model

e5deb34

fix: test

9f8ca22

fix: test

901dc66

Merge branch 'develop' into memory-delegates-2

dadf49a

faustbrian added 5 commits October 18, 2018 06:49

Merge branch 'develop' into memory-delegates-2

62eeaa4

Merge branch 'develop' into memory-delegates-2

bb0f54a

refactor: enable public key sorting

b80d196

Merge branch 'develop' into memory-delegates-2

a80e6b8

Merge branch 'develop' into memory-delegates-2

6ff1e62

fix reviewed Oct 18, 2018

View reviewed changes

faustbrian and others added 5 commits October 18, 2018 13:49

misc: bit of clean up

db08358

misc: bit of clean up

01282d5

Merge branch 'memory-delegates-2' of https://github.com/ArkEcosystem/…

655fc50

…core into memory-delegates-2

misc: remove #1152 from this branch

a89718c

Merge branch 'develop' into memory-delegates-2

222e2e3

faustbrian changed the title ~~[WIP] refactor: build delegate list from memory~~ refactor: build delegate list from memory Oct 18, 2018

Merge branch 'develop' into memory-delegates-2

0e4dd45

fix reviewed Oct 18, 2018

View reviewed changes

faustbrian added 2 commits October 18, 2018 15:34

refactor: enable logging and throw an error on duplicates

df786a6

refactor: use comparedTo from bignumber.js instead of numbers

037eb2a

fix approved these changes Oct 18, 2018

View reviewed changes

spkjp merged commit c95e170 into develop Oct 18, 2018

faustbrian deleted the memory-delegates-2 branch October 18, 2018 12:59

spkjp mentioned this pull request Oct 26, 2018

Optimise processing of blocks #975

Closed

refactor: build delegate list from memory #1134

refactor: build delegate list from memory #1134

Conversation

faustbrian commented Oct 17, 2018 • edited

Proposed changes

Types of changes

Checklist

codecov-io commented Oct 17, 2018 • edited

Codecov Report

faustbrian commented Oct 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix Oct 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustbrian Oct 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix commented Oct 17, 2018

faustbrian commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix commented Oct 18, 2018

faustbrian commented Oct 18, 2018

spkjp commented Oct 18, 2018

faustbrian commented Oct 17, 2018 •

edited

codecov-io commented Oct 17, 2018 •

edited

faustbrian commented Oct 17, 2018 •

edited

fix Oct 17, 2018 •

edited

faustbrian Oct 17, 2018 •

edited