New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce expected outbound services #7749

Merged
merged 6 commits into from Jun 13, 2016

Conversation

Projects
None yet
@sipa
Member

sipa commented Mar 26, 2016

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 jonasschnelli added the P2P label Mar 27, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 28, 2016

Member

Travis fail was unrelated (#7463), retriggering.

Member

laanwj commented Mar 28, 2016

Travis fail was unrelated (#7463), retriggering.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Mar 30, 2016

Contributor

utACK

Contributor

petertodd commented Mar 30, 2016

utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Mar 30, 2016

utACK d4b854d

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 30, 2016

Contributor

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)?

Contributor

paveljanik commented Mar 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 30, 2016

Member

utACK. @paveljanik "the" is appropriate there.

Member

gmaxwell commented Mar 30, 2016

utACK. @paveljanik "the" is appropriate there.

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;

This comment has been minimized.

@kazcw

kazcw Apr 16, 2016

Contributor

return here?

@kazcw

kazcw Apr 16, 2016

Contributor

return here?

This comment has been minimized.

@sipa

sipa Jun 12, 2016

Member

Fixed.

@sipa

sipa Jun 12, 2016

Member

Fixed.

@sipa sipa referenced this pull request Apr 19, 2016

Closed

Segregated witness #7910

5 of 7 tasks complete
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Apr 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.

Member

sdaftuar commented Apr 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa 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.

Member

sipa commented May 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 25, 2016

Member

Rebased, and fixed the behaviour @paveljanik saw.

Member

sipa commented May 25, 2016

Rebased, and fixed the behaviour @paveljanik saw.

@MarcoFalke

View changes

Show outdated Hide outdated src/protocol.h
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 8, 2016

Member

Rebased.

Member

sipa commented Jun 8, 2016

Rebased.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 8, 2016

Member

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

Member

sipa commented Jun 8, 2016

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

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 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?

Member

theuni commented Jun 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 8, 2016

Member

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

Member

sipa commented Jun 8, 2016

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

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 8, 2016

Member

ahhh, much better. Thanks.

Member

theuni commented Jun 8, 2016

ahhh, much better. Thanks.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 9, 2016

Member

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

ACK

Member

laanwj commented Jun 9, 2016

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

ACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 12, 2016

Member

Rebased and addressed @kazcw 's comment.

Member

sipa commented Jun 12, 2016

Rebased and addressed @kazcw 's comment.

@laanwj

View changes

Show outdated Hide outdated src/main.cpp
@laanwj

View changes

Show outdated Hide outdated src/net.cpp
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 13, 2016

Member

ACK

Member

gmaxwell commented Jun 13, 2016

ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 13, 2016

Member

utACK ecd7fd3

Member

laanwj commented Jun 13, 2016

utACK ecd7fd3

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 13, 2016

Merge #7749: Enforce expected outbound services
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)
{
addrman.SetServices(pfrom->addr, pfrom->nServices);
}
if (pfrom->nServicesExpected & ~pfrom->nServices)

This comment has been minimized.

@rebroad

rebroad Aug 24, 2016

Contributor

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

rebroad Aug 24, 2016

Contributor

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?

This comment has been minimized.

@rebroad

rebroad Sep 1, 2016

Contributor

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

rebroad Sep 1, 2016

Contributor

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?

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

This comment has been minimized.

@rebroad

rebroad Aug 24, 2016

Contributor

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.

@rebroad

rebroad Aug 24, 2016

Contributor

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.

This comment has been minimized.

@sipa

sipa Aug 24, 2016

Member

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

sipa Aug 24, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 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.

Member

sipa commented Sep 1, 2016

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.

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

This comment has been minimized.

@rebroad

rebroad Oct 16, 2016

Contributor

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

@rebroad

rebroad Oct 16, 2016

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment