Enforce expected outbound services #7749

sipa commented at 8:18 PM on March 26, 2016: member

At this point, we do not:

check that relayed/stored addr messages have the NODE_NETWORK bit set.
check that nodes selected for outbound connection have NODE_NETWORK advertized.
check that the nServices flag in the "version" message by a node corresponds to what we expected when deciding to connect to it.
store the updated nServices flag from the "version" message in addrman.

Fix this.

jonasschnelli added the label P2P on Mar 27, 2016

laanwj commented at 10:15 AM on March 28, 2016: member

Travis fail was unrelated (#7463), retriggering.

petertodd commented at 12:30 PM on March 30, 2016: contributor

utACK

jonasschnelli commented at 12:57 PM on March 30, 2016: contributor

utACK d4b854dccfd6afef4a1daf056b15d4f3f0d52eaa

paveljanik commented at 1:22 PM on March 30, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/pull/7749/commits/d4b854dccfd6afef4a1daf056b15d4f3f0d52eaa

btctest addnode 188.226.202.220 onetry producing:

2016-03-30 13:17:37 trying connection 188.226.202.220 lastseen=0.0hrs
2016-03-30 13:17:37 Added connection to 188.226.202.220 peer=12
2016-03-30 13:17:37 send version message: version 70013, blocks=754598, us=0.0.0.0:18333, them=188.226.202.220:18333, peer=12
2016-03-30 13:17:37 sending: version (103 bytes) peer=12
2016-03-30 13:17:38 received: version (102 bytes) peer=12
2016-03-30 13:17:38 peer=12 does not offer the expected services (00000004 offered, 00000001 expected); disconnecting
2016-03-30 13:17:38 sending: reject (45 bytes) peer=12
2016-03-30 13:17:38 sending: verack (0 bytes) peer=12
2016-03-30 13:17:38 sending: getaddr (0 bytes) peer=12

Minor nit: remove 'the' before 'expected services' (I'm not native English speaker though)?

gmaxwell commented at 4:17 PM on March 30, 2016: contributor

utACK. @paveljanik "the" is appropriate there.

in src/main.cpp:None in d4b854dccf outdated

4512 | +        if (pfrom->nServicesExpected & ~pfrom->nServices)
4513 | +        {
4514 | +            LogPrint("net", "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom->id, pfrom->nServices, pfrom->nServicesExpected);
4515 | +            pfrom->PushMessage(NetMsgType::REJECT, strCommand, REJECT_NONSTANDARD,
4516 | +                               strprintf("Expected to offer services %08x", pfrom->nServicesExpected));
4517 | +            pfrom->fDisconnect = true;

kazcw commented at 4:42 AM on April 16, 2016:

return here?

sipa commented at 9:50 PM on June 12, 2016:

Fixed.

sdaftuar commented at 2:58 PM on April 22, 2016: member

Is the behavior that @paveljanik observed with his test the intended behavior? I would have guessed that we'd want addnode (or connect) to bypass the expected services requirement.

sipa commented at 7:19 AM on May 17, 2016: member

Mental note: we should do an exact assignment (rather than or'ing) when directly connected to a peer and it tells us its nVersion.

sipa force-pushed on May 25, 2016

sipa commented at 3:26 PM on May 25, 2016: member

Rebased, and fixed the behaviour @paveljanik saw.

sipa force-pushed on May 25, 2016

in src/protocol.h:None in 68146f6217 outdated

 250 | @@ -251,7 +251,7 @@ class CAddress : public CService
 251 |  {
 252 |  public:
 253 |      CAddress();
 254 | -    explicit CAddress(CService ipIn, uint64_t nServicesIn = NODE_NETWORK);
 255 | +    explicit CAddress(CService ipIn, uint64_t nServicesIn);

MarcoFalke commented at 1:17 PM on June 5, 2016:

@sipa Have you made sure the test are not using the default?

I get a failure in test/addrman_tests.cpp:71

test/addrman_tests.cpp: In member function ‘void addrman_tests::addrman_simple::test_method()’:
test/addrman_tests.cpp:71:31: error: no matching function for call to ‘CAddress::CAddress(CService&)’
     addrman.Add(CAddress(addr1), source);
                               ^

sipa commented at 9:41 AM on June 6, 2016:

Fixed!

sipa force-pushed on Jun 6, 2016

sipa force-pushed on Jun 8, 2016

sipa commented at 12:12 PM on June 8, 2016: member

Rebased.

sipa force-pushed on Jun 8, 2016

sipa commented at 4:19 PM on June 8, 2016: member

Rebased on top of #8083, and making it use this PR's nRelevantServices as the nRequiredServices value requested from filtering seeds.

theuni commented at 4:40 PM on June 8, 2016: member

Nit: For readability, can we get a constant NODE_SERVICES_NONE or so (i'm at a loss for a good name) rather than passing 0 everywhere?

sipa commented at 5:14 PM on June 8, 2016: member

For discussion: I've instead switched the NODE_* flags to an enum.

theuni commented at 5:34 PM on June 8, 2016: member

ahhh, much better. Thanks.

laanwj commented at 5:44 AM on June 9, 2016: member

For discussion: I've instead switched the NODE_* flags to an enum.

ACK

sipa force-pushed on Jun 12, 2016

sipa commented at 9:51 PM on June 12, 2016: member

Rebased and addressed @kazcw 's comment.

in src/main.cpp:None in d2acb5164b outdated

4784 | @@ -4785,6 +4785,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
4785 |          {
4786 |              boost::this_thread::interruption_point();
4787 |  
4788 | +            if (!(addr.nServices & NODE_NETWORK))

laanwj commented at 1:09 PM on June 13, 2016:

Maybe make the bitmask of required services a constant

in src/net.cpp:None in d2acb5164b outdated

1595 | @@ -1596,6 +1596,10 @@ void ThreadOpenConnections()
1596 |              if (IsLimited(addr))
1597 |                  continue;
1598 |  
1599 | +            // only connect to full nodes
1600 | +            if (!(addr.nServices & NODE_NETWORK))

laanwj commented at 1:13 PM on June 13, 2016:

Or wait, shouldn't this be nRelevantServices? Though I'm not sure "relevantservices" is a good name if we only consider nodes with those service bits.

sipa commented at 1:17 PM on June 13, 2016:

There may be relevant services that are not required, but still preferred. I'll add a constant for required services.

sipa commented at 2:01 PM on June 13, 2016:

Done.

sipa force-pushed on Jun 13, 2016

Keep addrman's nService bits consistent with outbound observations 3764dec36c

Verify that outbound connections have expected services fc83f18153

Only store and connect to NODE_NETWORK nodes 5e7ab16d29

Don't require services in -addnode 15bf863219

Introduce enum ServiceFlags for service flags ee06e04369

Introduce REQUIRED_SERVICES constant ecd7fd37c8

sipa force-pushed on Jun 13, 2016

gmaxwell commented at 5:25 PM on June 13, 2016: contributor

ACK

laanwj commented at 5:29 PM on June 13, 2016: member

utACK ecd7fd3

laanwj merged this on Jun 13, 2016

laanwj closed this on Jun 13, 2016

laanwj referenced this in commit be9711e597 on Jun 13, 2016

in src/main.cpp:None in ecd7fd37c8

4617 | +        pfrom->nServices = ServiceFlags(nServiceInt);
4618 | +        if (!pfrom->fInbound)
4619 | +        {
4620 | +            addrman.SetServices(pfrom->addr, pfrom->nServices);
4621 | +        }
4622 | +        if (pfrom->nServicesExpected & ~pfrom->nServices)

rebroad commented at 6:43 AM on August 24, 2016:

Is disconnection really the right solution here? For example, let's say we were expecting NODE_NETWORK yet we connect and now find the node is a pruned node. This wouldn't be an issue if we're already caught up with the blockchain, would it?

rebroad commented at 7:43 AM on September 1, 2016:

Ok, I think I have found a problem with this line. Shouldn't it instead read:-

if (nRelevantServices & ~pfrom->nServices)

?

i.e. the ExpectedServices are irrelevant (which are ANDed with nRelevantServices anyway). E.g. if the relevant services are NODE_NETWORK (as they usually are) and you connect to a node which HAS NODE_NETWORK but it wasn't expected to have NODE_NETWORK, then why would you still want it to disconnect? Afterall ServicesExpected is simply an old version of what the node used to advertise. nServices is the up-to-date information so why refer to the out-of-date information when you have the up-to-date information?

in src/net.cpp:None in ecd7fd37c8

1594 | @@ -1592,6 +1595,10 @@ void ThreadOpenConnections()
1595 |              if (IsLimited(addr))
1596 |                  continue;
1597 |  
1598 | +            // only connect to full nodes

rebroad commented at 6:56 AM on August 24, 2016:

Why only connect to full nodes? Surely this is only a requirement when we are more than 550 blocks behind on the blockchain. Also, with this change, it is possible that a node that was previously pruning becomes a full-node, and it will never be rediscovered again as a full-node for as long as it's IP address remains the same.

sipa commented at 7:04 AM on August 24, 2016:

Why only connect to full nodes? Surely this is only a requirement when we are more than 550 blocks behind on the blockchain.

No, we always want this requirement.

Otherwise we may end up trying to connect to lightweight clients that do not even relay blocks at all.

sipa commented at 7:49 AM on September 1, 2016: member

No, there can be relevant services that a peer does not advertize, and we decide to connect to them anyway. We shouldn't disconnect them later just because they happen to not have that service listed.

I'm tired of explaining this over and over again. Relevant does not mean required; it means relevant. You're welcome to add comments with explanations about the meaning of variables and logic, but please don't try to change code you don't understand.

in src/main.cpp:None in ecd7fd37c8

4623 | +        {
4624 | +            LogPrint("net", "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom->id, pfrom->nServices, pfrom->nServicesExpected);
4625 | +            pfrom->PushMessage(NetMsgType::REJECT, strCommand, REJECT_NONSTANDARD,
4626 | +                               strprintf("Expected to offer services %08x", pfrom->nServicesExpected));
4627 | +            pfrom->fDisconnect = true;
4628 | +            return false;

rebroad commented at 1:25 AM on October 16, 2016:

Why return false rather than true? False just causes an extra error message to be displayed in debug.log (saying ProcessMessage failed).

random-zebra referenced this in commit f295ee4e4e on May 13, 2020

DrahtBot locked this on Sep 8, 2021