Lisk-sdk: Next forgers endpoint returning invalid data

[lsilvs]

Expected behavior

When I request /api/delegates/getNextForgers?limit=101 during the first block of a new round I want to get back the list of delegates of the new round.

Actual behavior

It returns the delegates list of the previous round

Steps to reproduce

Call /api/delegates/getNextForgers?limit=101 when currentHeight % 101 == 0

Which version(s) does this affect? (Environment, OS, etc...)

0.9.14, 1.0.0

[lsilvs]

After investigating, I reckon it should be fixed by using currentBlock.height + 1 rather than currentBlock.height alone in https://github.com/LiskHQ/lisk/blob/0.9.14/modules/delegates.js#L782

[webmaster128]

+1 but I think you want to bump the round, not the height.

In generateDelegateList, you can pull out a private function

// shuffles array `delegatesList` in-place
function shuffleDelegatesList(delegatesList, round) {
    let seedSource = round.toString();
    let currentSeed = crypto.createHash('sha256').update(seedSource, 'utf8').digest();
    let delCount = delegatesList.length;

    for (var i = 0; i < delCount; i++) {
        for (var x = 0; x < 4 && i < delCount; i++, x++) {
            const newIndex = currentSeed[x] % delCount;

            // swap i and newIndex
            const b = delegatesList[newIndex];
            delegatesList[newIndex] = delegatesList[i];
            delegatesList[i] = b;
        }
        currentSeed = crypto.createHash('sha256').update(currentSeed).digest();
    }
}

Then in generateDelegateList you just do (pseudo code)

let round;
if (height % 101 == 0) {
    // last block of the round done, return list of next round
    round = modules.rounds.calc(height) + 1;
} else {
    round = modules.rounds.calc(height);
}
shuffleDelegatesList(truncDelegateList, round);

[lsilvs]

getNextForgers should always returns the list of forgers for the block currently being forged. The problem with the current code is that it returns the forgers list using the last forged block height. That's why we should use currentBlock.height + 1 (because currentBlock is the last forged block and not the one being forged)

[webmaster128]

I don't think that is true.

See API responses from https://gist.github.com/webmaster128/2245fd76c424d3fb9d89226323fa9925:

Block at height 5116255 (https://testnet-explorer.lisk.io/block/8020498083563698661) was forged by phoenix1969b (90a0ef7eb62c70dacd751bb8bb18dffcb6c49effe330a76d64341ae100375db4). This delegate was first in queue in the previous call.

[4miners]

@webmaster128 This issue is valid, you can see it clearly in your tests:
Last block of round is 5116256 (not 5116255), on that height API response (https://gist.github.com/webmaster128/2245fd76c424d3fb9d89226323fa9925#file-5116256-json) points delegate 4d6e32111dc36f8074bda232f07119394180b11ac8e9f3698537c909ef24637e to be next forger, but next forger was 1a959e338f0c5d7df03c1f4b9a61447a1fa97335bab65d85ece7bbfef963bc71. currentBlock.height + 1 will work, because list is the same during the round.

However we're not going to make another patch release to fix that issue. I'll verify if it's present in 1.0.0 if yes - we will fix it for that version.

[webmaster128]

Okay, you're right :) I just realized that using currentBlock.height + 1 does only affect the shuffling, not the slicing of the delegates. I misunderstood

The problem with the current code is that it returns the forgers list using the last forged block height.

which is true for the shuffling but not true for the slicing.

However, I still think generateDelegateList should take a round number, not a height because the height is irrelevant for the function.

[lsilvs]

@4miners I've just checked and 1.0.0 has the same behaviour as 0.9

[4miners]

@lsilvs Thank you, so we will fix that in 1.0.0-beta.8.