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.

Sign up for GitHub

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

Spec - New ValidatorRegistration/ValidatorExit duties stop old ones #317

Closed
wants to merge 28 commits into from
Closed

Spec - New ValidatorRegistration/ValidatorExit duties stop old ones #317

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
29d4a62
take care of height equals 0 case
GalRogozinski Oct 22, 2023
59733f5
new duty first height tests
GalRogozinski Oct 22, 2023
15e21a2
fix broadcast messages
GalRogozinski Oct 23, 2023
937289d
change should process duty
GalRogozinski Oct 31, 2023
2287a99
base start new duty
GalRogozinski Oct 31, 2023
92b6962
delete future messages test
GalRogozinski Oct 31, 2023
2b013b4
old duty slot
GalRogozinski Oct 31, 2023
f14bfd0
run test
GalRogozinski Oct 31, 2023
5912576
duplicate duty fix tests
GalRogozinski Oct 31, 2023
fe71e3b
Revert "run test"
GalRogozinski Oct 31, 2023
646fef0
duplicate duty fix tests
GalRogozinski Oct 31, 2023
d282c0d
add missing SCs
GalRogozinski Oct 31, 2023
d2cfd6e
finished.go
GalRogozinski Oct 31, 2023
5b9a6e2
ssv msgs
GalRogozinski Oct 31, 2023
2ea86c7
TestingValidatorRegistrationBySlot
GalRogozinski Nov 1, 2023
442da4e
VoluntaryExitBySlot
GalRogozinski Nov 1, 2023
1993e1d
remove TestingEpoch2
GalRogozinski Nov 1, 2023
1ff2739
duty finished jsons
GalRogozinski Nov 1, 2023
c8dc152
consensus_not_started.go
GalRogozinski Nov 1, 2023
6d4e6c4
generate jsons
GalRogozinski Nov 1, 2023
2e974c2
Merge branch 'main' into fix/non-qbft-runners
GalRogozinski Nov 1, 2023
bfa1d32
fix comment
GalRogozinski Nov 1, 2023
07c7173
fix old duty
GalRogozinski Nov 1, 2023
1b6c21f
fix old duty
GalRogozinski Nov 1, 2023
2b400c6
fix: re-generate JSONs (#1)
moshe-blox Nov 5, 2023
355de01
Fallback to QBFT controller if needed
GalRogozinski Nov 5, 2023
15579cf
Revert "Fallback to QBFT controller if needed"
GalRogozinski Nov 6, 2023
4273b12
"delete run one"
GalRogozinski Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ssv/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,10 @@ func (b *BaseRunner) hasRunningDuty() bool {
}

func (b *BaseRunner) ShouldProcessDuty(duty *types.Duty) error {
if b.QBFTController.Height >= qbft.Height(duty.Slot) && b.QBFTController.Height != 0 {
// assume StartingDuty is not nil if state is not nil
if b.State != nil && b.State.StartingDuty.Slot >= duty.Slot {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just add b.QBFTController != nill check?
Why change it to the above which changes the whole logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While fixing the issue I thought about making things better.

  1. In general each validator duty runner can run only one duty at a time.
  2. There is no real reason to make rule (1) dependent on QBFT controller. If we do separation of concerns and processes it is unclear why the runner has to even have access to the internal state of QBFT controller.
  3. With my current change all duties (including the ones that aren't beacon duties) follow the exact same pattern. Because of the special exemptions we did to duties with no QBFT we ran into this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a fallback that uses QBFTController just to get this approved

Copy link
Contributor

Choose a reason for hiding this comment

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

From my perspective, indeed, the change makes module separation better and avoids the problem with the validator registration and exit duties

return errors.Errorf("duty for slot %d already passed. Current height is %d", duty.Slot,
b.QBFTController.Height)
b.State.StartingDuty.Slot)
}
return nil
}
3 changes: 1 addition & 2 deletions ssv/spectest/all_tests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ var AllTests = []tests.TestF{
newduty.PostDecided,
newduty.Finished,
newduty.Valid,
newduty.PostWrongDecided,
newduty.PostInvalidDecided,
newduty.PostFutureDecided,
newduty.OldSlotDuty,
newduty.DuplicateDutyFinished,
newduty.DuplicateDutyNotFinished,
newduty.FirstHeight,
Expand Down
2 changes: 1 addition & 1 deletion ...wduty_MultiStartNewRunnerDutySpecTest/duplicate_duty_finished/validator registration.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
"ValidatorCommitteeIndex": 0,
"ValidatorSyncCommitteeIndices": null
},
"Finished": false
"Finished": true
},
"Share": {
"OperatorID": 1,
Expand Down
2 changes: 1 addition & 1 deletion ...rison/newduty_MultiStartNewRunnerDutySpecTest/duplicate_duty_finished/voluntary exit.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
"ValidatorCommitteeIndex": 0,
"ValidatorSyncCommitteeIndices": null
},
"Finished": false
"Finished": true
},
"Share": {
"OperatorID": 1,
Expand Down
134 changes: 134 additions & 0 deletions ...ultiStartNewRunnerDutySpecTest/new_duty_consensus_not_started/validator registration.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
{
"BaseRunner": {
"State": {
"PreConsensusContainer": {
"Signatures": {},
"Quorum": 3
},
"PostConsensusContainer": {
"Signatures": {},
"Quorum": 3
},
"RunningInstance": null,
"DecidedValue": null,
"StartingDuty": {
"Type": 5,
"PubKey": [
142,
128,
6,
101,
81,
168,
27,
49,
130,
88,
112,
158,
218,
247,
221,
31,
99,
205,
104,
106,
14,
77,
184,
178,
155,
187,
122,
207,
230,
86,
8,
103,
122,
245,
165,
39,
217,
68,
142,
228,
120,
53,
72,
94,
2,
181,
11,
192
],
"Slot": 50,
"ValidatorIndex": 1,
"CommitteeIndex": 0,
"CommitteeLength": 0,
"CommitteesAtSlot": 0,
"ValidatorCommitteeIndex": 0,
"ValidatorSyncCommitteeIndices": null
},
"Finished": false
},
"Share": {
"OperatorID": 1,
"ValidatorPubKey": "joAGZVGoGzGCWHCe2vfdH2PNaGoOTbiym7t6z+ZWCGd69aUn2USO5Hg1SF4CtQvA",
"SharePubKey": "l9lKgR1kSTYFKp0tSs1kcYl0z2eNvv0mcyTI6fjnA0pKa32HeeJ6AZU4w8Qlw+Xn",
"Committee": [
{
"OperatorID": 1,
"PubKey": "l9lKgR1kSTYFKp0tSs1kcYl0z2eNvv0mcyTI6fjnA0pKa32HeeJ6AZU4w8Qlw+Xn"
},
{
"OperatorID": 2,
"PubKey": "przr4wl9dBcbQMcSoDHOsDcds9PEAs8s5pG5Eg87q3XU1W36DzdZFUSZm/GMU1Pt"
},
{
"OperatorID": 3,
"PubKey": "gJDgt2ZqRezF1O90GKyZ8J5sskQCn+pqCn/Mvp7gi8U53g36Zr5rq8hJPdmd0amN"
},
{
"OperatorID": 4,
"PubKey": "p8CidrcKXuM5XH1tJlXtYFKKolLU0h7KX8xSI+UMxCvRaLKAq3q1MXNU3d/PPfnk"
}
],
"Quorum": 3,
"PartialQuorum": 2,
"DomainType": [
0,
0,
3,
1
],
"FeeRecipientAddress": [
83,
89,
83,
181,
166,
4,
0,
116,
148,
140,
241,
133,
234,
167,
210,
171,
189,
102,
128,
143
],
"Graffiti": null
},
"QBFTController": null,
"BeaconNetwork": "prater",
"BeaconRoleType": 5
}
}
134 changes: 134 additions & 0 deletions ...ewduty_MultiStartNewRunnerDutySpecTest/new_duty_consensus_not_started/voluntary exit.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
{
"BaseRunner": {
"State": {
"PreConsensusContainer": {
"Signatures": {},
"Quorum": 3
},
"PostConsensusContainer": {
"Signatures": {},
"Quorum": 3
},
"RunningInstance": null,
"DecidedValue": null,
"StartingDuty": {
"Type": 6,
"PubKey": [
142,
128,
6,
101,
81,
168,
27,
49,
130,
88,
112,
158,
218,
247,
221,
31,
99,
205,
104,
106,
14,
77,
184,
178,
155,
187,
122,
207,
230,
86,
8,
103,
122,
245,
165,
39,
217,
68,
142,
228,
120,
53,
72,
94,
2,
181,
11,
192
],
"Slot": 50,
"ValidatorIndex": 1,
"CommitteeIndex": 0,
"CommitteeLength": 0,
"CommitteesAtSlot": 0,
"ValidatorCommitteeIndex": 0,
"ValidatorSyncCommitteeIndices": null
},
"Finished": false
},
"Share": {
"OperatorID": 1,
"ValidatorPubKey": "joAGZVGoGzGCWHCe2vfdH2PNaGoOTbiym7t6z+ZWCGd69aUn2USO5Hg1SF4CtQvA",
"SharePubKey": "l9lKgR1kSTYFKp0tSs1kcYl0z2eNvv0mcyTI6fjnA0pKa32HeeJ6AZU4w8Qlw+Xn",
"Committee": [
{
"OperatorID": 1,
"PubKey": "l9lKgR1kSTYFKp0tSs1kcYl0z2eNvv0mcyTI6fjnA0pKa32HeeJ6AZU4w8Qlw+Xn"
},
{
"OperatorID": 2,
"PubKey": "przr4wl9dBcbQMcSoDHOsDcds9PEAs8s5pG5Eg87q3XU1W36DzdZFUSZm/GMU1Pt"
},
{
"OperatorID": 3,
"PubKey": "gJDgt2ZqRezF1O90GKyZ8J5sskQCn+pqCn/Mvp7gi8U53g36Zr5rq8hJPdmd0amN"
},
{
"OperatorID": 4,
"PubKey": "p8CidrcKXuM5XH1tJlXtYFKKolLU0h7KX8xSI+UMxCvRaLKAq3q1MXNU3d/PPfnk"
}
],
"Quorum": 3,
"PartialQuorum": 2,
"DomainType": [
0,
0,
3,
1
],
"FeeRecipientAddress": [
83,
89,
83,
181,
166,
4,
0,
116,
148,
140,
241,
133,
234,
167,
210,
171,
189,
102,
128,
143
],
"Graffiti": null
},
"QBFTController": null,
"BeaconNetwork": "prater",
"BeaconRoleType": 6
}
}
Loading