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.

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

fix(benches): Fixes compiler errors for benches #416

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

mw2000
Copy link
Contributor

@mw2000 mw2000 commented Oct 30, 2024

We fix compiler errors that we see while benchmarking.

We fix compiler errors that we see while running tests and benchmarking.
@mw2000
Copy link
Contributor Author

mw2000 commented Oct 30, 2024

A few runtime errors that are still to be fixed, which I wanted some feedback on -

Tests:

I see these tests failing - consensus::tests::test_verify_finality_invalid_finality and consensus::tests::test_verify_update_invalid_finality. Both fail at thread 'consensus::tests::test_verify_update_invalid_finality' panicked at ethereum/src/consensus.rs:793:55: and thread 'consensus::tests::test_verify_finality_invalid_finality' panicked at ethereum/src/consensus.rs:836:64: respectively.

They fail because the verify_finality_update and verify_update calls don't result in errors. How can we make these calls fail so we can pass the tests?


Benchmarks:
Benchmarking also seem to fail at get_balance

Benchmarking get_balance: Warming up for 3.0000 sthread 'main' panicked at benches/get_balance.rs:35:50:
called `Result::unwrap()` on an `Err` value: out of sync: 1730275786 seconds behind

Location:
    /Users/<user>/Desktop/helios/core/src/client/node.rs:60:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@mw2000 mw2000 changed the title chore(ethereum, benches): Fixes compiler errors for tests + benches fix(ethereum, benches): Fixes compiler errors for tests + benches Oct 30, 2024
@mw2000
Copy link
Contributor Author

mw2000 commented Oct 30, 2024

Believe @0xRampey is also working on the compiler errors within tests. Since he found the issue first, we should merge his PR in. I'll reshape this PR to target the benches crate while his PR targets tests

@mw2000 mw2000 changed the title fix(ethereum, benches): Fixes compiler errors for tests + benches fix(benches): Fixes compiler errors for benches Oct 30, 2024
@0xRampey
Copy link
Contributor

They fail because the verify_finality_update and verify_update calls don't result in errors. How can we make these calls fail so we can pass the tests?

Turns out thatLightClientHeader::default() when passed as finalized header, converts to None along the way and slips through verify_generic_update without being handled! Hence, the calls are passing.
I've added a new Error to handle that case and replaced ::default() header with those from older updates. That seems to be doing the trick.

Copy link
Collaborator

@ncitron ncitron left a comment

Choose a reason for hiding this comment

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

LGTM!

@ncitron ncitron merged commit dcda271 into a16z:master Oct 31, 2024
2 of 5 checks passed
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