feat: skip validation if peer is known #2491
Conversation
|
@istae just a suggestion, but we might also want not to check the same peer twice, in case the result was a negative. for this we can reuse the LRU cache used in the chainsync PR, to basically cache the results (true/false) for an address - no entry means we didn't try, false means unreachable (dont retry) and true means reachable. having an upper bound of addresses in the cache would mean we could potentially retry after a while (but it should be well into the thousands range). i would also consider not to have this validation on the bootnode at all since it does not actively dial |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @istae)
pkg/hive/hive.go, line 268 at r1 (raw file):
// if peer exists already, skip validation if _, err := s.addressBook.Get(swarm.NewAddress(p.Overlay)); err == nil {
The errors thrown by the Get method should be handled (at least logged).
istae
force-pushed
the
hive-addressbook-check
branch
from
442b315 to
4dcdeef
Compare
istae
changed the title
pkg/hive/hive.go
Outdated
| @@ -39,6 +40,8 @@ const ( | |||
| maxBatchSize = 30 | |||
| pingTimeout = time.Second * 5 // time to wait for ping to succeed | |||
| batchValidationTimeout = 5 * time.Minute // prevent lock contention on peer validation | |||
| cacheSize = 25000 | |||
| cachePrefix = 4 // enough bytes (32 bits) to uniquely identify a peer | |||
There was a problem hiding this comment.
i'm not sure if this is strictly needed. even when keeping 25k addresses in memory, the total overhead is around 800kb.
There was a problem hiding this comment.
So I increased the cache size to 100,000 and kept the cachePrefix, it's best if cache is big for hive.
pkg/hive/hive.go
Outdated
| return | ||
| } | ||
| if s.bootnode { | ||
| for _, p := range peers.Peers { |
There was a problem hiding this comment.
not sure what is the desirable outcome here. this basically means - if we are a bootnode we persist every peer we get over discovery. if anything, we should not persist any node which we are not connected to
There was a problem hiding this comment.
because bootnodes do not dial, the newest commit discards the received peers without validating or persisting them. thanks.
pkg/hive/hive.go
Outdated
| if err == nil { | ||
| continue | ||
| } else { | ||
| s.logger.Errorf("hive: addressbook %v", err) |
There was a problem hiding this comment.
this means that you return an error here if the peer is not in the address book, so you basically print an error for every peer you don't know
There was a problem hiding this comment.
@mrekucci's request was that we should at least log it, but I agree, logging here is does not add much value.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @acud and @istae)
pkg/hive/hive.go
Outdated
| err error | ||
| ) | ||
|
|
||
| if !bootnode { |
There was a problem hiding this comment.
i think it is fine to initialize this even in bootnode mode. the cache size is negligible when empty, and we don't risk any panics on bootnodes
|
Kudos, SonarCloud Quality Gate passed!
|
This change is