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

Net: Improve blocks only mode. #7046

Merged
merged 4 commits into from Nov 22, 2015

Conversation

Projects
None yet
7 participants
@pstratem
Contributor

pstratem commented Nov 17, 2015

  • Process inv messages in blocks only mode if we would relay the peers transactions
  • Bail early in tx message processing if we will neither AcceptToMemoryPool nor RelayTransaction
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 17, 2015

Member

Don't ignore for whitelisted peers?

Member

gmaxwell commented Nov 17, 2015

Don't ignore for whitelisted peers?

@pstratem pstratem changed the title from [WIP] Net: Ignore "tx" messages in blocks only mode. to [WIP] Net: Improve blocks only mode. Nov 17, 2015

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 18, 2015

Contributor

concept ACK, utACK

Contributor

dcousens commented Nov 18, 2015

concept ACK, utACK

@pstratem pstratem changed the title from [WIP] Net: Improve blocks only mode. to Net: Improve blocks only mode. Nov 18, 2015

@jonasschnelli jonasschnelli added the P2P label Nov 18, 2015

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Nov 18, 2015

Contributor

utACK

We should probably document/popularize blocks only mode, because it can be very useful for some usecases - thank you for it!

Contributor

paveljanik commented Nov 18, 2015

utACK

We should probably document/popularize blocks only mode, because it can be very useful for some usecases - thank you for it!

@tulip0

This comment has been minimized.

Show comment
Hide comment
@tulip0

tulip0 Nov 20, 2015

When documenting some care needs to be taken to ensure people don't enable this on their nodes thinking it will let them receive blocks faster, "not processing transaction messages will leave more time to do blocks quickly" isn't an unreasonable assumption to make. In actuality running with blocksonly has a massive negative impact on the speed of block verification by never filling ECDSA signature cache.

tulip0 commented Nov 20, 2015

When documenting some care needs to be taken to ensure people don't enable this on their nodes thinking it will let them receive blocks faster, "not processing transaction messages will leave more time to do blocks quickly" isn't an unreasonable assumption to make. In actuality running with blocksonly has a massive negative impact on the speed of block verification by never filling ECDSA signature cache.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 20, 2015

Member

The impact on mining is much of why I wasn't thinking to promote it yet. We also yet don't do smart things with peers that send no relay. E.g. a node that somehow ends up with 8 blocksonly peers won't do anything smart about it, and probably we'd like to do something about that before encouraging it's widespread use.

Member

gmaxwell commented Nov 20, 2015

The impact on mining is much of why I wasn't thinking to promote it yet. We also yet don't do smart things with peers that send no relay. E.g. a node that somehow ends up with 8 blocksonly peers won't do anything smart about it, and probably we'd like to do something about that before encouraging it's widespread use.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 20, 2015

Contributor

@gmaxwell wasn't this going undocumented (or at least, a huge "highly discouraged" warning) next to if it at all made it in to 0.12?

Contributor

dcousens commented Nov 20, 2015

@gmaxwell wasn't this going undocumented (or at least, a huge "highly discouraged" warning) next to if it at all made it in to 0.12?

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 20, 2015

Contributor

The help message is only displayed in debug more.
On Nov 20, 2015 8:48 AM, "Daniel Cousens" notifications@github.com wrote:

@gmaxwell https://github.com/gmaxwell wasn't this going undocumented
(or at least, a huge "highly discouraged" warning) next to if it at all
made it in to 0.12?


Reply to this email directly or view it on GitHub
#7046 (comment).

Contributor

pstratem commented Nov 20, 2015

The help message is only displayed in debug more.
On Nov 20, 2015 8:48 AM, "Daniel Cousens" notifications@github.com wrote:

@gmaxwell https://github.com/gmaxwell wasn't this going undocumented
(or at least, a huge "highly discouraged" warning) next to if it at all
made it in to 0.12?


Reply to this email directly or view it on GitHub
#7046 (comment).

@petertodd

View changes

