Skip to content

Conversation

@anorth
Copy link
Member

@anorth anorth commented Jun 3, 2024

Closes #208

@anorth anorth requested a review from Stebalien June 3, 2024 23:13
gpbft/gpbft.go Outdated
roundsReceived := map[uint64]struct{}{}
for _, msg := range msgs {
if err := i.receiveOne(msg); err != nil {
if _, err := i.receiveOne(msg); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

This path is reached only once per instance, so I don't think it's worth the check here. But I can accumulate a stateChanged here too if requested.

Copy link
Member

Choose a reason for hiding this comment

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

We're limited to maxLookaheadRounds * powerLookback anyways (except for justified messages). So I'd say this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Although... it should be trivial to just not add this round to roundsReceived, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes,done

Comment on lines +404 to +409
if !state.prepared.ReceivedFromWeakQuorum() {
return nil, nil, false
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this cheaper and harder-to-meet check to happen first

@codecov
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.33%. Comparing base (8bef14d) to head (f01fae6).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #300      +/-   ##
==========================================
- Coverage   85.66%   85.33%   -0.34%     
==========================================
  Files          14       14              
  Lines        1458     1459       +1     
==========================================
- Hits         1249     1245       -4     
- Misses        130      136       +6     
+ Partials       79       78       -1     
Files Coverage Δ
gpbft/gpbft.go 83.30% <82.50%> (-0.76%) ⬇️

gpbft/gpbft.go Outdated
roundsReceived := map[uint64]struct{}{}
for _, msg := range msgs {
if err := i.receiveOne(msg); err != nil {
if _, err := i.receiveOne(msg); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

We're limited to maxLookaheadRounds * powerLookback anyways (except for justified messages). So I'd say this is fine.

gpbft/gpbft.go Outdated
roundsReceived := map[uint64]struct{}{}
for _, msg := range msgs {
if err := i.receiveOne(msg); err != nil {
if _, err := i.receiveOne(msg); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Although... it should be trivial to just not add this round to roundsReceived, right?

@anorth anorth force-pushed the anorth/roundstate-dos branch from 9c8bf3f to a61c437 Compare June 4, 2024 21:54
@anorth anorth force-pushed the anorth/roundstate-dos branch from a61c437 to f01fae6 Compare June 4, 2024 21:55
@anorth anorth enabled auto-merge June 4, 2024 21:55
@anorth anorth added this pull request to the merge queue Jun 4, 2024
Merged via the queue into main with commit c8532ae Jun 4, 2024
@anorth anorth deleted the anorth/roundstate-dos branch June 4, 2024 22:01
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.

instance.roundState(round) is a (minor) DoS vector

3 participants