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

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

Closed
wants to merge 12 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Jul 7, 2021

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

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.

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

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
Copy link
Member Author

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.jparrowsec.cn-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

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

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

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

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

@paulhauner
Copy link
Member Author

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
## 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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants