Enforce expected outbound services #7749

sipa · 2016-03-26T20:18:49Z

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.

laanwj · 2016-03-28T10:15:39Z

Travis fail was unrelated (#7463), retriggering.

petertodd · 2016-03-30T12:30:40Z

utACK

jonasschnelli · 2016-03-30T12:57:33Z

utACK d4b854d

paveljanik · 2016-03-30T13:22:16Z

ACK d4b854d

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 · 2016-03-30T16:17:06Z

utACK. @paveljanik "the" is appropriate there.

kazcw · 2016-04-16T04:42:46Z

src/main.cpp

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


return here?

sdaftuar · 2016-04-22T14:58:45Z

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 · 2016-05-17T07:19:33Z

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 · 2016-05-25T15:26:30Z

Rebased, and fixed the behaviour @paveljanik saw.

maflcko · 2016-06-05T13:17:02Z

src/protocol.h

@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 · 2016-06-08T12:12:41Z

Rebased.

sipa · 2016-06-08T16:19:22Z

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

theuni · 2016-06-08T16:40:51Z

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 · 2016-06-08T17:14:26Z

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

theuni · 2016-06-08T17:34:07Z

ahhh, much better. Thanks.

laanwj · 2016-06-09T05:44:54Z

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

ACK

sipa · 2016-06-12T21:51:20Z

Rebased and addressed @kazcw 's comment.

laanwj · 2016-06-13T13:09:10Z

src/main.cpp

Maybe make the bitmask of required services a constant

gmaxwell · 2016-06-13T17:25:46Z

ACK

laanwj · 2016-06-13T17:29:04Z

utACK ecd7fd3

ecd7fd3 Introduce REQUIRED_SERVICES constant (Pieter Wuille) ee06e04 Introduce enum ServiceFlags for service flags (Pieter Wuille) 15bf863 Don't require services in -addnode (Pieter Wuille) 5e7ab16 Only store and connect to NODE_NETWORK nodes (Pieter Wuille) fc83f18 Verify that outbound connections have expected services (Pieter Wuille) 3764dec Keep addrman's nService bits consistent with outbound observations (Pieter Wuille)

rebroad · 2016-08-24T06:43:57Z

src/main.cpp

+        {
+            addrman.SetServices(pfrom->addr, pfrom->nServices);
+        }
+        if (pfrom->nServicesExpected & ~pfrom->nServices)


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?

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?

rebroad · 2016-08-24T06:56:55Z

src/net.cpp

            if (IsLimited(addr))
                continue;

+            // only connect to full nodes


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.

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 · 2016-09-01T07:49:05Z

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.

rebroad · 2016-10-16T01:25:04Z

src/main.cpp

+            pfrom->PushMessage(NetMsgType::REJECT, strCommand, REJECT_NONSTANDARD,
+                               strprintf("Expected to offer services %08x", pfrom->nServicesExpected));
+            pfrom->fDisconnect = true;
+            return false;


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

a48639f Introduce REQUIRED_SERVICES constant (Pieter Wuille) d1391e3 Introduce enum ServiceFlags for service flags (Pieter Wuille) b6a0455 Don't require services in -addnode (Pieter Wuille) 6dba254 Only store and connect to NODE_NETWORK nodes (Pieter Wuille) 1905a6a Verify that outbound connections have expected services (Pieter Wuille) 0c23c01 Keep addrman's nService bits consistent with outbound observations (Pieter Wuille) Pull request description: Backport of bitcoin#7749. This is next in line for #1374. Original description: > 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. ACKs for top commit: furszy: one-one with 7749 with the only exception of the tier two area, looks good, utACK a48639f random-zebra: All good. ACK a48639f and merging... Tree-SHA512: 82f99df4244504cfe6fbc490126485da40d912b9b06390cb91a924426bded550bb939d352a17ba2c5142b4dae4099699b8e5777b7cb49f316342ebea4f9ec71d

jonasschnelli added the P2P label Mar 27, 2016

kazcw reviewed Apr 16, 2016
View reviewed changes

sipa mentioned this pull request Apr 19, 2016

Segregated witness #7910

Closed

7 tasks

sipa force-pushed the checkservices branch from d4b854d to 28d8b89 Compare May 25, 2016 15:22

sipa force-pushed the checkservices branch from 28d8b89 to 68146f6 Compare May 25, 2016 15:28

maflcko reviewed Jun 5, 2016
View reviewed changes

sipa force-pushed the checkservices branch 2 times, most recently from 0984386 to c1d79e6 Compare June 8, 2016 12:11

sipa force-pushed the checkservices branch from c1d79e6 to f3bcd75 Compare June 8, 2016 16:15

sipa force-pushed the checkservices branch from 1ebb48d to df318fa Compare June 12, 2016 21:50

laanwj reviewed Jun 13, 2016
View reviewed changes

src/main.cpp Outdated

Copy link

Member

laanwj Jun 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make the bitmask of required services a constant

sipa force-pushed the checkservices branch from 36891cd to c0b268a Compare June 13, 2016 14:50

sipa added 2 commits June 13, 2016 17:40

Keep addrman's nService bits consistent with outbound observations

3764dec

Verify that outbound connections have expected services

fc83f18

sipa added 4 commits June 13, 2016 17:40

Only store and connect to NODE_NETWORK nodes

5e7ab16

Don't require services in -addnode

15bf863

Introduce enum ServiceFlags for service flags

ee06e04

Introduce REQUIRED_SERVICES constant

ecd7fd3

sipa force-pushed the checkservices branch from c0b268a to ecd7fd3 Compare June 13, 2016 15:40

laanwj merged commit ecd7fd3 into bitcoin:master Jun 13, 2016

sipa mentioned this pull request Jun 15, 2016

Do not set extra flags for unfiltered DNS seed results #8208

Merged

rebroad reviewed Aug 24, 2016
View reviewed changes

rebroad mentioned this pull request Aug 24, 2016

[WIP] Disconnect when a node doesn't offer the relevant services. #8571

Closed

rebroad reviewed Aug 24, 2016
View reviewed changes

rebroad reviewed Oct 16, 2016

View reviewed changes

kyuupichan mentioned this pull request May 7, 2017

[Port] Introduce ServiceFlags enum BitcoinUnlimited/BitcoinUnlimited#536

Closed

This was referenced May 20, 2017

Net 7749 1 BitcoinUnlimited/BitcoinUnlimited#610

Closed

[WIP][port] Network services-related changes BitcoinUnlimited/BitcoinUnlimited#611

Closed

OlegGirko mentioned this pull request Jul 5, 2017

Backport Bitcoin PR#7749: Enforce expected outbound services dashpay/dash#1508

Merged

Fuzzbawls mentioned this pull request Mar 4, 2020

Tracking: Backport upstream network encapsulation PIVX-Project/PIVX#1374

Closed

21 tasks

Fuzzbawls mentioned this pull request May 11, 2020

[P2P] Enforce expected outbound services PIVX-Project/PIVX#1616

Merged

PRCYDev mentioned this pull request Jan 25, 2021

Tracking: Backport upstream network encapsulation PRCYCoin/PRCYCoin#4

Open

21 tasks

nuttycom mentioned this pull request Feb 23, 2021

Verify that expected services are available from peer nodes & disconnect otherwise. zcash/zcash#5004

Draft

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Enforce expected outbound services #7749

Enforce expected outbound services #7749

Uh oh!

Conversation

sipa commented Mar 26, 2016

Uh oh!

laanwj commented Mar 28, 2016

Uh oh!

petertodd commented Mar 30, 2016

Uh oh!

jonasschnelli commented Mar 30, 2016

Uh oh!

paveljanik commented Mar 30, 2016

Uh oh!

gmaxwell commented Mar 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Apr 22, 2016

Uh oh!

sipa commented May 17, 2016

Uh oh!

sipa commented May 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 8, 2016

Uh oh!

sipa commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jun 8, 2016

Uh oh!

sipa commented Jun 8, 2016

Uh oh!

theuni commented Jun 8, 2016

Uh oh!

laanwj commented Jun 9, 2016

Uh oh!

sipa commented Jun 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Jun 13, 2016

Uh oh!

laanwj commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebroad Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

sipa commented Jun 8, 2016 •

edited

Loading

rebroad Aug 24, 2016 •

edited

Loading