Skip to content

Commit

Permalink
switch to enum
Browse files Browse the repository at this point in the history
  • Loading branch information
yossigi committed Sep 3, 2023
1 parent 291a48c commit f2fcc97
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 36 deletions.
20 changes: 12 additions & 8 deletions agreement/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -596,18 +596,22 @@ func (e payloadProcessedEvent) ComparableStr() string {
return fmt.Sprintf("%v: %.5v", e.t().String(), e.Proposal.BlockDigest.String())
}

type CredentialTrackingEffect uint8

Check failure on line 599 in agreement/events.go

View workflow job for this annotation

GitHub Actions / Lint Errors

[Lint Errors] agreement/events.go#L599

exported: exported type CredentialTrackingEffect should have comment or be unexported (revive)
Raw output
agreement/events.go:599:6: exported: exported type CredentialTrackingEffect should have comment or be unexported (revive)
type CredentialTrackingEffect uint8
     ^

const (
NoCredentialTrackingImpact CredentialTrackingEffect = iota

Check failure on line 602 in agreement/events.go

View workflow job for this annotation

GitHub Actions / Lint Errors

[Lint Errors] agreement/events.go#L602

exported: exported const NoCredentialTrackingImpact should have comment (or a comment on this block) or be unexported (revive)
Raw output
agreement/events.go:602:2: exported: exported const NoCredentialTrackingImpact should have comment (or a comment on this block) or be unexported (revive)
	NoCredentialTrackingImpact CredentialTrackingEffect = iota
	^
MayImpactCredentialTracking
NewBestCredential
)

type filteredEvent struct {
// {proposal,vote,bundle}{Filtered,Malformed}
T eventType

// StateUpdated indicates whether the filtered message caused any change in
// the state machine
StateUpdated bool

// ContinueProcessingVoteForCredentialTracking indicates whether its a vote
// present that we should continue processing only for tracking its
// credential arrival time.
ContinueProcessingVoteForCredentialTracking bool
// CredentialTrackingNote indicates the impact of the filtered event on the
// credential tracking machinary used for dynamically setting the filter
// timeout.
CredentialTrackingNote CredentialTrackingEffect

// Err is the reason cryptographic verification failed and is set for
// events {proposal,vote,bundle}Malformed.
Expand Down
14 changes: 9 additions & 5 deletions agreement/player.go
Original file line number Diff line number Diff line change
Expand Up @@ -620,17 +620,21 @@ func (p *player) handleMessageEvent(r routerHandle, e messageEvent) (actions []a
err := ef.(filteredEvent).Err
return append(actions, ignoreAction(e, err))
}
if ef.(filteredEvent).StateUpdated {
switch ef.(filteredEvent).CredentialTrackingNote {
case NewBestCredential:
// Dynamic filter timeout feature enabled, and current message
// updated the best credential arrival time
v := e.Input.Vote
return append(actions, relayAction(e, protocol.AgreementVoteTag, v.u()))
} else if !ef.(filteredEvent).ContinueProcessingVoteForCredentialTracking {
// Dynamic filter timeout feature enabled, and current message
// may update the best credential arrival time, so we should
// continue processing.
case NoCredentialTrackingImpact:
// Dynamic filter timeout feature enabled, but current message
// may not update the best credential arrival time, so we should
// ignore it.
err := ef.(filteredEvent).Err
return append(actions, ignoreAction(e, err))
// There is another case, where the message
// MayImpactCredentialTracking. This case does not return here,
// so we continue processing the message.
}
}

Expand Down
16 changes: 12 additions & 4 deletions agreement/proposalManager.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,11 @@ func (m *proposalManager) handleMessageEvent(r routerHandle, p player, e filtera
err := m.filterProposalVote(p, r, e.Input.UnauthenticatedVote, e.FreshnessData)
if err != nil {
// mark filtered votes that may still update the best credential arrival time
credTrackingProcessing := proposalUsedForCredentialHistory(e.FreshnessData.PlayerRound, e.Input.UnauthenticatedVote)
return filteredEvent{T: voteFiltered, Err: makeSerErr(err), ContinueProcessingVoteForCredentialTracking: credTrackingProcessing}
credTrackingNote := NoCredentialTrackingImpact
if proposalUsedForCredentialHistory(e.FreshnessData.PlayerRound, e.Input.UnauthenticatedVote) {
credTrackingNote = MayImpactCredentialTracking
}
return filteredEvent{T: voteFiltered, Err: makeSerErr(err), CredentialTrackingNote: credTrackingNote}
}
return emptyEvent{}

Expand Down Expand Up @@ -174,12 +177,17 @@ func (m *proposalManager) handleMessageEvent(r routerHandle, p player, e filtera
// we only continued processing this vote to see whether it updates the credential arrival time
err := makeSerErrf("proposalManager: ignoring proposal-vote due to age: %v", err)
if e.t() == voteFiltered {
credNote := e.(filteredEvent).CredentialTrackingNote
if credNote != NewBestCredential && credNote != NoCredentialTrackingImpact {

Check warning on line 181 in agreement/proposalManager.go

View check run for this annotation

Codecov / codecov/patch

agreement/proposalManager.go#L180-L181

Added lines #L180 - L181 were not covered by tests
// It should be impossible to hit this condition
credNote = NoCredentialTrackingImpact

Check warning on line 183 in agreement/proposalManager.go

View check run for this annotation

Codecov / codecov/patch

agreement/proposalManager.go#L183

Added line #L183 was not covered by tests
}
// indicate whether it updated
return filteredEvent{T: voteFiltered, Err: err, StateUpdated: e.(filteredEvent).StateUpdated}
return filteredEvent{T: voteFiltered, Err: err, CredentialTrackingNote: credNote}

Check warning on line 186 in agreement/proposalManager.go

View check run for this annotation

Codecov / codecov/patch

agreement/proposalManager.go#L186

Added line #L186 was not covered by tests
}
// the proposalMachineRound didn't filter the vote, so it must have had a better credential,
// indicate that it did cause updating its state
return filteredEvent{T: voteFiltered, Err: err, StateUpdated: true}
return filteredEvent{T: voteFiltered, Err: err, CredentialTrackingNote: NewBestCredential}
}
return e

Expand Down
2 changes: 1 addition & 1 deletion agreement/proposalManager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ func TestProposalFreshAdjacentPeriods(t *testing.T) {
},
},
}
b.AddInOutPair(inMsg, filteredEvent{T: voteFiltered, ContinueProcessingVoteForCredentialTracking: true})
b.AddInOutPair(inMsg, filteredEvent{T: voteFiltered, CredentialTrackingNote: MayImpactCredentialTracking})

// vote from credentialRoundLag ago and period > 0 should be filtered
pV = helper.MakeRandomProposalValue()
Expand Down
18 changes: 9 additions & 9 deletions agreement/proposalTracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,27 +43,27 @@ type proposalSeeker struct {
// accept compares a given vote with the current lowest-credentialled vote and
// sets it if freeze has not been called. Returns true if any internal proposalSeeker
// state has been updated, whether to Lowest or lowestAfterFreeze.
func (s proposalSeeker) accept(v vote) (proposalSeeker, bool, error) {
func (s proposalSeeker) accept(v vote) (proposalSeeker, CredentialTrackingEffect, error) {
if s.Frozen {
updated := false
effect := NoCredentialTrackingImpact
// continue tracking and forwarding the lowest proposal even when frozen
if !s.hasLowestAfterFreeze || v.Cred.Less(s.lowestAfterFreeze.Cred) {
s.lowestAfterFreeze = v
s.hasLowestAfterFreeze = true
updated = true
effect = NewBestCredential
}
return s, updated, errProposalSeekerFrozen{}
return s, effect, errProposalSeekerFrozen{}
}

if s.Filled && !v.Cred.Less(s.Lowest.Cred) {
return s, false, errProposalSeekerNotLess{NewSender: v.R.Sender, LowestSender: s.Lowest.R.Sender}
return s, NoCredentialTrackingImpact, errProposalSeekerNotLess{NewSender: v.R.Sender, LowestSender: s.Lowest.R.Sender}
}

s.Lowest = v
s.Filled = true
s.lowestAfterFreeze = v
s.hasLowestAfterFreeze = true
return s, true, nil
return s, NewBestCredential, nil
}

// freeze freezes the state of the proposalSeeker so that future calls no longer
Expand Down Expand Up @@ -163,12 +163,12 @@ func (t *proposalTracker) handle(r routerHandle, p player, e event) event {
return filteredEvent{T: voteFiltered, Err: makeSerErr(err)}
}

var updated bool
var effect CredentialTrackingEffect
var err error
t.Freezer, updated, err = t.Freezer.accept(v)
t.Freezer, effect, err = t.Freezer.accept(v)
if err != nil {
err := errProposalTrackerPS{Sub: err}
return filteredEvent{T: voteFiltered, StateUpdated: updated, Err: makeSerErr(err)}
return filteredEvent{T: voteFiltered, CredentialTrackingNote: effect, Err: makeSerErr(err)}
}

return proposalAcceptedEvent{
Expand Down
18 changes: 9 additions & 9 deletions agreement/proposalTracker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,26 +66,26 @@ func TestProposalTrackerProposalSeeker(t *testing.T) {
assert.False(t, s.hasLowestAfterFreeze)

// issue events in the following order: 2, 3, 1, (freeze), 0
var updated bool
s, updated, err = s.accept(votes[2])
var effect CredentialTrackingEffect
s, effect, err = s.accept(votes[2])
assert.NoError(t, err)
assert.True(t, updated)
assert.Equal(t, effect, NewBestCredential)
assert.False(t, s.Frozen)
assert.True(t, s.Filled)
assert.True(t, s.Lowest.equals(votes[2]))
assert.True(t, s.hasLowestAfterFreeze)

s, updated, err = s.accept(votes[3])
s, effect, err = s.accept(votes[3])
assert.Error(t, err)
assert.False(t, updated)
assert.Equal(t, effect, NoCredentialTrackingImpact)
assert.False(t, s.Frozen)
assert.True(t, s.Filled)
assert.True(t, s.Lowest.equals(votes[2]))
assert.True(t, s.hasLowestAfterFreeze)

s, updated, err = s.accept(votes[1])
s, effect, err = s.accept(votes[1])
assert.NoError(t, err)
assert.True(t, updated)
assert.Equal(t, effect, NewBestCredential)
assert.False(t, s.Frozen)
assert.True(t, s.Filled)
assert.True(t, s.Lowest.equals(votes[1]))
Expand All @@ -98,9 +98,9 @@ func TestProposalTrackerProposalSeeker(t *testing.T) {
assert.True(t, s.Lowest.equals(votes[1]))
assert.True(t, s.hasLowestAfterFreeze)

s, updated, err = s.accept(votes[0])
s, effect, err = s.accept(votes[0])
assert.Error(t, err)
assert.True(t, updated)
assert.Equal(t, effect, NewBestCredential)
assert.Equal(t, s.Lowest, lowestBeforeFreeze)
assert.True(t, s.Frozen)
assert.True(t, s.Filled)
Expand Down

0 comments on commit f2fcc97

Please sign in to comment.