Remove duplication in block service for bn-proposer#4
Merged
AgeManning merged 299 commits intoAgeManning:bn-proposerfrom Mar 20, 2023
Merged
Remove duplication in block service for bn-proposer#4AgeManning merged 299 commits intoAgeManning:bn-proposerfrom
bn-proposer#4AgeManning merged 299 commits intoAgeManning:bn-proposerfrom
Conversation
* Ran Cargo fmt * Added Capella Data Structures to consensus/types
FIXME's: * consensus/fork_choice/src/fork_choice.rs * consensus/state_processing/src/per_epoch_processing/capella.rs * consensus/types/src/execution_payload_header.rs TODO's: * consensus/state_processing/src/per_epoch_processing/capella/partial_withdrawals.rs * consensus/state_processing/src/per_epoch_processing/capella/full_withdrawals.rs
* add capella gossip boiler plate * get everything compiling Co-authored-by: realbigsean <sean@sigmaprime.io Co-authored-by: Mark Mackey <mark@sigmaprime.io> * small cleanup * small cleanup * cargo fix + some test cleanup * improve block production * add fixme for potential panic Co-authored-by: Mark Mackey <mark@sigmaprime.io>
* Revert "Add more gossip verification conditions" This reverts commit 1430b56. * Revert "Add todos" This reverts commit 91efb9d. * Revert "Reprocess blob sidecar messages" This reverts commit 21bf3d3. * Add the coupled topic * Decode SignedBeaconBlockAndBlobsSidecar correctly * Process Block and Blobs in beacon processor * Remove extra blob publishing logic from vc * Remove blob signing in vc * Ugly hack to compile
* add eip4844 block processing * fix blob processing code * consensus logic fixes and cleanup * use safe arith
* Add transparent support * Add `Config` struct * Deprecate `enum_behaviour` * Partially remove enum_behaviour from project * Revert "Partially remove enum_behaviour from project" This reverts commit 46ffb7f. * Revert "Deprecate `enum_behaviour`" This reverts commit 89b64a6. * Add `struct_behaviour` * Tidy * Move tests into `ssz_derive` * Bump ssz derive * Fix comment * newtype transaparent ssz * use ssz transparent and create macros for per fork implementations * use superstruct map macros Co-authored-by: Paul Hauner <paul@paulhauner.com>
* start feature gating * feature gate withdrawals
## Issue Addressed Closes sigp#3460 ## Proposed Changes `blocks` and `block_min_delay` are never updated in the epoch summary Co-authored-by: Michael Sproul <micsproul@gmail.com>
## Issue Addressed ethereum/beacon-APIs#241 ethereum/beacon-APIs#242 ## Proposed Changes Implement two new endpoints for fetching blinded blocks and RANDAO mixes. Co-authored-by: realbigsean <sean@sigmaprime.io>
## Issue Addressed sigp#3702 Which issue # does this PR address? sigp#3702 ## Proposed Changes Added checkpoint-sync-url-timeout flag to cli. Added timeout field to ClientGenesis::CheckpointSyncUrl to utilize timeout set ## Additional Info Please provide any additional information. For example, future considerations or information useful for reviewers. Co-authored-by: GeemoCandama <104614073+GeemoCandama@users.noreply.github.com> Co-authored-by: Michael Sproul <micsproul@gmail.com>
## Issue Addressed Part of sigp#3651. ## Proposed Changes Add a flag for enabling the light client server, which should be checked before gossip/RPC traffic is processed (e.g. sigp#3693, sigp#3711). The flag is available at runtime from `beacon_chain.config.enable_light_client_server`. Additionally, a new method `BeaconChain::with_mutable_state_for_block` is added which I envisage being used for computing light client updates. Unfortunately its performance will be quite poor on average because it will only run quickly with access to the tree hash cache. Each slot the tree hash cache is only available for a brief window of time between the head block being processed and the state advance at 9s in the slot. When the state advance happens the cache is moved and mutated to get ready for the next slot, which makes it no longer useful for merkle proofs related to the head block. Rather than spend more time trying to optimise this I think we should continue prototyping with this code, and I'll make sure `tree-states` is ready to ship before we enable the light client server in prod (cf. sigp#3206). ## Additional Info I also fixed a bug in the implementation of `BeaconState::compute_merkle_proof` whereby the tree hash cache was moved with `.take()` but never put back with `.restore()`.
## Issue Addressed Closes sigp#3612 ## Proposed Changes - Iterates through BNs until it finds a non-optimistic head. A slight change in error behavior: - Previously: `spawn_contribution_tasks` did not return an error for a non-optimistic block head. It returned `Ok(())` logged a warning. - Now: `spawn_contribution_tasks` returns an error if it cannot find a non-optimistic block head. The caller of `spawn_contribution_tasks` then logs the error as a critical error. Co-authored-by: Michael Sproul <micsproul@gmail.com>
## Issue Addressed Closes sigp#3656 ## Proposed Changes * Replace `set-output` by `$GITHUB_OUTPUT` usage * Avoid rate-limits when installing `protoc` by making authenticated requests (continuation of sigp#3621) * Upgrade all Ubuntu 18.04 usage to 22.04 (18.04 is end of life) * Upgrade macOS-latest to explicit macOS-12 to silence warning * Use `actions/checkout@v3` and `actions/cache@v3` to avoid deprecated NodeJS v12 ## Additional Info Can't silence the NodeJS warnings entirely due to sigp#3705. Can fix that in future.
This fixes issues with certain metrics scrapers, which might error if the content-type is not correctly set. ## Issue Addressed Fixes sigp#3437 ## Proposed Changes Simply set header: `Content-Type: text/plain` on metrics server response. Seems like the errored branch does this correctly already. ## Additional Info This is needed also to enable influx-db metric scraping which work very nicely with Geth.
## Issue Addressed NA ## Proposed Changes Updates our `ef_tests` to use: https://github.com/ethereum/consensus-specs/releases/tag/v1.3.0-rc.3 This required: - Skipping a `merkle_proof_validity` test (see sigp#4022) - Account for the `eip4844` tests changing name to `deneb` - My IDE did some Python linting during this change. It seemed simple and nice so I left it there. ## Additional Info NA
This adds some documentation for the Siren app into the Lighthouse book. Co-authored-by: Mavrik <mrricki.m.usmc@gmail.com>
## Issue Addressed NA ## Proposed Changes Adds two new `DEBG` logs to the HTTP API: 1. As soon as we are requested to produce a block. 2. As soon as a signed block is received. In sigp#3858 we added some very helpful logs to the VC so we could see when things are happening with block proposals in the VC. After doing some more debugging, I found that I can tell when the VC is sending a block but I *can't* tell the time that the BN receives it (I can only get the time after the BN has started doing some work with the block). Knowing when the VC published and as soon as the BN receives is useful for determining the delays introduced by network latency (and some other things like JSON decoding, etc). ## Additional Info NA
## Issue Addressed Cleans up all the remnants of 4844 in capella. This makes sure when 4844 is reviewed there is nothing we are missing because it got included here ## Proposed Changes drop a bomb on every 4844 thing ## Additional Info Merge process I did (locally) is as follows: - squash merge to produce one commit - in new branch off unstable with the squashed commit create a `git revert HEAD` commit - merge that new branch onto 4844 with `--strategy ours` - compare local 4844 to remote 4844 and make sure the diff is empty - enjoy Co-authored-by: Paul Hauner <paul@paulhauner.com>
## Proposed Changes Remove built-in support for Ropsten and Kiln via the `--network` flag. Both testnets are long dead and deprecated. This shaves about 30MiB off the binary size, from 135MiB to 103MiB (maxperf), or 165MiB to 135MiB (release).
## Issue Addressed Cleaner resolution for sigp#4006 ## Proposed Changes We are currently subscribing to core topics of new forks way before the actual fork since we had just a single `CORE_TOPICS` array. This PR separates the core topics for every fork and subscribes to only required topics based on the current fork. Also adds logic for subscribing to the core topics of a new fork only 2 slots before the fork happens. 2 slots is to give enough time for the gossip meshes to form. Currently doesn't add logic to remove topics from older forks in new forks. For e.g. in the coupled 4844 world, we had to remove the `BeaconBlock` topic in favour of `BeaconBlocksAndBlobsSidecar` at the 4844 fork. It should be easy enough to add though. Not adding it because I'm assuming that sigp#4019 will get merged before this PR and we won't require any deletion logic. Happy to add it regardless though.
…igp#4036) ## Issue Addressed sigp#3985 ## Proposed Changes Log a debug message when a BN candidate returns an error. `Mar 01 16:40:24.011 DEBG Request to beacon node failed error: ServerMessage(ErrorMessage { code: 503, message: "SERVICE_UNAVAILABLE: beacon node is syncing: head slot is 8416, current slot is 5098402", stacktraces: [] }), node: http://localhost:5052/`
…4037) ## Issue Addressed NA ## Proposed Changes As discovered in sigp#4034, Lighthouse is not accepting `latest_valid_hash == None` in an `INVALID` response to `newPayload`. The `null`/`None` response *was* illegal at one point, however it was added in ethereum/execution-apis#254. This PR brings Lighthouse in line with the standard and should fix the root cause of what sigp#4034 patched around. ## Additional Info NA
## Issue Addressed
NA
## Proposed Changes
Adds a service which periodically polls (11s into each mainnet slot) the `node/version` endpoint on each BN and roughly measures the round-trip latency. The latency is exposed as a `DEBG` log and a Prometheus metric.
The `--latency-measurement-service` has been added to the VC, with the following options:
- `--latency-measurement-service true`: enable the service (default).
- `--latency-measurement-service`: (without a value) has the same effect.
- `--latency-measurement-service false`: disable the service.
## Additional Info
Whilst looking at our staking setup, I think the BN+VC latency is contributing to late blocks. Now that we have to wait for the builders to respond it's nice to try and do everything we can to reduce that latency. Having visibility is the first step.
## Issue Addressed Closes sigp#3896 Closes sigp#3998 Closes sigp#3700 ## Proposed Changes - Optimise the calculation of withdrawals for payload attributes by avoiding state clones, avoiding unnecessary state advances and reading from the snapshot cache if possible. - Use the execution layer's payload attributes cache to avoid re-calculating payload attributes. I actually implemented a new LRU cache just for withdrawals but it had the exact same key and most of the same data as the existing payload attributes cache, so I deleted it. - Add a new SSE event that fires when payloadAttributes are calculated. This is useful for block builders, a la ethereum/beacon-APIs#244. - Add a new CLI flag `--always-prepare-payload` which forces payload attributes to be sent with every fcU regardless of connected proposers. This is intended for use by builders/relays. For maximum effect, the flags I've been using to run Lighthouse in "payload builder mode" are: ``` --always-prepare-payload \ --prepare-payload-lookahead 12000 \ --suggested-fee-recipient 0x0000000000000000000000000000000000000000 ``` The fee recipient is required so Lighthouse has something to pack in the payload attributes (it can be ignored by the builder). The lookahead causes fcU to be sent at the start of every slot rather than at 8s. As usual, fcU will also be sent after each change of head block. I think this combination is sufficient for builders to build on all viable heads. Often there will be two fcU (and two payload attributes) sent for the same slot: one sent at the start of the slot with the head from `n - 1` as the parent, and one sent after the block arrives with `n` as the parent. Example usage of the new event stream: ```bash curl -N "http://localhost:5052/eth/v1/events?topics=payload_attributes" ``` ## Additional Info - [x] Tests added by updating the proposer re-org tests. This has the benefit of testing the proposer re-org code paths with withdrawals too, confirming that the new changes don't interact poorly. - [ ] Benchmarking with `blockdreamer` on devnet-7 showed promising results but I'm yet to do a comparison to `unstable`. Co-authored-by: Michael Sproul <micsproul@gmail.com>
## Issue Addressed Closes sigp#3963 (hopefully) ## Proposed Changes Compute attestation selection proofs gradually each slot rather than in a single `join_all` at the start of each epoch. On a machine with 5k validators this replaces 5k tasks signing 5k proofs with 1 task that signs 5k/32 ~= 160 proofs each slot. Based on testing with Goerli validators this seems to reduce the average time to produce a signature by preventing Tokio and the OS from falling over each other trying to run hundreds of threads. My testing so far has been with local keystores, which run on a dynamic pool of up to 512 OS threads because they use [`spawn_blocking`](https://docs.rs/tokio/1.11.0/tokio/task/fn.spawn_blocking.html) (and we haven't changed the default). An earlier version of this PR hyper-optimised the time-per-signature metric to the detriment of the entire system's performance (see the reverted commits). The current PR is conservative in that it avoids touching the attestation service at all. I think there's more optimising to do here, but we can come back for that in a future PR rather than expanding the scope of this one. The new algorithm for attestation selection proofs is: - We sign a small batch of selection proofs each slot, for slots up to 8 slots in the future. On average we'll sign one slot's worth of proofs per slot, with an 8 slot lookahead. - The batch is signed halfway through the slot when there is unlikely to be contention for signature production (blocks are <4s, attestations are ~4-6 seconds, aggregates are 8s+). ## Performance Data _See first comment for updated graphs_. Graph of median signing times before this PR:  Graph of update attesters metric (includes selection proof signing) before this PR:  Median signing time after this PR (prototype from 12:00, updated version from 13:30):  99th percentile on signing times (bounded attestation signing from 16:55, now removed):  Attester map update timing after this PR:  Selection proof signings per second change:  ## Link to late blocks I believe this is related to the slow block signings because logs from Stakely in sigp#3963 show these two logs almost 5 seconds apart: > Feb 23 18:56:23.978 INFO Received unsigned block, slot: 5862880, service: block, module: validator_client::block_service:393 > Feb 23 18:56:28.552 INFO Publishing signed block, slot: 5862880, service: block, module: validator_client::block_service:416 The only thing that happens between those two logs is the signing of the block: https://github.com/sigp/lighthouse/blob/0fb58a680d6f0c9f0dc8beecf142186debff9a8d/validator_client/src/block_service.rs#L410-L414 Helpfully, Stakely noticed this issue without any Lighthouse BNs in the mix, which pointed to a clear issue in the VC. ## TODO - [x] Further testing on testnet infrastructure. - [x] Make the attestation signing parallelism configurable.
## Proposed Changes Fix the cargo audit failure caused by [RUSTSEC-2023-0018](https://rustsec.org/advisories/RUSTSEC-2023-0018) which we were exposed to via `tempfile`. ## Additional Info I've held back the libp2p crate for now because it seemed to introduce another duplicate dependency on libp2p-core, for a total of 3 copies. Maybe that's fine, but we can sort it out later.
## Issue Addressed NA ## Proposed Changes In sigp#4024 we added metrics to expose the latency measurements from a VC to each BN. Whilst playing with these new metrics on our infra I realised it would be great to have a single metric to make sure that the primary BN for each VC has a reasonable latency. With the current "metrics for all BNs" it's hard to tell which is the primary. ## Additional Info NA
## Issue Addressed NA ## Proposed Changes Sets the Capella fork epoch as per eth-clients/goerli#160. The fork will occur at: - Epoch: 162304 - Slot: 5193728 - UTC: 14/03/2023, 10:25:36 pm ## Additional Info - [x] Blocked on eth-clients/goerli#160 being merged
## Issue Addressed sigp#4040 ## Proposed Changes - Add the `always_prefer_builder_payload` field to `Config` in `beacon_node/client/src/config.rs`. - Add that same field to `Inner` in `beacon_node/execution_layer/src/lib.rs` - Modify the logic for picking the payload in `beacon_node/execution_layer/src/lib.rs` - Add the `always-prefer-builder-payload` flag to the beacon node CLI - Test the new flags in `lighthouse/tests/beacon_node.rs` Co-authored-by: Paul Hauner <paul@paulhauner.com>
## Issue Addressed NA ## Proposed Changes Bumps versions to v3.5.1. ## Additional Info - [x] Requires further testing
See: sigp#4027 ## Proposed Changes The order of the arguments to `log_count` is swapped in `beacon_node/beacon_chain/src/events.rs`.
## Issue Addressed sigp#3435 ## Proposed Changes Fire a warning with the path of JWT to be created when the path given by --execution-jwt is not found Currently, the same error is logged if the jwt is found but doesn't match the execution client's jwt, and if no jwt was found at the given path. This makes it very hard to tell if you accidentally typed the wrong path, as a new jwt is created silently that won't match the execution client's jwt. So instead, it will now fire a warning stating that a jwt is being generated at the given path. ## Additional Info In the future, it may be smarter to handle this case by adding an InvalidJWTPath member to the Error enum in lib.rs or auth.rs that can be handled during upcheck() This is my first PR and first project with rust. so thanks to anyone who looks at this for their patience and help! Co-authored-by: Sebastian Richel <47844429+sebastianrich18@users.noreply.github.com>
## Proposed Changes The current `/lighthouse/nat` implementation checks for _zero_ address updated messages, when it should check for a _non-zero_ number. This was spotted while debugging an issue on Discord where a user's ports weren't forwarded but `/lighthouse/nat` was still returning `true`.
## Issue Addressed Add support for ipv6 and dual stack in lighthouse. ## Proposed Changes From an user perspective, now setting an ipv6 address, optionally configuring the ports should feel exactly the same as using an ipv4 address. If listening over both ipv4 and ipv6 then the user needs to: - use the `--listen-address` two times (ipv4 and ipv6 addresses) - `--port6` becomes then required - `--discovery-port6` can now be used to additionally configure the ipv6 udp port ### Rough list of code changes - Discovery: - Table filter and ip mode set to match the listening config. - Ipv6 address, tcp port and udp port set in the ENR builder - Reported addresses now check which tcp port to give to libp2p - LH Network Service: - Can listen over Ipv6, Ipv4, or both. This uses two sockets. Using mapped addresses is disabled from libp2p and it's the most compatible option. - NetworkGlobals: - No longer stores udp port since was not used at all. Instead, stores the Ipv4 and Ipv6 TCP ports. - NetworkConfig: - Update names to make it clear that previous udp and tcp ports in ENR were Ipv4 - Add fields to configure Ipv6 udp and tcp ports in the ENR - Include advertised enr Ipv6 address. - Add type to model Listening address that's either Ipv4, Ipv6 or both. A listening address includes the ip, udp port and tcp port. - UPnP: - Kept only for ipv4 - Cli flags: - `--listen-addresses` now can take up to two values - `--port` will apply to ipv4 or ipv6 if only one listening address is given. If two listening addresses are given it will apply only to Ipv4. - `--port6` New flag required when listening over ipv4 and ipv6 that applies exclusively to Ipv6. - `--discovery-port` will now apply to ipv4 and ipv6 if only one listening address is given. - `--discovery-port6` New flag to configure the individual udp port of ipv6 if listening over both ipv4 and ipv6. - `--enr-udp-port` Updated docs to specify that it only applies to ipv4. This is an old behaviour. - `--enr-udp6-port` Added to configure the enr udp6 field. - `--enr-tcp-port` Updated docs to specify that it only applies to ipv4. This is an old behaviour. - `--enr-tcp6-port` Added to configure the enr tcp6 field. - `--enr-addresses` now can take two values. - `--enr-match` updated behaviour. - Common: - rename `unused_port` functions to specify that they are over ipv4. - add functions to get unused ports over ipv6. - Testing binaries - Updated code to reflect network config changes and unused_port changes. ## Additional Info TODOs: - use two sockets in discovery. I'll get back to this and it's on sigp/discv5#160 - lcli allow listening over two sockets in generate_bootnodes_enr - add at least one smoke flag for ipv6 (I have tested this and works for me) - update the book
## Issue Addressed In sigp#4027 I forgot to add the `parent_block_number` to the payload attributes SSE. ## Proposed Changes Compute the parent block number while computing the pre-payload attributes. Pass it on to the SSE stream. ## Additional Info Not essential for v3.5.1 as I suspect most builders don't need the `parent_block_root`. I would like to use it for my dummy no-op builder however.
AgeManning
pushed a commit
that referenced
this pull request
Oct 16, 2023
* introduce availability pending block * add intoavailableblock trait * small fixes * add 'gossip blob cache' and start to clean up processing and transition types * shard memory blob cache * Initial commit * Fix after rebase * Add gossip verification conditions * cache cleanup * general chaos * extended chaos * cargo fmt * more progress * more progress * tons of changes, just tryna compile * everything, everywhere, all at once * Reprocess an ExecutedBlock on unavailable blobs * Add sus gossip verification for blobs * Merge stuff * Remove reprocessing cache stuff * lint * Add a wrapper to allow construction of only valid `AvailableBlock`s * rename blob arc list to blob list * merge cleanuo * Revert "merge cleanuo" This reverts commit 5e98326. * Revert "Revert "merge cleanuo"" This reverts commit 3a40094. * fix rpc methods * move beacon block and blob to eth2/types * rename gossip blob cache to data availability checker * lots of changes * fix some compilation issues * fix compilation issues * fix compilation issues * fix compilation issues * fix compilation issues * fix compilation issues * cargo fmt * use a common data structure for block import types * fix availability check on proposal import * refactor the blob cache and split the block wrapper into two types * add type conversion for signed block and block wrapper * fix beacon chain tests and do some renaming, add some comments * Partial processing (#4) * move beacon block and blob to eth2/types * rename gossip blob cache to data availability checker * lots of changes * fix some compilation issues * fix compilation issues * fix compilation issues * fix compilation issues * fix compilation issues * fix compilation issues * cargo fmt * use a common data structure for block import types * fix availability check on proposal import * refactor the blob cache and split the block wrapper into two types * add type conversion for signed block and block wrapper * fix beacon chain tests and do some renaming, add some comments * cargo update (#6) --------- Co-authored-by: realbigsean <sean@sigmaprime.io> Co-authored-by: realbigsean <seananderson33@gmail.com>
This file contains hidden or 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
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
NA
Proposed Changes
Related to sigp#3328, this PR removes the duplication in the
block_service.rsmentioned in sigp#3328 (comment).To make this work I had to merge in
unstable, which has created a huge diff. You can ignore the merge noise by just looking at this commit: paulhauner@23aa8e7You can also see the diff between this branch and
unstableat https://github.com/sigp/lighthouse/compare/unstable...paulhauner:lighthouse:bn-proposer-paul?expand=1You can also do the
unstable->bn-proposermerge yourself if you want, but it was a bit annoying (not too bad tho).Additional Info
NA