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

Improve handling of unconnecting headers #8305

Merged
merged 2 commits into from Jul 18, 2016

Conversation

Projects
None yet
8 participants
@sdaftuar
Member

sdaftuar commented Jul 5, 2016

I found a situation on testnet where a peer could send a header that wouldn't be accepted locally, because of the 2-hour-in-the-future rule. This would then cause BIP-130 headers sync to fail, because the sender doesn't realize that the recipient didn't accept the header, and so continues to send headers (one-by-one) that build on top of the rejected header, until eventually the sender is disconnected.

This PR tries to improve upon the situation: if the first header in a received headers message doesn't connect, try once to request connecting headers.

If we get two headers messages in a row that don't connect, give up on trying to request connecting headers (to avoid infinite looping with the remote peer), until our peer delivers a headers message that does connect.

If this approach is acceptable, I think we should backport to 0.12 (looks like it doesn't merge cleanly, but I can open a new PR for the backport).

cc: @TheBlueMatt @sipa

@laanwj laanwj added the P2P label Jul 6, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 6, 2016

Member

Needs rebase after #8306

Member

laanwj commented Jul 6, 2016

Needs rebase after #8306

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 6, 2016

Member

Rebased.

Member

sdaftuar commented Jul 6, 2016

Rebased.

@mrbandrews

View changes

Show outdated Hide outdated src/main.cpp
@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jul 6, 2016

Contributor

@sdaftuar can you split this into two commits, one with changes and a second with tests?

Contributor

pstratem commented Jul 6, 2016

@sdaftuar can you split this into two commits, one with changes and a second with tests?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 7, 2016

Member

Added logging, and split into two commits.

Member

sdaftuar commented Jul 7, 2016

Added logging, and split into two commits.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jul 7, 2016

Contributor

@sdaftuar thank you

Contributor

pstratem commented Jul 7, 2016

@sdaftuar thank you

@jonasschnelli jonasschnelli added this to the 0.13.0 milestone Jul 7, 2016

@mrbandrews

This comment has been minimized.

Show comment
Hide comment
@mrbandrews

mrbandrews Jul 11, 2016

Contributor

ACK

Contributor

mrbandrews commented Jul 11, 2016

ACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 11, 2016

Member

I've tried to reason through what would happen if two subsequent blocks happen with a very small time between them, before the peer has time to respond to the getheaders sent as response to the first block, but couldn't find any problem. I believe that convergence would only fail in case every future pair of blocks happens with a sufficiently short time between them.

Member

sipa commented Jul 11, 2016

I've tried to reason through what would happen if two subsequent blocks happen with a very small time between them, before the peer has time to respond to the getheaders sent as response to the first block, but couldn't find any problem. I believe that convergence would only fail in case every future pair of blocks happens with a sufficiently short time between them.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 11, 2016

Member

ACK

Member

sipa commented Jul 11, 2016

ACK

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jul 11, 2016

Member

Is it necessary to recover from this more than once per remote-node? As-is, it looks like someone could just respond with alternating connecting/non-connecting headers forever since a ban score isn't set. I can't imagine any motivation for doing so, though...

Also, I don't think this should be allowed during IBD? Seems the approach here could be unified with the one when a non-connecting CMPCTBLOCK is received.

Member

theuni commented Jul 11, 2016

Is it necessary to recover from this more than once per remote-node? As-is, it looks like someone could just respond with alternating connecting/non-connecting headers forever since a ban score isn't set. I can't imagine any motivation for doing so, though...

Also, I don't think this should be allowed during IBD? Seems the approach here could be unified with the one when a non-connecting CMPCTBLOCK is received.

sdaftuar added some commits Jul 7, 2016

Improve handling of unconnecting headers
When processing a headers message that looks like a block announcement,
send peer a getheaders if the headers message won't connect.

Apply DoS points after too many consecutive unconnecting headers messages.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 12, 2016

Member

Discussed with @gmaxwell on IRC yesterday. He pointed out one drawback to this approach is that if we were to receive 2 blocks in short succession, such that the first block was still not valid according to the 2hr rule when the second one was received, then we would still fall out of sync.

So I'm going to push an alternate approach, where instead of just trying once to recover, we do the following: when receiving a HEADERS message with at most a few headers, if the first header doesn't connect, increment a counter and send the peer a getheaders. If the counter gets too big, assign DoS points, but if a connecting header comes in before that happens, reset the counter to 0.

@theuni Since DoS points don't ever decay back to 0, I was reluctant to assign DoS points at all when this type of thing happens, because otherwise if you were up and running long enough, you'd eventually disconnect your peers as these situations accumulated points over time.

One difference between this and the CMPCTBLOCK handling is that we have to worry about potential infinite looping if our peer is broken, since we're sending a getheaders in response to a headers message.

Member

sdaftuar commented Jul 12, 2016

Discussed with @gmaxwell on IRC yesterday. He pointed out one drawback to this approach is that if we were to receive 2 blocks in short succession, such that the first block was still not valid according to the 2hr rule when the second one was received, then we would still fall out of sync.

So I'm going to push an alternate approach, where instead of just trying once to recover, we do the following: when receiving a HEADERS message with at most a few headers, if the first header doesn't connect, increment a counter and send the peer a getheaders. If the counter gets too big, assign DoS points, but if a connecting header comes in before that happens, reset the counter to 0.

@theuni Since DoS points don't ever decay back to 0, I was reluctant to assign DoS points at all when this type of thing happens, because otherwise if you were up and running long enough, you'd eventually disconnect your peers as these situations accumulated points over time.

One difference between this and the CMPCTBLOCK handling is that we have to worry about potential infinite looping if our peer is broken, since we're sending a getheaders in response to a headers message.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 12, 2016

Member

Updated with new approach. @gmaxwell How does this look?

Member

sdaftuar commented Jul 12, 2016

Updated with new approach. @gmaxwell How does this look?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 13, 2016

Member

This looks reasonable to me. I am testing it.

Member

gmaxwell commented Jul 13, 2016

This looks reasonable to me. I am testing it.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 14, 2016

Member

re-utACk

Member

sipa commented Jul 14, 2016

re-utACk

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 14, 2016

Member

ACK

Member

gmaxwell commented Jul 14, 2016

ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 18, 2016

Member

utACK e91cf4b

Member

laanwj commented Jul 18, 2016

utACK e91cf4b

@laanwj laanwj merged commit e91cf4b into bitcoin:master Jul 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 18, 2016

Merge #8305: Improve handling of unconnecting headers
e91cf4b Add test for handling of unconnecting headers (Suhas Daftuar)
96fa953 Improve handling of unconnecting headers (Suhas Daftuar)
@@ -5773,6 +5776,35 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
return true;
}
CNodeState *nodestate = State(pfrom->GetId());

