Skip to content

Commit

Permalink
address some of the comments
Browse files Browse the repository at this point in the history
  • Loading branch information
yossigi committed Aug 17, 2023
1 parent 1404411 commit 3580617
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 47 deletions.
32 changes: 12 additions & 20 deletions agreement/player.go
Original file line number Diff line number Diff line change
Expand Up @@ -295,54 +295,46 @@ func (p *player) updateDynamicLambdaTimings(r routerHandle, ver protocol.Consens
if re.Vote.validatedAt != 0 {
p.lowestCredentialArrivals = append(p.lowestCredentialArrivals, re.Vote.validatedAt)
}
p.resizeLowestCredentialArrivals(ver)
}

func (p *player) resizeLowestCredentialArrivals(ver protocol.ConsensusVersion) {
// trim history to the last proto.DynamicFilterCredentialArrivalHistory samples.
proto := config.Consensus[ver]
if len(p.lowestCredentialArrivals) > proto.DynamicFilterCredentialArrivalHistory {
p.lowestCredentialArrivals = p.lowestCredentialArrivals[len(p.lowestCredentialArrivals)-proto.DynamicFilterCredentialArrivalHistory:]

Check warning on line 302 in agreement/player.go

View check run for this annotation

Codecov / codecov/patch

agreement/player.go#L302

Added line #L302 was not covered by tests
}
for len(p.lowestCredentialArrivals) < proto.DynamicFilterCredentialArrivalHistory {
p.lowestCredentialArrivals = append([]time.Duration{FilterTimeout(0, ver)}, p.lowestCredentialArrivals...)
}
}

