Skip to content

[Merged by Bors] - Fix genesis state download panic when running in debug mode#4753

Closed
jimmygchen wants to merge 3 commits intosigp:unstablefrom
jimmygchen:fix-debug-genesis-download-panic
Closed

[Merged by Bors] - Fix genesis state download panic when running in debug mode#4753
jimmygchen wants to merge 3 commits intosigp:unstablefrom
jimmygchen:fix-debug-genesis-download-panic

Conversation

@jimmygchen
Copy link
Copy Markdown
Member

Issue Addressed

#4738

Proposed Changes

See the above issue for details. Went with option #2 to use the async reqwest client in Eth2NetworkConfig and propagate the async-ness.

@jimmygchen jimmygchen added ready-for-review The code is ready for review v4.5.0 ETA Q4 2023 labels Sep 19, 2023
bn_matches,
eth2_network_config,
log,
))?;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Move config parsing and dumping into server::run as they need to be async.

.map(|state| state.genesis_validators_root())
.map(Result::Ok)
.transpose()
self.get_genesis_state_from_bytes::<E>()
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Switching to the new sync get_genesis_state_from_bytes method, as we don't need to download genesis state and genesis_state method is now async.

@michaelsproul
Copy link
Copy Markdown
Member

Looks like there's a small type issue in the boot_node: https://github.com/sigp/lighthouse/actions/runs/6232160760/job/16930311594?pr=4753#step:4:1310

@jimmygchen
Copy link
Copy Markdown
Member Author

Looks like there's a small type issue in the boot_node: https://github.com/sigp/lighthouse/actions/runs/6232160760/job/16930311594?pr=4753#step:4:1310

Thanks! this one is slightly annoying as it's only an issue on the msrv - I'll look into it.

@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Sep 20, 2023
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Sep 20, 2023
@jimmygchen
Copy link
Copy Markdown
Member Author

I've just fixed a copy paste bug I introduced ...tests should pass now 🤞

@jimmygchen
Copy link
Copy Markdown
Member Author

I've tested running this branch on --network holesky in debug mode, and can confirm that it no longer panics.

@jimmygchen jimmygchen added the backwards-incompat Backwards-incompatible API change label Sep 20, 2023
@jimmygchen
Copy link
Copy Markdown
Member Author

Marking this as backwards-incompat as I've bumped the msrv to 1.69.0.

Copy link
Copy Markdown
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Very nice! I like the simplification for getting the genesis validators root.

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 21, 2023
@michaelsproul
Copy link
Copy Markdown
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 21, 2023
## Issue Addressed

#4738 

## Proposed Changes

See the above issue for details. Went with option #2 to use the async reqwest client in `Eth2NetworkConfig` and propagate the async-ness.
@bors
Copy link
Copy Markdown

bors bot commented Sep 21, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Sep 21, 2023
## Issue Addressed

#4738 

## Proposed Changes

See the above issue for details. Went with option #2 to use the async reqwest client in `Eth2NetworkConfig` and propagate the async-ness.
@jimmygchen
Copy link
Copy Markdown
Member Author

Very nice! I like the simplification for getting the genesis validators root.

It was @michaelsproul's idea! I like it too ☺️

@bors
Copy link
Copy Markdown

bors bot commented Sep 21, 2023

@bors bors bot changed the title Fix genesis state download panic when running in debug mode [Merged by Bors] - Fix genesis state download panic when running in debug mode Sep 21, 2023
@bors bors bot closed this Sep 21, 2023
@jimmygchen jimmygchen deleted the fix-debug-genesis-download-panic branch September 22, 2023 05:03
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

sigp#4738 

## Proposed Changes

See the above issue for details. Went with option #2 to use the async reqwest client in `Eth2NetworkConfig` and propagate the async-ness.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
See the above issue for details. Went with option #2 to use the async reqwest client in `Eth2NetworkConfig` and propagate the async-ness.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
See the above issue for details. Went with option #2 to use the async reqwest client in `Eth2NetworkConfig` and propagate the async-ness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. v4.5.0 ETA Q4 2023

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants