Skip to content

[blockchain] respond with nil on block request to indicate no block (…#540

Closed
melekes wants to merge 1 commit intodevelopfrom
feature/514-respond-to-block-request
Closed

[blockchain] respond with nil on block request to indicate no block (…#540
melekes wants to merge 1 commit intodevelopfrom
feature/514-respond-to-block-request

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Jun 19, 2017

…Refs #514)

@codecov-io
Copy link

codecov-io commented Jun 19, 2017 •
edited
Loading

Codecov Report

Merging #540 into develop will increase coverage by 0.35%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           develop    #540      +/-   ##
==========================================
+ Coverage    53.84%   54.2%   +0.35%     
==========================================
  Files           76      76              
  Lines         7941    7940       -1     
==========================================
+ Hits          4276    4304      +28     
+ Misses        3306    3278      -28     
+ Partials       359     358       -1

@ebuchman
Copy link
Contributor

ebuchman commented Jun 24, 2017

I'm not sure about this, since I think the reactor is designed so this doesn't happen. If it does happen, it might be a sign of a malicious peer. @jaekwon ?

@jaekwon
Copy link
Contributor

jaekwon commented Jul 11, 2017

This can help with debugging. But lets also add information about the block, so bcBlockResponseMessage includes a height, and nil means we don't have it yet. Or, we can maybe create a new message, like bcNoBlockMessage

@jaekwon
Copy link
Contributor

jaekwon commented Jul 11, 2017

Lets also add a comment to note that according to Tendermint spec, if all nodes are honest, it shouldn't happen.

@jaekwon jaekwon mentioned this pull request Jul 11, 2017
Closed
@zramsay
Copy link
Contributor

zramsay commented Aug 16, 2017

what's this blocking on? Looks like only addressing jae's last comment (of adding a comment). @melekes can you add the comment?

@ebuchman
Copy link
Contributor

ebuchman commented Aug 25, 2017

lets add the bcNoBlockMessage so it's not a breaking change.

@odeke-em want to take a stab at this ?

@odeke-em
Copy link
Contributor

odeke-em commented Aug 25, 2017

@ebuchman for sure, I can take a stab at it, thanks for the tag.

@ebuchman ebuchman added this to the 0.11.0 milestone Sep 5, 2017
@odeke-em odeke-em mentioned this pull request Sep 7, 2017
Closed
@ebuchman
Copy link
Contributor

ebuchman commented Sep 12, 2017

Closing in favour of #648

@ebuchman ebuchman closed this Sep 12, 2017
odeke-em added a commit that referenced this pull request Sep 19, 2017
@odeke-em
blockchain/reactor: respondWithNoResponseMessage for missing height
7dfffc0 
Fixes #514
Replaces #540

If a peer requests a block with a height that we don't have
respond with a bcNoBlockResponseMessage.
However, according to the Tendermint spec, if all nodes are honest
this condition shouldn't occur, so this is a possible hint of an
dishonest node.
@zramsay zramsay deleted the feature/514-respond-to-block-request branch September 21, 2017 12:19
odeke-em added a commit that referenced this pull request Oct 4, 2017
@odeke-em
blockchain/reactor: bcNoBlockResponseMessage for missing height
c89619a 
Fixes #514
Replaces #540

If a peer requests a block with a height that we don't have
respond with a bcNoBlockResponseMessage.
However, according to the Tendermint spec, if all nodes are honest
this condition shouldn't occur, so this is a possible hint of a
dishonest node.
ebuchman pushed a commit that referenced this pull request Oct 4, 2017
@odeke-em @ebuchman
blockchain/reactor: respondWithNoResponseMessage for missing height
068f013 
Fixes #514
Replaces #540

If a peer requests a block with a height that we don't have
respond with a bcNoBlockResponseMessage.
However, according to the Tendermint spec, if all nodes are honest
this condition shouldn't occur, so this is a possible hint of an
dishonest node.
srmo pushed a commit to srmo/tendermint that referenced this pull request Oct 15, 2017
@odeke-em @srmo
blockchain/reactor: respondWithNoResponseMessage for missing height
6324a46 
Fixes tendermint#514
Replaces tendermint#540

If a peer requests a block with a height that we don't have
respond with a bcNoBlockResponseMessage.
However, according to the Tendermint spec, if all nodes are honest
this condition shouldn't occur, so this is a possible hint of an
dishonest node.
faddat referenced this pull request in notional-labs/tendermint Apr 3, 2023
@sergio-mena @jmalicevic
Fix deficiency in ABCI interface: SignedLastBlock (#540)
27d2a18 
* proto changes, first version

* Fix usage of `SignedLastBlock`

* make proto-gen

* Update proto/tendermint/abci/types.proto

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

---------

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.11.0

Development

Successfully merging this pull request may close these issues.

6 participants

@melekes @codecov-io @ebuchman @jaekwon @zramsay @odeke-em