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

feat: add reason to BlockResponse messages #607

Merged
merged 5 commits into from
Jul 30, 2023

Conversation

amirvalhalla
Copy link
Member

Description

add reason of rejection to block response

Related issue(s)

If this Pull Request is related to an issue, mention it here.

@amirvalhalla amirvalhalla requested a review from b00f July 28, 2023 16:09
@amirvalhalla amirvalhalla changed the title Add reason to block response feat:block_response_reason Jul 28, 2023
@amirvalhalla amirvalhalla changed the title feat:block_response_reason AddBlockResponseReason Jul 28, 2023
@amirvalhalla amirvalhalla changed the title AddBlockResponseReason feat Jul 28, 2023
@amirvalhalla amirvalhalla changed the title feat feat: add reason to BlockResponse Jul 28, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #607 (6bfa148) into main (6c36120) will decrease coverage by 0.29%.
Report is 3 commits behind head on main.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #607      +/-   ##
==========================================
- Coverage   83.46%   83.17%   -0.29%     
==========================================
  Files         154      154              
  Lines        7292     7287       -5     
==========================================
- Hits         6086     6061      -25     
- Misses        922      939      +17     
- Partials      284      287       +3     

@b00f b00f changed the title feat: add reason to BlockResponse feat: add reason to BlockResponse messages Jul 29, 2023
@b00f
Copy link
Collaborator

b00f commented Jul 29, 2023

The main idea behind this PR is to provide a message to peers once their request is rejected. There are several reasons why a request might be rejected. For example, the peer is not known (here), or they request a block that we don't have (here), etc.

In my opinion, we should not consider a rejected request as a failure(or byzantine). It might be, but we can't consider it as such. Consequently, our current practice of increasing the failure counter is incorrect. In this context, we could merge the Busy and Rejected codes. I'm sorry, I didn't notice earlier that we were adding to the failure counter.

Please note that a reason could be added as a new parameter for the NewBlocksResponseMessage function.

…d add reason as paramater of new block response message
@amirvalhalla
Copy link
Member Author

The main idea behind this PR is to provide a message to peers once their request is rejected. There are several reasons why a request might be rejected. For example, the peer is not known (here), or they request a block that we don't have (here), etc.

In my opinion, we should not consider a rejected request as a failure(or byzantine). It might be, but we can't consider it as such. Consequently, our current practice of increasing the failure counter is incorrect. In this context, we could merge the Busy and Rejected codes. I'm sorry, I didn't notice earlier that we were adding to the failure counter.

Please note that a reason could be added as a new parameter for the NewBlocksResponseMessage function.

request changes resolved, please review.

sync/bundle/bundle_test.go Outdated Show resolved Hide resolved
sync/handler_blocks_request.go Outdated Show resolved Hide resolved
@amirvalhalla amirvalhalla requested a review from b00f July 30, 2023 07:33
@b00f b00f enabled auto-merge (squash) July 30, 2023 08:47
@b00f b00f merged commit 5ccaeb3 into pactus-project:main Jul 30, 2023
12 checks passed
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.

Sync: Adding rejection reason to blocks response message
2 participants