From cb3c6e2837e6a212df84dc032ae3b186c2c8b209 Mon Sep 17 00:00:00 2001 From: Ishan Date: Thu, 31 Aug 2023 16:24:58 +0200 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20=20Use=20getBFTParametersA?= =?UTF-8?q?ctiveValidators=20for=20certificate=20generation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ♻️ Update based on LIP0053 updates 🐛 Filter out bftWeight===0 when saving validatorsPreimage 🐛 Filter out bftWeight===0 when computing validatorsHash 🐛 Filter out bftWeight===0 when selecting and validating aggregateCommit --- .../src/chain_connector_plugin.ts | 6 +- framework/src/engine/bft/method.ts | 13 ++++ framework/src/engine/bft/utils.ts | 3 +- .../certificate_generation/commit_pool.ts | 63 +++++++++++++---- framework/src/engine/endpoint/consensus.ts | 28 ++++++++ .../base_interoperability_internal_methods.ts | 24 ++++--- .../commit_pool.spec.ts | 68 +++++++++++++------ 7 files changed, 160 insertions(+), 45 deletions(-) diff --git a/framework-plugins/lisk-framework-chain-connector-plugin/src/chain_connector_plugin.ts b/framework-plugins/lisk-framework-chain-connector-plugin/src/chain_connector_plugin.ts index 3749904cf7e..feeff20b6f2 100644 --- a/framework-plugins/lisk-framework-chain-connector-plugin/src/chain_connector_plugin.ts +++ b/framework-plugins/lisk-framework-chain-connector-plugin/src/chain_connector_plugin.ts @@ -566,7 +566,7 @@ export class ChainConnectorPlugin extends BasePlugin // Get validatorsData at new block header height const bftParametersJSON = await this._sendingChainClient.invoke( - 'consensus_getBFTParameters', + 'consensus_getBFTParametersActiveValidators', { height: newBlockHeader.height }, ); @@ -576,9 +576,11 @@ export class ChainConnectorPlugin extends BasePlugin ); // Save validatorsData if there is a new validatorsHash if (validatorsDataIndex === -1) { + // Filter out standby validators with bftWeight > BigInt(0) + const activeValidators = bftParametersObj.validators.filter(v => v.bftWeight > BigInt(0)); validatorsHashPreimage.push({ certificateThreshold: bftParametersObj.certificateThreshold, - validators: bftParametersObj.validators, + validators: activeValidators, validatorsHash: bftParametersObj.validatorsHash, }); } diff --git a/framework/src/engine/bft/method.ts b/framework/src/engine/bft/method.ts index da6feb17930..228879e5a04 100644 --- a/framework/src/engine/bft/method.ts +++ b/framework/src/engine/bft/method.ts @@ -89,6 +89,19 @@ export class BFTMethod { return getBFTParameters(paramsStore, height); } + public async getBFTParametersActiveValidators( + stateStore: StateStore, + height: number, + ): Promise { + const bftParams = await this.getBFTParameters(stateStore, height); + + return { + ...bftParams, + // Filter standby validators with bftWeight === 0 + validators: bftParams.validators.filter(v => v.bftWeight > BigInt(0)), + }; + } + public async getBFTHeights(stateStore: StateStore): Promise { const votesStore = stateStore.getStore(MODULE_STORE_PREFIX_BFT, STORE_PREFIX_BFT_VOTES); const bftVotes = await votesStore.getWithSchema(EMPTY_KEY, bftVotesSchema); diff --git a/framework/src/engine/bft/utils.ts b/framework/src/engine/bft/utils.ts index d1e8cb30462..f73a68fc565 100644 --- a/framework/src/engine/bft/utils.ts +++ b/framework/src/engine/bft/utils.ts @@ -88,7 +88,8 @@ export const computeValidatorsHash = (validators: Validator[], certificateThresh } sortValidatorsByBLSKey(activeValidators); const input: ValidatorsHashInput = { - activeValidators, + // Exclude standby validators with bftWeight === 0 + activeValidators: activeValidators.filter(v => v.bftWeight > BigInt(0)), certificateThreshold, }; const encodedValidatorsHashInput = codec.encode(validatorsHashInputSchema, input); diff --git a/framework/src/engine/consensus/certificate_generation/commit_pool.ts b/framework/src/engine/consensus/certificate_generation/commit_pool.ts index f2f0e0e41bd..f55ecfe6fe7 100644 --- a/framework/src/engine/consensus/certificate_generation/commit_pool.ts +++ b/framework/src/engine/consensus/certificate_generation/commit_pool.ts @@ -13,7 +13,7 @@ */ import { BlockHeader, Chain, StateStore } from '@liskhq/lisk-chain'; -import { dataStructures } from '@liskhq/lisk-utils'; +import { dataStructures, objects } from '@liskhq/lisk-utils'; import { bls } from '@liskhq/lisk-cryptography'; import { Database } from '@liskhq/lisk-db'; import { codec } from '@liskhq/lisk-codec'; @@ -132,7 +132,10 @@ export class CommitPool { } // Validation Step 5 - const { validators } = await this._bftMethod.getBFTParameters(methodContext, commit.height); + const { validators } = await this._bftMethod.getBFTParametersActiveValidators( + methodContext, + commit.height, + ); const validator = validators.find(v => v.address.equals(commit.validatorAddress)); if (!validator) { throw new Error('Commit validator was not active for its height.'); @@ -238,13 +241,13 @@ export class CommitPool { aggregationBits: aggregateCommit.aggregationBits, signature: aggregateCommit.certificateSignature, }; - const { validators, certificateThreshold } = await this._bftMethod.getBFTParameters( - stateStore, - aggregateCommit.height, - ); + const { validators: activeValidators, certificateThreshold } = + await this._bftMethod.getBFTParametersActiveValidators(stateStore, aggregateCommit.height); + // Filter out all the standby validators with bftWeight === 0 + const activeValidators = validators.filter(v => v.bftWeight > BigInt(0)); return verifyAggregateCertificateSignature( - validators, + activeValidators, certificateThreshold, this._chain.chainID, certificate, @@ -266,11 +269,18 @@ export class CommitPool { const { height } = singleCommits[0]; // assuming this list of validators includes all validators corresponding to each singleCommit.validatorAddress - const { validators } = await this._bftMethod.getBFTParameters(methodContext, height); + const { validators } = await this._bftMethod.getBFTParametersActiveValidators( + methodContext, + height, + ); const addressToBlsKey: dataStructures.BufferMap = new dataStructures.BufferMap(); const validatorKeys: Buffer[] = []; - for (const validator of validators) { + // Filter out all the standby validators with bftWeight === 0 + const addressesWithBFTWeightZero = validators + .filter(v => v.bftWeight === BigInt(0)) + .map(v => v.address); + for (const validator of validators.filter(v => v.bftWeight > BigInt(0))) { addressToBlsKey.set(validator.address, validator.blsKey); validatorKeys.push(validator.blsKey); } @@ -278,6 +288,10 @@ export class CommitPool { const pubKeySignaturePairs: PkSigPair[] = []; for (const commit of singleCommits) { + // Skip any standby validator + if (objects.bufferArrayIncludes(addressesWithBFTWeightZero, commit.validatorAddress)) { + continue; + } const publicKey = addressToBlsKey.get(commit.validatorAddress); if (!publicKey) { throw new Error( @@ -330,13 +344,29 @@ export class CommitPool { ...this._nonGossipedCommitsLocal.getByHeight(nextHeight), ...this._gossipedCommits.getByHeight(nextHeight), ]; - const nextValidators = singleCommits.map(commit => commit.validatorAddress); let aggregateBFTWeight = BigInt(0); // Assume BFT parameters exist for next height const { validators: bftParamValidators, certificateThreshold } = - await this._bftMethod.getBFTParameters(methodContext, nextHeight); + await this._bftMethod.getBFTParametersActiveValidators(methodContext, nextHeight); + + const activeValidatorAddresses = bftParamValidators.map(v => v.address); + // Filter out any single commits from standby delegates + const singleCommitsByActiveValidators = singleCommits.filter(commit => + objects.bufferArrayIncludes(activeValidatorAddresses, commit.validatorAddress), + ); + const nextValidators = singleCommitsByActiveValidators.map(commit => commit.validatorAddress); + const filteredSingleCommits = singleCommits.filter(commit => { + const foundValidator = bftParamValidators.find(v => + v.address.equals(commit.validatorAddress), + ); + if (foundValidator && foundValidator.bftWeight === BigInt(0)) { + return false; + } + return true; + }); + const nextValidators = filteredSingleCommits.map(commit => commit.validatorAddress); for (const matchingAddress of nextValidators) { const bftParamsValidatorInfo = bftParamValidators.find(bftParamValidator => bftParamValidator.address.equals(matchingAddress), @@ -344,12 +374,16 @@ export class CommitPool { if (!bftParamsValidatorInfo) { throw new Error('Validator address not found in commit pool'); } + // Skip validators with BFT Weight zero when someone is running node with standby validators + if (bftParamsValidatorInfo.bftWeight === BigInt(0)) { + continue; + } aggregateBFTWeight += bftParamsValidatorInfo.bftWeight; } if (aggregateBFTWeight >= certificateThreshold) { - return this.aggregateSingleCommits(methodContext, singleCommits); + return this.aggregateSingleCommits(methodContext, singleCommitsByActiveValidators); } nextHeight -= 1; @@ -408,7 +442,10 @@ export class CommitPool { // 2. Select commits to gossip const nextHeight = this._chain.lastBlock.header.height + 1; - const { validators } = await this._bftMethod.getBFTParameters(methodContext, nextHeight); + const { validators } = await this._bftMethod.getBFTParametersActiveValidators( + methodContext, + nextHeight, + ); const maxSelectedCommitsLength = 2 * validators.length; // Get a list of commits sorted by ascending order of height diff --git a/framework/src/engine/endpoint/consensus.ts b/framework/src/engine/endpoint/consensus.ts index 97c522ca92a..dc70e36cafd 100644 --- a/framework/src/engine/endpoint/consensus.ts +++ b/framework/src/engine/endpoint/consensus.ts @@ -73,6 +73,34 @@ export class ConsensusEndpoint { }; } + public async getBFTParametersActiveValidators(ctx: RequestContext): Promise { + const stateStore = new StateStore(this._blockchainDB); + const { + certificateThreshold, + precommitThreshold, + prevoteThreshold, + validators, + validatorsHash, + } = await this._bftMethod.getBFTParametersActiveValidators( + stateStore, + ctx.params.height as number, + ); + + const validatorsJSON = validators.map(v => ({ + address: address.getLisk32AddressFromAddress(v.address), + bftWeight: v.bftWeight.toString(), + blsKey: v.blsKey.toString('hex'), + })); + + return { + validators: validatorsJSON, + certificateThreshold: certificateThreshold.toString(), + precommitThreshold: precommitThreshold.toString(), + prevoteThreshold: prevoteThreshold.toString(), + validatorsHash: validatorsHash.toString('hex'), + }; + } + public async getBFTHeights(_ctx: RequestContext): Promise { const stateStore = new StateStore(this._blockchainDB); const result = await this._bftMethod.getBFTHeights(stateStore); diff --git a/framework/src/modules/interoperability/base_interoperability_internal_methods.ts b/framework/src/modules/interoperability/base_interoperability_internal_methods.ts index 1a4ca0e8a91..a5cdb9fca63 100644 --- a/framework/src/modules/interoperability/base_interoperability_internal_methods.ts +++ b/framework/src/modules/interoperability/base_interoperability_internal_methods.ts @@ -335,17 +335,25 @@ export abstract class BaseInteroperabilityInternalMethod extends BaseInternalMet 'The number of 1s in the bitmap is not equal to the number of new BFT weights.', ); } + + let bftWeightIndex = 0; for (let i = 0; i < allBLSKeys.length; i += 1) { - // existing key does not need to be checked - if (!objects.bufferArrayIncludes(blsKeysUpdate, allBLSKeys[i])) { - continue; - } + const blsKey = allBLSKeys[i]; + // Get digit of bitmap at index idx (starting from the right). const digit = (bftWeightsUpdateBitmapBin >> BigInt(i)) & BigInt(1); - if (digit !== BigInt(1)) { - throw new Error('New validators must have a BFT weight update.'); + if (objects.bufferArrayIncludes(blsKeysUpdate, blsKey)) { + // New validators must have a BFT weight update. + if (digit !== BigInt(1)) { + throw new Error('New validators must have a BFT weight update.'); + } + // New validators must have a positive BFT weight. + // The BFT weight of current validator is specified by bftWeightsUpdate[bftWeightIndex]. + if (bftWeightsUpdate[bftWeightIndex] === BigInt(0)) { + throw new Error('New validators must have a positive BFT weight.'); + } } - if (bftWeightsUpdate[i] === BigInt(0)) { - throw new Error('New validators must have a positive BFT weight.'); + if (digit === BigInt(1)) { + bftWeightIndex += 1; } } diff --git a/framework/test/unit/engine/consensus/certificate_generation/commit_pool.spec.ts b/framework/test/unit/engine/consensus/certificate_generation/commit_pool.spec.ts index 2662cdf39d3..5ebb891a7f2 100644 --- a/framework/test/unit/engine/consensus/certificate_generation/commit_pool.spec.ts +++ b/framework/test/unit/engine/consensus/certificate_generation/commit_pool.spec.ts @@ -66,6 +66,7 @@ describe('CommitPool', () => { bftMethod = { getValidator: jest.fn(), getBFTHeights: jest.fn(), + getBFTParametersActiveValidators: jest.fn(), getBFTParameters: jest.fn(), getNextHeightBFTParameters: jest.fn(), selectAggregateCommit: jest.fn(), @@ -170,7 +171,7 @@ describe('CommitPool', () => { commitPool['_bftMethod'].getBFTHeights = jest .fn() .mockResolvedValue({ maxHeightPrecommitted }); - commitPool['_bftMethod'].getBFTParameters = jest.fn().mockResolvedValue({ + commitPool['_bftMethod'].getBFTParametersActiveValidators = jest.fn().mockResolvedValue({ validators: Array.from({ length: numActiveValidators }, () => utils.getRandomBytes(32)), }); }); @@ -525,10 +526,12 @@ describe('CommitPool', () => { maxHeightPrecommitted, }); - when(bftMethod.getBFTParameters).calledWith(stateStore, commit.height).mockReturnValue({ - certificateThreshold: threshold, - validators, - }); + when(bftMethod.getBFTParametersActiveValidators) + .calledWith(stateStore, commit.height) + .mockReturnValue({ + certificateThreshold: threshold, + validators, + }); when(bftMethod.getValidator) .calledWith(stateStore, commit.validatorAddress, commit.height) @@ -686,7 +689,7 @@ describe('CommitPool', () => { }); it('should throw error when bls key of the validator is not matching with the certificate signature', async () => { - when(bftMethod.getBFTParameters) + when(bftMethod.getBFTParametersActiveValidators) .calledWith(stateStore, commit.height) .mockReturnValue({ validators: [{ address: commit.validatorAddress, blsKey: utils.getRandomBytes(48) }], @@ -892,10 +895,12 @@ describe('CommitPool', () => { maxHeightPrecommitted, }); - when(bftMethod.getBFTParameters).calledWith(stateStore, height).mockReturnValue({ - certificateThreshold: threshold, - validators, - }); + when(bftMethod.getBFTParametersActiveValidators) + .calledWith(stateStore, height) + .mockReturnValue({ + certificateThreshold: threshold, + validators, + }); when(bftMethod.getNextHeightBFTParameters) .calledWith(stateStore, maxHeightCertified + 1) @@ -1206,7 +1211,7 @@ describe('CommitPool', () => { aggregationBits: aggregationBits1, certificateSignature: aggregateSignature1, }; - bftMethod.getBFTParameters.mockReturnValue({ + bftMethod.getBFTParametersActiveValidators.mockReturnValue({ validators: [{ address: validatorInfo1.address, blsKey: validatorInfo1.blsPublicKey }], }); @@ -1217,7 +1222,7 @@ describe('CommitPool', () => { it('should return aggregated commit for multiple single commits', async () => { expectedCommit = { height, aggregationBits, certificateSignature: aggregateSignature }; - bftMethod.getBFTParameters.mockReturnValue({ + bftMethod.getBFTParametersActiveValidators.mockReturnValue({ validators: [ { address: validatorInfo1.address, blsKey: validatorInfo1.blsPublicKey }, { address: validatorInfo2.address, blsKey: validatorInfo2.blsPublicKey }, @@ -1231,8 +1236,7 @@ describe('CommitPool', () => { }); it('should throw if no bls public key is found for the validator', async () => { - expectedCommit = { height, aggregationBits, certificateSignature: aggregateSignature }; - bftMethod.getBFTParameters.mockReturnValue({ + bftMethod.getBFTParametersActiveValidators.mockReturnValue({ validators: [ { address: validatorInfo1.address, blsKey: validatorInfo1.blsPublicKey }, { address: validatorInfo2.address, blsKey: validatorInfo2.blsPublicKey }, @@ -1248,7 +1252,7 @@ describe('CommitPool', () => { it('should call validator keys in lexicographical order', async () => { const spy = jest.spyOn(crypto.bls, 'createAggSig'); - bftMethod.getBFTParameters.mockReturnValue({ + bftMethod.getBFTParametersActiveValidators.mockReturnValue({ validators: [ { address: validatorInfo1.address, blsKey: validatorInfo1.blsPublicKey }, { address: validatorInfo2.address, blsKey: validatorInfo2.blsPublicKey }, @@ -1324,7 +1328,7 @@ describe('CommitPool', () => { bftMethod.getNextHeightBFTParameters.mockResolvedValue(heightNextBFTParameters); - bftMethod.getBFTParameters.mockResolvedValue({ + bftMethod.getBFTParametersActiveValidators.mockResolvedValue({ certificateThreshold: threshold, validators: [ { address: validatorInfo1.address, bftWeight: BigInt(1) }, @@ -1352,18 +1356,18 @@ describe('CommitPool', () => { ); }); - it('should call bft method getBFTParameters with min(heightNextBFTParameters - 1, maxHeightPrecommitted)', async () => { + it('should call bft method getBFTParametersActiveValidators with min(heightNextBFTParameters - 1, maxHeightPrecommitted)', async () => { // Act await commitPool['_selectAggregateCommit'](stateStore); // Assert - expect(commitPool['_bftMethod'].getBFTParameters).toHaveBeenCalledWith( + expect(commitPool['_bftMethod'].getBFTParametersActiveValidators).toHaveBeenCalledWith( stateStore, Math.min(heightNextBFTParameters - 1, maxHeightPrecommitted), ); }); - it('should call getBFTParameters with maxHeightPrecommitted if getNextHeightBFTParameters does not return a valid height', async () => { + it('should call getBFTParametersActiveValidators with maxHeightPrecommitted if getNextHeightBFTParameters does not return a valid height', async () => { // Arrange bftMethod.getNextHeightBFTParameters.mockRejectedValue( new BFTParameterNotFoundError('Error'), @@ -1373,7 +1377,7 @@ describe('CommitPool', () => { await commitPool['_selectAggregateCommit'](stateStore); // Assert - expect(commitPool['_bftMethod'].getBFTParameters).toHaveBeenCalledWith( + expect(commitPool['_bftMethod'].getBFTParametersActiveValidators).toHaveBeenCalledWith( stateStore, maxHeightPrecommitted, ); @@ -1389,9 +1393,31 @@ describe('CommitPool', () => { ]); }); + it('should call aggregateSingleCommits with single commits only from active validators when it reaches threshold', async () => { + // Act + const singleCommitByStandbyValidator = { + blockID: blockHeader2.id, + height: blockHeader2.height, + validatorAddress: utils.getRandomBytes(20), + certificateSignature: signCertificate( + utils.getRandomBytes(32), + chainID, + unsignedCertificate2, + ), + }; + commitPool['_nonGossipedCommitsLocal'].add(singleCommitByStandbyValidator); + + await commitPool['_selectAggregateCommit'](stateStore); + + // Assert + expect(commitPool['aggregateSingleCommits']).toHaveBeenCalledWith(stateStore, [ + singleCommit2, + ]); + }); + it('should not call aggregateSingleCommits when it does not reach threshold and return default aggregateCommit', async () => { // Arrange - bftMethod.getBFTParameters.mockReturnValue({ + bftMethod.getBFTParametersActiveValidators.mockReturnValue({ certificateThreshold: 10, validators: [ { address: validatorInfo1.address, bftWeight: BigInt(1) },