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
Changes from 40 commits
8d40c23
82991ea
d4dd3bb
f032d1a
ff8cadc
38e71e5
42d4b72
2158326
72954ff
733bfd9
0e4efb0
4ed5105
3dfe02a
d34160f
365c30d
b92664b
5936fce
732a594
27bbfd6
8603d40
feb0077
922b7b0
7449a67
7b4b12f
f2dd03f
abd77f4
d01df04
d1e1875
60bfa48
9f2903b
05f6474
e5deb34
9f8ca22
901dc66
dadf49a
62eeaa4
bb0f54a
b80d196
a80e6b8
6ff1e62
db08358
01282d5
655fc50
a89718c
222e2e3
0e4dd45
df786a6
037eb2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ const Promise = require('bluebird') | |
const { Bignum, crypto } = require('@arkecosystem/crypto') | ||
const { Wallet } = require('@arkecosystem/crypto').models | ||
const { TRANSACTION_TYPES } = require('@arkecosystem/crypto').constants | ||
const { roundCalculator } = require('@arkecosystem/core-utils') | ||
const container = require('@arkecosystem/core-container') | ||
const config = container.resolvePlugin('config') | ||
const logger = container.resolvePlugin('logger') | ||
|
@@ -184,18 +185,61 @@ module.exports = class WalletManager { | |
Object.values(this.byAddress).map(wallet => (wallet.dirty = false)) | ||
} | ||
|
||
/** | ||
* Load a list of all active delegates. | ||
* @param {Number} maxDelegates | ||
* @return {Array} | ||
*/ | ||
activeDelegation (maxDelegates, height) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about loadActiveDelegateList as a function name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed |
||
if (height > 1 && height % maxDelegates !== 1) { | ||
throw new Error('Trying to build delegates outside of round change') | ||
} | ||
|
||
const { round } = roundCalculator.calculateRound(height, maxDelegates) | ||
let delegates = this.allByUsername() | ||
|
||
if (delegates.length < maxDelegates) { | ||
throw new Error(`Expected to find ${maxDelegates} delegates but only found ${delegates.length}. This indicates an issue with the genesis block & delegates.`) | ||
} | ||
|
||
delegates = delegates.sort((a, b) => { | ||
const aBalance = +a.voteBalance.toFixed() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by the way what is the point of this? i guess keeping a.voteBalance is enough.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use https://mikemcl.github.io/bignumber.js/#cmp There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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()}.`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can keep this imo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it because it issues warnings for all standby delegates as well unless we make it more strict. |
||
|
||
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.`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we should throw here, because it means something even wronger happened |
||
} | ||
|
||
return a.publicKey.localeCompare(b.publicKey, 'en-US-u-kf-lower') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this 'en-US-u-kf-lower' ? 'en' should be ok as there is only '0123456789abcdef' as letters There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
return bBalance - aBalance | ||
}).slice(0, maxDelegates) | ||
|
||
logger.debug(`Loaded ${delegates.length} active delegates`) | ||
|
||
return delegates.map(delegate => ({ ...{ round }, ...delegate })) | ||
} | ||
|
||
/** | ||
* Update the vote balances of delegates. | ||
* @return {void} | ||
*/ | ||
updateDelegates () { | ||
Object.values(this.byUsername).forEach(delegate => (delegate.voteBalance = Bignum.ZERO)) | ||
Object | ||
.values(this.byUsername) | ||
.forEach(delegate => (delegate.voteBalance = Bignum.ZERO)) | ||
|
||
Object.values(this.byPublicKey) | ||
.filter(voter => !!voter.vote) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is slower than using forEach directly and then testing if(voter.vote) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed |
||
.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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to make sure we are actually sorting by the numerical value instead of string that is returned by |
||
.forEach((delegate, index) => (delegate.rate = index + 1)) | ||
|
@@ -354,18 +398,18 @@ module.exports = class WalletManager { | |
|
||
/** | ||
* 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] | ||
} | ||
|
||
|
@@ -382,6 +426,7 @@ module.exports = class WalletManager { | |
*/ | ||
__isDelegate (publicKey) { | ||
const delegateWallet = this.byPublicKey[publicKey] | ||
|
||
if (delegateWallet && delegateWallet.username) { | ||
return !!this.byUsername[delegateWallet.username] | ||
} | ||
|
@@ -397,5 +442,4 @@ module.exports = class WalletManager { | |
__canBePurged (wallet) { | ||
return wallet.balance.isZero() && !wallet.secondPublicKey && !wallet.multisignature && !wallet.username | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the rationale for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a comparison somewhere that was always
false
because it comparedstring === integer
, this fixes the comparison tointeger === integer
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean why adding delegate.round property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah you mean you override the round property of delegates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@supaiku0 added it for here https://github.com/ArkEcosystem/core/blob/develop/packages/core-database-postgres/lib/connection.js#L167
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fix See #1143 and #1152