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

refactor: build delegate list from memory #1134

Merged
merged 48 commits into from Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
8d40c23
chore: remove unused round queries
faustbrian Oct 16, 2018
82991ea
refactor: load delegates from memory
faustbrian Oct 16, 2018
d4dd3bb
docs: update types
faustbrian Oct 16, 2018
f032d1a
refactor: update vote balance for transfers
faustbrian Oct 17, 2018
ff8cadc
fix: remove non-existent query files
faustbrian Oct 17, 2018
38e71e5
fix: compare numbers, not objects
faustbrian Oct 17, 2018
42d4b72
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 17, 2018
2158326
refactor: handle filler wallets
faustbrian Oct 17, 2018
72954ff
chore: update yarn.lock
faustbrian Oct 17, 2018
733bfd9
fix: handle voteBalance as BigNumber
faustbrian Oct 17, 2018
0e4efb0
refactor: add public key sorting for testing
faustbrian Oct 17, 2018
4ed5105
misc: add pubkey sort fixme
faustbrian Oct 17, 2018
3dfe02a
fix: change sort direction to previous (b-a vs. a-b)
faustbrian Oct 17, 2018
d34160f
refactor: log equal balances as warnings
faustbrian Oct 17, 2018
365c30d
misc: wording
faustbrian Oct 17, 2018
b92664b
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 17, 2018
5936fce
refactor: throw if not enough delegates are found
faustbrian Oct 17, 2018
732a594
refactor: handle apply/revert in __updateVoteBalance
faustbrian Oct 17, 2018
27bbfd6
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 17, 2018
8603d40
docs: wording
faustbrian Oct 17, 2018
feb0077
refactor: make buildDelegates sync
faustbrian Oct 17, 2018
922b7b0
refactor: drop __updateVoteBalance for now and instead update all votes
faustbrian Oct 17, 2018
7449a67
misc: try all delegates with equal balance
spkjp Oct 17, 2018
7b4b12f
Merge branch 'develop' into memory-delegates-2
spkjp Oct 17, 2018
f2dd03f
Merge branch 'develop' into memory-delegates-2
spkjp Oct 17, 2018
abd77f4
refactor: remove buildDelegates
spkjp Oct 17, 2018
d01df04
fix: round to number
spkjp Oct 18, 2018
d1e1875
fix: sort by voteBalance
spkjp Oct 18, 2018
60bfa48
fix: map round balance to voteBalance
spkjp Oct 18, 2018
9f2903b
misc: reduce noise
spkjp Oct 18, 2018
05f6474
misc: remove ugly workaround
spkjp Oct 18, 2018
e5deb34
refactor: map voteBalance in model
spkjp Oct 18, 2018
9f8ca22
fix: test
spkjp Oct 18, 2018
901dc66
fix: test
spkjp Oct 18, 2018
dadf49a
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 18, 2018
62eeaa4
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 18, 2018
bb0f54a
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 18, 2018
b80d196
refactor: enable public key sorting
faustbrian Oct 18, 2018
a80e6b8
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 18, 2018
6ff1e62
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 18, 2018
db08358
misc: bit of clean up
faustbrian Oct 18, 2018
01282d5
misc: bit of clean up
faustbrian Oct 18, 2018
655fc50
Merge branch 'memory-delegates-2' of https://github.com/ArkEcosystem/…
faustbrian Oct 18, 2018
a89718c
misc: remove #1152 from this branch
faustbrian Oct 18, 2018
222e2e3
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 18, 2018
0e4dd45
Merge branch 'develop' into memory-delegates-2
faustbrian Oct 18, 2018
df786a6
refactor: enable logging and throw an error on duplicates
faustbrian Oct 18, 2018
037eb2a
refactor: use comparedTo from bignumber.js instead of numbers
faustbrian Oct 18, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 4 additions & 21 deletions packages/core-database-postgres/lib/connection.js
Expand Up @@ -209,31 +209,14 @@ module.exports = class PostgresConnection extends ConnectionInterface {
throw new Error('Trying to build delegates outside of round change')
}

