Adds Validator Set Replication property check to core diff model #589
Conversation
danwt
requested review from
jtremback,
mpoke,
sainoe and
shaspitz
as code owners
danwt
force-pushed
the
danwt/diff-test-core-validator-set-replication
branch
from
6d47f23 to
4445454
Compare
danwt
changed the title
| @@ -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 | |||
There was a problem hiding this comment.
Could you give more context in this comment?
Maybe explain why a "phantom" vscId is needed?
There was a problem hiding this comment.
Good point! Please see change.
| // make sense to try to check the property for height 0. | ||
| return | ||
| } | ||
| const statusP = blocks[P].get(hP - 1)!.invariantSnapshot.status; |
There was a problem hiding this comment.
Maybe refactor and explain why 1 is subtracted from provider height?
// Note: explain why subtraction happens
const hP = ss.vscIDtoH[vscid] - 1;There was a problem hiding this comment.
Good point! Please see change.
| // 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good point! I've changed it.
|
Great stuff! I can take a better look later, but just checked throttle doesn't break this prop https://github.com/cosmos/interchain-security/tree/circuit-breaker-w-test |
| // 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 |
There was a problem hiding this comment.
Explain better the logic here. You can refer to the spec on how ss.hToVscID and ss.vscIDtoH are set.
There was a problem hiding this comment.
The function is pretty extensively documented in the latest commit with link to the spec
I think I had not pushed the last commits when you reviewed
|
I accidentally deleted a comment in vscode. My bad! |
Co-authored-by: Simon Noetzlin <[email protected]>
| // the consumer valset valsetC committed at hC-1 | ||
| const vscid = snapshotC.hToVscID[hC]; | ||
| // The VSU packet was sent at height hP-1 | ||
| const hP = snapshotC.vscIDtoH[vscid]; |
There was a problem hiding this comment.
Im probably misunderstanding the model but can you explain how you get a provider height from a consumer block history (snapshotC)?
There was a problem hiding this comment.
Hey, the snapshotC is a snapshot of a committed block state on the consumer. Maybe it should be renamed. I've noted this down so when do a larger refactor I'll try to make this more clear.
Description
Adds the validator set replicatoin property as a checkable property to the diff test core model, and checks it.
Linked issues
Closes: #320
Type of change
Please delete options that are not relevant.
How was the feature tested?
Checklist:
Please delete options that are not relevant.
make test)