Show outdated Hide outdated src/main.cpp
else
{
if (fBlocksOnly)
LogPrint("net", "peer sent inv %s in violation of protocol peer=%d\n", inv.ToString(), pfrom->id);

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

Maybe I'm missing something, but why wouldn't a peer send us a inv? We're advertising NODE_NETWORK, and we haven't sent them a bloom filter.

@petertodd

petertodd Nov 20, 2015

Contributor

Maybe I'm missing something, but why wouldn't a peer send us a inv? We're advertising NODE_NETWORK, and we haven't sent them a bloom filter.

This comment has been minimized.

@pstratem

pstratem Nov 20, 2015

Contributor

In the previous PR (which was merged into master) we set fRelayTxs = false in the version message in blocks only mode.

@pstratem

pstratem Nov 20, 2015

Contributor

In the previous PR (which was merged into master) we set fRelayTxs = false in the version message in blocks only mode.

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

Weird, I can't seem to find the actual code that does that. :(

@petertodd

petertodd Nov 20, 2015

Contributor

Weird, I can't seem to find the actual code that does that. :(

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

Ah! Sorry, I'm blind, found it!

@petertodd

petertodd Nov 20, 2015

Contributor

Ah! Sorry, I'm blind, found it!

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

Also reads better if reworded to say "peer %d sent"

@petertodd

petertodd Nov 20, 2015

Contributor

Also reads better if reworded to say "peer %d sent"

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 20, 2015

Contributor

@gmaxwell Re: the only blocksonly peers risk, it probably makes sense to have blocksonly unset NODE_NETWORK for now.

Contributor

petertodd commented Nov 20, 2015

@gmaxwell Re: the only blocksonly peers risk, it probably makes sense to have blocksonly unset NODE_NETWORK for now.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 20, 2015

Member

@petertodd I'd rather not, we still serve and relay blocks. You can IBD off a blocks only node, etc. There are already quite a few nodes out there that silently won't relay transactions, at least with the option undocumented I don't think it's a huge marginal risk. -- also, compare to setting your fee settings very high, or your mempool very small. Same outcome.

Member

gmaxwell commented Nov 20, 2015

@petertodd I'd rather not, we still serve and relay blocks. You can IBD off a blocks only node, etc. There are already quite a few nodes out there that silently won't relay transactions, at least with the option undocumented I don't think it's a huge marginal risk. -- also, compare to setting your fee settings very high, or your mempool very small. Same outcome.

@petertodd

View changes

Show outdated Hide outdated src/main.cpp
// We are in blocks only mode and peer is either not whitelisted or whitelistalwaysrelay is off
if (GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY) && (!pfrom->fWhitelisted || !GetBoolArg("-whitelistalwaysrelay", DEFAULT_WHITELISTALWAYSRELAY)))
{
LogPrint("net", "peer sent transaction in violation of protocol peer=%d\n", pfrom->id);

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

Reads better if reworded to say "peer %d sent"

@petertodd

petertodd Nov 20, 2015

Contributor

Reads better if reworded to say "peer %d sent"

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 20, 2015

Contributor

Tests done:

[x] Triggered inv and tx "violation of protocol" cases
[x] Tested with a bitcoinj wallet, which just ignored the fRelayTxs (edit: actually, looks like it only does that in the "trusted peer" case)

ACK (modulo https://github.com/pstratem/bitcoin/pull/3)

Contributor

petertodd commented Nov 20, 2015

Tests done:

[x] Triggered inv and tx "violation of protocol" cases
[x] Tested with a bitcoinj wallet, which just ignored the fRelayTxs (edit: actually, looks like it only does that in the "trusted peer" case)

ACK (modulo https://github.com/pstratem/bitcoin/pull/3)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 21, 2015

Contributor

@gmaxwell Fair enough; I'm convinced.

Contributor

petertodd commented Nov 21, 2015

@gmaxwell Fair enough; I'm convinced.

pstratem and others added some commits Nov 18, 2015

Fix relay mechanism for whitelisted peers under blocks only mode.
Previously in blocks only mode all inv messages where type!=MSG_BLOCK would be
rejected without regard for whitelisting or whitelistalwaysrelay.

As such whitelisted peers would never send the transaction (which would be
processed).
Bail early in processing transactions in blocks only mode.
Previously unsolicited transactions would be processed as normal.
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 21, 2015

Member

Sorry to nitpick, esp at odds with PT's nitpick: but I think these log entries should be made consistent:

2015-11-21 23:42:08 getblocks 384665 to 00000000000000000c8d710c468516e11f76188c18def174581317b8faac716d limit 500 from peer=86
2015-11-21 23:42:08 getblocks stopping at 384666 00000000000000000c8d710c468516e11f76188c18def174581317b8faac716d
2015-11-21 23:42:08 sending: inv (37 bytes) peer=86
2015-11-21 23:42:08 received: pong (8 bytes) peer=86
2015-11-21 23:42:08 sending: ping (8 bytes) peer=56
2015-11-21 23:42:08 received: tx (29246 bytes) peer=1
2015-11-21 23:42:08 peer 1 sent transaction in violation of protocol

E.g. "transaction (txid?) sent in violation of protocol peer=%d"

Member

gmaxwell commented Nov 21, 2015

Sorry to nitpick, esp at odds with PT's nitpick: but I think these log entries should be made consistent:

2015-11-21 23:42:08 getblocks 384665 to 00000000000000000c8d710c468516e11f76188c18def174581317b8faac716d limit 500 from peer=86
2015-11-21 23:42:08 getblocks stopping at 384666 00000000000000000c8d710c468516e11f76188c18def174581317b8faac716d
2015-11-21 23:42:08 sending: inv (37 bytes) peer=86
2015-11-21 23:42:08 received: pong (8 bytes) peer=86
2015-11-21 23:42:08 sending: ping (8 bytes) peer=56
2015-11-21 23:42:08 received: tx (29246 bytes) peer=1
2015-11-21 23:42:08 peer 1 sent transaction in violation of protocol

E.g. "transaction (txid?) sent in violation of protocol peer=%d"

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 22, 2015

Member

@petertodd come reack that I undid your nit. :)

Member

gmaxwell commented Nov 22, 2015

@petertodd come reack that I undid your nit. :)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 22, 2015

Contributor

@gmaxwell That wording looks fine, although I'd point out that phrasing it "[THING] sent in violation of protocol, peer=%d" - note the added comma! - should be fine too.

What can I call that? A μnit? :)

Contributor

petertodd commented Nov 22, 2015

@gmaxwell That wording looks fine, although I'd point out that phrasing it "[THING] sent in violation of protocol, peer=%d" - note the added comma! - should be fine too.

What can I call that? A μnit? :)

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 22, 2015

Member

ACK

Member

gmaxwell commented Nov 22, 2015

ACK

@gmaxwell gmaxwell merged commit 80ae230 into bitcoin:master Nov 22, 2015

1 check passed

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

gmaxwell added a commit that referenced this pull request Nov 22, 2015

Merge pull request #7046
80ae230 Improve log messages for blocks only violations. (Patick Strateman)
08843ed Add relaytxes status to getpeerinfo (Peter Todd)
d8aaa51 Bail early in processing transactions in blocks only mode. (Patick Strateman)
3587f6a Fix relay mechanism for whitelisted peers under blocks only mode. (Patick Strateman)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment