Skip to content

Conversation

@lmanners
Copy link
Contributor

@lmanners lmanners commented May 8, 2018

Added handling for the case where headers are announced over more than one message.
refs #12453

Added handling for the case where headers are announced over more than one message.
refs bitcoin#12453
@fanquake fanquake added the Tests label May 8, 2018
@fanquake fanquake requested a review from jnewbery May 9, 2018 00:34
@laanwj
Copy link
Member

laanwj commented May 9, 2018

Concept ACK

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution @lmanners. Looks great!

Tested ACK 12d1b77. I've run this 100 times locally and not been able to reproduce the error (although note that this was a timing error that was previously only seen on Travis).

A nit and a comment inline, but no need to update the branch for either of those. I think this can be merged as is.

if len(message.headers):
self.block_announced = True
message.headers[-1].calc_sha256()
for x in message.headers:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer header as variable name over x


inv_node.check_last_announcement(inv=[tip_hash], headers=[])
test_node.check_last_announcement(inv=[tip_hash], headers=[])
inv_node.check_last_inv_announcement(inv=[tip_hash])
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling the method with a named argument isn't strictly necessary, since the method only takes a single argument now, but it does no harm.

@maflcko
Copy link
Member

maflcko commented May 10, 2018

utACK 12d1b77

@maflcko maflcko added this to the 0.16.1 milestone May 10, 2018
@maflcko maflcko merged commit 12d1b77 into bitcoin:master May 10, 2018
maflcko pushed a commit that referenced this pull request May 10, 2018
12d1b77 [tests] Fixed intermittent failure in p2p_sendheaders.py. (lmanners)

Pull request description:

  Added handling for the case where headers are announced over more than one message.
  refs #12453

Tree-SHA512: 2c5b48ff019089b86e358181ba170d3aac09d4ae41ec79c2718e0ee83705860501bbcb8fd94d0f5c4f86c0d54a96781a967716621bb8c5ecc991b39af3cec506
@lmanners lmanners deleted the p2p_sendheaders branch May 13, 2018 12:18
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request May 29, 2018
Added handling for the case where headers are announced over more than one message.
refs bitcoin#12453

Github-Pull: bitcoin#13192
Rebased-From: 12d1b77
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jul 12, 2018
Added handling for the case where headers are announced over more than one message.
refs bitcoin#12453

Github-Pull: bitcoin#13192
Rebased-From: 12d1b77
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020
…aders.py.

12d1b77 [tests] Fixed intermittent failure in p2p_sendheaders.py. (lmanners)

Pull request description:

  Added handling for the case where headers are announced over more than one message.
  refs bitcoin#12453

Tree-SHA512: 2c5b48ff019089b86e358181ba170d3aac09d4ae41ec79c2718e0ee83705860501bbcb8fd94d0f5c4f86c0d54a96781a967716621bb8c5ecc991b39af3cec506
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants