net: don't send feefilter messages before the version handshake is complete #9117

Merged
merged 1 commit into from Nov 21, 2016

Projects

None yet

6 participants

@theuni
Member
theuni commented Nov 9, 2016 edited

Caused by changes in #8708. Same fix as #8862.

This is a quick fix, I'll PR a small cleanup of the disconnect logic so that these cases don't continue to crop up.

Edit: Forgot to mention, this currently asserts due to #8708, which was intended to point out issues like this one. So this should be considered a crash-fix.

@MarcoFalke
Member

utACK 4662553

@MarcoFalke MarcoFalke added the P2P label Nov 9, 2016
@sipa
Member
sipa commented Nov 9, 2016

What does !pto->fDisconnect have to do with the version handshake?

@theuni
Member
theuni commented Nov 9, 2016

@sipa in this case, the handshake started, but stopped halfway-through and set fDisconnected because the node didn't have the services expected. I suppose I should've ignored that and just called this "don't send feefilter messages if fDisconnect has been set".

For a more correct/complete (but untested) change, see here: https://github.com/theuni/bitcoin/commits/feefilter-assert2 .

@jonasschnelli
Member

utACK 4662553

@rebroad
Contributor
rebroad commented Nov 11, 2016

I still don't understand why we don't simply put the fDisconnect check along with the check for nVersion != 0 then all of these little issues would not exist. The only messages we need to send once fDisconnect is set are messages directly relating to the disconnection, e.g. the "bye" message @laanwj proposed a while back.

@theuni
Member
theuni commented Nov 11, 2016

@rebroad #9128 contains the complete fix to this, similar to what you're suggesting.

I'll leave this open for now in case the quick fix is desired to avoid the crash while the bigger PR is in review.

@rebroad
Contributor
rebroad commented Nov 11, 2016

@theuni oh! you got there before me! Well, #9129 contains just the fix we're talking about, so might need less review...

@theuni
Member
theuni commented Nov 11, 2016

@rebroad That only stops processing if fDisconnect is already set when SendMessages is called. But with your changes, if it's set part-way through SendMessages (as it is a few times), messages further down will now be allowed out!

@laanwj laanwj merged commit 4662553 into bitcoin:master Nov 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Nov 21, 2016
@laanwj laanwj Merge #9117: net: don't send feefilter messages before the version ha…
…ndshake is complete


4662553 net: don't send feefilter messages before the version handshake is complete (Cory Fields)
c390640
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 2, 2016
@theuni @luke-jr theuni + luke-jr net: don't send feefilter messages before the version handshake is co…
…mplete

Github-Pull: #9117
Rebased-From: 4662553
6fe3981
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment