Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @alpe's task in 7m 21s —— View job Code Review
|
Summary of ChangesHello @alpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the performance of the block production pipeline by implementing extensive caching strategies and optimizing data handling. It also upgrades the P2P protocol to streamline block verification for peers and introduces robust new benchmarking tools and refactored test utilities to ensure the stability and efficiency of these changes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive performance optimizations to the block execution and storage pathways, focusing on reducing allocations, caching, and minimizing redundant computations. However, a critical cache poisoning vulnerability has been introduced in the P2P verification logic due to insecure handling of the new PrevHeaderHash and PrevDataHash fields. This flaw allows malicious peers to bypass core integrity and signature checks, compromising the node's state, and requires immediate attention. Additionally, while a potential race condition in P2P broadcasting was addressed, there is one suggestion to simplify a small portion of the code for improved maintainability.
| func (p *P2PSignedHeader) Verify(untrusted *P2PSignedHeader) error { | ||
| if len(untrusted.PrevHeaderHash) > 0 { | ||
| p.Header.SetCachedHash(untrusted.PrevHeaderHash) | ||
| } | ||
| return p.SignedHeader.Verify(untrusted.SignedHeader) |
There was a problem hiding this comment.
A critical cache poisoning vulnerability exists here. The Verify method trusts the PrevHeaderHash field from an untrusted peer (untrusted) and uses it to set the cached hash of a local, trusted header object (p). Any subsequent operation calling p.Header.Hash() will use this attacker-controlled value, bypassing signature and integrity checks. This can lead to the acceptance of invalid blocks and chain corruption. The input from the untrusted peer must never be used to modify the state of a trusted local object.
| func (p *P2PData) Verify(untrusted *P2PData) error { | ||
| if len(untrusted.PrevDataHash) > 0 { | ||
| p.Data.SetCachedHash(untrusted.PrevDataHash) | ||
| } | ||
| return p.Data.Verify(untrusted.Data) |
There was a problem hiding this comment.
This section contains the same critical cache poisoning vulnerability as found earlier with headers. The Verify method for P2PData trusts the PrevDataHash from an untrusted peer and uses it to poison the hash cache of a local, trusted data object (p.Data). This allows an attacker to bypass data integrity checks, which could lead to the acceptance of blocks with invalid transactions. The input from the untrusted peer must be considered malicious and should not be used to modify trusted local data structures.
block/internal/executing/executor.go
Outdated
| // Cache static signer info — computed once, reused every CreateBlock call. | ||
| if e.signer != nil { | ||
| pubKey, err := e.signer.GetPublic() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to cache public key: %w", err) | ||
| } | ||
| e.cachedPubKey = pubKey | ||
|
|
||
| vHash, err := e.options.ValidatorHasherProvider(e.genesis.ProposerAddress, pubKey) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to cache validator hash: %w", err) | ||
| } | ||
| e.cachedValidatorHash = vHash | ||
| } else { | ||
| vHash, err := e.options.ValidatorHasherProvider(e.genesis.ProposerAddress, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to cache validator hash: %w", err) | ||
| } | ||
| e.cachedValidatorHash = vHash | ||
| } |
There was a problem hiding this comment.
The logic for caching the validator hash is duplicated in the if and else branches. This can be refactored to remove the duplication by making a single call to ValidatorHasherProvider after the if block. The pubKey variable will be correctly nil if e.signer is nil, which seems to be handled by the provider.
// Cache static signer info — computed once, reused every CreateBlock call.
var pubKey crypto.PubKey
if e.signer != nil {
var err error
pubKey, err = e.signer.GetPublic()
if err != nil {
return fmt.Errorf("failed to cache public key: %w", err)
}
e.cachedPubKey = pubKey
}
vHash, err := e.options.ValidatorHasherProvider(e.genesis.ProposerAddress, pubKey)
if err != nil {
return fmt.Errorf("failed to cache validator hash: %w", err)
}
e.cachedValidatorHash = vHash
alpe
force-pushed
the
alex/bench_phase1_start
branch
3 times, most recently
from
7150135 to
8efaece
Compare
alpe
force-pushed
the
alex/bench_phase1_start
branch
from
53311f4 to
7dea42f
Compare
alpe
force-pushed
the
alex/ai_moonshot2
branch
from
daedae4 to
dbe6446
Compare
1 participant
Overview