feat: add flags to enable broadcast blocks and transactions to all peers #1219
Conversation
WalkthroughThis change renames two transaction gossip-related flags and associated configuration fields, adopting a unified "gossip" prefix. It also introduces new flags and configuration options to enable broadcasting blocks and transactions to all peers, updating the handler logic to support this wider broadcast when the new flag is set. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant Handler
CLI->>Config: Parse gossip flags (disable broadcast, disable receiving, enable broadcast to all)
Config->>Handler: Pass GossipTxBroadcastDisabled, GossipTxReceivingDisabled, GossipBroadcastToAllEnabled, BroadcastToAllCap
Handler->>Handler: On BroadcastBlock/BroadcastTransactions
alt GossipBroadcastToAllEnabled is true
Handler->>Peers: Broadcast to all peers (capped by BroadcastToAllCap)
else
Handler->>Peers: Broadcast to sqrt(total peers)
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eth/handler.go (1)
483-487: Implementation looks correct but consider the performance implications.The logic correctly implements broadcasting to all peers (capped at 100) when the flag is enabled, replacing the previous sqrt-based approach. This will significantly increase network traffic for nodes with many peers but provides the intended broadcast coverage.
The 100-peer cap is a sensible safeguard against resource exhaustion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/geth/main.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(2 hunks)eth/backend.go(1 hunks)eth/ethconfig/config.go(1 hunks)eth/handler.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/geth/usage.go (1)
cmd/utils/flags.go (3)
GossipTxBroadcastDisabledFlag(897-900)GossipTxReceivingDisabledFlag(901-904)GossipBroadcastToAllEnabledFlag(905-908)
cmd/geth/main.go (1)
cmd/utils/flags.go (3)
GossipTxBroadcastDisabledFlag(897-900)GossipTxReceivingDisabledFlag(901-904)GossipBroadcastToAllEnabledFlag(905-908)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
cmd/geth/usage.go (1)
251-253: LGTM! Consistent flag naming and clear new feature addition.The renaming adopts a unified "Gossip" prefix for all gossip-related flags, improving consistency. The new
GossipBroadcastToAllEnabledFlagclearly indicates the broadcast-to-all feature.eth/backend.go (1)
286-288: LGTM! Proper configuration propagation.The field renaming maintains consistency with the config changes, and the new
EnableBroadcastToAllfield correctly propagates the new feature flag to the handler configuration.eth/ethconfig/config.go (1)
234-236: LGTM! Clean configuration structure with consistent naming.The renaming establishes a unified "Gossip" prefix for all gossip-related configuration fields. The new
GossipBroadcastToAllEnabledfield defaults tofalse, ensuring backward compatibility.cmd/geth/main.go (1)
179-181: LGTM! Consistent CLI flag registration.The flag changes align perfectly with the usage.go updates, ensuring the renamed and new gossip flags are properly registered for the main geth command.
eth/handler.go (4)
97-99: LGTM! Configuration structure properly updated.The
handlerConfigstruct correctly incorporates the renamed fields and the newEnableBroadcastToAllflag.
138-141: LGTM! Handler struct properly updated.The handler struct fields are consistently renamed and the new
enableBroadcastToAllfield is added to support the feature.
161-163: LGTM! Proper configuration initialization.The handler initialization correctly maps the configuration values to the handler fields, maintaining the renamed field consistency and enabling the new feature.
529-532: Transaction broadcast logic correctly mirrors block broadcast behavior.The implementation maintains consistency with the block broadcast approach, broadcasting transactions to all peers (up to 100) when the flag is enabled instead of the sqrt-based subset.
cmd/utils/flags.go (1)
896-908: LGTM! Flag definitions follow good naming conventions.The gossip-related flags are well-structured with consistent naming and clear descriptions.
| @@ -477,7 +480,12 @@ func (h *handler) BroadcastBlock(block *types.Block, propagate bool) { | |||
| return | |||
| } | |||
| // Send the block to a subset of our peers | |||
| transfer := peers[:int(math.Sqrt(float64(len(peers))))] | |||
| numDirect := int(math.Sqrt(float64(len(peers)))) | |||
| // If enableBroadcastToAll is true, broadcast blocks directly to all peers (capped at 100). | |||
There was a problem hiding this comment.
A gut feeling is 20 to 30 might be sufficient. But for robustness, this can also be added to the configuration, enabled together with enableBroadcastToAll.
There was a problem hiding this comment.
Nice advise, added this with a default cap 30.
There was a problem hiding this comment.
Based on the chart of bootnode-0 provided by @colinlyguo it looks like it has a max peers of 163. For bootnodes and other nodes operated by Scroll, will we be setting this value higher to include all 163 peers?
yiweichi
changed the title
1. Purpose or design rationale of this PR
This PR is a part of project reduce transaction latency.
It adds a 2 flags
gossip.enablebroadcasttoallto enable node to broadcast transactions/blocks to all peers. andgossip.broadcasttoallcapto set the maximum number of peers for broadcasting, preventing network congestion.Meanwhile changed existing flag names from
txgossip.disablebroadcastandtxgossip.disablereceivingtogossip.disabletxbroadcastandgossip.disabletxreceivingfor consistency of flag name.2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor