Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increased ProviderAddrTTL from RFM17.1 isn't applied #868

Closed
dennis-tra opened this issue Aug 22, 2023 · 1 comment · Fixed by #870
Closed

Increased ProviderAddrTTL from RFM17.1 isn't applied #868

dennis-tra opened this issue Aug 22, 2023 · 1 comment · Fixed by #870

Comments

@dennis-tra
Copy link
Contributor

dennis-tra commented Aug 22, 2023

Looking through the code, I believe that the increased ProviderAddrTTL doesn’t have any effect. We increased that value as a result of RFM 17.1.

In this line we pass the new provider of a certain key to the ProviderManager:

dht.providerStore.AddProvider(ctx, key, peer.AddrInfo{ID: p})

The value that we're passing is just peer.AddrInfo{ID: p}, so there are no addresses attached to it. In the ProviderManger this value gets handled here:

if provInfo.ID != pm.self {
  pm.pstore.AddAddrs(provInfo.ID, provInfo.Addrs, ProviderAddrTTL)
}

provInfo is the peer.AddrInfo{ID: p} from above. This means the Addrs field will be always empty, so, I believe, ProviderAddrTTL won't have any effect.

However, we're still keeping the addresses around for a bit because the peer that stores the records with us gets added to the peerstore just because it connected to us - the TTL of their addresses will just be different. I think the values are ConnectedAddrTTL while we're connected to that peer (basically infinite TTL) and TempAddrTTL after we have disconnected. The latter value is set to 2 minutes.

cc @cortze

@dennis-tra
Copy link
Contributor Author

This is how it worked initially: 3b37c43

	for _, pi := range pinfos {
		if pi.ID != p {
			// we should ignore this provider reccord! not from originator.
			// (we chould sign them and check signature later...)
			log.Errorf("handleAddProvider received provider %s from %s. Ignore.", pi.ID, p)
			continue
		}

		if len(pi.Addrs) < 1 {
			log.Errorf("got no valid addresses for provider %s. Ignore.", p)
			continue
		}

		log.Infof("received provider %s for %s (addrs: %s)", p, key, pi.Addrs)
		for _, maddr := range pi.Addrs {
			// add the received addresses to our peerstore.
			dht.peerstore.AddAddress(p, maddr)
		}
		dht.providers.AddProvider(key, p)
	}

That's where it capsized: 7724838

dennis-tra added a commit that referenced this issue Aug 22, 2023
dennis-tra added a commit that referenced this issue Aug 22, 2023
Jorropo pushed a commit that referenced this issue Sep 18, 2023
fixes issue #868

(cherry picked from commit 777160f)
cortze pushed a commit to cortze/go-libp2p-kad-dht that referenced this issue Jul 30, 2024
added tests for addrfilter

fix: don't add unresponsive DHT servers to the Routing Table (libp2p#820)

* added check to avoid adding unresponsive dht peers to the dht routing table

* removed lock in adding peers to the rt

* made variable names more meaningful

* fixed network loop and corrected tests

* added UsefulPeer() references from current PR

* go mod tidy

* added delay in TestRefreshBelowMinRTThreshold

* addressed review

* go mod tidy

* addressed Jorropo review

* added comments

* removed state of peers probed recently

* fix conflicts merge

* updated deps

* added optimizations documentation

* Update dht.go

* updated md files

---------

Co-authored-by: Jorropo <[email protected]>

fix: don't add unresponsive DHT servers to the Routing Table (libp2p#820)

* added check to avoid adding unresponsive dht peers to the dht routing table

* removed lock in adding peers to the rt

* made variable names more meaningful

* fixed network loop and corrected tests

* added UsefulPeer() references from current PR

* go mod tidy

* added delay in TestRefreshBelowMinRTThreshold

* addressed review

* go mod tidy

* addressed Jorropo review

* added comments

* removed state of peers probed recently

* fix conflicts merge

* updated deps

* added optimizations documentation

* Update dht.go

* updated md files

---------

Co-authored-by: Jorropo <[email protected]>
(cherry picked from commit 8c9fdff)

chore: release v0.24.0

chore: Update .github/workflows/stale.yml [skip ci]

fix: leaking go routines

fix: bump kbucket for abba bug

refactor: remove goprocess

fix: decrease tests noise, update kbucket and fix fixRTIUfNeeded

chore: release v0.24.1

chore: update go-libp2p to a non deadlocky version

chore: release v0.24.2

chore: Update .github/workflows/stale.yml [skip ci]

tracing: fix DHT keys as string attribute not being valid utf-8

chore: release v0.24.3

fix the server mode log in handleNewMessage by changing it to debug

chore: delete templates [skip ci] (libp2p#861)

ci: uci/copy-templates (libp2p#862)

* chore: add or force update .github/workflows/go-test.yml

* chore: add or force update .github/workflows/go-check.yml

* chore: add or force update .github/workflows/releaser.yml

* chore: add or force update .github/workflows/release-check.yml

* chore: add or force update .github/workflows/tagpush.yml

chore: bump go.mod to Go 1.20 and run go fix

chore: bump go-libp2p and/or quic-go to latest version

tracing: add protocol messages client tracing

chore: release v0.25.0

add provider record addresses to peerstore

fixes issue libp2p#868

fixing base32 import

fix: apply addrFilters in the dht (libp2p#872)

* fix: correctly apply addrFilters in the dht

This still does not do the fullrt client but it wasn't doing it before either.

* Add address filter tests

* use channel instead of waitgroup

to catch timeouts

* filter multiaddresses when serving provider records

---------

Co-authored-by: Dennis Trautwein <[email protected]>

fix: properly iterate in tracing for protocol messenger

MB Copy Paste Typo,

This caused panics while tracing.

chore: use go-libp2p-routing-helpers for tracing needs

go-libp2p-routing-helpers has an optimized implementation that does nothing if we are not tracing, it also properly log all IO of the request.

perf: don't buffer the output of FindProvidersAsync

We always select it against ctx and we don't rely on the fact it will always be non blocking since when count is zero we can't realistically preallocate.
Buffering use more memory makes more garbage and is less efficient than direct copies.

chore: release v0.25.1

add ctx canceled err check

return canceled err not retry

reset stream and return err when ctx canceled

release v0.25.2

chore: Update .github/workflows/stale.yml [skip ci]

chore: bump go.mod to Go 1.21 and run go fix

chore: run go mod tidy

chore: add or force update .github/workflows/go-test.yml

chore: add or force update .github/workflows/go-check.yml

chore: add or force update .github/workflows/releaser.yml

chore: add or force update .github/workflows/release-check.yml

chore: add or force update .github/workflows/tagpush.yml

chore: add or force update .github/workflows/go-test.yml

chore: add or force update .github/workflows/go-check.yml

chore: add or force update .github/workflows/releaser.yml

chore: add or force update .github/workflows/release-check.yml

chore: add or force update .github/workflows/tagpush.yml

chore: add or force update .github/workflows/go-test.yml

chore: add or force update .github/workflows/go-check.yml

chore: add or force update .github/workflows/releaser.yml

chore: add or force update .github/workflows/release-check.yml

chore: add or force update .github/workflows/tagpush.yml

findnode(self) now returns multiple peers

don't lookup check if not enough peers in table

enhanced tests

moved rt size check after request

Upgrade to go-log v2.5.1
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 a pull request may close this issue.

1 participant