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

[Merged by Bors] - Use JSON by default for Deposit Snapshot Sync #4397

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Use JSON by default for Deposit Snapshot Sync #4397

wants to merge 2 commits into from

Conversation

ethDreamer
Copy link
Member

@ethDreamer ethDreamer commented Jun 13, 2023 •
edited
Loading

Checkpointz now supports deposit snapshot but they only support returning them in JSON so I've modified lighthouse to request them in JSON by default.

There's also get_opt & get_opt_with_timeout methods which seem to expect responses in JSON but were not adding Accept: application/json to the request headers so I fixed that as well.

Also the beacon API puts quantities in quotes so I fixed that in the snapshot JSON serialization

@michaelsproul michaelsproul added the ready-for-review The code is ready for review label Jun 13, 2023
michaelsproul
michaelsproul approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

LGTM!

@chong-he
Copy link
Member

chong-he commented Jun 15, 2023 •
edited
Loading

I have tested this and it works, details as follows:

Command:

/home/hi/testing/lighthouse_before bn  \   (or `lighthouse_after`)
--network mainnet \
--http \
--execution-endpoint http://localhost:8551 \
--execution-jwt /var/lib/jwtsecret/jwt.hex \
--checkpoint-sync-url https://mainnet.checkpoint.sigp.io/ \
--purge-db

lighthouse_before is v4.2.0, lighthouse_after is the lighthouse binary built from this head repository bf48970 of this PR.

On successful checkpoint-sync, lighthouse_before logs:

Jun 15 01:11:13.470 INFO Starting checkpoint sync                remote_url: https://mainnet.checkpoint.sigp.io/, service: beacon
Jun 15 01:11:14.178 WARN Remote BN does not support EIP-4881 fast deposit sync, error: Error fetching deposit snapshot from remote: ServerMessage(ErrorMessage { code: 415, message: "unsupported content-type: application/octet-stream", stacktraces: [] }), service: beacon
Jun 15 01:12:09.747 INFO Loaded checkpoint block and state       state_root: 0xc7ce748cbce6f2f030ef60ad043d7e8202c9600ecc38edda7be687535f0a9f38, block_root: 0x7d9dd5f6f1caa161ae815fcc6ee505282452e1acd4caa114a74c5f674731c3bc, slot: 6663840, service: beacon
Jun 15 01:12:13.315 INFO Block production enabled                method: json rpc via http, endpoint: Auth { endpoint: "http://localhost:8551/", jwt_path: "/var/lib/jwtsecret/jwt.hex", jwt_id: None, jwt_version: None }

lighthouse_after logs:

Jun 15 01:13:43.604 INFO Starting checkpoint sync                remote_url: https://mainnet.checkpoint.sigp.io/, service: beacon
Jun 15 01:14:42.872 INFO Loaded checkpoint block and state       state_root: 0xc7ce748cbce6f2f030ef60ad043d7e8202c9600ecc38edda7be687535f0a9f38, block_root: 0x7d9dd5f6f1caa161ae815fcc6ee505282452e1acd4caa114a74c5f674731c3bc, slot: 6663840, service: beacon
Jun 15 01:14:42.877 INFO Loaded deposit tree snapshot            deposits loaded: 802231, service: beacon
Jun 15 01:14:46.444 INFO Block production enabled                method: json rpc via http, endpoint: Auth { endpoint: "http://localhost:8551/", jwt_path: "/var/lib/jwtsecret/jwt.hex", jwt_id: None, jwt_version: None }

and the warn message is no longer emitted.

Additional information:

  1. In the case of TimedOut error, the WARN log is also not emitted, both will proceed to shut down.

lighthouse_before:

Jun 15 00:48:31.339 WARN Remote BN does not support EIP-4881 fast deposit sync, error: Error fetching deposit snapshot from remote: ServerMessage(ErrorMessage { code: 415, message: "unsupported content-type: application/octet-stream", stacktraces: [] }), service: beacon
Jun 15 00:49:52.673 CRIT Failed to start beacon node             reason: Error loading checkpoint state from remote 0x614d40639274b81fb50ff1af20583dd24467d7cdecfcc43b66821adf8a87931b: Reqwest(reqwest::Error { kind: Body, source: TimedOut })

lighthouse_after:

