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.

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

Enhancement: minor change in agreement/type.go comment, numbers are not matching #5186

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Mar 8, 2023

Summary

Minor fix in agreement/type.go comment, the number seems to be not matching with the configuration.

@ahangsu ahangsu requested a review from cce March 8, 2023 21:22
@ahangsu ahangsu force-pushed the agreement-type-comment-change branch from afceb71 to f2ec0fd Compare March 8, 2023 21:27
@ahangsu ahangsu force-pushed the agreement-type-comment-change branch from f2ec0fd to 50e6fe0 Compare March 8, 2023 21:35
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #5186 (496d866) into master (4bbedc0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5186      +/-   ##
==========================================
+ Coverage   53.51%   53.52%   +0.01%     
==========================================
  Files         439      439              
  Lines       54985    54985              
==========================================
+ Hits        29423    29430       +7     
+ Misses      23271    23267       -4     
+ Partials     2291     2288       -3     
Impacted Files Coverage Δ
agreement/types.go 87.93% <100.00%> (ø)

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

agreement/types.go Outdated Show resolved Hide resolved
@ahangsu ahangsu requested a review from cce March 9, 2023 19:38

for i := next; i < s; i++ {
extra *= 2
lower = upper
upper = lower + extra
}

// e.g. if s == 14
// extra = 2 ^ 8 * 2500ms = 256 * 2.5 = 512 + 128 = 640s
Copy link
Contributor

Choose a reason for hiding this comment

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

where 2500ms or 2000ms come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

small lambda = 2000ms, while the comment said it is 2500ms, so it is wrong.

extra started from 2000 ms, so I need to fix the computation in comments too...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's here:

// Protocol holds the global configuration settings for the agreement protocol,
// initialized with our current defaults. This is used across all nodes we create.
var Protocol = Global{
SmallLambda: 2000 * time.Millisecond,
BigLambda: 15000 * time.Millisecond,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

And here

var deadlineTimeout = config.Protocol.BigLambda + config.Protocol.SmallLambda
var partitionStep = next + 3
var recoveryExtraTimeout = config.Protocol.SmallLambda

@cce cce merged commit dba5a08 into algorand:master Mar 10, 2023
@ahangsu ahangsu deleted the agreement-type-comment-change branch March 10, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants