Skip to content

Switch to downloading blobs from the Prysm RPC API#34

Closed
mdehoog wants to merge 4 commits intoInphi:masterfrom
mdehoog:blobs-api
Closed

Switch to downloading blobs from the Prysm RPC API#34
mdehoog wants to merge 4 commits intoInphi:masterfrom
mdehoog:blobs-api

Conversation

@mdehoog
Copy link
Copy Markdown
Collaborator

@mdehoog mdehoog commented Oct 4, 2022

No description provided.

@mdehoog mdehoog marked this pull request as draft October 4, 2022 20:34
@mdehoog mdehoog marked this pull request as ready for review October 5, 2022 20:09
@roberto-bayardo
Copy link
Copy Markdown
Collaborator

roberto-bayardo commented Oct 6, 2022

One thought though: would we want to keep both approaches for more thorough testing? Or is most of that removed code duplicating what's now in the Prysm RPC API?

@Inphi
Copy link
Copy Markdown
Owner

Inphi commented Oct 6, 2022

The removed code isn't duplicating the RPC API. I think we should keep and test both approaches. The beacon-node-follower receives sidecars from the beacon-node broadcast so it doesn't need to use the p2p RPC.

We can still merge this PR, as I'd be glad to get rid of that code, but only if we add a new test scenario that asserts p2p RPC works. For example, we setup a test with a late beacon-node-follower; which triggers initial-sync so it requests sidecars via p2p RPC. This is a good test to have anyways as intiial-sync is a pretty important codepath.

@Inphi
Copy link
Copy Markdown
Owner

Inphi commented Nov 7, 2022

This was implemented via #44, which also adds a historical sync e2e test so we don't lose test coverage. Closing.

@Inphi Inphi closed this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants