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

Shortcut timeout in PREPARE when outcome is determined #289

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

masih
Copy link
Member

@masih masih commented May 29, 2024

When sufficient justification is received do not wait for timeouts to expire in PREPARE phase.

Part of #242

@masih masih marked this pull request as draft May 29, 2024 17:54
@masih
Copy link
Member Author

masih commented May 29, 2024

@anorth I would love your early feedback on this please 🙏

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

The changes to tryPrepare/Commit look right to me.

These will be hard to cover with sim tests, but we should endeavour to cover them with unit tests of participant/instance #9 (won't affect line coverage, which is too crude for us here).

gpbft/gpbft.go Outdated Show resolved Hide resolved
gpbft/gpbft.go Outdated Show resolved Hide resolved
@masih masih force-pushed the masih/timeout-shortcut branch 4 times, most recently from 5f4b29e to 696b632 Compare June 5, 2024 12:08
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.29%. Comparing base (b3c0149) to head (e4f24f9).

Current head e4f24f9 differs from pull request most recent head 518f739

Please upload reports for the commit 518f739 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
+ Coverage   84.90%   85.29%   +0.39%     
==========================================
  Files          14       14              
  Lines        1477     1476       -1     
==========================================
+ Hits         1254     1259       +5     
+ Misses        139      136       -3     
+ Partials       84       81       -3     
Files Coverage Δ
gpbft/gpbft.go 83.64% <100.00%> (+0.23%) ⬆️

... and 1 file with indirect coverage changes

@masih masih requested a review from anorth June 5, 2024 12:18
@masih masih marked this pull request as ready for review June 5, 2024 12:59
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

I see you've reverted the change to commit phase. This PR can be good to merge if the title is updated and it does not automatically close #242.

gpbft/gpbft.go Outdated Show resolved Hide resolved
gpbft/gpbft.go Outdated Show resolved Hide resolved
@anorth anorth changed the title Shortcut timeouts in PREPARE and COMMIT when justified Shortcut timeout in PREPARE when outcome is determined Jun 7, 2024
@anorth anorth force-pushed the masih/timeout-shortcut branch from 1e8667b to e4f24f9 Compare June 7, 2024 00:21
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

I pushed a commit and adjusted the title and description so that I can approve this and you can merge when you are back online.

test/spam_test.go Outdated Show resolved Hide resolved
When sufficient justification is received do not wait for timeouts to
expire in PREPARE phase.

Part of #242
@masih masih force-pushed the masih/timeout-shortcut branch from e4f24f9 to 518f739 Compare June 7, 2024 08:14
@masih masih added this pull request to the merge queue Jun 7, 2024
Merged via the queue into main with commit bc77de6 Jun 7, 2024
5 checks passed
@masih masih deleted the masih/timeout-shortcut branch June 7, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants