Skip to content

[Merged by Bors] - Capture a missed VC error#2436

Closed
paulhauner wants to merge 12 commits intosigp:unstablefrom
paulhauner:vc-start-error
Closed

[Merged by Bors] - Capture a missed VC error#2436
paulhauner wants to merge 12 commits intosigp:unstablefrom
paulhauner:vc-start-error

Conversation

@paulhauner
Copy link
Member

@paulhauner paulhauner commented Jul 7, 2021 •
edited
Loading

Issue Addressed

Related to #2430, #2394

Proposed Changes

As per #2430 (comment), ensure that the ProductionValidatorClient::new error raises a log and shuts down the VC. Also, I implemened spawn_ignoring_error, as per @michaelsproul's suggestion in #2436 (comment).

I got unlucky and CI picked up a new rustsec vuln. To fix this, I had to update the following crates:

  • tokio
  • web3
  • tokio-compat-02

Additional Info

NA

@paulhauner paulhauner added the ready-for-review The code is ready for review label Jul 7, 2021
@michaelsproul
Copy link
Member

michaelsproul commented Jul 8, 2021

This is glorious! I just tested with a few of the scenarios from #2430 and they're all fixed by this PR 🎉

On a related note, I noticed we often use .map(|_| ()) to throw away errors in the VC, which is fine as long as the error type is (), but could accidentally throw away non-unit errors. We could maybe add a convenience method to executor to make this pattern safer, something like spawn_ignoring_error that enforces E = (). Or a mixin trait for Result<(), ()> that adds an ignore method.

lighthouse/validator_client/src/attestation_service.rs

Lines 208 to 218 in 3b600ac

self.inner.context.executor.spawn(
self.clone()
.publish_attestations_and_aggregates(
slot,
committee_index,
validator_duties,
aggregate_production_instant,
)
.map(|_| ()),
"attestation publish",
);

@paulhauner
Copy link
Member Author

paulhauner commented Jul 8, 2021

something like spawn_ignoring_error that enforces E = ()

Good call, I've added that too.

I had to jig around with deps to solve a cargo audit vuln. Let's see if CI still passes 🤞

@paulhauner paulhauner mentioned this pull request Jul 8, 2021
Open
@paulhauner
Copy link
Member Author

paulhauner commented Jul 9, 2021

Looks like Windows doesn't like the latest version of web3:

 error[E0432]: unresolved import `tokio::net::UnixStream`
  --> C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\web3-0.16.0\src\transports\ipc.rs:18:5
   |
18 |     net::UnixStream,
   |     ^^^^^^^^^^^^^^^ no `UnixStream` in `net`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0432`.
error: could not compile `web3`

@paulhauner paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jul 9, 2021
@michaelsproul
Copy link
Member

michaelsproul commented Jul 9, 2021 •
edited
Loading

Oh! They restored IPC support in v0.16, I didn't notice!

That probably only works on UNIX so we should disable it with default-features = false

https://github.com/tomusdrw/rust-web3#installation-on-windows

https://github.com/tomusdrw/rust-web3/blob/c153128c78860ce83d5d5db3ad418e18fb42da77/Cargo.toml#L68

@michaelsproul
Copy link
Member

michaelsproul commented Jul 9, 2021 •
edited
Loading

I think ideally we'll want some form of platform deps to enable IPC on UNIX only https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

Actually, looks like we decided not to support IPC (#1049). We can leave it disabled for now and come back for it if we want

@paulhauner
Copy link
Member Author

paulhauner commented Jul 9, 2021 •
edited
Loading

That probably only works on UNIX so we should disable it with default-features = false

I've redefined the features to just drop ipc-tokio, as per 0.14.0. That seemed safest.

Lets see how Windows likes that.

@michaelsproul
Copy link
Member

michaelsproul commented Jul 9, 2021

Ah, need default-features = false. It's still trying to compile it

@paulhauner paulhauner mentioned this pull request Jul 9, 2021
Closed
@paulhauner
Copy link
Member Author

paulhauner commented Jul 9, 2021

In the interests of getting this merged sooner, I'm going to skip straight to bors.

bors r+

bors bot pushed a commit that referenced this pull request Jul 9, 2021
@paulhauner
Capture a missed VC error (#2436)
78e5c0c 
## Issue Addressed

Related to #2430, #2394

## Proposed Changes

As per #2430 (comment), ensure that the `ProductionValidatorClient::new` error raises a log and shuts down the VC. Also, I implemened `spawn_ignoring_error`, as per @michaelsproul's suggestion in #2436 (comment).

I got unlucky and CI picked up a [new rustsec vuln](https://rustsec.org/advisories/RUSTSEC-2021-0072). To fix this, I had to update the following crates:

- `tokio`
- `web3`
- `tokio-compat-02`

## Additional Info

NA
@bors
Copy link

bors bot commented Jul 9, 2021

Pull request successfully merged into unstable.

Build succeeded:

@bors bors bot changed the title Capture a missed VC error [Merged by Bors] - Capture a missed VC error Jul 9, 2021
@bors bors bot closed this Jul 9, 2021
@paulhauner paulhauner mentioned this pull request Jul 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

waiting-on-author The reviewer has suggested changes and awaits thier implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@paulhauner @michaelsproul