Replace LeafNodeRef with leaf index. #658
Conversation
There was a problem hiding this comment.
I don't have a strong opinion on this question, but this change seems sensible to me. The use of LeafNodeRef requires either (a) a linear scan through the tree, which is time-expensive in large trees, or (b) keeping an index that maps LeafNodeRef values to indices, which is storage-intensive in large trees.
IIRC, the change to LeafNodeRef was made in the context of @TWal's work to make the tree description less tied to the array representation. In general, that is the right thing to do, and it's good that we did it. But we still have leaf indices anyway, for unmerged_leaves. Since we still have the concept, we might as well use it where it makes sense. And it certainly seems to me like Sender.sender and removed make sense, since they are internal to the protocol.
In case folks are worried about the application-facing API -- MLS stacks can certainly be more intelligent, e.g., exposing remove(LeafNodeRef) or remove(ApplicationIDFromExtension) or remove(SignaturePublicKey). But they can do that regardless of this PR.
| Internally to the protocol, group members are uniquely identified by their leaf | ||
| index. However, a leaf index is only valid for referring to members in a given | ||
| epoch. The same leaf index may represent a different member, or no member at | ||
| all, in a subsequent epoch. |
There was a problem hiding this comment.
This could be a little more concrete: A member's leaf index is stable while that member is part of the group, but before that member joins or after they leave, a different member may occupy that leaf.
This seems to be a layer of indirection that doesn't add any value to implementation.