let data = await this.db.rounds.delegates()

// 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 (data.length < maxDelegates) {
const chosen = data.map(delegate => delegate.publicKey)

const fillerWallets = chosen.length
? await this.db.rounds.placeholdersWithout(maxDelegates - data.length, chosen)
: await this.db.rounds.placeholders(maxDelegates - data.length)

data = data.concat(fillerWallets)
}

// logger.info(`got ${data.length} voted delegates`)
const round = Math.floor((height - 1) / maxDelegates) + 1
data = data
.sort((a, b) => b.balance - a.balance)
.slice(0, maxDelegates)

const delegates = await this.walletManager.allActiveDelegates(maxDelegates)
.map(delegate => ({ ...{ round }, ...delegate }))

logger.debug(`Loaded ${data.length} active delegates`)
logger.debug(`Loaded ${delegates.length} active delegates`)

return data
return delegates
}

/**
Expand Down
3 changes: 0 additions & 3 deletions packages/core-database-postgres/lib/queries/index.js
Expand Up @@ -14,9 +14,6 @@ module.exports = {
top: loadQueryFile(__dirname, './blocks/top.sql')
},
rounds: {
delegates: loadQueryFile(__dirname, './rounds/delegates.sql'),
placeholders: loadQueryFile(__dirname, './rounds/placeholders.sql'),
placeholdersWithout: loadQueryFile(__dirname, './rounds/placeholders-without.sql'),
delete: loadQueryFile(__dirname, './rounds/delete.sql'),
find: loadQueryFile(__dirname, './rounds/find.sql')
},
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

28 changes: 0 additions & 28 deletions packages/core-database-postgres/lib/repositories/rounds.js
Expand Up @@ -12,34 +12,6 @@ module.exports = class RoundsRepository extends Repository {
return this.db.manyOrNone(sql.find, { round })
}

/**
* Get all of the delegates for the current round.
* @return {Promise}
*/
async delegates () {
return this.db.manyOrNone(sql.delegates)
}

/**
* Get all of the delegate placeholders for the current round.
* @param {Number} limit
* @return {Promise}
*/
async placeholders (limit) {
return this.db.many(sql.placeholders, { limit })
}

/**
* Get all of the delegate placeholders for the current round. This excludes
* already selected delegates from the initial pick & choose.
* @param {Number} limit
* @param {Array} publicKeys
* @return {Promise}
*/
async placeholdersWithout (limit, publicKeys) {
return this.db.many(sql.placeholdersWithout, { limit, publicKeys })
}

