Performance and bug-fix backports (#443)

neilalexander · web-flow · commit e86ab16eabe8 · 2024-12-15T10:48:29.000+01:00
This contains various backports from Harmony: * Reduced allocations and performance improvements in state resolution * Reduced allocations and performance improvements in JSON handling * Event auth fixes, including correct error surfacing * Correct event ordering when returning from `PerformBackfill` calls * Improved handling rejected events in state resolution applies Closes #439 as superseded by changes in this PR. Signed-off-by: Neil Alexander <git@neilalexander.dev> --------- Co-authored-by: Neil Alexander <neilalexander@users.noreply.github.com>
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -21,7 +21,7 @@ jobs:
       - name: Install Go
         uses: actions/setup-go@v3
         with:
-          go-version: 1.21
+          go-version: 1.22
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3
 
@@ -33,7 +33,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ["1.20", "1.21"]
+        go: ["stable", "1.22"]
     steps:
       - uses: actions/checkout@v3
       - name: Setup go
@@ -83,7 +83,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ["1.21"]
+        go: ["stable", "1.22"]
     steps:
       - uses: actions/checkout@v3
         with:
diff --git a/README.md b/README.md
@@ -3,4 +3,4 @@ gomatrixserverlib
 
 [![GoDoc](https://godoc.org/github.com/matrix-org/gomatrixserverlib?status.svg)](https://godoc.org/github.com/matrix-org/gomatrixserverlib)
 
-Go library for common functions needed by matrix servers. This library assumes Go 1.18+.
+Go library for common functions needed by matrix servers. This library assumes Go 1.22+.
diff --git a/authstate.go b/authstate.go
@@ -165,7 +165,7 @@ func checkAllowedByAuthEvents(
 	event PDU, eventsByID map[string]PDU,
 	missingAuth EventProvider, userIDForSender spec.UserIDForSender,
 ) error {
-	authEvents := NewAuthEvents(nil)
+	authEvents, _ := NewAuthEvents(nil)
 
 	for _, ae := range event.AuthEventIDs() {
 	retryEvent:
@@ -214,7 +214,7 @@ func checkAllowedByAuthEvents(
 
 	// If we made it this far then we've successfully got as many of the auth events as
 	// as described by AuthEventIDs(). Check if they allow the event.
-	if err := Allowed(event, &authEvents, userIDForSender); err != nil {
+	if err := Allowed(event, authEvents, userIDForSender); err != nil {
 		return fmt.Errorf(
 			"gomatrixserverlib: event with ID %q is not allowed by its auth_events: %s",
 			event.EventID(), err.Error(),
@@ -335,7 +335,7 @@ func CheckSendJoinResponse(
 	}
 
 	eventsByID := map[string]PDU{}
-	authEventProvider := NewAuthEvents(nil)
+	authEventProvider, _ := NewAuthEvents(nil)
 
 	// Since checkAllowedByAuthEvents needs to be able to look up any of the
 	// auth events by ID only, we will build a map which contains references
@@ -369,7 +369,7 @@ func CheckSendJoinResponse(
 	}
 
 	// Now check that the join event is valid against the supplied state.
-	if err := Allowed(joinEvent, &authEventProvider, userIDForSender); err != nil {
+	if err := Allowed(joinEvent, authEventProvider, userIDForSender); err != nil {
 		return nil, fmt.Errorf(
 			"gomatrixserverlib: event with ID %q is not allowed by the current room state: %w",
 			joinEvent.EventID(), err,
diff --git a/backfill.go b/backfill.go
@@ -99,7 +99,8 @@ func RequestBackfill(ctx context.Context, origin spec.ServerName, b BackfillRequ
 		}
 	}
 
-	return result, lastErr
+	// Since we pulled in results from multiple servers we need to sort again...
+	return ReverseTopologicalOrdering(result, TopologicalOrderByPrevEvents), lastErr
 }
 
 /*
diff --git a/eventV1.go b/eventV1.go
@@ -215,8 +215,7 @@ func (e *eventV1) SetUnsignedField(path string, value interface{}) error {
 	eventJSON = CanonicalJSONAssumeValid(eventJSON)
 
 	res := gjson.GetBytes(eventJSON, "unsigned")
-	unsigned := RawJSONFromResult(res, eventJSON)
-	e.eventFields.Unsigned = unsigned
+	e.eventFields.Unsigned = []byte(res.Raw)
 
 	e.eventJSON = eventJSON
 
diff --git a/eventauth.go b/eventauth.go
@@ -301,15 +301,17 @@ func (a *AuthEvents) Clear() {
 
 // NewAuthEvents returns an AuthEventProvider backed by the given events. New events can be added by
 // calling AddEvent().
-func NewAuthEvents(events []PDU) AuthEvents {
+func NewAuthEvents(events []PDU) (*AuthEvents, error) {
 	a := AuthEvents{
 		events:  make(map[StateKeyTuple]PDU, len(events)),
 		roomIDs: make(map[string]struct{}),
 	}
 	for _, e := range events {
-		a.AddEvent(e) // nolint: errcheck
+		if err := a.AddEvent(e); err != nil {
+			return nil, err
+		}
 	}
-	return a
+	return &a, nil
 }
 
 // A NotAllowed error is returned if an event does not pass the auth checks.
diff --git a/eventauth_test.go b/eventauth_test.go
@@ -104,7 +104,7 @@ func TestStateNeededForMessage(t *testing.T) {
 	// Message events need the create event, the sender and the power_levels.
 	testStateNeededForAuth(t, `[{
 		"type": "m.room.message",
-		"sender": "@u1:a", 
+		"sender": "@u1:a",
         "room_id": "!r1:a"
 	}]`, &ProtoEvent{
 		Type:     "m.room.message",
@@ -139,7 +139,7 @@ func TestStateNeededForJoin(t *testing.T) {
 		"type": "m.room.member",
 		"state_key": "@u1:a",
 		"sender": "@u1:a",
-		"content": {"membership": "join"}, 
+		"content": {"membership": "join"},
         "room_id": "!r1:a"
 	}]`, &b, StateNeeded{
 		Create:      true,
@@ -163,7 +163,7 @@ func TestStateNeededForInvite(t *testing.T) {
 		"type": "m.room.member",
 		"state_key": "@u2:b",
 		"sender": "@u1:a",
-		"content": {"membership": "invite"}, 
+		"content": {"membership": "invite"},
         "room_id": "!r1:a"
 	}]`, &b, StateNeeded{
 		Create:      true,
@@ -199,7 +199,7 @@ func TestStateNeededForInvite3PID(t *testing.T) {
 					"token": "my_token"
 				}
 			}
-		}, 
+		},
         "room_id": "!r1:a"
 	}]`, &b, StateNeeded{
 		Create:           true,
@@ -1035,7 +1035,7 @@ func TestAuthEvents(t *testing.T) {
 	if err != nil {
 		t.Fatalf("TestAuthEvents: failed to create power_levels event: %s", err)
 	}
-	a := NewAuthEvents([]PDU{power})
+	a, _ := NewAuthEvents([]PDU{power})
 	var e PDU
 	if e, err = a.PowerLevels(); err != nil || e != power {
 		t.Errorf("TestAuthEvents: failed to get same power_levels event")
@@ -1685,15 +1685,15 @@ func TestMembershipBanned(t *testing.T) {
 			"state_key": "@u3:a",
 			"event_id": "$e4:a",
 			"content": {"membership": "ban"}
-		}, 
+		},
 		{
 			"type": "m.room.member",
 			"sender": "@u2:a",
 			"room_id": "!r1:a",
 			"state_key": "@u3:a",
 			"event_id": "$e4:a",
 			"content": {"membership": "ban"}
-		}, 
+		},
 		{
 			"type": "m.room.member",
 			"sender": "@u2:a",
diff --git a/go.mod b/go.mod
@@ -31,4 +31,4 @@ require (
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
 
-go 1.18
+go 1.22
diff --git a/handleinvite_test.go b/handleinvite_test.go
@@ -38,17 +38,19 @@ func (r *TestStateQuerier) GetAuthEvents(ctx context.Context, event PDU) (AuthEv
 		return nil, fmt.Errorf("failed getting auth provider")
 	}
 
-	eventProvider := AuthEvents{}
+	eventProvider, _ := NewAuthEvents(nil)
 	if r.createEvent != nil {
-		eventProvider = NewAuthEvents([]PDU{r.createEvent})
+		if err := eventProvider.AddEvent(r.createEvent); err != nil {
+			return nil, err
+		}
 		if r.inviterMemberEvent != nil {
 			err := eventProvider.AddEvent(r.inviterMemberEvent)
 			if err != nil {
 				return nil, err
 			}
 		}
 	}
-	return &eventProvider, nil
+	return eventProvider, nil
 }
 
 func (r *TestStateQuerier) GetState(ctx context.Context, roomID spec.RoomID, stateWanted []StateKeyTuple) ([]PDU, error) {
diff --git a/handlejoin.go b/handlejoin.go
@@ -116,8 +116,11 @@ func HandleMakeJoin(input HandleMakeJoinInput) (*HandleMakeJoinResponse, error)
 		return nil, spec.InternalServerError{Err: fmt.Sprintf("expected join event from template builder. got: %s", event.Type())}
 	}
 
-	provider := NewAuthEvents(state)
-	if err = Allowed(event, &provider, input.UserIDQuerier); err != nil {
+	provider, err := NewAuthEvents(state)
+	if err != nil {
+		return nil, spec.Forbidden(err.Error())
+	}
+	if err = Allowed(event, provider, input.UserIDQuerier); err != nil {
 		return nil, spec.Forbidden(err.Error())
 	}
 
diff --git a/handlejoin_test.go b/handlejoin_test.go
@@ -632,6 +632,7 @@ func TestHandleMakeJoinNilContext(t *testing.T) {
 	})
 }
 
+//nolint:unparam
 func createMemberEventBuilder(roomVersion RoomVersion, sender string, roomID string, stateKey *string, content spec.RawJSON) *EventBuilder {
 	return MustGetRoomVersion(roomVersion).NewEventBuilderFromProtoEvent(&ProtoEvent{
 		SenderID:   sender,
diff --git a/handleleave.go b/handleleave.go
@@ -81,8 +81,11 @@ func HandleMakeLeave(input HandleMakeLeaveInput) (*HandleMakeLeaveResponse, erro
 		return nil, spec.InternalServerError{Err: fmt.Sprintf("expected leave event from template builder. got: %s", event.Type())}
 	}
 
-	provider := NewAuthEvents(stateEvents)
-	if err := Allowed(event, &provider, input.UserIDQuerier); err != nil {
+	provider, err := NewAuthEvents(stateEvents)
+	if err != nil {
+		return nil, spec.Forbidden(err.Error())
+	}
+	if err = Allowed(event, provider, input.UserIDQuerier); err != nil {
 		return nil, spec.Forbidden(err.Error())
 	}
 
diff --git a/json.go b/json.go
@@ -18,7 +18,7 @@ package gomatrixserverlib
 import (
 	"encoding/binary"
 	"errors"
-	"sort"
+	"slices"
 	"strings"
 	"unicode/utf16"
 	"unicode/utf8"
@@ -159,40 +159,33 @@ func CanonicalJSONAssumeValid(input []byte) []byte {
 // by codepoint. The input must be valid JSON.
 func SortJSON(input, output []byte) []byte {
 	result := gjson.ParseBytes(input)
-
-	RawJSON := RawJSONFromResult(result, input)
-	return sortJSONValue(result, RawJSON, output)
+	return sortJSONValue(result, output)
 }
 
 // sortJSONValue takes a gjson.Result and sorts it. inputJSON must be the
 // raw JSON bytes that gjson.Result points to.
-func sortJSONValue(input gjson.Result, inputJSON, output []byte) []byte {
+func sortJSONValue(input gjson.Result, output []byte) []byte {
 	if input.IsArray() {
-		return sortJSONArray(input, inputJSON, output)
+		return sortJSONArray(input, output)
 	}
-
 	if input.IsObject() {
-		return sortJSONObject(input, inputJSON, output)
+		return sortJSONObject(input, output)
 	}
-
 	// If its neither an object nor an array then there is no sub structure
 	// to sort, so just append the raw bytes.
-	return append(output, inputJSON...)
+	return append(output, input.Raw...)
 }
 
 // sortJSONArray takes a gjson.Result and sorts it, assuming its an array.
 // inputJSON must be the raw JSON bytes that gjson.Result points to.
-func sortJSONArray(input gjson.Result, inputJSON, output []byte) []byte {
+func sortJSONArray(input gjson.Result, output []byte) []byte {
 	sep := byte('[')
 
 	// Iterate over each value in the array and sort it.
 	input.ForEach(func(_, value gjson.Result) bool {
 		output = append(output, sep)
 		sep = ','
-
-		RawJSON := RawJSONFromResult(value, inputJSON)
-		output = sortJSONValue(value, RawJSON, output)
-
+		output = sortJSONValue(value, output)
 		return true // keep iterating
 	})
 
@@ -209,29 +202,30 @@ func sortJSONArray(input gjson.Result, inputJSON, output []byte) []byte {
 
 // sortJSONObject takes a gjson.Result and sorts it, assuming its an object.
 // inputJSON must be the raw JSON bytes that gjson.Result points to.
-func sortJSONObject(input gjson.Result, inputJSON, output []byte) []byte {
+func sortJSONObject(input gjson.Result, output []byte) []byte {
 	type entry struct {
-		key    string // The parsed key string
-		rawKey []byte // The raw, unparsed key JSON string
-		value  gjson.Result
+		key   string // The parsed key string
+		value gjson.Result
 	}
 
-	var entries []entry
+	// Try to stay on the stack here if we can.
+	var _entries [128]entry
+	entries := _entries[:0]
 
 	// Iterate over each key/value pair and add it to a slice
 	// that we can sort
 	input.ForEach(func(key, value gjson.Result) bool {
 		entries = append(entries, entry{
-			key:    key.String(),
-			rawKey: RawJSONFromResult(key, inputJSON),
-			value:  value,
+			key:   key.String(),
+			value: value,
 		})
 		return true // keep iterating
 	})
 
-	// Sort the slice based on the *parsed* key
-	sort.Slice(entries, func(a, b int) bool {
-		return entries[a].key < entries[b].key
+	// Using slices.SortFunc here instead of sort.Slice avoids
+	// heap escapes due to reflection.
+	slices.SortFunc(entries, func(a, b entry) int {
+		return strings.Compare(a.key, b.key)
 	})
 
 	sep := byte('{')
@@ -241,12 +235,10 @@ func sortJSONObject(input gjson.Result, inputJSON, output []byte) []byte {
 		sep = ','
 
 		// Append the raw unparsed JSON key, *not* the parsed key
-		output = append(output, entry.rawKey...)
-		output = append(output, ':')
-
-		RawJSON := RawJSONFromResult(entry.value, inputJSON)
-
-		output = sortJSONValue(entry.value, RawJSON, output)
+		output = append(output, '"')
+		output = append(output, entry.key...)
+		output = append(output, '"', ':')
+		output = sortJSONValue(entry.value, output)
 	}
 	if sep == '{' {
 		// If sep is still '{' then the object was empty and we never wrote the
@@ -375,10 +367,3 @@ func readHexDigits(input []byte) rune {
 	hex |= hex >> 8
 	return rune(hex & 0xFFFF)
 }
-
-// RawJSONFromResult extracts the raw JSON bytes pointed to by result.
-// input must be the json bytes that were used to generate result
-// TODO: Why do we do this?
-func RawJSONFromResult(result gjson.Result, _ []byte) []byte {
-	return []byte(result.Raw)
-}
diff --git a/performinvite.go b/performinvite.go
@@ -123,7 +123,7 @@ func PerformInvite(ctx context.Context, input PerformInviteInput, fedClient Fede
 
 	input.EventTemplate.Depth = latestEvents.Depth
 
-	authEvents := NewAuthEvents(nil)
+	authEvents, _ := NewAuthEvents(nil)
 
 	for _, event := range latestEvents.StateEvents {
 		err := authEvents.AddEvent(event)
@@ -132,7 +132,7 @@ func PerformInvite(ctx context.Context, input PerformInviteInput, fedClient Fede
 		}
 	}
 
-	refs, err := stateNeeded.AuthEventReferences(&authEvents)
+	refs, err := stateNeeded.AuthEventReferences(authEvents)
 	if err != nil {
 		return nil, fmt.Errorf("eventsNeeded.AuthEventReferences: %w", err)
 	}
diff --git a/performjoin_test.go b/performjoin_test.go
@@ -13,7 +13,6 @@ import (
 	"github.com/matrix-org/gomatrix"
 	"github.com/matrix-org/gomatrixserverlib/spec"
 	"github.com/stretchr/testify/assert"
-	ed255192 "golang.org/x/crypto/ed25519"
 )
 
 type TestMakeJoinResponse struct {
@@ -388,7 +387,7 @@ func TestPerformJoinPseudoID(t *testing.T) {
 		return res, nil
 	}
 
-	idCreator := func(ctx context.Context, userID spec.UserID, roomID spec.RoomID, roomVersion string) (spec.SenderID, ed255192.PrivateKey, error) {
+	idCreator := func(ctx context.Context, userID spec.UserID, roomID spec.RoomID, roomVersion string) (spec.SenderID, ed25519.PrivateKey, error) {
 		return spec.SenderIDFromPseudoIDKey(userPriv), userPriv, nil
 	}
 
diff --git a/stateresolutionv2.go b/stateresolutionv2.go
diff --git a/stateresolutionv2_test.go b/stateresolutionv2_test.go
diff --git a/stateresolutionv2heaps.go b/stateresolutionv2heaps.go

Original file line number	Diff line number	Diff line change
`@@ -3,4 +3,4 @@ gomatrixserverlib`
`3`	`3`
`4`	`4`	`[![GoDoc](https://godoc.org/github.com/matrix-org/gomatrixserverlib?status.svg)](https://godoc.org/github.com/matrix-org/gomatrixserverlib)`
`5`	`5`
`6`		`-Go library for common functions needed by matrix servers. This library assumes Go 1.18+.`
	`6`	`+Go library for common functions needed by matrix servers. This library assumes Go 1.22+.`
Original file line number	Diff line number	Diff line change
`@@ -99,7 +99,8 @@ func RequestBackfill(ctx context.Context, origin spec.ServerName, b BackfillRequ`
`99`	`99`	`}`
`100`	`100`	`}`
`101`	`101`
`102`		`- return result, lastErr`
	`102`	`+ // Since we pulled in results from multiple servers we need to sort again...`
	`103`	`+ return ReverseTopologicalOrdering(result, TopologicalOrderByPrevEvents), lastErr`
`103`	`104`	`}`
`104`	`105`
`105`	`106`	`/*`
Original file line number	Diff line number	Diff line change
`@@ -301,15 +301,17 @@ func (a *AuthEvents) Clear() {`
`301`	`301`
`302`	`302`	`// NewAuthEvents returns an AuthEventProvider backed by the given events. New events can be added by`
`303`	`303`	`// calling AddEvent().`
`304`		`-func NewAuthEvents(events []PDU) AuthEvents {`
	`304`	`+func NewAuthEvents(events []PDU) (*AuthEvents, error) {`
`305`	`305`	`a := AuthEvents{`
`306`	`306`	`events: make(map[StateKeyTuple]PDU, len(events)),`
`307`	`307`	`roomIDs: make(map[string]struct{}),`
`308`	`308`	`}`
`309`	`309`	`for _, e := range events {`
`310`		`- a.AddEvent(e) // nolint: errcheck`
	`310`	`+ if err := a.AddEvent(e); err != nil {`
	`311`	`+ return nil, err`
	`312`	`+ }`
`311`	`313`	`}`
`312`		`- return a`
	`314`	`+ return &a, nil`
`313`	`315`	`}`
`314`	`316`
`315`	`317`	`// A NotAllowed error is returned if an event does not pass the auth checks.`
Original file line number	Diff line number	Diff line change
`@@ -31,4 +31,4 @@ require (`
`31`	`31`	`gopkg.in/yaml.v3 v3.0.1 // indirect`
`32`	`32`	`)`
`33`	`33`
`34`		`-go 1.18`
	`34`	`+go 1.22`
Original file line number	Diff line number	Diff line change
`@@ -38,17 +38,19 @@ func (r *TestStateQuerier) GetAuthEvents(ctx context.Context, event PDU) (AuthEv`
`38`	`38`	`return nil, fmt.Errorf("failed getting auth provider")`
`39`	`39`	`}`
`40`	`40`
`41`		`- eventProvider := AuthEvents{}`
	`41`	`+ eventProvider, _ := NewAuthEvents(nil)`
`42`	`42`	`if r.createEvent != nil {`
`43`		`- eventProvider = NewAuthEvents([]PDU{r.createEvent})`
	`43`	`+ if err := eventProvider.AddEvent(r.createEvent); err != nil {`
	`44`	`+ return nil, err`
	`45`	`+ }`
`44`	`46`	`if r.inviterMemberEvent != nil {`
`45`	`47`	`err := eventProvider.AddEvent(r.inviterMemberEvent)`
`46`	`48`	`if err != nil {`
`47`	`49`	`return nil, err`
`48`	`50`	`}`
`49`	`51`	`}`
`50`	`52`	`}`
`51`		`- return &eventProvider, nil`
	`53`	`+ return eventProvider, nil`
`52`	`54`	`}`
`53`	`55`
`54`	`56`	`func (r *TestStateQuerier) GetState(ctx context.Context, roomID spec.RoomID, stateWanted []StateKeyTuple) ([]PDU, error) {`
Original file line number	Diff line number	Diff line change
`@@ -116,8 +116,11 @@ func HandleMakeJoin(input HandleMakeJoinInput) (*HandleMakeJoinResponse, error)`
`116`	`116`	`return nil, spec.InternalServerError{Err: fmt.Sprintf("expected join event from template builder. got: %s", event.Type())}`
`117`	`117`	`}`
`118`	`118`
`119`		`- provider := NewAuthEvents(state)`
`120`		`- if err = Allowed(event, &provider, input.UserIDQuerier); err != nil {`
	`119`	`+ provider, err := NewAuthEvents(state)`
	`120`	`+ if err != nil {`
	`121`	`+ return nil, spec.Forbidden(err.Error())`
	`122`	`+ }`
	`123`	`+ if err = Allowed(event, provider, input.UserIDQuerier); err != nil {`
`121`	`124`	`return nil, spec.Forbidden(err.Error())`
`122`	`125`	`}`
`123`	`126`