Skip to content
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.

Sign up for GitHub

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

Jump to bottom

net: introduce block tracker to retry to download blocks after failure #27837

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

net: introduce block tracker to retry to download blocks after failure #27837

wants to merge 6 commits into from

Conversation

furszy
Copy link
Member

@furszy furszy commented Jun 7, 2023 •
edited

Coming from #27652, part of #29183.

The general idea is to keep track of the user requested blocks so, in
case of a bad behaving peer or a network disconnection, they can be
fetched from another one automatically without any further user interaction.

This was requested by users because the getblockfrompeer RPC command
lacks the functionality to notify them about block request failures or peer
disconnections (which is expected due to the asynchronous nature of the block
requests).

Currently, this new functionality is limited to blocks requested by the
user via the 'getblockfrompeer' RPC command.

In the future, this class could expand its scope and be utilized in the
regular chain synchronization process. Or, even could be employed in
special procedures like a prune node rescan that uses BIP158 block filters,
or even into BIP157 itself.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 7, 2023 •
edited

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28488 (p2p: Evict outbound peers with high minFeeRate by naumenkogs)
  • #28055 (Bugfix: net_processing: Restore "Already requested" error for FetchBlock by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Jun 8, 2023
Draft
Closed
@maflcko
Copy link
Member

maflcko commented Jun 13, 2023

Maybe mark as draft if CI is red and this is still based on something else?

@fanquake fanquake marked this pull request as draft June 13, 2023 08:27
@furszy
Copy link
Member Author

furszy commented Jun 13, 2023 •
edited

Oh, didn't know about the CI failure. Seems to be just an unused variable. But sure, draft until #27836 sounds good.

@furszy furszy mentioned this pull request Jun 18, 2023
Open
This was referenced Jul 5, 2023
Draft
Draft
This was referenced Jul 22, 2023
Draft
Open
@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from 1311875 to aab7a2f Compare August 1, 2023 21:27
@DrahtBot DrahtBot removed the CI failed label Aug 1, 2023
This was referenced Aug 2, 2023
Merged
Closed
Closed
Closed
Closed
Open
Closed
Closed
This was referenced Aug 15, 2023
Open
Merged
Merged
@DrahtBot DrahtBot mentioned this pull request Aug 24, 2023
Merged
This was referenced Sep 15, 2023
Draft
Closed
@DrahtBot DrahtBot mentioned this pull request Sep 30, 2023
Open
@furszy furszy mentioned this pull request Nov 22, 2023
Merged
@furszy furszy mentioned this pull request Dec 12, 2023
Closed
@furszy furszy mentioned this pull request Jan 4, 2024
Open
18 tasks
furszy
furszy commented Jan 17, 2024
View reviewed changes
Copy link
Member Author

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focus is on #28170, which is quite close.
Will rebase it as soon as it is merged.

@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from 4d65b99 to 7eb78f3 Compare March 11, 2024 12:56
@furszy furszy marked this pull request as ready for review March 11, 2024 12:57
@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from 7eb78f3 to 77fff41 Compare March 11, 2024 13:06
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22512552586

@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from 77fff41 to ab0d925 Compare March 12, 2024 14:03
@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from ab0d925 to ad4fc1b Compare April 18, 2024 12:21
@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch 2 times, most recently from 3c7fd2c to fdab5b8 Compare May 1, 2024 14:22
@DrahtBot DrahtBot marked this pull request as draft May 13, 2024 08:05
@DrahtBot
Copy link
Contributor

Turned into draft for now, due to failing CI. If this is no longer a WIP, you can move it out of draft.

@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from fdab5b8 to fe0f9be Compare May 15, 2024 20:36
@furszy furszy mentioned this pull request May 15, 2024
Merged
@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from fe0f9be to d589be0 Compare May 22, 2024 19:43
achow101 added a commit that referenced this pull request May 23, 2024
@achow101
Merge #30118: test: improve robustness of connect_nodes()
e163d86 
6629d1d test: improve robustness of connect_nodes() (furszy)

Pull request description:

  Decoupled from #27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200.

  The `connect_nodes` function in the test framework relies on a stable number of peer
  connections to verify that the new connection between the nodes is successfully established.
  This approach is fragile, as any of the peers involved in the process can drop, lose, or
  create a connection at any step, causing subsequent `wait_until` checks to stall indefinitely
  even when the peers in question were connected successfully.

  This commit improves the situation by using the nodes' subversion and the connection
  direction (inbound/outbound) to identify the exact peer connection and perform the
  checks exclusively on it.

ACKs for top commit:
  stratospher:
    reACK 6629d1d.
  achow101:
    ACK 6629d1d
  maflcko:
    utACK 6629d1d
  AngusP:
    re-ACK 6629d1d

Tree-SHA512: 5f345c0ce49ea81b643e97c5cffd133e182838752c27592fcdeac14ad10919fb4b7ff38e289e42a7c3c638a170bd0d0b7a9cd493898997a2082a7b7ceef4aeeb
@furszy
net: FetchBlock, disallow requesting blocks from network limited peers
1309f25
@furszy
net: FetchBlock, disallow requesting block to the same peer multiple …
0066f11 
…times
@furszy
net: introduce BlockRequestTracker
0a85111
@furszy
rpc: getblockfrompeer, connect block tracking mechanism and add coverage
d81dda9 
If the initial block fetching process fails, the p2p layer will be in
charge of fetching the block from 'any' connected peer.
Re-trying to download the block from different peers until it is
received.
@furszy
RPC: getblockfrompeer, introduce fetch from "any" peer functionality
8f43a8f 
If no 'peer_id' is provided, 'getblockfrompeer' will just delegate
the peer selection to the internal block downloading logic.
@furszy
rpc: getblockfrompeer, add one-shot block requests capability
0545d24 
Allowing what we had before, a single block request with no automatic
retry nor tracking mechanism.
@furszy furszy force-pushed the 2023_introduce_block_request_tracker branch from d589be0 to 0545d24 Compare May 23, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DrahtBot DrahtBot DrahtBot left review comments

Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@furszy @DrahtBot @maflcko