*: migrate to netip #567

jzelinskie · 2022-04-14T03:30:27Z

This change replaces net.IP usage with netip.AddrPort.
As a part of changing bittorrent.Peer to embed a netip.AddrPort, this introduces Peer.MarshalBinary() and replaces ad-hoc storage peer keys with this implementation.

Closes #540
Fixes #538

mrd0ll4r

Nice! I'm mostly okay with this. Something about the redis implementation seems odd, and I don't remember how exactly that was laid out.
Now that I think about it, wasn't there also some markdown doc about that, which should maybe be updated with the new redis keys?

bittorrent/bittorrent.go

storage/redis/peer_store.go

mrd0ll4r · 2022-04-14T07:52:29Z

storage/redis/peer_store.go

-	ct := ps.getClock()
+	addr := p.AddrPort.Addr()
+	sk := swarmKey(ih, true, addr)
+	ihKey := sk[0]


Maybe name this differently, took me a while to see that this holds information about the address family...

Yeah, this confused me a bit too looking back. I'm open to suggestions because it's non obvious to me, haha.

Hmm, maybe just af or addrFamily or something like that?
This is a bit weird, not sure what I was thinking back then. We store a modtime (ok) at a key like 4S12345... in a hashmap called 4. So the leading 4 in the swarm key is redundant here. It's not redundant for it's actual use to identify a hashmap, though...

So, we could remove one byte off that key, but meh.
The first byte is the address family. We could return that as a second value from swarmKey or maybe have a function to extract it from an IP. I think both of those would clear up some of the confusion here.

EDIT: Maybe we can even remove the leading two bytes? The second byte is S/L, but if we only use the address family hashmap for garbage collection, do we need to distinguish between seeder and leecher side for the modtime of a swarm?
I still don't have the full overview of what I was doing with this, tbh. Maybe I'll open an issue to look into it at some point in the future...

storage/redis/peer_store.go

This should reduce the number programmer errors producing keys.

mrd0ll4r · 2022-04-16T17:39:23Z

(replying to this via email, hope that works)Yeah, that makes sense. I've looked this up for another project a while back: these 4-in-6 constructions were added for backwards compatibility: for applications that only support IPv6, on hosts that are dual stacked or IPv4, the OS can wrap those into IPv6. Maybe that also applies if we bind to IPv6 only and some netfilter stuff translates IPv4 to that. All in all probably somewhat unlikely... Maybe we should have never opened this can of worms 😅Anyway. Sanitizing in the sanitization Middleware seems like the sane thing. On Apr 16, 2022 18:47, Jimmy Zelinskie ***@***.***> wrote: @jzelinskie commented on this pull request. In storage/redis/peer_store.go:

…

}

-func (ps *peerStore) seederCountKey(af string) string { - return af + "_S_count" +// Key that stores the cardinality of the peers of an IP version. +// +// "4L" => IPv4 Leechers +// "6S" => IPv6 Seeders +func peerCountKey(seed bool, addr netip.Addr) string { + key := "4" + if addr.Is6() { We call addr.Unmap() in SanitizeAnnounce so there really should never be any time that the storage systems have to deal with 4in6. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because your review was requested.Message ID: ***@***.***>

jzelinskie · 2022-04-16T18:06:52Z

I think this is good to go now @mrd0ll4r. I'd like to create some follow ups before I add too many changes to this one PR.

frontend/http/writer.go

mrd0ll4r · 2022-04-20T06:28:23Z

.github/workflows/lint.yaml

-          skip-pkg-cache: true
-          skip-build-cache: false
+          go-version: "^1.18"
+      - uses: "authzed/actions/gofumpt@main"


I will steal your github actions for my personal stuff :)

mrd0ll4r

Alright. A few more small ones and one more relevant one about sanitization of scrapes.
The redis stuff is still unclear in my head. I hope the tests catch the bugs...

mrd0ll4r · 2022-04-20T06:29:15Z

bittorrent/bittorrent.go