This comment has been minimized.

@sipa

sipa Aug 25, 2016

Member

Can you please stop ask basic programming questions? You can learn these things online, or on various other forums (including stackoverflow or chat channels) that don't require the developers of the project to waste their time teaching you.

You're free to ask for rationale behind design decisions, but this is getting unreasonable.

One last time: it's generally good style to use accessors instead of field variables directly. No, we don't do that everywhere yet. No, that doesn't mean we should continue that practice. No, that also doesn't mean we'll always complain when someone access the field directly.

@sipa

sipa Aug 25, 2016

Member

Can you please stop ask basic programming questions? You can learn these things online, or on various other forums (including stackoverflow or chat channels) that don't require the developers of the project to waste their time teaching you.

You're free to ask for rationale behind design decisions, but this is getting unreasonable.

One last time: it's generally good style to use accessors instead of field variables directly. No, we don't do that everywhere yet. No, that doesn't mean we should continue that practice. No, that also doesn't mean we'll always complain when someone access the field directly.

rebroad pushed a commit to rebroad/bitcoin that referenced this pull request Dec 7, 2016

Improve handling of unconnecting headers
When processing a headers message that looks like a block announcement,
send peer a getheaders if the headers message won't connect.

Apply DoS points after too many consecutive unconnecting headers
messages.

This solution is made by @sdaftuar in Bitcoin Core PR bitcoin#8305 (96fa953). It
is rewritten by me to fit into XT architecture.

I also added a unit test.

@ajtowns ajtowns referenced this pull request Jun 1, 2017

Closed

add a -bip148 option #10442

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge bitcoin#8305: Improve handling of unconnecting headers
e91cf4b Add test for handling of unconnecting headers (Suhas Daftuar)
96fa953 Improve handling of unconnecting headers (Suhas Daftuar)

codablock added a commit to codablock/dash that referenced this pull request Dec 27, 2017

Merge bitcoin#8305: Improve handling of unconnecting headers
e91cf4b Add test for handling of unconnecting headers (Suhas Daftuar)
96fa953 Improve handling of unconnecting headers (Suhas Daftuar)

codablock added a commit to codablock/dash that referenced this pull request Dec 28, 2017

Merge bitcoin#8305: Improve handling of unconnecting headers
e91cf4b Add test for handling of unconnecting headers (Suhas Daftuar)
96fa953 Improve handling of unconnecting headers (Suhas Daftuar)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment