From fc1d8e479b282b56dd0eb707559785dc20fc65a6 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Fri, 16 Sep 2022 10:35:32 +0100 Subject: [PATCH 1/5] Ensure that all state event IDs are included in the `added` section when rewriting state (#2725) This should hopefully fix an entire class of problems where components downstream from the roomserver (i.e. the sync API) could just lose a whole bunch of state after a rewrite operation like a federated join. The root of the bug is that we set `RewritesState` in the output event which instructs downstream components to purge their copy of any room state, but then didn't send the entire state snapshot in `adds_state_event_ids` so the downstream state ends up being incomplete as a result. --- .../internal/input/input_latest_events.go | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/roomserver/internal/input/input_latest_events.go b/roomserver/internal/input/input_latest_events.go index 205a33e83..a223820ef 100644 --- a/roomserver/internal/input/input_latest_events.go +++ b/roomserver/internal/input/input_latest_events.go @@ -264,16 +264,27 @@ func (u *latestEventsUpdater) latestState() error { return fmt.Errorf("roomState.CalculateAndStoreStateAfterEvents: %w", err) } - // Now that we have a new state snapshot based on the latest events, - // we can compare that new snapshot to the previous one and see what - // has changed. This gives us one list of removed state events and - // another list of added ones. Replacing a value for a state-key tuple - // will result one removed (the old event) and one added (the new event). - u.removed, u.added, err = roomState.DifferenceBetweeenStateSnapshots( - ctx, u.oldStateNID, u.newStateNID, - ) - if err != nil { - return fmt.Errorf("roomState.DifferenceBetweenStateSnapshots: %w", err) + // Include information about what changed in the state transition. If the + // event rewrites the state (i.e. is a federated join) then we will simply + // include the entire state snapshot as added events, as the "RewritesState" + // flag in the output event signals downstream components to purge their + // room state first. If it doesn't rewrite the state then we will work out + // what the difference is between the state snapshots and send that. In all + // cases where a state event is being replaced, the old state event will + // appear in "removed" and the replacement will appear in "added". + if u.rewritesState { + u.removed = []types.StateEntry{} + u.added, err = roomState.LoadStateAtSnapshot(ctx, u.newStateNID) + if err != nil { + return fmt.Errorf("roomState.LoadStateAtSnapshot: %w", err) + } + } else { + u.removed, u.added, err = roomState.DifferenceBetweeenStateSnapshots( + ctx, u.oldStateNID, u.newStateNID, + ) + if err != nil { + return fmt.Errorf("roomState.DifferenceBetweenStateSnapshots: %w", err) + } } if removed := len(u.removed) - len(u.added); !u.rewritesState && removed > 0 { From 7bfc3074d10b2cd91b37290e39d3882119853107 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Fri, 16 Sep 2022 13:30:20 +0200 Subject: [PATCH 2/5] Fix origin on device list update EDUs --- federationapi/producers/syncapi.go | 4 ++-- federationapi/routing/send.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/federationapi/producers/syncapi.go b/federationapi/producers/syncapi.go index 4abd3fbe5..659ff1bcf 100644 --- a/federationapi/producers/syncapi.go +++ b/federationapi/producers/syncapi.go @@ -163,10 +163,10 @@ func (p *SyncAPIProducer) SendPresence( } func (p *SyncAPIProducer) SendDeviceListUpdate( - ctx context.Context, deviceListUpdate gomatrixserverlib.RawJSON, origin string, + ctx context.Context, deviceListUpdate gomatrixserverlib.RawJSON, origin gomatrixserverlib.ServerName, ) (err error) { m := nats.NewMsg(p.TopicDeviceListUpdate) - m.Header.Set("origin", origin) + m.Header.Set("origin", string(origin)) m.Data = deviceListUpdate log.Debugf("Sending device list update: %+v", m.Header) _, err = p.JetStream.PublishMsg(m, nats.Context(ctx)) diff --git a/federationapi/routing/send.go b/federationapi/routing/send.go index a9714c65a..060af676d 100644 --- a/federationapi/routing/send.go +++ b/federationapi/routing/send.go @@ -359,7 +359,7 @@ func (t *txnReq) processEDUs(ctx context.Context) { } } case gomatrixserverlib.MDeviceListUpdate: - if err := t.producer.SendDeviceListUpdate(ctx, e.Content, e.Origin); err != nil { + if err := t.producer.SendDeviceListUpdate(ctx, e.Content, t.Origin); err != nil { util.GetLogger(ctx).WithError(err).Error("failed to InputDeviceListUpdate") } case gomatrixserverlib.MReceipt: From 99f6b6a95234f59a16806b9dc6b16cb41f038504 Mon Sep 17 00:00:00 2001 From: Tak Wai Wong <64229756+tak-hntlabs@users.noreply.github.com> Date: Mon, 19 Sep 2022 09:39:06 -0700 Subject: [PATCH 3/5] Bug fix #2718 appservice txnid should be different for each batch of events (#2719) See issue: [#2718](https://github.com/matrix-org/dendrite/issues/2718) for more details. The fix assumes that if the number of transaction items are different, then the txnid should be different. txnid := OriginalServerTS()_len(transactions) The case that it doesn't address is if the txnid generated this way is the same for 2 different batches of events which have the same OriginalServerTS and the same array length. Another option: txnid := OriginalServerTS()_hash(transactions) Would love to hear other ideas and ways to fix this. ### Pull Request Checklist * [x ] I have added added tests for PR _or_ I have justified why this PR doesn't need tests. * [x ] Pull request includes a [sign off](https://github.com/matrix-org/dendrite/blob/main/docs/CONTRIBUTING.md#sign-off) Signed-off-by: `Tak Wai Wong ` Co-authored-by: Tak Wai Wong --- appservice/consumers/roomserver.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/appservice/consumers/roomserver.go b/appservice/consumers/roomserver.go index a4bcfa7d9..d44f32b38 100644 --- a/appservice/consumers/roomserver.go +++ b/appservice/consumers/roomserver.go @@ -22,6 +22,7 @@ import ( "math" "net/http" "net/url" + "strconv" "time" "github.com/matrix-org/gomatrixserverlib" @@ -151,10 +152,17 @@ func (s *OutputRoomEventConsumer) onMessage( return true } + txnID := "" + // Try to get the message metadata, if we're able to, use the timestamp as the txnID + metadata, err := msgs[0].Metadata() + if err == nil { + txnID = strconv.Itoa(int(metadata.Timestamp.UnixNano())) + } + // Send event to any relevant application services. If we hit // an error here, return false, so that we negatively ack. log.WithField("appservice", state.ID).Debugf("Appservice worker sending %d events(s) from roomserver", len(events)) - return s.sendEvents(ctx, state, events) == nil + return s.sendEvents(ctx, state, events, txnID) == nil } // sendEvents passes events to the appservice by using the transactions @@ -162,6 +170,7 @@ func (s *OutputRoomEventConsumer) onMessage( func (s *OutputRoomEventConsumer) sendEvents( ctx context.Context, state *appserviceState, events []*gomatrixserverlib.HeaderedEvent, + txnID string, ) error { // Create the transaction body. transaction, err := json.Marshal( @@ -173,13 +182,14 @@ func (s *OutputRoomEventConsumer) sendEvents( return err } - // TODO: We should probably be more intelligent and pick something not - // in the control of the event. A NATS timestamp header or something maybe. - txnID := events[0].Event.OriginServerTS() + // If txnID is not defined, generate one from the events. + if txnID == "" { + txnID = fmt.Sprintf("%d_%d", events[0].Event.OriginServerTS(), len(transaction)) + } // Send the transaction to the appservice. // https://matrix.org/docs/spec/application_service/r0.1.2#put-matrix-app-v1-transactions-txnid - address := fmt.Sprintf("%s/transactions/%d?access_token=%s", state.URL, txnID, url.QueryEscape(state.HSToken)) + address := fmt.Sprintf("%s/transactions/%s?access_token=%s", state.URL, txnID, url.QueryEscape(state.HSToken)) req, err := http.NewRequestWithContext(ctx, "PUT", address, bytes.NewBuffer(transaction)) if err != nil { return err From cf01d29277accf42ac31358ff1407abc7bf63fea Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 20 Sep 2022 09:47:57 +0100 Subject: [PATCH 4/5] Update contributing documentation --- docs/CONTRIBUTING.md | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 771af9ecf..6ba05f46f 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -11,8 +11,8 @@ possible to get started. ## Sign off -We ask that everyone who contributes to the project signs off their contributions -in accordance with the [DCO](https://github.com/matrix-org/matrix-spec/blob/main/CONTRIBUTING.rst#sign-off). +We require that everyone who contributes to the project signs off their contributions +in accordance with the [Developer Certificate of Origin](https://github.com/matrix-org/matrix-spec/blob/main/CONTRIBUTING.rst#sign-off). In effect, this means adding a statement to your pull requests or commit messages along the lines of: @@ -20,7 +20,18 @@ along the lines of: Signed-off-by: Full Name ``` -Unfortunately we can't accept contributions without it. +Unfortunately we can't accept contributions without a sign-off. + +Please note that we can only accept contributions under a legally identifiable name, +such as your name as it appears on government-issued documentation or common-law names +(claimed by legitimate usage or repute). We cannot accept sign-offs from a pseudonym or +alias and cannot accept anonymous contributions. + +If you would prefer to sign off privately instead (so as to not reveal your full +name on a public pull request), you can do so by emailing a sign-off declaration +and a link to your pull request directly to the [Matrix.org Foundation](https://matrix.org/foundation/) +at `dco@matrix.org`. Once a private sign-off has been made, you will not be required +to do so for future contributions. ## Getting up and running From 47af4bff5b930c86630c7879e14f5b07b9d08497 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 20 Sep 2022 09:52:22 +0100 Subject: [PATCH 5/5] Update database documentation --- docs/installation/4_database.md | 40 ++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/docs/installation/4_database.md b/docs/installation/4_database.md index f6222a8d2..68f2d44d0 100644 --- a/docs/installation/4_database.md +++ b/docs/installation/4_database.md @@ -10,24 +10,6 @@ permalink: /installation/database Dendrite uses SQL databases to store data. Depending on the database engine being used, you may need to perform some manual steps outlined below. -## SQLite - -SQLite deployments do not require manual database creation. Simply configure the database -filenames in the Dendrite configuration file and start Dendrite. The databases will be created -and populated automatically. - -Note that Dendrite **cannot share a single SQLite database across multiple components**. Each -component must be configured with its own SQLite database filename. You will have to remove -the `global.database` section from your Dendrite config and add it to each individual section -instead in order to use SQLite. - -### Connection strings - -Connection strings for SQLite databases take the following forms: - -* Current working directory path: `file:dendrite_component.db` -* Full specified path: `file:///path/to/dendrite_component.db` - ## PostgreSQL Dendrite can automatically populate the database with the relevant tables and indexes, but @@ -106,3 +88,25 @@ for i in appservice federationapi mediaapi mscs roomserver syncapi keyserver use sudo -u postgres createdb -O dendrite dendrite_$i done ``` + +## SQLite + +**WARNING:** The Dendrite SQLite backend is slower, less reliable and not recommended for +production usage. You should use PostgreSQL instead. We may not be able to provide support if +you run into issues with your deployment while using the SQLite backend. + +SQLite deployments do not require manual database creation. Simply configure the database +filenames in the Dendrite configuration file and start Dendrite. The databases will be created +and populated automatically. + +Note that Dendrite **cannot share a single SQLite database across multiple components**. Each +component must be configured with its own SQLite database filename. You will have to remove +the `global.database` section from your Dendrite config and add it to each individual section +instead in order to use SQLite. + +### Connection strings + +Connection strings for SQLite databases take the following forms: + +* Current working directory path: `file:dendrite_component.db` +* Full specified path: `file:///path/to/dendrite_component.db`