Assertion failed: (nSendVersion != 0), function GetSendVersion, file ./net.h, line 775. #9212

1. 
   paveljanik commented at 1:40 pm on November 23, 2016: contributor

   Current master on testnet:

   02016-11-23 13:28:25 AcceptToMemoryPool: peer=1: accepted facb01dba78b58bcd4a2edd4edecf71816075e1ae5e2e66f886e82a7798ccac5 (poolsz 55 txn, 183 kB)
   12016-11-23 13:28:25 received: version (111 bytes) peer=2
   22016-11-23 13:28:25 peer=2 does not offer the expected services (00000001 offered, 00000009 expected); disconnecting
   32016-11-23 13:28:25 sending reject (45 bytes) peer=2
   42016-11-23 13:28:25 ProcessMessages(version, 111 bytes) FAILED peer=2
   52016-11-23 13:28:25 SendMessages: sending inv peer=2 hash=00000000000019d53bb75ddae964e7cec1d39ef6f01a41b90bca4de869438298
   6Assertion failed: (nSendVersion != 0), function GetSendVersion, file ./net.h, line 775.
   7Abort trap: 6
   

   Can’t reproduce even with the same peer. @theuni Any idea?

2. 
   paveljanik commented at 1:44 pm on November 23, 2016: contributor

   Are we really sending inv to the peer which failed VERSION processing? Is this another instance fixed by the first commit in #9128?
3. 
   theuni commented at 5:04 pm on November 23, 2016: member

   @paveljanik looks that way :( But that’s why the assertion was added, to obviate what we might’ve been leaking for a while.

   This should be fixed by #9128, but we should still track down the individual case so that we can backport a simpler fix.

4. fanquake added the label Bug on Nov 24, 2016
5. 
   fanquake commented at 10:09 am on November 24, 2016: member

   I’ve just seen this , however running master on main-net.

   02016-11-24 10:02:35 UpdateTip: new best=000000000000000000b3bc203b1183b19e19c4552eefa4fa3b582b7773e0a1c5 height=440334 version=0x20000000 log2_work=85.584251 tx=173289265 date='2016-11-24 10:02:29' progress=1.000000 cache=742.7MiB(784785tx) warning='1 of last 100 blocks have unexpected version'
   12016-11-24 10:02:53 receive version message: /Satoshi:0.9.2.1/: version 70002, blocks=440335, us=203.59.204.161:8333, peer=23
   22016-11-24 10:02:53 UpdateTip: new best=000000000000000000a42b9772a3801d7d001918308de0d4807fe193534e6fe6 height=440335 version=0x20000000 log2_work=85.584282 tx=173291724 date='2016-11-24 10:02:34' progress=1.000000 cache=743.8MiB(787074tx) warning='1 of last 100 blocks have unexpected version'
   32016-11-24 10:02:54 ProcessMessages(version, 102 bytes) FAILED peer=22
   4Assertion failed: (nSendVersion != 0), function GetSendVersion, file ./net.h, line 775.
   5Abort trap: 6
   

6. 
   MarcoFalke commented at 12:03 pm on November 26, 2016: member

   Anything left to do here after #9128? I know we want to backport those fixes such as #9117, but I couldn’t find the commit for backport for this issue report.
7. MarcoFalke added the label P2P on Nov 26, 2016
8. 
   laanwj commented at 4:42 pm on December 14, 2016: member

   This is still happening with master, on mainnet, as of last week (with #9128 merged), or came back with one of the pulls I was testing, see #9348.

   Nit: we should avoid using assert() for error handling in network code. We should just log a message in these cases and boot the peer. It’s only a small step from a bug/oversight to a P2P-triggerable DoS.

9. 
   TheBlueMatt commented at 10:21 pm on December 14, 2016: member

   In my race-detection testing I had a fix for this (not sure if it was for a detected race, another bug, or actually seeing this assert, I dont recall now), which was to not deserialize the version message directly into pfrom->nVersion, but deserialize into a temp and set pfrom->nVersion only after pfrom->SetSendVersion. I’d bet pretty heavily on the use of nVersion != 0 as “are we connected” prior to having set send version is what triggers this.
10. TheBlueMatt referenced this in commit 1434795382 on Dec 15, 2016
11. TheBlueMatt referenced this in commit 741dcaa1e9 on Dec 15, 2016
12. 
    laanwj commented at 6:56 am on December 15, 2016: member

    Indeed, it must be a race. I checked my traceback and it checks for pto->nVersion the line before calling GetSendVersion which asserts out because the nVersion is 0.

    0        // Don't send anything until we get its version message
    1        if (pto->nVersion == 0 || pto->fDisconnect)
    2            return true;
    3
    4        // If we get here, the outgoing message serialization version is set and can't change.
    5        CNetMsgMaker msgMaker(pto->GetSendVersion());
    

13. TheBlueMatt referenced this in commit 22ddf7da9c on Dec 16, 2016
14. TheBlueMatt referenced this in commit 37ebc507d8 on Dec 17, 2016
15. laanwj added this to the milestone 0.14.0 on Dec 20, 2016
16. TheBlueMatt referenced this in commit 8e2c2cf418 on Dec 21, 2016
17. theuni referenced this in commit 2edbf038a0 on Jan 18, 2017
18. theuni referenced this in commit 438230ff27 on Jan 20, 2017
19. theuni referenced this in commit 80ff0344ae on Feb 2, 2017
20. laanwj closed this on Feb 4, 2017

    ---

21. laanwj referenced this in commit 496691741d on Feb 4, 2017
22. UdjinM6 referenced this in commit b9c67258ba on Aug 17, 2017
23. HashUnlimited referenced this in commit d018508555 on Feb 27, 2018
24. lateminer referenced this in commit b055e5d90c on Jan 4, 2019
25. MarcoFalke locked this on Sep 8, 2021

Contributors
paveljanik theuni fanquake MarcoFalke laanwj TheBlueMatt

Labels
Bug P2P

Milestone
0.14.0