Port Event.signatures field to Rust#19706
Merged
Merged
Conversation
erikjohnston
commented
Apr 17, 2026
|
|
||
| [features] | ||
| extension-module = ["pyo3/extension-module"] | ||
| default = ["extension-module"] |
Member
Author
There was a problem hiding this comment.
Using this feature means that cargo test doesn't work when interacting with pyo3 types. This is a known issue, and so PyO3 have moved to having maturin and setuptools_rust set the equivalent env var.
This should be a no-op change.
c.f. PyO3/pyo3#5202
erikjohnston
force-pushed
the
erikj/port_signatures_field
branch
from
8f525d6 to
e5648a0
Compare
erikjohnston
force-pushed
the
erikj/port_signatures_field
branch
from
e5648a0 to
5b57b4e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Ports the Event.signatures structure from a Python dict to a Rust-backed Signatures PyO3 class, as part of the incremental Rust migration of the event model.
Changes:
- Introduces a new Rust
SignaturesPyO3 class with helpers likeget_signature(),update(), andas_dict(). - Updates Python call sites to use the new API (
get_signature,in,update,as_dict) instead of direct nested-dict access patterns. - Wraps event signatures in
EventBaseand ensures JSON materialization usesas_dict().
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/handlers/test_federation.py | Updates test to avoid direct item assignment on signatures, using update() instead. |
| synapse/synapse_rust/events.pyi | Adds type stub for the new Signatures class. |
| synapse/storage/databases/main/events_bg_updates.py | Switches old-key signature lookup to get_signature(). |
| synapse/handlers/room_policy.py | Uses update() for applying policy server signatures and moves request creation into try. |
| synapse/handlers/message.py | Converts remote invite signatures via as_dict() before updating local event signatures. |
| synapse/events/init.py | Wraps incoming signatures in Signatures and serializes via as_dict() in get_dict(). |
| synapse/event_auth.py | Uses __contains__ (in) rather than .get(...) on signatures. |
| synapse/crypto/keyring.py | Updates signature existence checks and key-id extraction to use in + __getitem__. |
| synapse/crypto/event_signing.py | Uses get_signature() for determining whether an event needs re-signing. |
| rust/src/events/signatures.rs | Adds the Rust implementation of the Signatures PyO3 class (plus unit tests). |
| rust/src/events/mod.rs | Registers the new Signatures class in the Rust → Python module. |
| rust/Cargo.toml | Enables serde rc feature (for Arc serialization support). |
| changelog.d/19706.misc | Adds a changelog entry for the port. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anoadragon453
requested changes
May 1, 2026
Member
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
A few minor points, but looks good on the whole 🦀
erikjohnston
added a commit
that referenced
this pull request
May 6, 2026
Similar to #19706, let's port the `unsigned` field into a Rust class. This does change things a bit in that we now define exactly what unsigned fields that are allowed to be added to an event, and what actually gets persisted. This should be a noop though, as we carefully filter out what unsigned fields we allow in from federation, for example As a side effect of this cleanup, I think this fixes handling `unsigned.age` on events received over federation.
tulir
reviewed
May 21, 2026
| event.signatures.setdefault(policy_server.server_name, {}).update( | ||
| signature.get(policy_server.server_name, {}) | ||
| ) | ||
| event.signatures.update(signature) |
Contributor
There was a problem hiding this comment.
Is this just casually re-introducing the bug that the preceding 12-line comment warns about?
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is another stepping stone in porting the event class fully to Rust.
The new
Signaturesclass is relatively simple, as we actually don't interact with it that much in the code. It does not implementMappingorMutableMappingas that takes quite a lot of effort that we don't need, even though it would be more ergonomic.