Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Validator Set Replication property check to core diff model #589

Merged
merged 16 commits into from
Jan 5, 2023
Merged
2 changes: 2 additions & 0 deletions tests/difference/core/model/src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ type InvariantSnapshot = {
undelegationQ: Undelegation[];
delegatorTokens: number;
consumerPower: (number | undefined)[];
vscIDtoH: Record<number, number>;
hToVscID: Record<number, number>;
};

/**
Expand Down
12 changes: 10 additions & 2 deletions tests/difference/core/model/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ const MODEL_INIT_STATE: ModelInitState = {
h: { provider: 0, consumer: 0 },
t: { provider: 0, consumer: 0 },
ccvC: {
hToVscID: { 0: 0, 1: 0 },
// The initial provider height is 0 but there must have been
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give more context in this comment?

Maybe explain why a "phantom" vscId is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Please see change.

// a vscid created at the previous height. However we do not
// use model this. Instead we use -1 to represent the phantom
// vscid.
hToVscID: { 0: -1, 1: -1 },
pendingChanges: [],
maturingVscs: new Map(),
outstandingDowntime: [false, false, false, false],
Expand All @@ -51,7 +55,11 @@ const MODEL_INIT_STATE: ModelInitState = {
ccvP: {
initialHeight: 0,
vscID: 0,
vscIDtoH: {},
// The initial provider height is 0 but there must have been
// a vscid created at the previous height. However we do not
// use model this. Instead we use -1 to represent the phantom
// vscid.
vscIDtoH: { [-1]: 0 },
vscIDtoOpIDs: new Map(),
downtimeSlashAcks: [],
tombstoned: [false, false, false, false],
Expand Down
5 changes: 5 additions & 0 deletions tests/difference/core/model/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
BlockHistory,
stakingWithoutSlashing,
bondBasedConsumerVotingPower,
validatorSetReplication,
} from './properties.js';
import { Model } from './model.js';
import {
Expand Down Expand Up @@ -322,6 +323,10 @@ function gen(seconds: number, checkProperties: boolean) {
if (checkProperties) {
// Checking properties is flagged because it is computationally
// expensive.
if (!validatorSetReplication(hist)) {
dumpTrace(`${DIR}trace_${i}.json`, actions, events);
throw 'validatorSetReplication property failure, trace written.';
}
if (!bondBasedConsumerVotingPower(hist)) {
dumpTrace(`${DIR}trace_${i}.json`, actions, events);
throw 'bondBasedConsumerVotingPower property failure, trace written.';
Expand Down
2 changes: 2 additions & 0 deletions tests/difference/core/model/src/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,8 @@ class Model {
undelegationQ: this.staking.undelegationQ,
delegatorTokens: this.staking.delegatorTokens,
consumerPower: this.ccvC.consumerPower,
vscIDtoH: this.ccvP.vscIDtoH,
hToVscID: this.ccvC.hToVscID
});
};

Expand Down
60 changes: 60 additions & 0 deletions tests/difference/core/model/src/properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,65 @@ function stakingWithoutSlashing(hist: BlockHistory): boolean {
return true;
}

/**
* Checks the validator set replication property as defined
* https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/system_model_and_properties.md#system-properties
*
* @param hist A history of blocks.
* @returns Is the property satisfied?
*/
function validatorSetReplication(hist: BlockHistory): boolean {
// return true;
const blocks = hist.blocks;
let good = true;
blocks[C].forEach((b: CommittedBlock, hC: number) => {
if (hC < 1) {
mpoke marked this conversation as resolved.
Show resolved Hide resolved
// The model starts at consumer height 0, so there is
// no committed block at height - 1. This means it does
// not make sense to try to check the property for height 0.
return
}
const ss = b.invariantSnapshot;
// Get the vscid of the last update which dictated
// the consumer valset valsetC committed at hC-1
const vscid = ss.hToVscID[hC];
// The VSU packet was sent at height hP-1
const hP = ss.vscIDtoH[vscid];
// Compare the validator sets at hC-1 and hP-1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain better the logic here. You can refer to the spec on how ss.hToVscID and ss.vscIDtoH are set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is pretty extensively documented in the latest commit with link to the spec

https://github.com/cosmos/interchain-security/blob/danwt/diff-test-core-validator-set-replication/tests/difference/core/model/src/properties.ts#L185-L192

I think I had not pushed the last commits when you reviewed

const valsetC = blocks[C].get(hC - 1)!.invariantSnapshot.consumerPower;
// The provider set is implicitly defined by the status and tokens (power)
if (hP < 1) {
// The model starts at provider height 0, so there is
// no committed block at height - 1. This means it does not
// make sense to try to check the property for height 0.
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I find it less error prone to employ for (const element of array1) pattern instead of object.forEach if you have cases that would skip over an array element if some condition is met.

.forEach is only cleaner when you always process each element in the same way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I've changed it.

}
const statusP = blocks[P].get(hP - 1)!.invariantSnapshot.status;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe refactor and explain why 1 is subtracted from provider height?

// Note: explain why subtraction happens
const hP = ss.vscIDtoH[vscid] - 1;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Please see change.

const tokensP = blocks[P].get(hP - 1)!.invariantSnapshot.tokens;
assert(valsetC.length === statusP.length, 'this should never happen.');
assert(valsetC.length === tokensP.length, 'this should never happen.');
valsetC.forEach((power, i) => {
if (power !== undefined) { // undefined means the validator is not in the set
// Check that the consumer power is strictly equal to the provider power
good = good && (tokensP[i] === power);
}
})
statusP.forEach((status, i) => {
if (status === Status.BONDED) {
const power = tokensP[i];
// Check that the consumer power is strictly equal to the provider power
good = good && (valsetC[i] === power);
}
else {
// Ensure that the consumer validator set does not contain a non-bonded validator
good = good && (valsetC[i] === undefined);
}
})

})
return good;
}

/**
* Checks the bond-based consumer voting power property as defined
* in https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/system_model_and_properties.md#system-properties
Expand Down Expand Up @@ -277,4 +336,5 @@ export {
BlockHistory,
stakingWithoutSlashing,
bondBasedConsumerVotingPower,
validatorSetReplication,
};