Use xxhash with salt for fastMsgIdFn - #4630
Closed
dapplion wants to merge 1 commit into
Closed
Conversation
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
Member
|
the benchmark looks great, suggest to test this on a node for 1 day before merge |
Member
|
@dapplion I deployed this branch to feat1 lg1k |
Member
|
this looks promising, initially it takes 200ms - 300ms
this correlates to the memory issue with this branch
to avoid the String creation, we should try ChainSafe/js-libp2p-gossipsub#355 |
dapplion
marked this pull request as draft
October 4, 2022 16:18
Contributor
Author
|
This is a low-priority optimization, not worth risking a memory leak for this. @tuyennhv if you want confirm that number causes the leak or not else let's close this PR |
Member
|
closing this PR as it still has a memory issue after I tried numbered FastMsgIdFn version (200ms - 300ms - this branch vs 1.5s - 2s - unstable |
This was referenced Oct 5, 2022
ensi321
added a commit
that referenced
this pull request
Jul 7, 2026
Squashed eip7688 implementation from the eip-7688 branch (lodestar #9390) @ 06b588d: converts Gloas containers to ProgressiveContainer/ProgressiveList (incl. additional containers #9586 and BlockAccessList -> ProgressiveByteList), bumps @chainsafe/ssz, updates light-client gindices and gossip size bounds, and reuses container nodes in upgradeStateToGloas for perf (#9601). Verified against #4630-generated spec vectors.
ensi321
added a commit
that referenced
this pull request
Jul 7, 2026
…nsensus-specs #4630) apply_parent_execution_payload performs no length checks in the spec. With EIP-7688 the ExecutionRequests lists are progressive (no SSZ max length), so the Electra-cap assertion (assertExecutionRequestsWithinLimits) is invalid -- requests exceeding the Electra caps are valid. Removes the assertion and its now-unused imports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ensi321
added a commit
that referenced
this pull request
Jul 7, 2026
Reverts 0395641. consensus-specs master still asserts the 5 execution request length limits in apply_parent_execution_payload (beacon-chain.md L1284-1290, tip a62bc326c) through the #4630 merge and all followups; the "drop the cap" idea was never adopted (#5420 instead tightened MAX_BUILDER_DEPOSIT_REQUESTS_PER_PAYLOAD 256->64). Keep parity with spec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ensi321
added a commit
that referenced
this pull request
Jul 7, 2026
The value shipped in the merged #4630 (and master, tip a62bc326c) is lower than the pre-merge PR snapshot this branch used: - mainnet 4082504 -> 4027336 - minimal 1993180 -> 1938012 (MAX_BUILDER_DEPOSIT_REQUESTS_PER_PAYLOAD 256->64 and PAYLOAD_DUE_BPS 7500->5000 are handled on nc/alpha.12-constants, not here.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ensi321
added a commit
that referenced
this pull request
Jul 7, 2026
… gossip consensus-specs #4630 (gloas p2p-interface) requires beacon_block gossip to REJECT when parent_execution_requests counts or block body operation counts exceed their per-block limits. EIP-7688 made these lists progressive (no SSZ type-level cap), so the bounds are enforced here at the gossip layer (they are also asserted in the state transition). deposits must be empty (limit 0) post-Fulu. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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
2 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
fastMsgIdFn() must produce a unique output per message within the message of the last few heartbeats. We can use any hash function so we should go for the cheapest possible with good safety.
sha256 on a node subscribed to all subnets it can take 0.5% of total CPU time.
xxhash is a cryptographically unsafe function that's has really good performance and good distribution of output. This PR also adds salt so an attacker can't pre-compute collisions.
Description
Closes #4603