/**
* Delete the round from the database.
* @param {Number} round
Expand Down
84 changes: 79 additions & 5 deletions packages/core-database/lib/wallet-manager.js
@@ -1,10 +1,11 @@
'use strict'

const Promise = require('bluebird')
const { sumBy, orderBy } = require('lodash')

const { Bignum, crypto } = require('@arkecosystem/crypto')
const { Wallet } = require('@arkecosystem/crypto').models
const { TRANSACTION_TYPES } = require('@arkecosystem/crypto').constants
const { ARKTOSHI, TRANSACTION_TYPES } = require('@arkecosystem/crypto').constants
const container = require('@arkecosystem/core-container')
const config = container.resolvePlugin('config')
const logger = container.resolvePlugin('logger')
Expand Down Expand Up @@ -349,39 +350,91 @@ module.exports = class WalletManager {
recipient.applyTransactionToRecipient(data)
}

this.__updateVoteBalance(sender, transaction)

return transaction
}

/**
* Remove the given transaction from a delegate.
* @param {Number} type
* @param {Object} data
* @param {Transaction} transaction
* @return {Transaction}
*/
revertTransaction ({ type, data }) {
revertTransaction (transaction) {
const { type, data } = transaction
const sender = this.findByPublicKey(data.senderPublicKey) // Should exist
const recipient = this.byAddress[data.recipientId]

sender.revertTransactionForSender(data)

// removing the wallet from the delegates index
if (data.type === TRANSACTION_TYPES.DELEGATE_REGISTRATION) {
if (type === TRANSACTION_TYPES.DELEGATE_REGISTRATION) {
delete this.byUsername[data.asset.delegate.username]
}

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

this.__updateVoteBalance(sender, transaction)

return data
}

/**
* Load a list of all active delegates.
* @param {Number} maxDelegates
* @return {Array}
*/
allActiveDelegates (maxDelegates) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The list is FIXED for 1 round

let delegates = this.allByUsername()

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const publicKeys = delegates.map(delegate => delegate.publicKey)

let fillerWallets = this.allByUsername()

if (publicKeys.length) {
fillerWallets = fillerWallets.filter(wallet => !publicKeys.includes(wallet.publicKey))
}

fillerWallets = orderBy(fillerWallets, ['publicKey'], ['asc'])

delegates = delegates.concat(fillerWallets.slice(0, maxDelegates - delegates.length))
}

return delegates.sort((a, b) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
})

const aBalance = +a.voteBalance.toFixed()
const bBalance = +b.voteBalance.toFixed()
Copy link
Contributor

@fix fix Oct 17, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

  • is supposed to cast to integer then, true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

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


// FIXME: The issues with block 178 and 441 are resolved on devnet but
// now other blocks cause issues where delegates have the same balance and
// sorting the public keys alphabetically doesn't solve the issue.

if (aBalance === bBalance) {
logger.warn(`Delegate ${a.username} (${a.publicKey}) and ${b.username} (${b.publicKey}) have a matching vote balance of ${a.voteBalance.dividedBy(ARKTOSHI).toLocaleString()}.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

good idea to log that event


// if (a.publicKey === b.publicKey) {
// throw new 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')
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is commented out?

}

return aBalance - bBalance
}).slice(0, maxDelegates)
}

/**
* Checks if a given publicKey is a registered delegate
* @param {String} publicKey
*/
__isDelegate (publicKey) {
const delegateWallet = this.byPublicKey[publicKey]

if (delegateWallet && delegateWallet.username) {
return !!this.byUsername[delegateWallet.username]
}
Expand All @@ -398,4 +451,25 @@ module.exports = class WalletManager {
return wallet.balance.isZero() && !wallet.secondPublicKey && !wallet.multisignature && !wallet.username
}

/**
* Update the vote balance of the delegate the vote is for.
* @param {Object} sender
* @param {Transaction} transaction
* @return {void}
*/
__updateVoteBalance (sender, transaction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

that is quite complex operation.

  • this is different if the tx is applied or reverted
  • the votes change for sender AND receiver

if (sender.vote && transaction.type === TRANSACTION_TYPES.TRANSFER) {
const delegate = this.findByPublicKey(sender.vote)
const voters = this.allByPublicKey().filter(wallet => (wallet.vote === sender.vote))

delegate.voteBalance = new Bignum(sumBy(voters, 'balance'))
}

if (transaction.type === TRANSACTION_TYPES.VOTE) {
const vote = transaction.asset.votes[0]
const delegate = this.findByPublicKey(vote.substr(1))

delegate.voteBalance[vote.startsWith('+') ? 'plus' : 'minus'](sender.balance)
}
}
}
2 changes: 1 addition & 1 deletion packages/crypto/lib/utils/sort-transactions.js
Expand Up @@ -6,7 +6,7 @@
module.exports = (transactions) => {
// Map to create a new array (sort is done in place)
// TODO does it matter modifying the order of the original array
return transactions.map(t => t).sort((a, b) => {
return transactions.sort((a, b) => {
if (a.type < b.type) {
return -1
}
Expand Down