[LiskHQ/lisk] Balances in mem_accounts off after snapshotting – Closes #2126 (#2127)

nazarhussain requested changes on this pull request.

> @@ -38,6 +38,9 @@ CREATE TABLE “rounds_rewards”(
“publicKey” BYTEA NOT NULL
);

+– Insert record for dummy round 1 when genesis block is in database
+INSERT INTO rounds_rewards SELECT timestamp, 0, 0, 1, “generatorPublicKey” FROM blocks WHERE height = 1;
+

Please change the query to

“`sql
INSERT INTO rounds_rewards (timestamp, fees, reward, round, “publicKey”)
SELECT timestamp, 0, 0, 1, “generatorPublicKey” FROM blocks WHERE height = 1;
“`
That makes more readable to know which values goes to which columns.

> @@ -387,6 +387,16 @@ __private.getOutsiders = function(scope, cb, tx) {
* @todo Add description for the params and the return value
*/
__private.sumRound = function(scope, cb, tx) {

The `__private.sumRound` is triggered from `tick`. There is a condition.

“`
// Establish if finishing round or not
scope.finishRound =
round !== nextRound || (block.height === 1 || block.height === 101);
“`

I can’t understand why this condition contains `|| block.height === 101`

> @@ -51,18 +54,32 @@ describe(‘snapshotting’, () => {

before(() => {
// Forge 201 blocks to reach height 202 (genesis block is already tyhere)
– return Promise.mapSeries([…Array(201)], () => {
+ return Promise.mapSeries([…Array(99)], () => {

Its a bit confusing you are forging `99` blocks here and comment above states `201` blocks. After `genesisBlock` at height `1` and then rest of `99` blocks we will have 100 blocks, how you calculated those to 201?

> @@ -51,18 +54,32 @@ describe(‘snapshotting’, () => {

before(() => {
// Forge 201 blocks to reach height 202 (genesis block is already tyhere)
– return Promise.mapSeries([…Array(201)], () => {
+ return Promise.mapSeries([…Array(99)], () => {

If we see steps just below the total blocks become

“`
1 + 99 + 101 + 101 = 302
“`

> @@ -387,6 +387,16 @@ __private.getOutsiders = function(scope, cb, tx) {
* @todo Add description for the params and the return value
*/
__private.sumRound = function(scope, cb, tx) {
+ // When we need to sum round just after genesis block (height: 1)
+ // – set data manually to 0, they will be distributed when actual round 1 is summed
+ if (scope.block.height === 1) {
+ library.logger.debug(`Summing round – ${scope.round} (genesis block)`);
+ scope.roundFees = 0;
+ scope.roundRewards = [0];
+ scope.roundDelegates = [scope.block.generatorPublicKey];
+ return setImmediate(cb);
+ }
+
library.logger.debug(‘Summing round’, scope.round);

(tx || library.db).rounds

Th eline below states:

“`
.summedRound(scope.round, constants.activeDelegates)
“`
To avoid any mistake we should always calculate `round` from the `height` of the block given in scope. Because its fully dependent value.

With this approach there can be mistake in future to sum a `round` with wrong block height.

> – {
– rewards: [1.001, 2, 3],
– fees: 100.001,
– delegates: [‘delegate1’, ‘delegate2’, ‘delegate3’],
– },
– ];
– stub = sinon.stub(db.rounds, ‘summedRound’).resolves(rows);
– done();
– });

– after(() => {
– return stub.restore();
– });
+ describe(‘when last block is genesis block’, () => {
+ const scope = {
+ round: 1,

As stated above, `round` should not be passed as scope to sumRound, it should always calculate `round` from the `height` of the block.

> @@ -402,37 +395,81 @@ describe(’rounds’, () => {
});

it(‘should set scope.roundFees correctly’, () => {

Better to set the test case explicitly to

> should set scope.roundFees correctly to zero

> @@ -402,37 +395,81 @@ describe(’rounds’, () => {
});

it(‘should set scope.roundFees correctly’, () => {
– return expect(scope.roundFees).to.equal(100);
+ return expect(scope.roundFees).to.equal(0);
});

it(‘should set scope.roundRewards correctly’, () => {

Same as above

> + describe(‘when summedRound query is successful’, () => {
+ before(done => {
+ var rows = [
+ {
+ rewards: [1.001, 2, 3],
+ fees: 100.001,
+ delegates: [‘delegate1’, ‘delegate2’, ‘delegate3’],
+ },
+ ];
+ stub = sinon.stub(db.rounds, ‘summedRound’).resolves(rows);
+ done();
+ });
+
+ after(() => {
+ return stub.restore();
+ });

Would be nice if convert these to `beforeEach` and `afterEach`, I notice difficulties with `before` and `after` maintaining tests in long run.

This post was last modified on June 14, 2018, 10:46 am