Fetch w/o CB if mempool empty, don't use HB mode if blocks only.

gmaxwell commented at 0:11 am on September 23, 2016: contributor

Fetching with CB when there are less than 30 hits for a full block wastes bandwidth; and getting three copies of a CB message which is of no use to us when we are in blocks only mode is not great.

This is a fairly small optimization, since even the worst case of getting three useless CB messages is only a 5% bandwidth increase.

[I will update the test if this gets Concept ACKs]

jonasschnelli added the label P2P on Sep 23, 2016

laanwj commented at 7:46 am on October 15, 2016: member

Concept ACK

jonasschnelli commented at 6:40 am on October 17, 2016: contributor

Concept ACK, needs rebase.

sipa commented at 7:57 am on October 31, 2016: member

Concept ACK, needs rebase.

gmaxwell commented at 0:14 am on November 3, 2016: contributor

Rebased, I still need to fix the tests.

in src/main.cpp: in 49a45658ea outdated

5340@@ -5341,7 +5341,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5341                         nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER &&
5342                         (!IsWitnessEnabled(chainActive.Tip(), chainparams.GetConsensus()) || State(pfrom->GetId())->fHaveWitness)) {
5343                         inv.type |= nFetchFlags;
5344-                        if (nodestate->fSupportsDesiredCmpctVersion)
5345+                        if (nodestate->fSupportsDesiredCmpctVersion && mempool.size() > 1)

rebroad commented at 9:08 am on November 3, 2016:

is 1 sufficient?

Fetch w/o CB if mempool empty, don't use HB mode if blocks only.

Fetching with CB when there are less than 30 hits for a full block
 wastes bandwidth; and getting three copies of a CB message which
 is of no use to us when we are in blocks only mode is not great.

This is a fairly small optimization, since even the worst case of
 getting three useless CB messages is only a 5% bandwidth increase.

3bd962d271

in src/main.cpp: in 49a45658ea outdated

6013                         // We seem to be rather well-synced, so it appears pfrom was the first to provide us
6014-                        // with this block! Let's get them to announce using compact blocks in the future.
6015-                        MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom, connman);
6016+                        // with this block! Let's get them to announce using compact blocks in the future, unless
6017+                        // either of us are blocks-only and would prefer to save bandwidth.
6018+                        if (pfrom->fRelayTxes && !GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) {

rebroad commented at 9:09 am on November 3, 2016:

Why use two different tests? I’d have though the mempool size test for both, or the blockonly test for both would bring more consistency.

sdaftuar commented at 9:47 pm on November 19, 2016:

I’m not sure why we wouldn’t ask this peer for compact blocks just because they are not relaying transactions. Our mempool might be perfectly good for block reconstruction, even if the peer who is relaying blocks to us fastest is blocksonly, no?

I know that in the future we’re likely to want to optimize the prefilled transactions based eg on what was missing from our own mempools, but I think at the time we do that we might as well start scoring our peers based on, say, how often a roundtrip is required or something. And if a blocksonly peer happens to be really fast to deliver us blocks then great…

gmaxwell commented at 7:37 pm on December 1, 2016:

The thinking there was that if the peer is trying to economize bandwidth, we shouldn’t ask them to send the unsolicited compact blocks (HB mode). We’ll still request compact blocks from them when advertised. I don’t fee super strongly, if they’re trying to economize on bandwidth they should probably delay their relays artificially. We don’t have a NAK for HB mode requests though.

in src/net_processing.cpp: in 3bd962d271

2100                         // We seem to be rather well-synced, so it appears pfrom was the first to provide us
2101-                        // with this block! Let's get them to announce using compact blocks in the future.
2102-                        MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom, connman);
2103+                        // with this block! Let's get them to announce using compact blocks in the future, unless
2104+                        // either of us are blocks-only and would prefer to save bandwidth.
2105+                        if (pfrom->fRelayTxes && !GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) {

rebroad commented at 5:40 pm on December 25, 2016:

Why not put this check within MaybeSetPeerAsAnnouncingHeaderAndIDs() ?

gmaxwell closed this on Dec 29, 2016

DrahtBot locked this on Sep 8, 2021

Fetch w/o CB if mempool empty, don’t use HB mode if blocks only. #8800