Net: Improve blocks only mode. #7046

pstratem commented at 9:51 PM on November 17, 2015: contributor

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 commented at 9:52 PM on November 17, 2015: contributor

Don't ignore for whitelisted peers?

pstratem renamed this:
~~[WIP] Net: Ignore "tx" messages in blocks only mode.~~
[WIP] Net: Improve blocks only mode.
on Nov 17, 2015

dcousens commented at 12:46 AM on November 18, 2015: contributor

concept ACK, utACK

pstratem renamed this:
~~[WIP] Net: Improve blocks only mode.~~
Net: Improve blocks only mode.
on Nov 18, 2015

jonasschnelli added the label P2P on Nov 18, 2015

paveljanik commented at 7:47 PM on November 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!

tulip0 commented at 6:41 AM on November 20, 2015: none

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 commented at 8:12 AM on November 20, 2015: contributor

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 commented at 4:47 PM on November 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?

pstratem commented at 5:34 PM on November 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).

in src/main.cpp:None in 67869e4575 outdated

4245 | @@ -4243,6 +4246,13 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
4246 |                      LogPrint("net", "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->id);
4247 |                  }
4248 |              }
4249 | +            else
4250 | +            {
4251 | +                if (fBlocksOnly)
4252 | +                    LogPrint("net", "peer sent inv %s in violation of protocol peer=%d\n", inv.ToString(), pfrom->id);

petertodd commented at 10:12 PM on November 20, 2015:

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.

pstratem commented at 10:14 PM on November 20, 2015:

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

petertodd commented at 10:15 PM on November 20, 2015:

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

petertodd commented at 10:16 PM on November 20, 2015:

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

petertodd commented at 11:37 PM on November 20, 2015:

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

petertodd commented at 10:14 PM on November 20, 2015: contributor

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

gmaxwell commented at 10:31 PM on November 20, 2015: contributor

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

in src/main.cpp:None in 67869e4575 outdated

4376 | @@ -4367,6 +4377,14 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
4377 |  
4378 |      else if (strCommand == "tx")
4379 |      {
4380 | +        // Stop processing the transaction early if
4381 | +        // We are in blocks only mode and peer is either not whitelisted or whitelistalwaysrelay is off
4382 | +        if (GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY) && (!pfrom->fWhitelisted || !GetBoolArg("-whitelistalwaysrelay", DEFAULT_WHITELISTALWAYSRELAY)))
4383 | +        {
4384 | +            LogPrint("net", "peer sent transaction in violation of protocol peer=%d\n", pfrom->id);

petertodd commented at 11:36 PM on November 20, 2015:

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

petertodd commented at 11:55 PM on November 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)

petertodd commented at 12:00 AM on November 21, 2015: contributor

@gmaxwell Fair enough; I'm convinced.

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

3587f6a024

Bail early in processing transactions in blocks only mode.

Previously unsolicited transactions would be processed as normal.

d8aaa51bec

Add relaytxes status to getpeerinfo 08843ed998

gmaxwell commented at 11:44 PM on November 21, 2015: contributor

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"

Improve log messages for blocks only violations. 80ae230a52

gmaxwell commented at 2:54 AM on November 22, 2015: contributor

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

petertodd commented at 9:03 PM on November 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? :)

gmaxwell commented at 10:01 PM on November 22, 2015: contributor

ACK

gmaxwell merged this on Nov 22, 2015

gmaxwell closed this on Nov 22, 2015

gmaxwell referenced this in commit c322652b71 on Nov 22, 2015

zkbot referenced this in commit ba4eb241e7 on Mar 31, 2021

MarcoFalke locked this on Sep 8, 2021