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

make proposal assembly time configurable #3165

Conversation

tsachiherman
Copy link
Contributor

@tsachiherman tsachiherman commented Oct 29, 2021

Summary

This PR is doing the following:

  1. Moves the proposal deadline computation outside of the agreement package and onto the node package. The agreement wasn't doing anything with the deadline anyway.
  2. Make the proposal deadline be configured using a configuration parameter rather then a global hard-coded value in the config package.

Test Plan

Existing unit test provides sufficient coverage for this change.

@tsachiherman tsachiherman self-assigned this Oct 29, 2021
@tsachiherman tsachiherman changed the title avoid passing deadline as part of AssembleBlock make proposal assembly time configurable Oct 29, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2021

Codecov Report

Merging #3165 (13644d1) into master (9ca31f8) will decrease coverage by 0.00%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3165      +/-   ##
==========================================
- Coverage   43.79%   43.79%   -0.01%     
==========================================
  Files         392      392              
  Lines       86880    86881       +1     
==========================================
- Hits        38050    38048       -2     
  Misses      42799    42799              
- Partials     6031     6034       +3     
Impacted Files Coverage Δ
agreement/abstractions.go 50.00% <ø> (ø)
config/config.go 47.05% <ø> (ø)
config/localTemplate.go 50.00% <ø> (ø)
daemon/algod/api/server/v2/test/helpers.go 77.09% <ø> (ø)
node/node.go 3.69% <0.00%> (-0.01%) ⬇️
data/pools/transactionPool.go 42.66% <33.33%> (+0.12%) ⬆️
agreement/pseudonode.go 71.91% <100.00%> (-0.12%) ⬇️
ledger/blockqueue.go 82.18% <0.00%> (-2.88%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ca31f8...13644d1. Read the comment docs.

@tsachiherman tsachiherman marked this pull request as ready for review October 29, 2021 19:39
@tsachiherman tsachiherman requested a review from a user October 30, 2021 20:50
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

LGTM! I was trying to think of any use cases where agreement would have information it would want to use to set a deadline for AssembleBlock, though we're not doing this today.

When agreement enters a new round, the "assemble" pseudonodeAction and rezeroAction are queued up consecutively, so agreement's call to time.Now() before calling AssembleBlock, and then rezeroAction's call to time.Now() in Clock.Zero() are roughly the same moment. In that scenario (new round, period 0), having transactionPool pick a deadline on its own is roughly equivalent to having agreement do it, and agreement doesn't have any more information how soon a block is needed (when the next vote is coming) than transactionPool does.

In the scenario where it's not period 0, however, and we are using different values of AgreementFilterTimeoutPeriod0 and AgreementFilterTimeout, you could imagine a scenario where agreement might want to tell the pool it has a little more time to put a block together, perhaps to relieve whatever resource issue was causing difficulty in period 0. If we ever do that, we could provide a hint to the pool using either a deadline or some other argument later.

@tsachiherman
Copy link
Contributor Author

LGTM! I was trying to think of any use cases where agreement would have information it would want to use to set a deadline for AssembleBlock, though we're not doing this today.

When agreement enters a new round, the "assemble" pseudonodeAction and rezeroAction are queued up consecutively, so agreement's call to time.Now() before calling AssembleBlock, and then rezeroAction's call to time.Now() in Clock.Zero() are roughly the same moment. In that scenario (new round, period 0), having transactionPool pick a deadline on its own is roughly equivalent to having agreement do it, and agreement doesn't have any more information how soon a block is needed (when the next vote is coming) than transactionPool does.

In the scenario where it's not period 0, however, and we are using different values of AgreementFilterTimeoutPeriod0 and AgreementFilterTimeout, you could imagine a scenario where agreement might want to tell the pool it has a little more time to put a block together, perhaps to relieve whatever resource issue was causing difficulty in period 0. If we ever do that, we could provide a hint to the pool using either a deadline or some other argument later.

Yes, I agree with that. The agreement could pass a hint that would adjust the timing for the assembly.

@tsachiherman tsachiherman merged commit 168cfe5 into algorand:master Nov 1, 2021
@tsachiherman tsachiherman deleted the tsachi/configurable_proposal_assemble_time branch November 1, 2021 16:08
cce pushed a commit to cce/go-algorand that referenced this pull request Nov 3, 2021
This PR is doing the following:
1. Moves the proposal deadline computation outside of the agreement package and onto the node package. The agreement wasn't doing anything with the deadline anyway.
2. Make the proposal deadline be configured using a configuration parameter rather then a global hard-coded value in the config package.

Existing unit test provides sufficient coverage for this change.
@egieseke egieseke mentioned this pull request Nov 23, 2021
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