phonebook: Persist initial phonebook peers; remove unused ExtendPeerList

[Eric-Warehime]

Summary

We never actually call ExtendPeerList anywhere. When we refresh the SRV list periodically it just fully replaces the peer list.

Test Plan

grep'ing ExtendPeerList shows nothing. Also existing tests should pass.

Info

Resolves #5545

[codecov]

Codecov Report

Merging #5615 (c54193a) into master (d82691d) will increase coverage by 0.01%.
Report is 22 commits behind head on master.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master    #5615      +/-   ##
==========================================
+ Coverage   54.95%   54.97%   +0.01%     
==========================================
  Files         460      460              
  Lines       64446    64447       +1     
==========================================
+ Hits        35417    35430      +13     
+ Misses      26653    26643      -10     
+ Partials     2376     2374       -2     

Files Changed	Coverage Δ
network/wsNetwork.go	72.87% <0.00%> (ø)
network/phonebook.go	91.33% <100.00%> (+0.06%)	⬆️

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[algorandskiy]

If I remember right, it does not replace but merges. Otherwise phonebook.json and -p would not work?

[Eric-Warehime]

If I remember right, it does not replace but merges. Otherwise phonebook.json and -p would not work?

All occurrences in the repo according to github search:
https://github.com/search?q=repo%3Aalgorand%2Fgo-algorand%20ExtendPeerList&type=code

SRV updates use this to replace peer list on refresh.
https://github.com/algorand/go-algorand/blob/master/network/wsNetwork.go#L1775-L1785C2

Replace peer list on ws network create--which is where the -p overrides get passed in.
https://github.com/algorand/go-algorand/blob/master/network/wsNetwork.go#L2452C12-L2452C27

[algorandskiy]

This sounds like a buggy behavior for me. -p entries must survive and not being replaced.

[iansuvak]

I agree that we probably want clear semantics on the -p and the method naming.This change itself is safe but as @algorandskiy said we want to make sure that DNS refresh never clobbers manual peers provided via -p. Might be outside of the scope of this cleanup PR though.

[algorandskiy]

Might be outside of the scope of this cleanup PR though.

We'll need Extend function for this...

[winder]

Seems fine.

[cce]

PR description no longer matches content but otherwise LGTM

[algorandskiy]

Blocking this, looks like folks think it should use replace logic with preserving seeded entries.

[Eric-Warehime]

Blocking this, looks like folks think it should use replace logic with preserving seeded entries.

That sounds like it should be done in a separate PR, but if we can't merge this without it then I can add it here.

[algorandskiy]

That sounds like it should be done in a separate PR, but if we can't merge this without it then I can add it here.

If we merge as is then dead relays could appear be in phonebook, not end of the world but still. This PR is just 5 LoC and the new fix will touch the same code, so I suggest to fix it right here.

[iansuvak]

LGTM -- I like the property based testing