Unit test review for block processing (LIP 0055)

[AndreasKendziorra]

Block

• Some more precise tests regarding MAX_TRANSACTIONS_SIZE_BYTES than the exiting one would be good. That means, one test where the size equals MAX_TRANSACTIONS_SIZE_BYTES and that passes. And one test where the size equals MAX_TRANSACTIONS_SIZE_BYTES+1 and that fails.
• It should be tested that assets.validate() and blockHeader.validate() are called during block processing and that block processing fails if one of them fails.
• It should be tested that for every transaction in the block, every hook ("Transaction verification", ..., "After command execution") is executed and that the whole block is discarded if one of the hook calls fails (except for "Command execution") .

Block Header

• The properties eventRoot and impliesMaxPrevotes are missing in blockHeaderProps in block_header.spec.ts.
• The test for the constructor misses the check for the impliesMaxPrevotes property.

Timestamp

• Test for "timestamp in the future":
  • It seems that this line assigns the same slot number to the timestamp of the block, the current system time and the timestamp of the previous block, which should not be the case. So, an error is thrown because the slots of the block and the previous block are equal, and not because the timestamp is too far in the future.
  • Also, the slot returned by the mocked function should be Math.floor(Date.now() / 10000) instead of Math.floor(Date.now() / 10)?

State Root

• There is no test for the stateRoot property. In particular, there is no test that expects that block processing fails if the value of header.stateRoot in incorrect.

BFT properties

• test for invalid impliesMaximalPrevotes property is missing
• in the test for the success case, also the function impliesMaximalPrevotes should be mocked to ensure that it returns true.

Block Assets

• Test for exceeding MAX_ASSET_DATA_SIZE_BYTES: It would be good if the size of the data property equals MAX_ASSET_DATA_SIZE_BYTES+1 instead of an arbitrary value.
• Also for the test for not exceeding the size, it may be better to use MAX_ASSET_DATA_SIZE_BYTES instead of 64 to make it independent of the value of MAX_ASSET_DATA_SIZE_BYTES. But this is not so important.
• test for non-sorted assets: it is still talking about the moduleId instead of module. Also in the expectation.
  • Also here and here.
• There seems to be no test to ensure that block processing fails if asset.module does not refer to a registered module for any asset asset in the block.

[shuse2]

It should be tested that assets.validate() and blockHeader.validate() are called during block processing and that block processing fails if one of them fails.

this is tested by checking chain.validateBlock is called. internally it calls those validation.

It should be tested that for every transaction in the block, every hook ("Transaction verification", ..., "After command execution") is executed and that the whole block is discarded if one of the hook calls fails (except for "Command execution") .

this is tested in the state machine https://github.com/LiskHQ/lisk-sdk/blob/bcfec00b6c918e635ef009249fcb6e7b563bd4c7/framework/test/unit/state_machine/state_machine.spec.ts

The properties eventRoot and impliesMaxPrevotes are missing in blockHeaderProps in block_header.spec.ts.

eventRoot are tested in consensus.spec.ts. impliesMaxPrevotes test should be added in there as well

State Root
There is no test for the stateRoot property. In particular, there is no test that expects that block processing fails if the value of header.stateRoot in incorrect.

State root verification is done in the lisk-db during the commit of the ABI. That's why they are not explicitly tested here

There seems to be no test to ensure that block processing fails if asset.module does not refer to a registered module for any asset asset in the block.

this is handled in another PR https://github.com/LiskHQ/lisk-sdk/pull/8440/files#diff-e5b4a5e9bd2d527e3be245f514f69edce40293da78958774fbb052a8f6e32201R316-R332

[AndreasKendziorra]

this is tested by checking chain.validateBlock is called. internally it calls those validation.

What I meant is that either

• for chain.validateBlock it should be tested that assets.validate and header.validate are called, or
• for chain.validateBlock it should be tested that block.validate is called, and for block.validate it is tested that assets.validate and header.validate are called.

And the failure of one of these functions (assets.validate and header.validate) is also not covered.

this is tested in the state machine https://github.com/LiskHQ/lisk-sdk/blob/bcfec00b6c918e635ef009249fcb6e7b563bd4c7/framework/test/unit/state_machine/state_machine.spec.ts

I see. But a test that command exectute is called seems to be missing. Could be added to this one? And do we test that these state machine functions are actually called during block processing? I see tests that check that they are called whenever a certain function in the abiHandler is called. But I don't see tests that these abiHandler functions are called during block processing.

eventRoot are tested in consensus.spec.ts. impliesMaxPrevotes test should be added in there as well

ok, but shouldn't they nevertheless be included in blockHeaderProps to be also covered, for instance, here?

State root verification is done in the lisk-db during the commit of the ABI. That's why they are not explicitly tested here

Ok. And do we have a test that checks that block processing fails if this lisk-db call fails?

this is handled in another PR https://github.com/LiskHQ/lisk-sdk/pull/8440/files#diff-e5b4a5e9bd2d527e3be245f514f69edce40293da78958774fbb052a8f6e32201R316-R332

ok

[shuse2]

I see. But a test that command exectute is called seems to be missing. Could be added to this one? And do we test that these state machine functions are actually called during block processing? I see tests that check that they are called whenever a certain function in the abiHandler is called. But I don't see tests that these abiHandler functions are called during block processing.

ill add the command execute check. the state machine is called from ABI Handler and they are checked in the test

[AndreasKendziorra]

Looks good. But was the following point skipped on purpose, or was it just missed?

Some more precise tests regarding MAX_TRANSACTIONS_SIZE_BYTES than the exiting one would be good. That means, one test where the size equals MAX_TRANSACTIONS_SIZE_BYTES and that passes. And one test where the size equals MAX_TRANSACTIONS_SIZE_BYTES+1 and that fails.

Note that the corresponding issue for assets was fixed, but not this one for transactions.

[shuse2]

@AndreasKendziorra isnt it addressed in https://github.com/LiskHQ/lisk-sdk/pull/8444/files#diff-fd4ebac2e0ca16a69d9204c350533e9c36767eab59c1c349b0db24ca6123d25dR22 ?

[AndreasKendziorra]

@AndreasKendziorra isnt it addressed in https://github.com/LiskHQ/lisk-sdk/pull/8444/files#diff-fd4ebac2e0ca16a69d9204c350533e9c36767eab59c1c349b0db24ca6123d25dR22 ?

No, your linked tests are about the transaction params size. MAX_TRANSACTIONS_SIZE_BYTES is about the size of all transactions in a block.
My statement "the corresponding issue for assets was fixed, but not this one for transactions" was actually wrong. Sorry for this.