nethermind icon indicating copy to clipboard operation
nethermind copied to clipboard

Fix StateSyncDownloader to fallback to alternative protocols on empty responses

Open Copilot opened this issue 4 months ago • 7 comments

Problem

StateSyncDownloader was using mutually exclusive if-else if chains to select protocols. Once a protocol was selected, the downloader would not try alternative protocols even if the peer returned an empty response. This caused sync failures on networks like OP Base where peers advertise protocol support but return empty data.

Solution

Modified the Dispatch method to use a sequential try pattern with automatic fallback. Protocols are now tried in order of preference: Snap → Eth66 → NodeData, prioritizing the newest and most commonly served protocol (Snap) first.

Key Changes:

  1. Refactored Dispatch Method: The main Dispatch method now tries protocols sequentially with fallback support instead of using mutually exclusive if-else chains.

  2. Protocol Try Methods: Created three helper methods:

    • TryGetNodeDataViaSnapProtocol: Attempts to fetch data via Snap protocol
    • TryGetNodeDataViaEthProtocol: Attempts to fetch data via Eth66 protocol
    • TryGetNodeDataViaNodeDataProtocol: Attempts to fetch data via NodeData protocol
  3. Protocol Ordering: Protocols are tried in this order (prioritizing newest and most served):

    • Snap protocol (newest and most commonly served)
    • Eth66 protocol (if version < 67)
    • NodeData protocol (fallback)
  4. Fallback Logic: Each protocol method:

    • Returns true if data is successfully retrieved (Count > 0)
    • Returns false if response is empty or an error occurs
    • Properly disposes of resources (HashList, GetTrieNodesRequest)
    • Logs trace messages for debugging
  5. Graceful Handling: When all protocols return empty responses:

    • Logs a debug message indicating peer might be out of sync
    • Returns without throwing exception (allows sync to continue)
  6. Test Updates:

    • Updated existing tests to provide mock responses
    • Updated SnapPeer_FallbackToNodeDataWhenEmpty test (was NodeDataPeer_FallbackToSnapWhenEmpty)
    • Updated SnapPeer_FallbackToEth66WhenEmpty test (was Eth66Peer_FallbackToSnapWhenEmpty)
    • Tests now reflect the new protocol ordering where Snap is tried first
    • Improved test code with collection initializers and proper using statements
  7. Code Quality Improvements:

    • Added using statements for better readability
    • Used collection initializer syntax
    • Removed unnecessary null checks (HashList.Rent never returns null)

Behavior:

  • Before: If any protocol returned empty, sync would fail with no fallback
  • After: Tries Snap first (most common), then Eth66, then NodeData. If any protocol returns empty, tries the next one. If all fail, logs debug message and continues gracefully (peer might be out of sync)

Fixes NethermindEth/nethermind#9531

Original prompt

This section details on the original issue you should resolve

<issue_title>Make StateSyncDownloader try other protocol when empty.</issue_title> <issue_description>Is your feature request related to a problem? Please describe.

  • StateSyncDownloader will try nodedata, eth66, then snap. If nodedata or eth66 is unable to serve node, then it will not try snap. Modify the downloader so that if the peer returned empty response, then try another protocol.

Describe the solution you'd like

  • Modify the downloader so that if the peer returned empty response, then try another protocol.

Describe alternatives you've considered

  • Technically this is fine if the network is spread out.

Additional context

  • Cannot sync op base. </issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes NethermindEth/nethermind#9531


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot avatar Oct 22 '25 04:10 Copilot

@copilot dont throw when all response is empty. It could be that the peer is simply out of sync.

Done in commit 5311e86. The exception has been removed - now logs a debug message when all protocols return empty.

Copilot avatar Oct 22 '25 05:10 Copilot

It does not solve it.

asdacap avatar Oct 26 '25 13:10 asdacap

It does not solve it.

Didn't mean it was a bad change, I am fine with merging it.

LukaszRozmej avatar Oct 27 '25 05:10 LukaszRozmej

@copilot reverse the protocol ordering

asdacap avatar Oct 30 '25 00:10 asdacap

@copilot reverse the protocol ordering

Done in commit 596709b. Protocol ordering reversed to prioritize Snap first.

Copilot avatar Oct 30 '25 00:10 Copilot

@copilot tests are failing, fix them

LukaszRozmej avatar Nov 01 '25 06:11 LukaszRozmej

@copilot tests are failing, fix them

Fixed in commit 0e501be. Updated the fallback tests to reflect the reversed protocol ordering - tests now correctly expect Snap to be tried first, with fallback to Eth66 or NodeData when Snap returns empty.

Copilot avatar Nov 01 '25 06:11 Copilot