Skip to content

Conversation

vipocenka
Copy link
Contributor

@vipocenka vipocenka commented Aug 22, 2025

Supersedes #32470.

What

  • snap: shorten stall watchdog in eth/protocols/snap/sync_test.go from 1m to 10s.
  • discover/v5: consolidate FINDNODE negative tests into a single table-driven test:
    • TestUDPv5_findnodeCall_InvalidNodes covers:
      • invalid IP (unspecified 0.0.0.0) → ignored
      • low UDP port (<=1024) → ignored

Why

  • Addresses TODOs:
    • “Make tests smaller” (reduce long 1m timeout).
    • “check invalid IPs”; also cover low port per verifyResponseNode rules (UDP must be >1024).

How it’s validated

  • Test-only changes; no production code touched.
  • Local runs:
    • go test ./p2p/discover -count=1 -timeout=300s → ok
    • go test ./eth/protocols/snap -count=1 -timeout=600s → ok
  • Lint:
    • go run build/ci.go lint → 0 issues on modified files.

Notes

  • The test harness uses enode.ValidSchemesForTesting (which includes the “null” scheme), so records signed with enode.SignNull are signature-valid; failures here are due to IP/port validation in verifyResponseNode and netutil.CheckRelayAddr.
  • Tests are written as a single table-driven function for clarity; no helpers or environment switching.

@lightclient lightclient self-assigned this Aug 22, 2025
@lightclient lightclient changed the title tests: add discv5 invalid NODES cases; shorten snap stall watchdog p2p/discover: add discv5 invalid findnodes result test cases Aug 22, 2025
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good now. Will wait for CI to go green first, then merge.

@lightclient lightclient added this to the 1.16.3 milestone Aug 22, 2025
@lightclient lightclient merged commit 276ed48 into ethereum:master Aug 22, 2025
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants