search

LiskArchive / lisk-sdk

🔩 Lisk software development kit
https://lisk.com
Apache License 2.0
2.72k stars 454 forks source link

Unit test review: BFT #7957

Closed janhack closed 1 year ago

janhack commented 1 year ago

bft_processing.spec.ts

  1. In this case, it should not make a difference, but generally it would be good to avoid floor and ceil on floating point numbers and instead only use integer arithmetic. Concretely, Math.floor((scenario.config.activeValidators * 2) / 3) + 1; could be replaced with integer division.
  2. I don't understand the cases expect(bftInfo?.prevoteWeight).toBeUndefined(); and expect(bftInfo?.precommitWeight).toBeUndefined();. I am not sure why why expect the weight to be undefined. Isn't it the case that the bftInfo array does not contain an object at that height in the first place.
  3. I assume this file runs all the tests generated from the CSV files in the bft_processing folder . As I checked that they are unchanged and I reviewed them in the past, I did not review them again.

bft_votes.spec.ts

  1. The state of the BFT votes is not consistent. maxHeightPrevoted == 103, but the block at height 149 has prevote weight 68. This can happen, of course, after calling updatePrevotesPrecommits and before then calling updateMaxHeightPrevoted. But in the BFT protocol no other function is called in between those two functions. Probably that does not create an issue with the test, but in principle it could have undesired side-effects and it would be best to test with a consistent state of BFT votes.
  2. 'should not stake on blocks if generator is not in the validators': It shoud still be called vote in this case, as these are BFT votes that have nothing to do with DPoS.
  3. 'should not increment the prevote when generator is not active': The provided block with maxHeightGenerated == 0 would not be added to the chain as it contradicts the block at height 149 by the same generator. Maybe this is still fine for the tests, but it could be better to have a valid block.
  4. 'should not update maxHeightPrevoted if no block info exceeds threshold': I think it would be better to test prevoteThreshold: BigInt(69), i.e., exactly just increment the minimal amount to get a different result, instead of choosing an extreme.
  5. 'should store maximum height where prevote exceeds threshold': You mean precommit here, I think. At least this is what is tested.
  6. 'should not update maxHeightPrevoted if no block info exceeds threshold': Same as for point 6 here.
  7. 'should not update maxHeightCertified when aggregateCommit is empty': The test is correct, but note that in that case, the block should be invalid as we should have aggregateCommit.height == bftVotes.maxHeightCertified. This validation is not defined in LIP 58, but part of the aggregate commit validation.

method.spec.ts

  1. areHeadersContradicting: These tests only cover the easy cases, but not the cases depending on maxHeightPrevoted and maxHeightGenerated. I guess we assume here those tests are covered by the function areDistinctHeadersContradicting.
  2. isHeaderContradictingChain: The initialization maxHeightPrevoted: 10, does not make sense. This probably does not affect the test.
  3. 'getBFTParameters': I think it would be nice to also test the return value for heights 29 and 30.
  4. 'getNextHeightBFTParameters': It would be good to test that for input 19 the function returns 20.
  5. 'should store BFT parameters with height maxHeightPrevoted + 1 if blockBFTInfo does not exist': The BFT parameters should be stored at genesisHeight + 1 in this case (note that only after processing the genesis block, blockBFTInfo should be empty and then we actually have maxHeightPrevoted == genesisHeight).

utils.spec.ts

  1. 'areDistinctHeadersContradicting': In some cases, we do not define maxHeightGenerated or maxHeightPrevoted for the block headers (I assume they are then just some random value). I think it would be better to also define those, to be sure that the headers are not considered contradicting for other reasons. Currently, it is also difficult to see that all cases in the function are covered.

General comment

  1. All the test cases seem with uniform BFT weights of 1. It would be nice to use general BFT weights, i.e., arbitrary integers as BFT weights, to check these are also handled correctly.
  2. There is no test currently for the functions getGeneratorAtTimestamp and getSlotNumber. They are maybe not directly part of the BFT protocol, but are defined in LIP 0058.
shuse2 commented 1 year ago

In this case, it should not make a difference, but generally it would be good to avoid floor and ceil on floating point numbers and instead only use integer arithmetic. Concretely, Math.floor((scenario.config.activeValidators * 2) / 3) + 1; could be replaced with integer division.

As far as the research by @AndreasKendziorra , the division is deterministic although there will be an rounding error when using floating number

I don't understand the cases expect(bftInfo?.prevoteWeight).toBeUndefined(); and expect(bftInfo?.precommitWeight).toBeUndefined();. I am not sure why why expect the weight to be undefined. Isn't it the case that the bftInfo array does not contain an object at that height in the first place.

This is because blockBFTInfo will be removed with batch size * 3 while inserting a block. although it exists in the fixture, the below min height, the value should be undefined

areHeadersContradicting: These tests only cover the easy cases, but not the cases depending on maxHeightPrevoted and maxHeightGenerated. I guess we assume here those tests are covered by the function areDistinctHeadersContradicting.

yes. those should be covered in areDistinctHeadersContradicting.

'should store BFT parameters with height maxHeightPrevoted + 1 if blockBFTInfo does not exist': The BFT parameters should be stored at genesisHeight + 1 in this case (note that only after processing the genesis block, blockBFTInfo should be empty and then we actually have maxHeightPrevoted == genesisHeight).

I think in the BFT, it is not considering genesis height. the logic is if the bft info is empty, store maxHeightPrevoted + 1 which only happens in genesis block case?

janhack commented 1 year ago

I think in the BFT, it is not considering genesis height. the logic is if the bft info is empty, store maxHeightPrevoted + 1 which only happens in genesis block case?

No, in this case the genesis height is used, see the function getCurrentHeight in https://github.com/LiskHQ/lips/blob/main/proposals/lip-0058.md#getcurrentheight

shuse2 commented 1 year ago

No, in this case the genesis height is used, see the function getCurrentHeight in https://github.com/LiskHQ/lips/blob/main/proposals/lip-0058.md#getcurrentheight

I think we missed to update this, ill try to address it in different PR

shuse2 commented 1 year ago

I was double checking and currently we store genesis height as maxHeightPrevoted in https://github.com/LiskHQ/lisk-sdk/blob/development/framework/src/engine/bft/module.ts#L49-L60

Therefore, when we use in the empty case https://github.com/LiskHQ/lisk-sdk/blob/development/framework/src/engine/bft/method.ts#L222, the maxHeightPrevoted is genesis height