perf: remove unnecessary copy in AggregateSignature::serialize #8560
+13,623
−13,327
Conversation
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
Co-Authored-By: kevaundray <[email protected]>
Co-Authored-By: Tan Chee Keong <[email protected]>
sigp#8012 Replace all instances of `VariableList::from` and `FixedVector::from` to their `try_from` variants. While I tried to use proper error handling in most cases, there were certain situations where adding an `expect` for situations where `try_from` can trivially never fail avoided adding a lot of extra complexity. Co-Authored-By: Mac L <[email protected]> Co-Authored-By: Michael Sproul <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
Use the recently published `compare_fields` and remove it from Lighthouse https://crates.io/crates/compare_fields Co-Authored-By: Mac L <[email protected]>
Currently the `eth2` crate lib file is a large monolith of almost 3000 lines of code. As part of the bosun migration we are trying to increase code readability and modularity in the lighthouse crates initially, which then can be transferred to bosun Co-Authored-By: hopinheimer <[email protected]> Co-Authored-By: hopinheimer <[email protected]>
Partially addresses sigp#8248 Run the beacon chain, http and network tests only for recent forks instead of everything from phase 0. Also added gloas also to the recent forks list. I thought that would be a good way to know if changes in the current fork affect future forks. Not completely sure if we should run for future forks, but added it so that we can discuss here. Co-Authored-By: Pawan Dhananjay <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>
Update the EF spec tests to v1.6.0-beta.1 There are a few new light client tests (which we pass), and some for progressive containers, which we haven't implemented (we ignore them). Co-Authored-By: Michael Sproul <[email protected]>
…rk (sigp#8259) Addresses this comment: sigp#8254 (comment) We're currently using `subscribe_all_data_column_subnets` here to subscribe to all subnets https://github.com/sigp/lighthouse/blob/522bd9e9c6ac167f2231525e937c9ebbcb86cf6e/beacon_node/lighthouse_network/src/types/topics.rs#L82-L92 But its unnecessary because the else path also works for supernode (uses `sampling_subnets` instead) The big diffs will disappear once sigp#8254 is merged. Co-Authored-By: Jimmy Chen <[email protected]>
Co-Authored-By: Tan Chee Keong <[email protected]>
sigp#8311 Removes the `git_version` crate from `lighthouse_version` and implements git `HEAD` tracking manually. This removes the (mostly) broken dirty tracking but prevents spurious recompilation of the `lighthouse_version` crate. This also reworks the way crate versions are handled by utilizing workspace version inheritance and Cargo environment variables. This means the _only_ place where Lighthouse's version is defined is in the top level `Cargo.toml` for the workspace. All relevant binaries then inherit this version. This largely makes the `change_version.sh` script useless so I've removed it, although we could keep a version which just alters the workspace version (if we need to maintain compatibility with certain build/release tooling. ### When is a Rebuild Triggered? 1. When the build.rs file is changed. 2. When the HEAD commit changes (added, removed, rebased, etc) 3. When the branch changes (this includes changing to the current branch, and creating a detached HEAD) Note that working/staged changes will not trigger a recompile of `lighthouse_version`. Co-Authored-By: Mac L <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
`beacon-chain-tests` is now regularly taking 1h+ on CI since Fulu fork was added. This PR attemtpts to reduce the test time by bringing down the number of blobs generated in tests - instead of generating 0..max_blobs, the generator now generates 0..1 blobs by default, and this can be modified by setting `harness.execution_block_generator.set_min_blob_count(n)`. Note: The blobs are pre-generated and doesn't require too much CPU to generate however processing a larger number of them on the beacon chain does take a lot of time. This PR also include a few other small improvements - Our slowest test (`chain_segment_varying_chunk_size`) runs 3x faster in Fulu just by reusing chain segments - Avoid re-running fork specific tests on all forks - Fix a bunch of tests that depends on the harness's existing random blob generation, which is fragile beacon chain test time on test machine is **~2x** faster: ### `unstable` ``` Summary [ 751.586s] 291 tests run: 291 passed (13 slow), 0 skipped ``` ### this branch ``` Summary [ 373.792s] 291 tests run: 291 passed (2 slow), 0 skipped ``` The next set of tests to optimise is the ones that use [`get_chain_segment`](https://github.com/sigp/lighthouse/blob/77a9af96de0f693127055e381ece3e98dceea0a8/beacon_node/beacon_chain/tests/block_verification.rs#L45), as it by default build 320 blocks with supernode - an easy optimisation would be to build these blocks with cgc = 8 for tests that only require fullnodes. Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>
This is an optimisation targeted at Fulu networks in non-finality. While debugging on Holesky, we found that `state_root_at_slot` was being called from `prepare_beacon_proposer` a lot, for the finalized state: https://github.com/sigp/lighthouse/blob/2c9b670f5d313450252c6cb40a5ee34802d54fef/beacon_node/http_api/src/lib.rs#L3860-L3861 This was causing `prepare_beacon_proposer` calls to take upwards of 5 seconds, sometimes 10 seconds, because it would trigger _multiple_ beacon state loads in order to iterate back to the finalized slot. Ideally, loading the finalized state should be quick because we keep it cached in the state cache (technically we keep the split state, but they usually coincide). Instead we are computing the finalized state root separately (slow), and then loading the state from the cache (fast). Although it would be possible to make the API faster by removing the `state_root_at_slot` call, I believe it's simpler to change `state_root_at_slot` itself and remove the footgun. Devs rightly expect operations involving the finalized state to be fast. Co-Authored-By: Michael Sproul <[email protected]>
Remove all Windows-related CI jobs Co-Authored-By: antondlr <[email protected]>
Co-Authored-By: Tan Chee Keong <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
while working on this sigp#7892 @michaelsproul pointed it might be a good metric to measure the delay from start of the slot instead of the current `slot_duration / 3`, since the attestations duties start before the `1/3rd` mark now with the change in the link PR. Co-Authored-By: hopinheimer <[email protected]> Co-Authored-By: hopinheimer <[email protected]>
### Downgrade a non error to `Debug` I noticed this error on one of our hoodi nodes: ``` Nov 04 05:13:38.892 ERROR Error during data column reconstruction block_root: 0x4271b9efae7deccec3989bd2418e998b83ce8144210c2b17200abb62b7951190, error: DuplicateFullyImported(0x4271b9efae7deccec3989bd2418e998b83ce8144210c2b17200abb62b7951190) ``` This shouldn't be logged as an error and it's due to a normal race condition, and it doesn't impact the node negatively. ### Remove spammy logs This logs is filling up the log files quite quickly and it is also something we'd expect during normal operation - getting columns via EL before gossip. We haven't found this debug log to be useful, so I propose we remove it to avoid spamming debug logs. ``` Received already available column sidecar. Ignoring the column sidecar ``` In the process of removing this, I noticed we aren't propagating the validation result, which I think we should so I've added this. The impact should be quite minimal - the message will stay in the gossip memcache for a bit longer but should be evicted in the next heartbeat. Co-Authored-By: Jimmy Chen <[email protected]>
Another good candidate for publishing separately from Lighthouse is `sensitive_url` as it's a general utility crate and not related to Ethereum. This PR prepares it to be spun out into its own crate. I've made the `full` field on `SensitiveUrl` private and instead provided an explicit getter called `.expose_full()`. It's a bit ugly for the diff but I prefer the explicit nature of the getter. I've also added some extra tests and doc strings along with feature gating `Serialize` and `Deserialize` implementations behind the `serde` feature. Co-Authored-By: Mac L <[email protected]>
This compiles, is there any reason to keep `ecdsa`? CC @jxs Co-Authored-By: Michael Sproul <[email protected]>
Self hosted GitHub Runners review and improvements local testnet workflow now uses warpbuild ci runner Co-Authored-By: lemon <[email protected]> Co-Authored-By: antondlr <[email protected]>
Use the recently published `sensitive_url` and remove it from Lighthouse Co-Authored-By: Mac L <[email protected]>
Fixes sigp#7001. Mostly mechanical replacement of `derivative` attributes with `educe` ones. ### **Attribute Syntax Changes** ```rust // Bounds: = "..." → (...) #[derivative(Hash(bound = "E: EthSpec"))] #[educe(Hash(bound(E: EthSpec)))] // Ignore: = "ignore" → (ignore) #[derivative(PartialEq = "ignore")] #[educe(PartialEq(ignore))] // Default values: value = "..." → expression = ... #[derivative(Default(value = "ForkName::Base"))] #[educe(Default(expression = ForkName::Base))] // Methods: format_with/compare_with = "..." → method(...) #[derivative(Debug(format_with = "fmt_peer_set_as_len"))] #[educe(Debug(method(fmt_peer_set_as_len)))] // Empty bounds: removed entirely, educe can infer appropriate bounds #[derivative(Default(bound = ""))] #[educe(Default)] // Transparent debug: manual implementation (educe doesn't support it) #[derivative(Debug = "transparent")] // Replaced with manual Debug impl that delegates to inner field ``` **Note**: Some bounds use strings (`bound("E: EthSpec")`) for superstruct compatibility (`expected ','` errors). Co-Authored-By: Javier Chávarri <[email protected]> Co-Authored-By: Mac L <[email protected]>
FIx flaky tests that depends on timing. Previously the test processes all 128 columns and expect reconstruction to happen after all columns are processed. There is a race here, and reconstruction could be triggered before all columns are processed. I've updated the tests to process 64 columns, just enough for reconstruction and wait for 50ms for reconstruction to be triggered. This PR requires the change made in sigp#8194 for the test to pass consistently (blob count set to 1 for all blocks instead of random blob count between 0..max) Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>
sigp#6022 Use `alloy_rpc_types::Transaction` to replace the `ethers_core::Transaction` inside the execution block generator. Co-Authored-By: Mac L <[email protected]>
Debugging sigp#8104 it would have been helpful to quickly see in the logs that a specific block was submitted into the HTTP API. Because we want to optimize the block root computation we don't include it in the logs, and just log the block slot. I believe we can take a minute performance hit to have the block root in all the logs during block publishing. Co-Authored-By: dapplion <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>
…p#8315) since block and blob both start with `bl`, it was not clear how to differentiate between `blbroots_queue` and `bbroots_queue` After renaming, there also seems to be a discrepancy Co-Authored-By: Kevaundray Wedderburn <[email protected]>
sigp#6022 Switches the `deposit_contract` crate to use the `alloy` ecosystem and removes the dependency on `ethabi` Co-Authored-By: Mac L <[email protected]>
Part of a fork-choice tech debt clean-up sigp#8325 sigp#7089 (non-finalized checkpoint sync) changes the meaning of the checkpoints inside fork-choice. It turns out that we persist the justified and finalized checkpoints **twice** in fork-choice 1. Inside the fork-choice store 2. Inside the proto-array There's no reason for 2. except for making the function signature of some methods smallers. It's not consistent with the rest of the crate, because in some functions we pass the external variable of time (current_slot) via args, but then read the finalized checkpoint from the internal state. Passing both variables as args makes fork-choice easier to reason about at the cost of a few extra lines. Remove the unnecessary state (`justified_checkpoint`, `finalized_checkpoint`) inside `ProtoArray`, to make it easier to reason about. Co-Authored-By: dapplion <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
Previously, we had a pinned version of `alloy` to fix some crate compatibility issues we encountered during the migration away from `ethers`. Now that the migration is complete we should remove the pin. This also updates alloy crates to their latest versions. Co-Authored-By: Mac L <[email protected]>
There are certain crates which we re-export within `types` which creates a fragmented DevEx, where there are various ways to import the same crates.
```rust
// consensus/types/src/lib.rs
pub use bls::{
AggregatePublicKey, AggregateSignature, Error as BlsError, Keypair, PUBLIC_KEY_BYTES_LEN,
PublicKey, PublicKeyBytes, SIGNATURE_BYTES_LEN, SecretKey, Signature, SignatureBytes,
get_withdrawal_credentials,
};
pub use context_deserialize::{ContextDeserialize, context_deserialize};
pub use fixed_bytes::FixedBytesExtended;
pub use milhouse::{self, List, Vector};
pub use ssz_types::{BitList, BitVector, FixedVector, VariableList, typenum, typenum::Unsigned};
pub use superstruct::superstruct;
```
This PR removes these re-exports and makes it explicit that these types are imported from a non-`consensus/types` crate.
Co-Authored-By: Mac L <[email protected]>
|
|
|
|
Fix an issue where a kurtosis testnet script was failing because no supernodes were provided ``` There was an error interpreting Starlark code Evaluation error: fail: Fulu fork is enabled (epoch: 0) but no supernodes are configured, no nodes have 128 or more validators, and perfect_peerdas_enabled is not enabled. Either configure a supernode, ensure at least one node has 128+ validators, or enable perfect_peerdas_enabled in network_params with 16 participants. at [github.com/ethpandaops/ethereum-package/main.star:83:57]: run at [github.com/ethpandaops/ethereum-package/src/package_io/input_parser.star:377:17]: input_parser at [0:0]: fail ``` Co-Authored-By: Eitan Seri-Levi <[email protected]> Co-Authored-By: Pawan Dhananjay <[email protected]>
…8559) Co-Authored-By: Tan Chee Keong <[email protected]>
Co-Authored-By: Eitan Seri-Levi <[email protected]>
Member
|
Please rebase your PR on At a glance it looks ok, although it's low impact due to this being the |
michaelsproul
added
optimization
michaelsproul
added
the
waiting-on-author
…ure (sigp#8496) Co-Authored-By: figtracer <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
sigp#8458) Closes sigp#8426 Added a new regression test: `reproduction_unaligned_checkpoint_sync_pruned_payload`. This test reproduces the bug where unaligned checkpoint syncs (skipped slots at epoch boundaries) fail to import the anchor block's execution payload when `prune_payloads` is enabled. The test simulates the failure mode by: - Skipping if execution payloads are not applicable. - Creating a harness with an unaligned checkpoint (gap of 3 slots). - Configuring the client with prune_payloads = true. It asserts that the Beacon Chain builds successfully (previously it panicked with `MissingFullBlockExecutionPayloadPruned`), confirming the fix logic in `try_get_full_block`. Co-Authored-By: Andrurachi <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
sigp#8547 We are currently using an older version of `syn` in `test_random_derive`. Updating this removes one of the sources of `syn` `1.0.109` in our dependency tree. Co-Authored-By: Mac L <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
Fixes the error `fatal: No names found, cannot describe anything.` that occurs when running `make` commands in CI (GitHub Actions). https://github.com/sigp/lighthouse/actions/runs/19839541042/job/56844781126#step:5:13 > fatal: No names found, cannot describe anything. Changed the `GIT_TAG` variable assignment in the Makefile from immediate evaluation to lazy evaluation: ```diff - GIT_TAG := $(shell git describe --tags --candidates 1) + GIT_TAG = $(shell git describe --tags --candidates 1) ``` This change ensures that git describe is only executed when `GIT_TAG` is actually used (in the `build-release-tarballs` target), rather than on every Makefile invocation. Co-Authored-By: ackintosh <[email protected]>
…sigp#8499) Which issue # does this PR address? None The `visualize_batch_state` functions uses the following loop `for mut batch_index in 0..BATCH_BUFFER_SIZE`, making it from `0` to `BATCH_BUFFER_SIZE - 1` (behind the scenes). Hence we would never hit the following condition: ```rust if batch_index != BATCH_BUFFER_SIZE { visualization_string.push(','); } ``` Replacing `!=` with `<` & `BATCH_BUFFER_SIZE -1` allows for the following change: `[A,B,C,D,E,]` to become: `[A,B,C,D,E]` Co-Authored-By: Antoine James <[email protected]>
* sigp#7201 Co-Authored-By: Tan Chee Keong <[email protected]> Co-Authored-By: chonghe <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Tan Chee Keong <[email protected]>
* sigp#7850 This is the first round of the conga line! 🎉 Just spec constants and container changes so far. Co-Authored-By: shane-moore <[email protected]> Co-Authored-By: Mark Mackey <[email protected]> Co-Authored-By: Shane K Moore <[email protected]> Co-Authored-By: Eitan Seri- Levi <[email protected]> Co-Authored-By: ethDreamer <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
While reviewing Gloas I noticed we were updating `PartialBeaconState`. This code isn't used since v7.1.0 introduced hdiffs, so we can delete it and stop maintaining it 🎉 Similarly the `chunked_vector`/`chunked_iter` code can also go! Co-Authored-By: Michael Sproul <[email protected]> Co-Authored-By: Pawan Dhananjay <[email protected]>
Closes: - sigp#8408 Add `cargo deny` on CI with deprecated crates (`ethers` and `ethereum-types`) banned and duplicates banned for `reqwest`. Co-Authored-By: Michael Sproul <[email protected]>
A few `cargo-deny` tweaks with @macladson Co-authored-by: Mac L <[email protected]> Co-Authored-By: Michael Sproul <[email protected]> Co-Authored-By: Mac L <[email protected]>
Lil-Duckling-22
force-pushed
the
fix/optimize-aggregate-signature-serialize
branch
from
bd6a3df to
f9e142e
Compare
Lil-Duckling-22
force-pushed
the
fix/optimize-aggregate-signature-serialize
branch
from
f9e142e to
7c13450
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
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.
Motivation
The AggregateSignature::serialize() method in fake_crypto.rs was performing an unnecessary array copy, creating a new array and copying data when it could return self.0 directly. This is inconsistent with other serialize methods in the same file (PublicKey and Signature) and adds unnecessary overhead.
Solution
Removed the redundant copy operation and return self.0 directly, matching the pattern used in other serialize methods. This improves consistency and eliminates unnecessary memory operations.