Stylistic; mostly using early returns to avoid the nested logic
Which issue # does this PR address?
Please list or describe the changes introduced by this PR.
A performance issue was discovered when devnet-3 was under non-finality - some of the lighthouse nodes are "stuck" with syncing because of handling proposer duties HTTP requests.
These validator requests are higher priority than Status processing, and if they are taking a long time to process, the node won't be able to progress. What's worse is - under long period of non finality, the proposer duties calculation function tries to do state advance for a large number of [slots](d545ddcbc7/beacon_node/beacon_chain/src/beacon_proposer_cache.rs (L183)) here, causing the node to spend all its CPU time on a task that doesn't really help, e.g. the computed duties aren't useful if the node is 20000 slots behind.
To solve this issue, we use the `not_while_syncing` filter to prevent serving these requests, until the node is synced. This should allow the node to focus on sync under non-finality situations.
Addresses #7866.
Use Rayon to speed up batch KZG verification during range / backfill sync.
While I was analysing the traces, I also discovered a bug that resulted in only the first 128 columns in a chain segment batch being verified. This PR fixes it, so we might actually observe slower range sync due to more cells being KZG verified.
I've also updated the handling of batch KZG failure to only find the first invalid KZG column when verification fails as this gets very expensive during range/backfill sync.
N/A
Currently, backfill is allowed to create upto 20 pending batches which is unnecessarily high imo. Forward sync also allows a max of 5 batches to be buffered at a time. This PR reduces the batch size to match with forward sync.
Having high number of batches is a little annoying with peerdas because we try to create and send 20 requests (even though we are processing them in a rate limited manner). Requests with peerdas is a lot more heavy as we distribute requests across multiple peers leading to lot of requests that may keep getting retried. This could take resources away from processing at head.
Previously `kurtosis service inspect` gives us output like this - flags in separate lines
```
CMD:
lighthouse
beacon_node
--debug-level=debug
--datadir=/data/lighthouse/beacon-data
--listen-address=0.0.0.0
--port=9000
--http
--http-address=0.0.0.0
--http-port=4000
--disable-packet-filter
--execution-endpoints=http://172.16.0.8:8551
--jwt-secrets=/jwt/jwtsecret
--suggested-fee-recipient=0x8943545177806ED17B9F23F0a21ee5948eCaa776
--disable-enr-auto-update
--enr-address=172.16.0.11
```
In the latest version this has been updated to a single line
```
CMD:
exec lighthouse beacon_node --debug-level=debug --datadir=/data/lighthouse/beacon-data --listen-address=0.0.0.0 --port=9000 --http --http-address=0.0.0.0 --http-port=4000 --disable-packet-filter --execution-endpoints=http://172.16.0.12:8551 --jwt-secrets=/jwt/jwtsecret --suggested-fee-recipient=0x8943545177806ED17B9F23F0a21ee5948eCaa776 --disable-enr-auto-update --enr-address=172.16.0.18 --enr-tcp-port=9000 --enr-udp-port=9000 --enr-quic-port=9001 --quic-port=9001 --metrics --metrics-address=0.0.0.0 --metrics-allow-origin=* --metrics-port=5054 --enable-private-discovery --testnet-dir=/network-configs --boot-nodes=enr:-N24QPYP7bj0aqoM2dXsP5hnosW27U6PTYJt1kYFhNkwIvlFQhGJ1om7f4zcHhVJwvUL7wCsVbDJbP_l-TF8X3q4pVEDh2F0dG5ldHOIAAAwAAAAAACGY2xpZW500YpMaWdodGhvdXNlhTcuMS4whGV0aDKQqFs_bWAAADj__________4JpZIJ2NIJpcISsEAAPhHF1aWOCIymJc2VjcDI1NmsxoQK_z4HQylgsOal74Jek9D_EhY0vcDX5AcLHnPD7iOeEdYhzeW5jbmV0cwCDdGNwgiMog3VkcIIjKA --target-peers=3
```
and it broke our script. This PR update the extraction logic.
This method is a footgun because it truncates the list. It is the source of a recent bug:
- https://github.com/sigp/lighthouse/pull/7927
- Delete uses of `RuntimeVariableList::from_vec` and replace them with `::new` which does validation and can fail.
- Propagate errors where possible, unwrap in tests and use `expect` for obviously-safe uses (in `chain_spec.rs`).
N/A
In https://github.com/sigp/lighthouse/pull/7897 , we seem to have modified data columns by range to return all the columns we have for the requested epoch disregarding what columns the peer requested.
When gossip data column processing completes and results in a block import, sync is currently not notified of the successful import. This is inconsistent with how blob processing and block processing both notify sync.
This fix ensures lookup sync receives block import notifications when blocks become available through gossip data column.
Similar to `BEACON_PROCESSOR_WORKERS_ACTIVE_TOTAL` but this metric also records the work type.
This is useful in identifying the task when a worker is stuck due to a deadlock or something else, and usually difficult to debug in production / release mode.
This PR enables some sync tests by default on CI - this will help catch breakages on sync happy paths, e.g. this would have caught the bug #7926 if lighthouse is not able to sync OR serve sync requests.
The enabled tests are genesis-sync with 120s / 300s offline time, and should cover both serving / consuming by root and by range requests.
I'm leaving the checkpoint sync tests optional, as it has external dependencies on checkpoint server (which may cause CI instability) and may cause extra loads on them.
Adds the required boilerplate code for the Gloas (Glamsterdam) hard fork. This allows PRs testing Gloas-candidate features to test fork transition.
This also includes de-duplication of post-Bellatrix readiness notifiers from #6797 (credit to @dapplion)
Fixes#7926
This was a bug I introduced in #7890 and @pawanjay176 noticed it on some running nodes, and added a rpc test to confirm it.
The culprit is this line, where I failed to fill the vec to it's max size, so it doesn't calculate the max size properly, resulting in all `DataColumnByRoot` requests exceeding the max size during validation:
d24a6d2a45/consensus/types/src/chain_spec.rs (L1984)
The PR fixes this and includes new regression tests for this fix.
With Fulu, we increment the engine API version for `get_payload` but we do not also increment `new_payload`.
In Lighthouse, we have a tight coupling between these version numbers and the Fork variants.
For example, both `get_payload_v3` and `new_payload_v3` correspond to Deneb, `v4` to Electra, etc.
However this coupling breaks with Fulu where only `get_payload_v5` is related to Fulu and `new_payload_v4` now also corresponds to Fulu (new_payload_v5 does not exist). While we can work around this, it creates a confusing situation where the versions and Fork variants are out of sync.
See the following code snippet where we are using the v4 endpoint, and yet passing a `V5` payload variant: 522bd9e9c6/beacon_node/execution_layer/src/engine_api/http.rs (L849-L870)
1. Remove `new_payload_v5` as it is unused in Fulu.
2. Rename the `JsonExecutionPayload` and `JsonGetExecutionPayloadResponse` types to use Fork variants instead of version variants. This clarifies the relationship between them.
Was going to leave this as a comment on #7877 but when noticed it had already been merged.
we have `DEFAULT_TARGET_PEERS` which was set to 50 and only used on the `Default` impl for `peer_manager`'s `Config`, which then get's overridden by this `lighthouse_network::Config`s default
This PR unifies everything on `DEFAULT_TARGET_PEERS`
This PR is an optimisation to avoid unnecessary database lookups when peer requests data columns that the node doesn't custody (advertised via `cgc`).
e.g. an extreme but realistic example - a full node only store 4 custody columns by default, but it may receive a range request of 32 slots with all 128 columns, and this would result in 4096 database lookups but the node is only able to get 128 (4 * 32) of them.
- Filter data column RPC requests (`DataColumnsByRoot`, `DataColumnsByRange`) to only lookup columns the node custodies
- Prevents unnecessary database queries that would always fail for non-custody columns
This PR adds tempo to kurtosis config and will collect lighthouse traces on kurtosis local testnet. The traces can be viewed / queried from Grafana.
Also updated fulu kurtosis configs to use latest geth image.
Allows users to customize the OpenTelemetry service name instead of using the hardcoded default `lighthouse`. Defaults to 'lighthouse-bn' for beacon node, 'lighthouse-vc' for validator client, or 'lighthouse' for other subcommands.
This is useful when analysing traces from multiple nodes, see Grafana screenshot below with service name overrides in Kurtosis (`ethereum-package` PR: https://github.com/ethpandaops/ethereum-package/pull/1160):
<img width="1148" height="627" alt="image" src="https://github.com/user-attachments/assets/7e875639-10f7-4756-837f-2006fa4b12e0" />
Since we updated to edition 2024 my Vim plugin for rustfmt is formatting code incorrectly, with 2018 settings:
889b9a7515/autoload/rustfmt.vim (L74-L75)
Arguably this plugin is a bit junk, but I think it's fairly harmless to add this config.
Add `rustfmt.toml`. This is a generic config file for `rustfmt` which is probably useful for `rustfmt` integration with other editors too.
We may want to add other config to `rustfmt.toml` over time as well, I think this was discussed recently.
#7181
Instead of storing the network key as binary data we store it as hex, allowing users to modify it via the file.
We can read old-binary forms, however we will migrate binary to hex as it will be the new standard.
Follow-up to:
- https://github.com/sigp/lighthouse/pull/7764
The `heaptrack` feature added in my previous PR was ineffective, because the jemalloc feature was turned on by the Linux target-specific dependency.
This PR tweaks the features such that:
- The jemalloc feature is just used to control whether jemalloc is compiled in. It is enabled on Linux by the target-specific dependency (see `lighthouse/Cargo.toml`), and completely disabled on Windows.
- If the `sysmalloc` feature is set on Linux then it overrides jemalloc when selecting an allocator, _even if_ the jemalloc feature is enabled (and the jemalloc dep was compiled).
- Remove explicit `env_logger` usage from `state_processing` tests and `lcli`.
- Set up tracing correctly for `lcli` (I've checked that we can see logs after this change).
- I didn't do anything to set up logging for the `state_processing` tests, as these are rarely run manually (they never fail). We could add `test_logger` in there on an as-needed basis.
We temporarily can't build sccache on windows runners, but it's still available on linux.
this smol change lets us use it when available, instead of disabling across the board.
The Windows runners now have a conditional check to disable (unset the `rustc-wrapper` env var) sccache in their entrypoint, just like the Linux ones have.
Also the workflows no longer fail when `sccache --show-stats` fails.
Re-export `context_deserialize_derive` inside of `context_deserialize` so they are both available from the same interface, which matches how popular crates (like `serde`) handle this.
This also nests both crates inside a new `context_deserialize` directory which will make it easier to eventually spin out into a different repo (if/when) we decide to do that (plus I prefer it aesthetically).
Attempt to fix this error reported by `beaconcha.in` on their Hoodi archive nodes:
> {"code":500,"message":"UNHANDLED_ERROR: DBError(CacheBuildError(BeaconState(MilhouseError(OutOfBoundsIterFrom { index: 1199549, len: 1060000 }))))","stacktraces":[]}
There are only a handful of places where we call `iter_from`.
This one is safe by construction (the check immediately prior ensures `self.pubkeys.len()` is not out of bounds):
cfb1f73310/beacon_node/beacon_chain/src/validator_pubkey_cache.rs (L84-L90)
This one should also be safe, and the indexes used here would not be as large as the ones in the reported error:
cfb1f73310/consensus/state_processing/src/per_epoch_processing/single_pass.rs (L365-L368)
Which leaves one remaining usage which must be the culprit:
cfb1f73310/consensus/types/src/beacon_state.rs (L2109-L2113)
This indexing relies on the invariant that `self.pubkey_cache().len() <= self.validators.len()`. We mostly maintain that invariant, except for in `rebase_caches_on` (fixed in this PR).
The other bug, is that we were calling `rebase_on_finalized` for all "hot" states, which post-v7.1.0 includes states prior to the split which are required by the hdiff grid. This is how we end up calling something like `genesis_state.rebase_on(&split_state)`, which then corrupts the pubkey cache of the genesis state using the newer pubkey cache from the split state.
This PR adds a created_timestamp to the beacon processor send channel. When work items are sent through that channel `try_send` will forward the work event along with the current timestamp to the beacon processor. When the work event is completed the `Drop` impl for `SendOnDrop` will track the time it took from work event creation to its completion. Previously we only had data on how long a work event took to process, but didn't have data on how long it sat in the queue + how long it took to process.
N/A
Add a flag to disable get blobs. I configured the flag to disable it regardless of version because its most likely something we use for testing anyway.
#7815
- removes all existing spans, so some span fields that appear in logs like `service_name` may be lost.
- instruments a few key code paths in the beacon node, starting from **root spans** named below:
* Gossip block and blobs
* `process_gossip_data_column_sidecar`
* `process_gossip_blob`
* `process_gossip_block`
* Rpc block and blobs
* `process_rpc_block`
* `process_rpc_blobs`
* `process_rpc_custody_columns`
* Rpc blocks (range and backfill)
* `process_chain_segment`
* `PendingComponents` lifecycle
* `pending_components`
To test locally:
* Run Grafana and Tempo with https://github.com/sigp/lighthouse-metrics/pull/57
* Run Lighthouse BN with `--telemetry-collector-url http://localhost:4317`
Some captured traces can be found here: https://hackmd.io/@jimmygchen/r1sLOxPPeg
Removing the old spans seem to have reduced the memory usage quite a lot - i think we were using them on long running tasks and too excessively:
<img width="910" height="495" alt="image" src="https://github.com/user-attachments/assets/5208bbe4-53b2-4ead-bc71-0b782c788669" />