// calculateFilterTimeout chooses the appropriate filter timeout.
func (p *player) calculateFilterTimeout(ver protocol.ConsensusVersion, tracer *tracer) time.Duration {

proto := config.Consensus[ver]

if !proto.DynamicFilterTimeout || p.Period != 0 {
if proto.DynamicFilterCredentialArrivalHistory <= 0 || p.Period != 0 {
// Either dynamic lambda is disabled, or we're not in period 0 and
// therefore, can't use dynamic lambda
return FilterTimeout(p.Period, ver)
}

var dynamicDelay time.Duration
defaultDelay := FilterTimeout(0, ver)
if proto.DynamicFilterCredentialArrivalHistory <= 0 {
// we don't keep any history, use the default
dynamicDelay = defaultDelay
} else if proto.DynamicFilterCredentialArrivalHistory > len(p.lowestCredentialArrivals) {
if proto.DynamicFilterCredentialArrivalHistory > len(p.lowestCredentialArrivals) {
// not enough samples, use the default
dynamicDelay = defaultDelay
} else {
sortedArrivals := make([]time.Duration, len(p.lowestCredentialArrivals))
copy(sortedArrivals[:], p.lowestCredentialArrivals[:])
sort.Slice(sortedArrivals, func(i, j int) bool { return sortedArrivals[i] < sortedArrivals[j] })
dynamicDelay = sortedArrivals[proto.DynamicFilterCredentialArrivalHistoryIdx] + 50*time.Millisecond
return defaultDelay
}

sortedArrivals := make([]time.Duration, len(p.lowestCredentialArrivals))
copy(sortedArrivals[:], p.lowestCredentialArrivals[:])
sort.Slice(sortedArrivals, func(i, j int) bool { return sortedArrivals[i] < sortedArrivals[j] })
dynamicDelay := sortedArrivals[proto.DynamicFilterCredentialArrivalHistoryIdx] + proto.DynamicFilterGraceTime

Check warning on line 325 in agreement/player.go

View check run for this annotation

Codecov / codecov/patch

agreement/player.go#L322-L325

Added lines #L322 - L325 were not covered by tests

// Make sure the dynamic delay is not too small or too large
if dynamicDelay < proto.DynamicFilterTimeoutLowerBound {
if tracer != nil {
tracer.log.Debugf("Calculated dynamic delay = %v for round %v, period %v. It's too low, setting to %v\n", dynamicDelay, p.Round, p.Period, proto.DynamicFilterTimeoutLowerBound)

Check warning on line 330 in agreement/player.go

View check run for this annotation

Codecov / codecov/patch

agreement/player.go#L328-L330

Added lines #L328 - L330 were not covered by tests
}
dynamicDelay = proto.DynamicFilterTimeoutLowerBound
} else if dynamicDelay > defaultDelay {
if tracer != nil {
tracer.log.Debugf("Calculated dynamic delay = %v for round %v, period %v. It's higher than the default %v\n", dynamicDelay, p.Round, p.Period, defaultDelay)

Check warning on line 335 in agreement/player.go

View check run for this annotation

Codecov / codecov/patch

agreement/player.go#L332-L335

Added lines #L332 - L335 were not covered by tests
}
dynamicDelay = defaultDelay
tracer.log.Warnf("Calculated dynamic delay = %v for round %v, period %v. It's higher than the default %v\n", dynamicDelay, p.Round, p.Period, defaultDelay)
} else if tracer != nil {
tracer.log.Debugf("Calculated dynamic delay = %v for round %v, period %v.\n", dynamicDelay, p.Round, p.Period)

Check warning on line 339 in agreement/player.go

View check run for this annotation

Codecov / codecov/patch

agreement/player.go#L337-L339

Added lines #L337 - L339 were not covered by tests
}
Expand Down
18 changes: 9 additions & 9 deletions agreement/player_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3236,7 +3236,7 @@ func TestPlayerAlwaysResynchsPinnedValue(t *testing.T) {
// test that ReceivedAt and ValidateAt timing information are retained in proposalStore
// when the payloadPresent, payloadVerified, and voteVerified events are processed, and that all timings
// are available when the ensureAction is called for the block.
func TestPlayerRetainsReceivedValidatedAt(t *testing.T) {
func TestPlayerRetainsReceivedValidatedAtOneSample(t *testing.T) {
partitiontest.PartitionTest(t)

const r = round(20239)
Expand Down Expand Up @@ -3265,15 +3265,15 @@ func TestPlayerRetainsReceivedValidatedAt(t *testing.T) {
// assert lowest vote validateAt time was recorded into payloadArrivals
historyLen := config.Consensus[protocol.ConsensusFuture].DynamicFilterCredentialArrivalHistory
require.NotZero(t, historyLen)
require.Len(t, pWhite.lowestCredentialArrivals, historyLen)
require.Equal(t, 501*time.Millisecond, pWhite.lowestCredentialArrivals[historyLen-1])
require.Len(t, pWhite.lowestCredentialArrivals, 1)
require.Equal(t, 501*time.Millisecond, pWhite.lowestCredentialArrivals[0])
}

// test that ReceivedAt and ValidateAt timing information are retained in proposalStore
// when the payloadPresent (as part of the CompoundMessage encoding used by PP messages),
// payloadVerified, and voteVerified events are processed, and that all timings
// are available when the ensureAction is called for the block.
func TestPlayerRetainsReceivedValidatedAtPP(t *testing.T) {
func TestPlayerRetainsReceivedValidatedAtPPOneSample(t *testing.T) {
partitiontest.PartitionTest(t)

const r = round(20239)
Expand Down Expand Up @@ -3309,16 +3309,16 @@ func TestPlayerRetainsReceivedValidatedAtPP(t *testing.T) {
// assert lowest vote validateAt time was recorded into payloadArrivals
historyLen := config.Consensus[protocol.ConsensusFuture].DynamicFilterCredentialArrivalHistory
require.NotZero(t, historyLen)
require.Len(t, pWhite.lowestCredentialArrivals, historyLen)
require.Equal(t, 502*time.Millisecond, pWhite.lowestCredentialArrivals[historyLen-1])
require.Len(t, pWhite.lowestCredentialArrivals, 1)
require.Equal(t, 502*time.Millisecond, pWhite.lowestCredentialArrivals[0])
}

// test that ReceivedAt and ValidateAt timing information are retained in proposalStore
// when the voteVerified event comes in first (as part of the AV message before PP),
// then the payloadPresent (as part of the CompoundMessage encoding used by PP messages)
// and payloadVerified events are processed, and that all timings
// are available when the ensureAction is called for the block.
func TestPlayerRetainsReceivedValidatedAtAVPP(t *testing.T) {
func TestPlayerRetainsReceivedValidatedAtAVPPOneSample(t *testing.T) {
partitiontest.PartitionTest(t)

const r = round(20239)
Expand Down Expand Up @@ -3364,8 +3364,8 @@ func TestPlayerRetainsReceivedValidatedAtAVPP(t *testing.T) {
// assert lowest vote validateAt time was recorded into payloadArrivals
historyLen := config.Consensus[protocol.ConsensusFuture].DynamicFilterCredentialArrivalHistory
require.NotZero(t, historyLen)
require.Len(t, pWhite.lowestCredentialArrivals, historyLen)
require.Equal(t, 502*time.Millisecond, pWhite.lowestCredentialArrivals[historyLen-1])
require.Len(t, pWhite.lowestCredentialArrivals, 1)
require.Equal(t, 502*time.Millisecond, pWhite.lowestCredentialArrivals[0])
}

func assertCorrectReceivedAtSet(t *testing.T, pWhite *player, pM ioAutomata, helper *voteMakerHelper,
Expand Down
2 changes: 1 addition & 1 deletion agreement/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,7 @@ func TestAgreementSynchronousFuture5_DynamicFilterRounds(t *testing.T) {
}

cfg := config.Consensus[protocol.ConsensusFuture]
if !cfg.DynamicFilterTimeout {
if cfg.DynamicFilterCredentialArrivalHistory <= 0 {
return
}

Expand Down
21 changes: 12 additions & 9 deletions config/consensus.go
Original file line number Diff line number Diff line change
Expand Up @@ -517,21 +517,24 @@ type ConsensusParams struct {
// by state proofs to use the same method (rather than excluding stake from the top N stakeholders as before).
ExcludeExpiredCirculation bool

// DynamicFilterTimeout specifies whether the timeout for the filter step
// should be determined dynamically, at run-time.
DynamicFilterTimeout bool
// DynamicFilterCredentialArrivalHistory specifies the number of past
// credential arrivals that are measured to determine the next filter
// timeout. If DynamicFilterCredentialArrivalHistory <= 0, then the dynamic
// timeout feature is off and the filter step timeout is calculated using
// the static configuration.
DynamicFilterCredentialArrivalHistory int

// DynamicFilterTimeoutLowerBound specifies a minimal duration that the
// filter timeout must meet.
DynamicFilterTimeoutLowerBound time.Duration

// DynamicFilterCredentialArrivalHistory specifies the number of past credential arrivals
// that are measured to determine the next filter timeout.
DynamicFilterCredentialArrivalHistory int

// DynamicFilterCredentialArrivalHistoryIdx specified which sample to use out of a sorted
// DynamicFilterCredentialArrivalHistory-sized array of time samples.
DynamicFilterCredentialArrivalHistoryIdx int

// DynamicFilterGraceTime is additional extention to the dynamic filter time atop the

Check failure on line 535 in config/consensus.go

View workflow job for this annotation

GitHub Actions / Lint Errors

[Lint Errors] config/consensus.go#L535

`extention` is a misspelling of `extension` (misspell)
Raw output
config/consensus.go:535:42: `extention` is a misspelling of `extension` (misspell)
	// DynamicFilterGraceTime is additional extention to the dynamic filter time atop the
	                                        ^
// one calculated based on the history of credential arrivals.
DynamicFilterGraceTime time.Duration
}

// PaysetCommitType enumerates possible ways for the block header to commit to
Expand Down Expand Up @@ -1379,12 +1382,12 @@ func initConsensusProtocols() {
vFuture.LogicSigVersion = 10 // When moving this to a release, put a new higher LogicSigVersion here
vFuture.EnableLogicSigCostPooling = true

vFuture.DynamicFilterTimeout = true
vFuture.DynamicFilterTimeoutLowerBound = time.Second
// history window of 40, so we have enough statistics to calculate the 95th
// percentile, which is the timestamp at index 37 in the history array.
vFuture.DynamicFilterCredentialArrivalHistory = 40
vFuture.DynamicFilterCredentialArrivalHistoryIdx = 37
vFuture.DynamicFilterTimeoutLowerBound = time.Second
vFuture.DynamicFilterGraceTime = 50 * time.Millisecond

Consensus[protocol.ConsensusFuture] = vFuture

Expand Down
14 changes: 6 additions & 8 deletions tools/x-repo-types/xrt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,12 @@ func TestCrossRepoTypes(t *testing.T) {
yType: "EvalDelta",
},
{
name: "goal-v-sdk-consensus",
xPkg: "github.com/algorand/go-algorand/config",
xType: "ConsensusParams",
yPkg: "github.com/algorand/go-algorand-sdk/v2/protocol/config",
yBranch: "develop",
yType: "ConsensusParams",
skip: true,
skipReason: `Changes to consensus parameters happen on the go-algorand repo, and cannot be synced in a single PR with changes to the go-algorand-sdk repo`,
name: "goal-v-sdk-consensus",
xPkg: "github.com/algorand/go-algorand/config",
xType: "ConsensusParams",
yPkg: "github.com/algorand/go-algorand-sdk/v2/protocol/config",
yBranch: "develop",
yType: "ConsensusParams",
},
{
name: "goal-v-sdk-blockheader",
Expand Down

0 comments on commit 3580617

Please sign in to comment.