Fix duplicate block requests to multiple peers.

[Infernoman]

fAlreadyHave or mapAskFor/mapAlreadyAskedFor isn't working correctly causing the client to request each block twice from every client they're connected to. This happens all the time, and degrades the performance of every node on the chain. During IBD this causes a huge amount of network resource usage, wasting time and data.

[panleone]

Thanks for the contribution, however your fix does not work:

The inv object for receiving a block is made simply by (MSG_BLOCK, block_hash), it does not contain the block itself.
To fetch a block the full sequence of events is:

your node receive the inv by a peer -> IF YOU HAVEN'T PROCESSED THE BLOCK you ask the peer for the full block -> the peer send you the full block -> you process the block (let's say it is valid)-> only at this point fAlreadyHave becomes true

Now your are basically changing the sequence in this way:

your node receive the inv by a peer -> IF YOU HAVEN'T RECEIVED THE SAME INV you ask the peer for the full block -> the peer send you the full block -> ...

What is the problem of this? If you received the inv once but for any reasons the peer did not answer with the full block then you will ignore all following inv even if you still need the block

[Infernoman]

Thanks for the contribution, however your fix does not work:

The inv object for receiving a block is made simply by (MSG_BLOCK, block_hash), it does not contain the block itself. To fetch a block the full sequence of events is:

your node receive the inv by a peer -> IF YOU HAVEN'T PROCESSED THE BLOCK you ask the peer for the full block -> the peer send you the full block -> you process the block (let's say it is valid)-> only at this point fAlreadyHave becomes true

Now your are basically changing the sequence in this way:

your node receive the inv by a peer -> IF YOU HAVEN'T RECEIVED THE SAME INV you ask the peer for the full block -> the peer send you the full block -> ...

What is the problem of this? If you received the inv once but for any reasons the peer did not answer with the full block then you will ignore all following inv even if you still need the block

Yes you are correct I think there's a better way to do this as well. But I'm just starting to get back into things again. And I'm still a bit unfamiliar with the new code.

Currently the process should be:

1. Receive INV for block and if you dont have it request the full block
2. Add block to mapAskFor. And request full block
3. Ask peer for block, then move to mapAlreadyAskedFor
4. Peer sends block, you process it and it gets added to fAlreadyHave
5. If the peer doesn't send the block. Wait 2 minutes and retry.

Here's what's happening:

1. Receive 2 inv's for block from EVERY peer.
2. Add block to mapAskFor. And request 2 inv's for block from every peer.
3. Ask peers for block, then move to mapAlreadyAskedFor
4. 24 of the same block inv's are received from 12 peers.
5. 23 of those end up being wasted resources.

[panleone]

• Receive INV for block and if you dont have it request the full block
• Add block to mapAskFor. And request full block
• Ask peer for block, then move to mapAlreadyAskedFor
• Peer sends block, you process it and it gets added to fAlreadyHave
• If the peer doesn't send the block. Wait 2 minutes and retry.

This is wrong, blocks are the only inv type that are not sent using AskFor() and all maps involved (they are instead sent directly).
Anyways I agree that the current system is not perfect and many blocks are received twice or more (for many different reasons) but fixing it is not trivial. In the future our idea is researching and switching to header first sync which should solve the problem but in the meantime any contribution that can make the blockchain download smaller is really appreciated

[Infernoman]

Thanks for correcting me lol. I'll have to look into it more then. I figured I would get the conversation started and get some idea's from you guys. I've found a few things on dash, but mostly relating to headers first sync. So I'm trying to go through the history of commits.

This commit was in 2017 but still relating to their headers first sync.
dashpay@169afaf

[Infernoman]

I believe that commit from dash may be the issue. its first referenced in bitcoin in 2015 bitcoin#6755

[Infernoman]

This works a lot better than the original proposal. And syncing has not stalled for me using this. On average its processing around 13.75k blocks per minute during IBD.

[panleone]

This works a lot better than the original proposal. And syncing has not stalled for me using this. On average its processing around 13.75k blocks per minute during IBD.

This fix doesn't speed up anything since the AlreadyHave returns true in the same cases as before (pindex != nullptr)
Before trying to make IBD faster I really suggest studying how it works since it seems you are a bit confused in general. This is a good place to start https://btcinformation.org/en/developer-guide#blocks-first

About the two links you sent before, they are both fixes related to header first sync that we don't have implemented yet

[Infernoman]

Thanks for the refresher lol. After looking at the link you've provided it helped clear things up.

So based on what that link says about blocks first, the client should only be downloading from one peer. So in its current state it is "broken".

https://github.com/bitcoin/bitcoin/pull/4468/files#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R3652
The commit for headers first on the bitcoin github, main.cpp, line 3652. Marks the block as in flight. And has the same check we do on line 3638.

Dash also uses a similar method to check and mark the block as in flight. I'm going to test that now and see how it goes. It seems like that may be a solution for the issue I'm talking about.

[Infernoman]

After going through net_processing.cpp further most of the code is already there for syncing headers, is this a partial implementation of headers first syncing? And would it be possible that is causing the issue?

Currently when you check "getpeerinfo" while syncing there are no blocks being marked as inflight, atleast if they are they're not being displayed here. If they aren't being marked as inflight that would explain why the checks weren't working. And why the client would continue to ask for the same block from different clients. Until the block was processed.

It seems like the partial implementation is causing issues.

[Infernoman]

Well I may have fixed headers first sync. But I am unable to test it because of the consensus.

Here's the patch file:
https://pastebin.com/KmM3iq1J

You may be able to test it by syncing one node fully, then using connect= from a second peer who is syncing. That will be my next attempt after I eat lol.