Skip to content
This repository was archived by the owner on Jun 11, 2024. It is now read-only.

Commit

Permalink
♻️ Use getBFTParametersActiveValidators for certificate generation
Browse files Browse the repository at this point in the history 
♻️ 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
  • Loading branch information
@ishantiw
ishantiw committed Aug 31, 2023
1 parent bde252d commit c6074fb
Show file tree
Hide file tree
Showing 7 changed files with 138 additions and 44 deletions.
5 changes: 3 additions & 2 deletions framework-plugins/lisk-framework-chain-connector-plugin/src/chain_connector_plugin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ export class ChainConnectorPlugin extends BasePlugin<ChainConnectorPluginConfig>

// Get validatorsData at new block header height
const bftParametersJSON = await this._sendingChainClient.invoke<BFTParametersJSON>(
'consensus_getBFTParameters',
'consensus_getBFTParametersActiveValidators',
{ height: newBlockHeader.height },
);

Expand All @@ -576,9 +576,10 @@ export class ChainConnectorPlugin extends BasePlugin<ChainConnectorPluginConfig>
);
// Save validatorsData if there is a new validatorsHash
if (validatorsDataIndex === -1) {
const activeValidators = bftParametersObj.validators;
validatorsHashPreimage.push({
certificateThreshold: bftParametersObj.certificateThreshold,
validators: bftParametersObj.validators,
validators: activeValidators,
validatorsHash: bftParametersObj.validatorsHash,
});
}
Expand Down
13 changes: 13 additions & 0 deletions framework/src/engine/bft/method.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,19 @@ export class BFTMethod {
return getBFTParameters(paramsStore, height);
}

public async getBFTParametersActiveValidators(
stateStore: StateStore,
height: number,
): Promise<BFTParameters> {
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<BFTHeights> {
const votesStore = stateStore.getStore(MODULE_STORE_PREFIX_BFT, STORE_PREFIX_BFT_VOTES);
const bftVotes = await votesStore.getWithSchema<BFTVotes>(EMPTY_KEY, bftVotesSchema);
Expand Down
3 changes: 2 additions & 1 deletion framework/src/engine/bft/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
41 changes: 29 additions & 12 deletions framework/src/engine/consensus/certificate_generation/commit_pool.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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.');
Expand Down Expand Up @@ -238,13 +241,11 @@ 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);

return verifyAggregateCertificateSignature(
validators,
activeValidators,
certificateThreshold,
this._chain.chainID,
certificate,
Expand All @@ -266,7 +267,10 @@ 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<Buffer> = new dataStructures.BufferMap();
const validatorKeys: Buffer[] = [];

Expand Down Expand Up @@ -330,12 +334,18 @@ 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);

for (const matchingAddress of nextValidators) {
const bftParamsValidatorInfo = bftParamValidators.find(bftParamValidator =>
Expand All @@ -344,12 +354,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;
Expand Down Expand Up @@ -408,7 +422,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
Expand Down
28 changes: 28 additions & 0 deletions framework/src/engine/endpoint/consensus.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,34 @@ export class ConsensusEndpoint {
};
}

public async getBFTParametersActiveValidators(ctx: RequestContext): Promise<BFTParametersJSON> {
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<BFTHeights> {
const stateStore = new StateStore(this._blockchainDB);
const result = await this._bftMethod.getBFTHeights(stateStore);
Expand Down
24 changes: 16 additions & 8 deletions framework/src/modules/interoperability/base_interoperability_internal_methods.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
68 changes: 47 additions & 21 deletions framework/test/unit/engine/consensus/certificate_generation/commit_pool.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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)),
});
});
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) }],
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -1206,7 +1211,7 @@ describe('CommitPool', () => {
aggregationBits: aggregationBits1,
certificateSignature: aggregateSignature1,
};
bftMethod.getBFTParameters.mockReturnValue({
bftMethod.getBFTParametersActiveValidators.mockReturnValue({
validators: [{ address: validatorInfo1.address, blsKey: validatorInfo1.blsPublicKey }],
});

Expand All @@ -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 },
Expand All @@ -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 },
Expand All @@ -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 },
Expand Down Expand Up @@ -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) },
Expand Down Expand Up @@ -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'),
Expand All @@ -1373,7 +1377,7 @@ describe('CommitPool', () => {
await commitPool['_selectAggregateCommit'](stateStore);

// Assert
expect(commitPool['_bftMethod'].getBFTParameters).toHaveBeenCalledWith(
expect(commitPool['_bftMethod'].getBFTParametersActiveValidators).toHaveBeenCalledWith(
stateStore,
maxHeightPrecommitted,
);
Expand All @@ -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) },
Expand Down

0 comments on commit c6074fb

Please sign in to comment.