-
Notifications
You must be signed in to change notification settings - Fork 8
feat: handle DoS node by sending block #475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
frisitano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Added a few comments inline. Can you add some test cases, please?
I'm also wondering if we can simplify the logic a bit. The idea would be that if we have a connection with a peer over scroll-wire, then we should only gossip on scroll-wire. If we don't have a scroll-wire connection, then we use eth-wire. I think this should allow us to consolidate the block cache per peer into a single cache.
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should change the logic below.
If we have a connection with a peer via scroll wire - only send via scroll wire. If we don't have a connection with a peer via scroll wire then send only via eth wire.
What do you think?
| // Check if this peer has already received this block via scroll-wire, if so | ||
| // penalize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Check if this peer has already received this block via scroll-wire, if so | |
| // penalize it. | |
| // Check if we have already received this block via scroll-wire from this peer, if so | |
| // penalize it. |
| // Update the state of the peer cache i.e. peer has seen this block. | ||
| // Update the state: peer has seen this block via scroll-wire | ||
| self.scroll_wire | ||
| .state_mut() | ||
| .peer_block_state_mut() | ||
| .entry(peer_id) | ||
| .or_insert_with(|| LruCache::new(LRU_CACHE_SIZE)) | ||
| .insert(block_hash); | ||
| .or_insert_with(|| PeerBlockState::new(LRU_CACHE_SIZE)) | ||
| .insert_scroll_wire(block_hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we already do this above on line 274 above?
| { | ||
| let block_hash = block.hash_slow(); | ||
|
|
||
| // Check if this peer has already sent this block to us via eth-wire, if so penalize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Check if this peer has already sent this block to us via eth-wire, if so penalize it. | |
| // Check if we have already received this block from this peer via eth-wire, if so, penalize the peer. |
| self.scroll_wire | ||
| .state_mut() | ||
| .peer_block_state_mut() | ||
| .entry(peer_id) | ||
| .or_insert_with(|| LruCache::new(LRU_CACHE_SIZE)) | ||
| .insert(block_hash); | ||
| .or_insert_with(|| PeerBlockState::new(LRU_CACHE_SIZE)) | ||
| .insert_eth_wire(block_hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we already do this above on line 396?
| /// is just a cache of the last 100 blocks seen by each peer. | ||
| state: HashMap<PeerId, LruCache<B256>>, | ||
| /// Unified state tracking block state and blocks received from each peer via both protocols. | ||
| peer_block_state: HashMap<PeerId, PeerBlockState>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose we rename this to peer_state and PeerBlockState to PeerState
Problem
Malicious peers can DoS the node by sending old blocks.
Solution
Two-layer defense:
if block_number <= finalized_height { penalize(peer); // Immediately reject}// Cache recent blocks per peerif peer_already_sent_this_block { penalize(peer); // DoS detected }Challenge
Multi-Protocol Handling
Problem: Nodes use both scroll-wire and eth-wire protocols. Same peer may legitimately send same block via both protocols.
Solution: Track duplicates per protocol
Fixes #307