Closed romanzac closed 7 months ago
Issue belongs to https://github.com/waku-org/go-waku/pull/1051
Have race condition treated or remove SetBootnodes() feature, as I saw it is not used and it is a bit counterintuitive.
I kind of agree with having SetBooNodes after starting discv5 may not be intutive, but wondering if this race condition actually causes any issues. The read is done in a doRefresh method which seems to be called in a loop, so the bootNodes would be used in the next iteration of the loop and hence this should not cause any functional issue.
cc @richard-ramos
For context, this function was added with the purpose of being used in status-go, in https://github.com/status-im/status-go/blob/develop/wakuv2/waku.go#L1686, because it was found that sometimes DNS Discovery failed to return the bootnodes.
Status-go has a loop which periodically attempts to retrieve bootnodes https://github.com/status-im/status-go/blob/develop/wakuv2/waku.go#L1583-L1615 and once it does, it will then execute SetBootnodes.
As @chaitanyaprem this race condition shouldn't cause issues as the values set on the table nursery table of discv5 will be read in next iteration, but just for the sake of not having this issue pop up in the CI, i'll protect the nursery attr with the mutex
Description Race condition error appears during TestDiscV5WithCapabilitiesFilter test run. Problem is with (d *DiscoveryV5) SetBootnodes() call. It looks setting boot nodes after DiscoveryV5 instance was created interferes with discovery process already going on.
To Reproduce 1) Checkout chore-discv5-tests-coverage-improvement 2) go test -race -timeout 300s
Expected behavior Have race condition treated or remove SetBootnodes() feature, as I saw it is not used and it is a bit counterintuitive.
Logs