Relay alerts prior to MIN_PEER_PROTO_VERSION disconnect #4929

pull wtogami wants to merge 1 commits into bitcoin:master from wtogami:2014_9_oldpeer_after_alert changing 1 files +9 −5

wtogami commented at 8:33 AM on September 16, 2014: contributor

Otherwise the old client may fail to receive an alert saying they need to upgrade.

in src/main.cpp:None in f137e32f90 outdated

3259 | @@ -3260,6 +3260,14 @@ string GetWarnings(string strFor)
3260 |      return "error";
3261 |  }
3262 |  
3263 | +void static RelayAlerts(CNode* pfrom)
3264 | +{
3265 | +    {

laanwj commented at 9:45 AM on September 16, 2014:

No need for this extra level of indentation / braces

laanwj commented at 9:45 AM on September 16, 2014: member

Seems like a sane thing to do. utACK.

Relay alerts prior to MIN_PEER_PROTO_VERSION disconnect

Otherwise the old client may fail to receive an alert saying they need to upgrade.

490c1c084b

wtogami force-pushed on Sep 16, 2014
wtogami commented at 10:05 AM on September 16, 2014: contributor

Got rid of the indentation.
BitcoinPullTester commented at 10:19 AM on September 16, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4929_490c1c084ba04d1e4228d225dcd980e76ed03221/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
jgarzik commented at 12:49 PM on September 16, 2014: contributor
1. The stated logic makes no sense. They immediately receive a reject message indicating an upgrade is needed.
2. MIN_PEER_PROTO_VERSION is for ancient nodes we simply refuse to talk to. Adding code for a test that will in practice never be executed is a waste.
laanwj commented at 1:57 PM on September 16, 2014: member

Right, we already send a reject before disconnecting, I forgot about that.
wtogami commented at 5:11 PM on September 16, 2014: contributor

"They immediately receive a reject message indicating an upgrade is needed."

Eh? The reject messages were not part of the protocol until 0.9.
jgarzik commented at 5:14 PM on September 16, 2014: contributor

If a node has $this commit, then you also have a bitcoind that sends reject messages.
wtogami commented at 5:28 PM on September 16, 2014: contributor

My point is older clients don't actually do anything with the reject message.
jgarzik commented at 5:37 PM on September 16, 2014: contributor

Older clients do not do anything with the alert message either.
wtogami commented at 5:38 PM on September 16, 2014: contributor

Is this patch actually harmful? No.

I'd like to also submit a patch to allow the user to configure MIN_PEER_PROTO_VERSION. Doing so lets you opt-out of talking to old, broken clients which in my experience cuts log spam.
jgarzik commented at 5:42 PM on September 16, 2014: contributor

No it's just redundant.
wtogami closed this on Sep 16, 2014
laanwj commented at 9:46 AM on September 18, 2014: member

@wtogami It's just that you have a misunderstanding what MIN_PEER_PROTO_VERSION is for. I've already explained that to you on IRC.
MarcoFalke locked this on Sep 8, 2021

Contributors