[Merged by Bors] - Web3Signer support for VC #2522
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paulhauner
force-pushed
the
remote-signer-client
branch
from
f338192
to
43bdab7
Compare
bors bot
pushed a commit
that referenced
this pull request
Sep 3, 2021
## Proposed Changes This PR deletes all `remote_signer` code from Lighthouse, for the following reasons: * The `remote_signer` code is unused, and we have no plans to use it now that we're moving to supporting the Web3Signer APIs: #2522 * It represents a significant maintenance burden. The HTTP API tests have been prone to platform-specific failures, and breakages due to dependency upgrades, e.g. #2400. Although the code is deleted it remains in the Git history should we ever want to recover it. For ease of reference: - The last commit containing remote signer code: 5a3bcd2 - The last Lighthouse version: v1.5.1
|
Thanks for the review @michaelsproul, they were great points about running the tasks in parallel! I've addressed all your comments and this is ready for review again. You can start your review now, but before we merge I'm going to:
|
paulhauner
added
ready-for-review
michaelsproul
approved these changes
Sep 16, 2021
michaelsproul
added
ready-for-merge
|
Thank you! I add couple more very minor metrics, FYI. bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Sep 16, 2021
[EIP-3030]: https://eips.ethereum.org/EIPS/eip-3030 [Web3Signer]: https://consensys.github.io/web3signer/web3signer-eth2.html ## Issue Addressed Resolves #2498 ## Proposed Changes Allows the VC to call out to a [Web3Signer] remote signer to obtain signatures. ## Additional Info ### Making Signing Functions `async` To allow remote signing, I needed to make all the signing functions `async`. This caused a bit of noise where I had to convert iterators into `for` loops. In `duties_service.rs` there was a particularly tricky case where we couldn't hold a write-lock across an `await`, so I had to first take a read-lock, then grab a write-lock. ### Move Signing from Core Executor Whilst implementing this feature, I noticed that we signing was happening on the core tokio executor. I suspect this was causing the executor to temporarily lock and occasionally trigger some HTTP timeouts (and potentially SQL pool timeouts, but I can't verify this). Since moving all signing into blocking tokio tasks, I noticed a distinct drop in the "atttestations_http_get" metric on a Prater node:  I think this graph indicates that freeing the core executor allows the VC to operate more smoothly. ### Refactor TaskExecutor I noticed that the `TaskExecutor::spawn_blocking_handle` function would fail to spawn tasks if it were unable to obtain handles to some metrics (this can happen if the same metric is defined twice). It seemed that a more sensible approach would be to keep spawning tasks, but without metrics. To that end, I refactored the function so that it would still function without metrics. There are no other changes made. ## TODO - [x] Restructure to support multiple signing methods. - [x] Add calls to remote signer from VC. - [x] Documentation - [x] Test all endpoints - [x] Test HTTPS certificate - [x] Allow adding remote signer validators via the API - [x] Add Altair support via [21.8.1-rc1](https://github.com/ConsenSys/web3signer/releases/tag/21.8.1-rc1) - [x] Create issue to start using latest version of web3signer. (See #2570) ## Notes - ~~Web3Signer doesn't yet support the Altair fork for Prater. See Consensys/web3signer#423 - ~~There is not yet a release of Web3Signer which supports Altair blocks. See Consensys/web3signer#391
michaelsproul
removed
the
waiting-on-author
|
Pull request successfully merged into unstable. Build succeeded: |
bors
bot
changed the title
paulhauner
added a commit
to paulhauner/lighthouse
that referenced
this pull request
Sep 21, 2021
commit a73dcb7 Author: Age Manning <[email protected]> Date: Fri Sep 17 04:02:31 2021 +0000 Improved handling of IP Banning (sigp#2530) This PR in general improves the handling around peer banning. Specifically there were issues when multiple peers under a single IP connected to us after we banned the IP for poor behaviour. This PR should now handle these peers gracefully as well as make some improvements around how we previously disconnected and banned peers. The logic now goes as follows: - Once a peer gets banned, its gets registered with its known IP addresses - Once enough banned peers exist under a single IP that IP is banned - We retain connections with existing peers under this IP - Any new connections under this IP are rejected commit 64ad2af Author: Pawan Dhananjay <[email protected]> Date: Fri Sep 17 01:11:16 2021 +0000 Subscribe to altair gossip topics 2 slots before fork (sigp#2532) ## Issue Addressed N/A ## Proposed Changes Add a fork_digest to `ForkContext` only if it is set in the config. Reject gossip messages on post fork topics before the fork happens. Edit: Instead of rejecting gossip messages on post fork topics, we now subscribe to post fork topics 2 slots before the fork. Co-authored-by: Age Manning <[email protected]> commit acdcea9 Author: Age Manning <[email protected]> Date: Thu Sep 16 04:45:07 2021 +0000 Update mainnet bootnodes (sigp#2594) Sigma Prime is transitioning our mainnet bootnodes and this PR represents the transition of our bootnodes. After a few releases, old boot-nodes will be deprecated. commit 56e0615 Author: Age Manning <[email protected]> Date: Thu Sep 16 04:45:05 2021 +0000 Experimental discovery (sigp#2577) # Description A few changes have been made to discovery. In particular a custom re-write of an LRU cache which previously was read/write O(N) for all our sessions ~5k, to a more reasonable hashmap-style O(1). Further there has been reported issues in the current discv5, so added error handling to help identify the issue has been added. commit c5c7476 Author: Paul Hauner <[email protected]> Date: Thu Sep 16 03:26:33 2021 +0000 Web3Signer support for VC (sigp#2522) [EIP-3030]: https://eips.ethereum.org/EIPS/eip-3030 [Web3Signer]: https://consensys.github.io/web3signer/web3signer-eth2.html ## Issue Addressed Resolves sigp#2498 ## Proposed Changes Allows the VC to call out to a [Web3Signer] remote signer to obtain signatures. ## Additional Info ### Making Signing Functions `async` To allow remote signing, I needed to make all the signing functions `async`. This caused a bit of noise where I had to convert iterators into `for` loops. In `duties_service.rs` there was a particularly tricky case where we couldn't hold a write-lock across an `await`, so I had to first take a read-lock, then grab a write-lock. ### Move Signing from Core Executor Whilst implementing this feature, I noticed that we signing was happening on the core tokio executor. I suspect this was causing the executor to temporarily lock and occasionally trigger some HTTP timeouts (and potentially SQL pool timeouts, but I can't verify this). Since moving all signing into blocking tokio tasks, I noticed a distinct drop in the "atttestations_http_get" metric on a Prater node:  I think this graph indicates that freeing the core executor allows the VC to operate more smoothly. ### Refactor TaskExecutor I noticed that the `TaskExecutor::spawn_blocking_handle` function would fail to spawn tasks if it were unable to obtain handles to some metrics (this can happen if the same metric is defined twice). It seemed that a more sensible approach would be to keep spawning tasks, but without metrics. To that end, I refactored the function so that it would still function without metrics. There are no other changes made. ## TODO - [x] Restructure to support multiple signing methods. - [x] Add calls to remote signer from VC. - [x] Documentation - [x] Test all endpoints - [x] Test HTTPS certificate - [x] Allow adding remote signer validators via the API - [x] Add Altair support via [21.8.1-rc1](https://github.com/ConsenSys/web3signer/releases/tag/21.8.1-rc1) - [x] Create issue to start using latest version of web3signer. (See sigp#2570) ## Notes - ~~Web3Signer doesn't yet support the Altair fork for Prater. See Consensys/web3signer#423 - ~~There is not yet a release of Web3Signer which supports Altair blocks. See Consensys/web3signer#391 commit 58012f8 Author: Michael Sproul <[email protected]> Date: Wed Sep 15 00:01:18 2021 +0000 Shutdown gracefully on panic (sigp#2596) ## Proposed Changes * Modify the `TaskExecutor` so that it spawns a "monitor" future for each future spawned by `spawn` or `spawn_blocking`. This monitor future joins the handle of the child future and shuts down the executor if it detects a panic. * Enable backtraces by default by setting the environment variable `RUST_BACKTRACE`. * Spawn the `ProductionBeaconNode` on the `TaskExecutor` so that if a panic occurs during start-up it will take down the whole process. Previously we were using a raw Tokio `spawn`, but I can't see any reason not to use the executor (perhaps someone else can). ## Additional Info I considered using [`std::panic::set_hook`](https://doc.rust-lang.org/std/panic/fn.set_hook.html) to instantiate a custom panic handler, however this doesn't allow us to send a shutdown signal because `Fn` functions can't move variables (i.e. the shutdown sender) out of their environment. This also prevents it from receiving a `Logger`. Hence I decided to leave the panic handler untouched, but with backtraces turned on by default. I did a run through the code base with all the raw Tokio spawn functions disallowed by Clippy, and found only two instances where we bypass the `TaskExecutor`: the HTTP API and `InitializedValidators` in the VC. In both places we use `spawn_blocking` and handle the return value, so I figured that was OK for now. In terms of performance I think the overhead should be minimal. The monitor tasks will just get parked by the executor until their child resolves. I've checked that this covers Discv5, as the `TaskExecutor` gets injected into Discv5 here: https://github.com/sigp/lighthouse/blob/f9bba92db3468321b28ddd9010e26b359f88bafe/beacon_node/src/lib.rs#L125-L126 commit 95b1713 Author: Age Manning <[email protected]> Date: Tue Sep 14 08:28:35 2021 +0000 Reduce network debug noise (sigp#2593) The identify network debug logs can get quite noisy and are unnecessary to print on every request/response. This PR reduces debug noise by only printing messages for identify messages that offer some new information. commit 4755d4b Author: Wink Saville <[email protected]> Date: Tue Sep 14 06:48:26 2021 +0000 Update sloggers to v2.0.2 (sigp#2588) fixes sigp#2584 commit f9bba92 Author: Paul Hauner <[email protected]> Date: Mon Sep 13 23:01:19 2021 +0000 v1.5.2 (sigp#2595) ## Issue Addressed NA ## Proposed Changes Version bump ## Additional Info Please do not `bors` without my approval, I am still testing. commit e4ed42a Author: Squirrel <[email protected]> Date: Sun Sep 12 23:55:20 2021 +0000 Fix nightly bump num bigint (sigp#2591) ## Issue Addressed Builds again on latest nightly ## Proposed Changes Break was caused by: rust-lang/rust#88581 commit f7dd24c Author: Mason Stallmo <[email protected]> Date: Sat Sep 11 23:56:16 2021 +0000 Add quoted u64/u64_vec to SyncCommitteeSubscription (sigp#2589) ## Issue Addressed Resolves sigp#2582 ## Proposed Changes Use `quoted_u64` and `quoted_u64_vec` custom serde deserializers from `eth2_serde_utils` to support the proper Eth2.0 API spec for `/eth/v1/validator/sync_committee_subscriptions` ## Additional Info N/A commit 46cd67d Author: Mason Stallmo <[email protected]> Date: Sat Sep 11 06:07:20 2021 +0000 Case insensitive match for ForkName (sigp#2587) ## Issue Addressed sigp#2583 ## Proposed Changes Case insensitive match on `fork_name` when calling `ForkName::from_str` ## Additional Info N/A
This was referenced Oct 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Resolves #2498
Proposed Changes
Allows the VC to call out to a Web3Signer remote signer to obtain signatures.
Additional Info
Making Signing Functions
asyncTo allow remote signing, I needed to make all the signing functions
async. This caused a bit of noise where I had to convert iterators intoforloops.In
duties_service.rsthere was a particularly tricky case where we couldn't hold a write-lock across anawait, so I had to first take a read-lock, then grab a write-lock.Move Signing from Core Executor
Whilst implementing this feature, I noticed that we signing was happening on the core tokio executor. I suspect this was causing the executor to temporarily lock and occasionally trigger some HTTP timeouts (and potentially SQL pool timeouts, but I can't verify this). Since moving all signing into blocking tokio tasks, I noticed a distinct drop in the "atttestations_http_get" metric on a Prater node:
I think this graph indicates that freeing the core executor allows the VC to operate more smoothly.
Refactor TaskExecutor
I noticed that the
TaskExecutor::spawn_blocking_handlefunction would fail to spawn tasks if it were unable to obtain handles to some metrics (this can happen if the same metric is defined twice). It seemed that a more sensible approach would be to keep spawning tasks, but without metrics. To that end, I refactored the function so that it would still function without metrics. There are no other changes made.TODO
Notes
Web3Signer doesn't yet support the Altair fork for Prater. See Add support for Prater Altair fork Consensys/web3signer#423.There is not yet a release of Web3Signer which supports Altair blocks. See Support Altair Consensys/web3signer#391.