+// String implements fmt.Stringer, returning the base16 encoded PeerID.
+func (p PeerID) String() string { return fmt.Sprintf("%x", p[:]) }
+
+// MarshalBinary returns a 20-byte string of the raw bytes of the ID.


Nitpick: Not a string

mrd0ll4r · 2022-04-20T06:29:44Z

bittorrent/bittorrent.go

-func (i InfoHash) RawString() string {
-	return string(i[:])
-}
+// MarshalBinary returns a 20-byte string of the raw bytes of the InfoHash.


Same as above

mrd0ll4r · 2022-04-20T06:32:46Z

bittorrent/sanitize.go

-		r.Peer.IP.AddressFamily = IPv6
-	} else {
+	r.AddrPort = netip.AddrPortFrom(r.AddrPort.Addr().Unmap(), r.AddrPort.Port())
+	if !r.AddrPort.Addr().IsValid() || r.AddrPort.Addr().IsUnspecified() {


Oh, sanitization is missing for scrape requests! Maybe add that, since we now have a Peer in the scrape request, instead of just an address family.

mrd0ll4r · 2022-04-20T06:41:07Z

frontend/udp/frontend.go

@@ -317,7 +300,7 @@ func (t *Frontend) handleRequest(r Request, w ResponseWriter) (actionName string
 			return
 		}

-		WriteAnnounce(w, txID, resp, actionID == announceV6ActionID, req.IP.AddressFamily == bittorrent.IPv6)
+		WriteAnnounce(w, txID, resp, actionID == announceV6ActionID, r.IP.Unmap().Is6())


Nitpick: This should already be Unmapped, from line 211 above. Maybe add a doc comment about that somewhere...

mrd0ll4r · 2022-04-20T06:47:59Z

storage/memory/peer_store.go

 	// There are twice the amount of shards specified by the user, the first
 	// half is dedicated to IPv4 swarms and the second half is dedicated to
 	// IPv6 swarms.
 	idx := binary.BigEndian.Uint32(infoHash[:4]) % (uint32(len(ps.shards)) / 2)
-	if af == bittorrent.IPv6 {
+	if addr.Is6() && !addr.Is4In6() {


I'm in favor of removing these Is4In6 checks (in everything but the sanitization middleware and frontends, of course). What do you think?

mrd0ll4r · 2022-04-20T06:49:36Z

storage/memory/peer_store.go

 	select {
 	case <-ps.closed:
 		panic("attempted to interact with stopped memory store")
 	default:
 	}

 	resp.InfoHash = ih
-	shard := ps.shards[ps.shardIndex(ih, addressFamily)]
+	shard := ps.shards[ps.shardIndex(ih, p.AddrPort.Addr())]


Ah, this breaks if we don't have the Is4In6 check and no sanitization for scrapes.

mrd0ll4r · 2022-04-20T06:59:19Z

storage/redis/peer_store.go

-	ct := ps.getClock()
+	addr := p.AddrPort.Addr()
+	sk := swarmKey(ih, true, addr)
+	ihKey := sk[0]


Hmm, maybe just af or addrFamily or something like that?
This is a bit weird, not sure what I was thinking back then. We store a modtime (ok) at a key like 4S12345... in a hashmap called 4. So the leading 4 in the swarm key is redundant here. It's not redundant for it's actual use to identify a hashmap, though...

So, we could remove one byte off that key, but meh.
The first byte is the address family. We could return that as a second value from swarmKey or maybe have a function to extract it from an IP. I think both of those would clear up some of the confusion here.

EDIT: Maybe we can even remove the leading two bytes? The second byte is S/L, but if we only use the address family hashmap for garbage collection, do we need to distinguish between seeder and leecher side for the modtime of a swarm?
I still don't have the full overview of what I was doing with this, tbh. Maybe I'll open an issue to look into it at some point in the future...

mrd0ll4r · 2022-04-20T07:08:21Z

storage/redis/peer_store.go

@@ -581,38 +586,38 @@ func (ps *peerStore) AnnouncePeers(ih bittorrent.InfoHash, seeder bool, numWant

 	if seeder {
 		// Append leechers as possible.
-		for _, pk := range conLeechers {
+		for _, sk := range conLeechers {


I'd prefer these to stay pk as in peerKey, since they are encoded peers.

sot-tech · 2022-04-26T19:55:15Z

middleware/logic_test.go

 	benchHookList(b, hooks, req)
 }

 func benchHookListV6(b *testing.B, hooks hookList) {
-	req := &bittorrent.AnnounceRequest{Peer: bittorrent.Peer{IP: bittorrent.IP{IP: net.ParseIP("fc00::0001"), AddressFamily: bittorrent.IPv6}}}
+	req := &bittorrent.AnnounceRequest{Peer: bittorrent.Peer{
+		AddrPort: netip.AddrPortFrom(netip.MustParseAddr("fc00:0001"), 0),


netip.MustParseAddr will panic here. You forgot second colon in ipv6 address.

jzelinskie requested a review from mrd0ll4r April 14, 2022 03:30

*: migrate to netip

e56ad81

jzelinskie force-pushed the netip branch from dcb4f82 to e56ad81 Compare April 14, 2022 03:30

jzelinskie added 2 commits April 13, 2022 23:40

.github: update workflows

24ab4ae

*: pass golangci-lint

dceec48

jzelinskie marked this pull request as draft April 14, 2022 03:50

mrd0ll4r requested changes Apr 14, 2022

View reviewed changes

jzelinskie added 3 commits April 16, 2022 12:12

*: address inconsistent documentation

e322fb4

storage/redis: fix incorrect key

5bfccdf

storage/redis: add addressFamily type

6580973

This should reduce the number programmer errors producing keys.

jzelinskie marked this pull request as ready for review April 16, 2022 18:06

sot-tech reviewed Apr 16, 2022

View reviewed changes

frontend/http/writer.go Outdated Show resolved Hide resolved

jzelinskie added 3 commits April 16, 2022 14:18

frontend/http: preallocate compact buffers

7a4c2ee

cmd: replace errors.Wrap with fmt.Errorf

b8146e7

frontend/udp: map 4in6 addresses

77b3bf9

jzelinskie force-pushed the netip branch from 2206018 to 77b3bf9 Compare April 16, 2022 18:18

jzelinskie added 2 commits April 17, 2022 19:03

frontend/http: remove superfluous WriteHeader call

9d04e4c

frontend/udp: replace sha256 with xxhash

4d79dfe

mrd0ll4r reviewed Apr 20, 2022

View reviewed changes

mrd0ll4r requested changes Apr 20, 2022

View reviewed changes

mrd0ll4r mentioned this pull request Apr 26, 2022

middleware/hooks: preserve prehook response changes #566

Open

sot-tech reviewed Apr 26, 2022

View reviewed changes

mrd0ll4r mentioned this pull request May 29, 2022

Remove unused import 'os/signal' for windows #575

Merged

mrd0ll4r mentioned this pull request Jul 1, 2022

Fix CI #586

Merged

mrd0ll4r mentioned this pull request Feb 2, 2023

storage: modify interface to pass a slice on AnnouncePeers #602

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: migrate to netip #567

*: migrate to netip #567

jzelinskie commented Apr 14, 2022 •

edited

mrd0ll4r left a comment

mrd0ll4r Apr 14, 2022

jzelinskie Apr 16, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r commented Apr 16, 2022 via email

jzelinskie commented Apr 16, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r left a comment

mrd0ll4r Apr 20, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r Apr 20, 2022

mrd0ll4r Apr 20, 2022

sot-tech Apr 26, 2022

*: migrate to netip #567

Are you sure you want to change the base?

*: migrate to netip #567

Conversation

jzelinskie commented Apr 14, 2022 • edited

mrd0ll4r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrd0ll4r commented Apr 16, 2022 via email

jzelinskie commented Apr 16, 2022

Choose a reason for hiding this comment

mrd0ll4r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzelinskie commented Apr 14, 2022 •

edited