-
Notifications
You must be signed in to change notification settings - Fork 879
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
feat(p2p/discovery)!: Implement archival
discovery + syncing of historic blocks and blobs
#3188
Conversation
50dfce3
to
6e201c8
Compare
15a2d9f
to
7acc848
Compare
archival
discovery + syncing of historic blocks and blobs
ffab4a1
to
b416f83
Compare
b416f83
to
dccf56d
Compare
Subscription was never cancelled on Stop, found while working on swamp tests for #3188
Subscription was never cancelled on Stop, found while working on swamp tests for #3188
3764930
to
d79a9c7
Compare
d79a9c7
to
586f97e
Compare
1684a62
to
e276af8
Compare
archival
discovery + syncing of historic blocks and blobsarchival
discovery + syncing of historic blocks and blobs
a9bf6ae
to
e9cd60f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't tell if nodebuilder is correct. Changes are quite complex and difficult to fully understand. So relying on tests for correctness. Not a good thing, so we should prioritise refactoring of share module construction.
…chival topic since default is set to true now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two main comments. The introduced Manager is net negative and the need for EnableAdvertise in config is unclear.
chore(share/getters): remove optionality for archival peer man - require it
…d put in private opts
…nt of both peermans to shrex and remove from nodebuilder, require availWindow as arg for shrex ctor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to ship pruning and unblock this PR, so approving,
Under different circumstances, I would request a redesign.
This PR leaves too much dech debt behind it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort on the nodebuilder in this PR! However, it's still pretty tough to tell if everything's correct. My biggest concern is that the feature needs thorough testing to ensure all setups work as intended. Given its complexity, future changes might get tricky, so let's prioritize refactoring as soon as we have the resources.
This PR introduces the discovery of nodes advertising on the
archival
topic for the purpose of being able to request blobs and blocks that are older than the sampling window.In order to do so, there are now 2 instances of discovery (one for
full
nodes -- all nodes running full block sync within the sampling window, and one forarchival
nodes -- nodes that sync, store and serve all historic data) and 2 instances of peer managers (one forfull
and one forarchival
).This PR is breaking due to changes to metrics names.
Please also note that this PR introduces the ability to perform archival sync of blobs + blocks for LIGHT AND FULL nodes only regardless of whether the full nodes have pruning enabled or not. Bridge nodes that have previously had pruning enabled will NOT be able to request historic data from the consensus core node. The bridge node must run in archival mode (without
--experimental-pruning
enabled) in order to access historic data.Bridge and Full nodes in archival mode (without
--experimental-pruning
)Bridge and Full nodes running
--experimental-pruning
Light nodes
TODO
full
-node-centricFor follow-up PR:
Nice to have;