From 9027ee0b45f14aae1f3a98f62f5353bf6807ad68 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Mon, 7 Nov 2022 19:19:02 +0100 Subject: [PATCH] p2p/discover: improve discv5 NODES response packing (#26033) Instead of using a limit of three nodes per message, we can pack more nodes into each message based on ENR size. In my testing, this halves the number of sent NODES messages, because ENR size is usually < 300 bytes. This also adds RLP helper functions that compute the encoded size of []byte and string. Co-authored-by: Martin Holst Swende --- p2p/discover/v5_udp.go | 26 ++++++++++++++++++-------- p2p/discover/v5_udp_test.go | 9 +++------ p2p/enr/enr.go | 18 ++++++++++++++++++ p2p/enr/enr_test.go | 31 ++++++++++++++++++++++++++++++- rlp/raw.go | 35 ++++++++++++++++++++++++++++++++++- rlp/raw_test.go | 33 +++++++++++++++++++++++++++++++++ 6 files changed, 136 insertions(+), 16 deletions(-) diff --git a/p2p/discover/v5_udp.go b/p2p/discover/v5_udp.go index 1c66602f87..321c5bd2a8 100644 --- a/p2p/discover/v5_udp.go +++ b/p2p/discover/v5_udp.go @@ -24,7 +24,6 @@ import ( "errors" "fmt" "io" - "math" "net" "sync" "time" @@ -41,7 +40,6 @@ const ( lookupRequestLimit = 3 // max requests against a single node during lookup findnodeResultLimit = 16 // applies in FINDNODE handler totalNodesResponseLimit = 5 // applies in waitForNodes - nodesResponseItemLimit = 3 // applies in sendNodes respTimeoutV5 = 700 * time.Millisecond ) @@ -832,17 +830,29 @@ func packNodes(reqid []byte, nodes []*enode.Node) []*v5wire.Nodes { return []*v5wire.Nodes{{ReqID: reqid, Total: 1}} } - total := uint8(math.Ceil(float64(len(nodes)) / 3)) + // This limit represents the available space for nodes in output packets. Maximum + // packet size is 1280, and out of this ~80 bytes will be taken up by the packet + // frame. So limiting to 1000 bytes here leaves 200 bytes for other fields of the + // NODES message, which is a lot. + const sizeLimit = 1000 + var resp []*v5wire.Nodes for len(nodes) > 0 { - p := &v5wire.Nodes{ReqID: reqid, Total: total} - items := min(nodesResponseItemLimit, len(nodes)) - for i := 0; i < items; i++ { - p.Nodes = append(p.Nodes, nodes[i].Record()) + p := &v5wire.Nodes{ReqID: reqid} + size := uint64(0) + for len(nodes) > 0 { + r := nodes[0].Record() + if size += r.Size(); size > sizeLimit { + break + } + p.Nodes = append(p.Nodes, r) + nodes = nodes[1:] } - nodes = nodes[items:] resp = append(resp, p) } + for _, msg := range resp { + msg.Total = uint8(len(resp)) + } return resp } diff --git a/p2p/discover/v5_udp_test.go b/p2p/discover/v5_udp_test.go index ca63688afa..ab0cb9a821 100644 --- a/p2p/discover/v5_udp_test.go +++ b/p2p/discover/v5_udp_test.go @@ -161,7 +161,7 @@ func TestUDPv5_findnodeHandling(t *testing.T) { defer test.close() // Create test nodes and insert them into the table. - nodes253 := nodesAtDistance(test.table.self().ID(), 253, 10) + nodes253 := nodesAtDistance(test.table.self().ID(), 253, 16) nodes249 := nodesAtDistance(test.table.self().ID(), 249, 4) nodes248 := nodesAtDistance(test.table.self().ID(), 248, 10) fillTable(test.table, wrapNodes(nodes253)) @@ -186,7 +186,7 @@ func TestUDPv5_findnodeHandling(t *testing.T) { // This request gets all the distance-253 nodes. test.packetIn(&v5wire.Findnode{ReqID: []byte{4}, Distances: []uint{253}}) - test.expectNodes([]byte{4}, 4, nodes253) + test.expectNodes([]byte{4}, 1, nodes253) // This request gets all the distance-249 nodes and some more at 248 because // the bucket at 249 is not full. @@ -194,7 +194,7 @@ func TestUDPv5_findnodeHandling(t *testing.T) { var nodes []*enode.Node nodes = append(nodes, nodes249...) nodes = append(nodes, nodes248[:10]...) - test.expectNodes([]byte{5}, 5, nodes) + test.expectNodes([]byte{5}, 1, nodes) } func (test *udpV5Test) expectNodes(wantReqID []byte, wantTotal uint8, wantNodes []*enode.Node) { @@ -208,9 +208,6 @@ func (test *udpV5Test) expectNodes(wantReqID []byte, wantTotal uint8, wantNodes if !bytes.Equal(p.ReqID, wantReqID) { test.t.Fatalf("wrong request ID %v in response, want %v", p.ReqID, wantReqID) } - if len(p.Nodes) > 3 { - test.t.Fatalf("too many nodes in response") - } if p.Total != wantTotal { test.t.Fatalf("wrong total response count %d, want %d", p.Total, wantTotal) } diff --git a/p2p/enr/enr.go b/p2p/enr/enr.go index 438c7b8a3b..2b093b2f1a 100644 --- a/p2p/enr/enr.go +++ b/p2p/enr/enr.go @@ -96,6 +96,24 @@ type pair struct { v rlp.RawValue } +// Size returns the encoded size of the record. +func (r *Record) Size() uint64 { + if r.raw != nil { + return uint64(len(r.raw)) + } + return computeSize(r) +} + +func computeSize(r *Record) uint64 { + size := uint64(rlp.IntSize(r.seq)) + size += rlp.BytesSize(r.signature) + for _, p := range r.pairs { + size += rlp.StringSize(p.k) + size += uint64(len(p.v)) + } + return rlp.ListSize(size) +} + // Seq returns the sequence number. func (r *Record) Seq() uint64 { return r.seq diff --git a/p2p/enr/enr_test.go b/p2p/enr/enr_test.go index bf3f104744..b85ee209d5 100644 --- a/p2p/enr/enr_test.go +++ b/p2p/enr/enr_test.go @@ -169,6 +169,32 @@ func TestDirty(t *testing.T) { } } +func TestSize(t *testing.T) { + var r Record + + // Empty record size is 3 bytes. + // Unsigned records cannot be encoded, but they could, the encoding + // would be [ 0, 0 ] -> 0xC28080. + assert.Equal(t, uint64(3), r.Size()) + + // Add one attribute. The size increases to 5, the encoding + // would be [ 0, 0, "k", "v" ] -> 0xC58080C26B76. + r.Set(WithEntry("k", "v")) + assert.Equal(t, uint64(5), r.Size()) + + // Now add a signature. + nodeid := []byte{1, 2, 3, 4, 5, 6, 7, 8} + signTest(nodeid, &r) + assert.Equal(t, uint64(45), r.Size()) + enc, _ := rlp.EncodeToBytes(&r) + if r.Size() != uint64(len(enc)) { + t.Error("Size() not equal encoded length", len(enc)) + } + if r.Size() != computeSize(&r) { + t.Error("Size() not equal computed size", computeSize(&r)) + } +} + func TestSeq(t *testing.T) { var r Record @@ -268,8 +294,11 @@ func TestSignEncodeAndDecodeRandom(t *testing.T) { } require.NoError(t, signTest([]byte{5}, &r)) - _, err := rlp.EncodeToBytes(r) + + enc, err := rlp.EncodeToBytes(r) require.NoError(t, err) + require.Equal(t, uint64(len(enc)), r.Size()) + require.Equal(t, uint64(len(enc)), computeSize(&r)) for k, v := range pairs { desc := fmt.Sprintf("key %q", k) diff --git a/rlp/raw.go b/rlp/raw.go index f355efc144..773aa7e614 100644 --- a/rlp/raw.go +++ b/rlp/raw.go @@ -28,13 +28,46 @@ type RawValue []byte var rawValueType = reflect.TypeOf(RawValue{}) +// StringSize returns the encoded size of a string. +func StringSize(s string) uint64 { + switch { + case len(s) == 0: + return 1 + case len(s) == 1: + if s[0] <= 0x7f { + return 1 + } else { + return 2 + } + default: + return uint64(headsize(uint64(len(s))) + len(s)) + } +} + +// BytesSize returns the encoded size of a byte slice. +func BytesSize(b []byte) uint64 { + switch { + case len(b) == 0: + return 1 + case len(b) == 1: + if b[0] <= 0x7f { + return 1 + } else { + return 2 + } + default: + return uint64(headsize(uint64(len(b))) + len(b)) + } +} + // ListSize returns the encoded size of an RLP list with the given // content size. func ListSize(contentSize uint64) uint64 { return uint64(headsize(contentSize)) + contentSize } -// IntSize returns the encoded size of the integer x. +// IntSize returns the encoded size of the integer x. Note: The return type of this +// function is 'int' for backwards-compatibility reasons. The result is always positive. func IntSize(x uint64) int { if x < 0x80 { return 1 diff --git a/rlp/raw_test.go b/rlp/raw_test.go index 46adff22c5..2812ef74c6 100644 --- a/rlp/raw_test.go +++ b/rlp/raw_test.go @@ -283,3 +283,36 @@ func TestAppendUint64Random(t *testing.T) { t.Fatal(err) } } + +func TestBytesSize(t *testing.T) { + tests := []struct { + v []byte + size uint64 + }{ + {v: []byte{}, size: 1}, + {v: []byte{0x1}, size: 1}, + {v: []byte{0x7E}, size: 1}, + {v: []byte{0x7F}, size: 1}, + {v: []byte{0x80}, size: 2}, + {v: []byte{0xFF}, size: 2}, + {v: []byte{0xFF, 0xF0}, size: 3}, + {v: make([]byte, 55), size: 56}, + {v: make([]byte, 56), size: 58}, + } + + for _, test := range tests { + s := BytesSize(test.v) + if s != test.size { + t.Errorf("BytesSize(%#x) -> %d, want %d", test.v, s, test.size) + } + s = StringSize(string(test.v)) + if s != test.size { + t.Errorf("StringSize(%#x) -> %d, want %d", test.v, s, test.size) + } + // Sanity check: + enc, _ := EncodeToBytes(test.v) + if uint64(len(enc)) != test.size { + t.Errorf("len(EncodeToBytes(%#x)) -> %d, test says %d", test.v, len(enc), test.size) + } + } +}