Improve handling of unconnecting headers #8305
Conversation
|
Needs rebase after #8306 |
sdaftuar
force-pushed
the
fix-relay-2hr-rule
branch
from
89d7079 to
ecfaf95
Compare
|
Rebased. |
src/main.cpp
Outdated
There was a problem hiding this comment.
Print a log message here?
|
@sdaftuar can you split this into two commits, one with changes and a second with tests? |
sdaftuar
force-pushed
the
fix-relay-2hr-rule
branch
from
ecfaf95 to
f5fd88c
Compare
|
Added logging, and split into two commits. |
|
@sdaftuar thank you |
|
ACK |
|
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. |
|
ACK |
|
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. |
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.
|
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
force-pushed
the
fix-relay-2hr-rule
branch
from
f5fd88c to
e91cf4b
Compare
|
Updated with new approach. @gmaxwell How does this look? |
|
This looks reasonable to me. I am testing it. |
|
re-utACk |
|
ACK |
|
utACK e91cf4b |
| return true; | ||
| } | ||
|
|
||
| CNodeState *nodestate = State(pfrom->GetId()); |
There was a problem hiding this comment.
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.
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.
8 participants
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