From f2fcc976cfbc7b1e2cd6f830dc8523b1cfe1473b Mon Sep 17 00:00:00 2001 From: Yossi Gilad Date: Sat, 2 Sep 2023 22:04:04 -0400 Subject: [PATCH] switch to enum --- agreement/events.go | 20 ++++++++++++-------- agreement/player.go | 14 +++++++++----- agreement/proposalManager.go | 16 ++++++++++++---- agreement/proposalManager_test.go | 2 +- agreement/proposalTracker.go | 18 +++++++++--------- agreement/proposalTracker_test.go | 18 +++++++++--------- 6 files changed, 52 insertions(+), 36 deletions(-) diff --git a/agreement/events.go b/agreement/events.go index 279c60b5f0..21b82aee82 100644 --- a/agreement/events.go +++ b/agreement/events.go @@ -596,18 +596,22 @@ func (e payloadProcessedEvent) ComparableStr() string { return fmt.Sprintf("%v: %.5v", e.t().String(), e.Proposal.BlockDigest.String()) } +type CredentialTrackingEffect uint8 + +const ( + 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. diff --git a/agreement/player.go b/agreement/player.go index b7536680bc..a0c19f2af7 100644 --- a/agreement/player.go +++ b/agreement/player.go @@ -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. } } diff --git a/agreement/proposalManager.go b/agreement/proposalManager.go index b0286bd6bd..8dea58949d 100644 --- a/agreement/proposalManager.go +++ b/agreement/proposalManager.go @@ -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{} @@ -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 { + // It should be impossible to hit this condition + credNote = NoCredentialTrackingImpact + } // indicate whether it updated - return filteredEvent{T: voteFiltered, Err: err, StateUpdated: e.(filteredEvent).StateUpdated} + return filteredEvent{T: voteFiltered, Err: err, CredentialTrackingNote: credNote} } // 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 diff --git a/agreement/proposalManager_test.go b/agreement/proposalManager_test.go index 770677d456..863e16292e 100644 --- a/agreement/proposalManager_test.go +++ b/agreement/proposalManager_test.go @@ -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() diff --git a/agreement/proposalTracker.go b/agreement/proposalTracker.go index a9e0380bf1..f93dfb3a82 100644 --- a/agreement/proposalTracker.go +++ b/agreement/proposalTracker.go @@ -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 @@ -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{ diff --git a/agreement/proposalTracker_test.go b/agreement/proposalTracker_test.go index 1b8a47aef6..383ef57beb 100644 --- a/agreement/proposalTracker_test.go +++ b/agreement/proposalTracker_test.go @@ -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])) @@ -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)