Jun 15 00:50:42.301 INFO Starting checkpoint sync                remote_url: https://mainnet.checkpoint.sigp.io/, service: beacon
Jun 15 00:51:54.579 CRIT Failed to start beacon node             reason: Error loading checkpoint state from remote 0x614d40639274b81fb50ff1af20583dd24467d7cdecfcc43b66821adf8a87931b: Reqwest(reqwest::Error { kind: Body, source: TimedOut })
Jun 15 00:51:54.580 INFO Internal shutdown received              reason: Failed to start beacon node

The TimedOut error is unrelated to this PR. It just that the TimedOut error happened quite often, not sure if it is the endpoint issue or lighthouse issue? I have tested all endpoints in https://eth-clients.github.io/checkpoint-sync-endpoints/ and more often than not, it will show TimedOut error.

  1. Some endpoints such as https://beaconstate-mainnet.chainsafe.io/ and https://sync.invis.tools/ will show another error with lighthouse_after:

Jun 15 00:54:38.718 WARN Remote BN does not support EIP-4881 fast deposit sync, error: Error fetching deposit snapshot from remote: ServerMessage(ErrorMessage { code: 500, message: "not found", stacktraces: [] }), service: beacon

I believe this error is not due to the changes introduced, but the endpoint does not support fast deposit sync?

  1. Tested on the Goerli testnet as well with https://prater.checkpoint.sigp.io/ and it works

@michaelsproul
Copy link
Member

Thanks for the comprehensive testing @chong-he!

The TimedOut error is unrelated to this PR. It just that the TimedOut error happened quite often, not sure if it is the endpoint issue or lighthouse issue?

Yeah this is kind of a tricky one, it's a combination of the endpoint being slow, and the latency between lighthouse and the checkpoint server. We have a CLI flag for adjusting the timeout, --checkpoint-sync-url-timeout.

I believe this error is not due to the changes introduced, but the endpoint does not support fast deposit sync?

Yeah, exactly right.

Given this, I think we're good to merge

bors r+

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jun 15, 2023
bors bot pushed a commit that referenced this pull request Jun 15, 2023
@ethDreamer
Use JSON by default for Deposit Snapshot Sync (#4397)
77fc511 
Checkpointz now supports deposit snapshot but [they only support returning them in JSON](ethpandaops/checkpointz#74) so I've modified lighthouse to request them in JSON by default.

There's also `get_opt` & `get_opt_with_timeout` methods which seem to expect responses in JSON but were not adding `Accept: application/json` to the request headers so I fixed that as well.

Also the beacon API puts quantities in quotes so I fixed that in the snapshot JSON serialization
@bors
Copy link

bors bot commented Jun 15, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Use JSON by default for Deposit Snapshot Sync [Merged by Bors] - Use JSON by default for Deposit Snapshot Sync Jun 15, 2023
@bors bors bot closed this Jun 15, 2023
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
@ethDreamer
Use JSON by default for Deposit Snapshot Sync (sigp#4397)
416d130 
Checkpointz now supports deposit snapshot but [they only support returning them in JSON](ethpandaops/checkpointz#74) so I've modified lighthouse to request them in JSON by default.

There's also `get_opt` & `get_opt_with_timeout` methods which seem to expect responses in JSON but were not adding `Accept: application/json` to the request headers so I fixed that as well.

Also the beacon API puts quantities in quotes so I fixed that in the snapshot JSON serialization
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@ethDreamer @Woodpile37
Use JSON by default for Deposit Snapshot Sync (sigp#4397)
c2c4b25 
Checkpointz now supports deposit snapshot but [they only support returning them in JSON](ethpandaops/checkpointz#74) so I've modified lighthouse to request them in JSON by default.

There's also `get_opt` & `get_opt_with_timeout` methods which seem to expect responses in JSON but were not adding `Accept: application/json` to the request headers so I fixed that as well.

Also the beacon API puts quantities in quotes so I fixed that in the snapshot JSON serialization
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@ethDreamer @Woodpile37
Use JSON by default for Deposit Snapshot Sync (sigp#4397)
b2dfa00 
Checkpointz now supports deposit snapshot but [they only support returning them in JSON](ethpandaops/checkpointz#74) so I've modified lighthouse to request them in JSON by default.

There's also `get_opt` & `get_opt_with_timeout` methods which seem to expect responses in JSON but were not adding `Accept: application/json` to the request headers so I fixed that as well.

Also the beacon API puts quantities in quotes so I fixed that in the snapshot JSON serialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethDreamer @chong-he @michaelsproul