MLS group chat: transactions and accounts - #113
Open
thantsintoe wants to merge 7 commits into
Open
Conversation
A group_commit names the epoch it was built against and the network accepts
one commit per epoch, so members lose ordinary races. That check needs the
GroupAccount, so it only ran in validate() -- which apply() reaches after
consensus, where a rejection still takes a full fee from the loser. The client
grew a leaf-index stagger and a backoff to make losing rarer, which is
treating the symptom.
Group types were absent from preCrackableTxTypes, so every commit went
straight to the queue. They are in it now, with a branch that loads the
GroupAccount and the sender, and validatePreCrack carrying the checks those
two accounts can answer. The GroupTreeAccount is deliberately not fetched: the
fence needs group.epoch alone, and the ratchet tree is ~112 kB at 32 members.
group_leave needed no new code -- its validate already reads only those two.
This is an optimization for honest senders and not a security boundary. It
runs only on the node a client injects to; a transaction spread by gossip
reaches handleSharedTX, which calls app.validate and never precracks. The
comments say so wherever someone might later be tempted to lean on it.
Also bounds treeDelta's node index, which was checked for non-negativity and
nothing else. applyTreeDelta grows an array to reach it while the size cap
measures only the node payload, so {i: 50000000, n: "AA"} was ~18 bytes that
cost fifty million array slots. The bound is in validate_fields because that
is the only check a gossiped transaction has to clear.
And drops two debug logs that printed the whole ratchet tree on every commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing a member blanks its ancestors, and someone has to spend a commit to fill them in. That someone is whichever member happens to sit nearby, so today a bystander pays for the group's upkeep. This adds the balance that will pay instead; spending it comes next. Funded per added member, by the admin doing the adding, so the cost of a member's eventual departure is paid by whoever chose to admit them. A commit that adds nobody owes nothing -- a rekey or a removal draws the balance down, it does not top it up. Held in LIB rather than as a count of prepaid repairs. What a deposit needs to buy is one future repair commit, and the price of that is whatever the fee is on the day it happens, so the deposit is a multiple of the live fee and solvency gets judged against the fee current when it is read. A count fixed at add time goes wrong the moment the fee moves. repairDepositOwed is shared by validate, validatePreCrack and apply on purpose: if they disagreed about the amount, a commit could pass validation and then underflow the admin's balance in apply. The serializer carries no version tag, so appending a field would make every existing group unloadable -- readString past the end of the buffer. The read is guarded on isAtOrPastEnd instead, and an older buffer decodes as an unfunded group. A test builds an old-layout buffer field by field to hold that compatibility in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The group's balance now pays the fee on a commit that repairs the tree, and the member who happened to perform it is left alone. That member was never the cause of the work: a removal blanks the departing leaf's ancestors, and whoever sits nearby ends up committing the path update that fills them in. Eligibility is derived, not declared. The tree is stored as a JSON array of node-or-null, so "was that node blank" is an array lookup -- no MLS parsing, and nothing the sender can assert. It deliberately asks whether the tree was damaged and this commit repairs it, rather than whether the sender is the member who ought to have done it; the server holds members[] but not leaf indices, so it could not answer the second question. Two calls worth knowing. An index at or past the end of the array counts as blank, because trailing blanks are trimmed on write and a removal on the right of the tree leaves its ancestors -- root included -- simply absent; refusing those would deny the subsidy to exactly the repairs it exists to fund. And a delta entry that blanks a node never counts, since blanking is damage. The sender's balance requirement is lifted for a repair the group will cover, or the member with an empty wallet still could not submit one. validate() can check that exactly. validatePreCrack cannot -- it has no tree, by design -- so it approximates with the questions the GroupAccount can answer and errs towards letting work through, because a pre-queue hook wrongly refusing an honest repair is the failure that would actually hurt. The balance still never pays for a FAILED commit. Anyone who can inject transactions can generate those at will, since precrack only screens the node a client injects to, so paying for them would be an open drain. Absent tx arrays are read as empty throughout. validate_fields rejects non-arrays before any of this runs, so the only way to arrive without them is a caller that skipped it, and "nobody was added" is the right reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
group_commit collects a deposit per added member and spends it repairing the tree, but between those two there was no way to put anything in. This adds group_maintenance_fund, and exposes the balance on GET /group/:groupId so a client can see when upkeep is running out. Funding is open to anyone, member or not. The balance is spendable on exactly one thing -- burning the fee on a repair commit -- and there is no withdrawal transaction anywhere in the system, so a contribution cannot be redirected or taken back out. That is what makes a stranger's contribution safe to accept and the balance uninteresting to steal. A test walks src/transactions and fails if anything other than group_commit ever subtracts from it, so the argument stays true rather than merely documented. The balance is exposed over the API because the server cannot raise the alarm itself: the transcript is ciphertext it holds no key for, so it can never write into a group chat. Warning about low upkeep has to be the client's job, and the client needs the number. No end-of-life payout, contrary to the plan. That was designed around the last member leaving and taking the remainder, but group_leave refuses to let the last member go at all -- so a group always keeps at least one, and a one-member group has no copath and can never need a repair. The balance is never stranded, only idle until the group grows again, at which point the adds that grow it top it up anyway. Adding an exit would have weakened the no-withdrawal invariant to solve a problem that does not exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An admin sitting on the Group info page does not see a join request arrive. The polling and the re-render both already exist -- syncGroup fetches /group/:groupId every tick, and onGroupUpdated re-renders an open Group info for that group -- but nothing in that chain notices a request, so the signal never fires and the page only updates when it is closed and reopened. The requests are on the cold tree account, and loading a ~112 kB ratchet tree on every poll to discover an integer would undo the reason the accounts are split. So the COUNT is mirrored onto the GroupAccount, which is polled already, and rides along on a response the client is fetching regardless. It is recomputed from the map at every site that touches it, never incremented. All three already hold the tree, so deriving it is free, and a derived number cannot drift the way a hand-maintained counter eventually does -- a test corrupts it and shows the next change heals it. group_join_reclaim had to grow its key set to name the group account. It only claimed the tree before, which is correct for a withdrawal and wrong once the count lives elsewhere: a withdrawn request would have stayed counted forever and admins would see a badge for someone no longer asking. Serialized last and read back on isAtOrPastEnd, so groups written before this field still load, same as maintenanceBalance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thantsintoe
changed the title
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
1 participant
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.
7 commits, 29 files, +5,692 / −2. Additive almost throughout: 20 new files
against 9 modified, and the only two deleted lines are a pair of debug
console.logs. Merges intodevwith no conflicts.The network becomes the delivery service for RFC 9420 MLS groups — ordering
commits, holding ciphertext, and enforcing the one rule MLS cannot enforce for
itself.
The epoch fence
MLS cannot tolerate two members committing at the same epoch; that is an
unrecoverable state fork. Because every group transaction targets one account,
Shardus orders them deterministically, so requiring
tx.epoch === group.epochmakes the network enforce exactly-one-commit-per-epoch no matter how many
members race. The loser applies the winner's commit and retries.
This is stronger than a conventional delivery service can offer, and it is the
reason MLS fits a blockchain well. Everything else here depends on it.
Two accounts per group, split on purpose
GroupAccountis the hot half — roster, epoch, message transcript — loaded andre-hashed on every
group_message.GroupTreeAccountis the cold half: theratchet tree (~112 kB at 32 members), commit transcript, welcomes, join
requests.
keys()for a message names only the former.That split shapes several decisions below, and reviewing them without it in mind
will make some of them look arbitrary.
What each commit does
50992776b4a4e3e0c11ba5ed2132045ddb774af9f7d0bcdfgroup_maintenance_fund, and the balance on the API6b7fedc2Testing
npm test— 6 new suites, 40 assertions, all passing.groupCommitPreCrackgroupCommitTreeDeltagroupMaintenanceBalancegroupRepairPaysFromBalanceapply()with real accountsgroupMaintenanceFundgroupPendingJoinCountNote on the pre-existing suite: 13 suites / 17 assertions fail on
devalready (DAO param resolver, serialisations,
shardus,main) — stale fixturesagainst changed factory signatures. This branch neither fixes nor worsens them;
verified by stashing and re-running. Also: tests need Node ≥ 22. The default
nodein this repo's environment is v12, which cannot parse??and fails inways that have nothing to do with the code.
Merge order
The web client's
feat/group-chat-mlsdepends on this. Two API fields areinvolved and they do not degrade the same way —
pendingJoinCountis guardedclient-side and skips cleanly, but
maintenanceBalanceis coerced to0, whichwould show a false "upkeep empty" warning on every group with 3+ members.
This should merge first.
Known gaps
fill no blank node, so they stay charged to the member.
applyTreeDeltawrites node blobs blindly, so a member can already wedge agroup with a malformed delta — pre-existing, and the client's
needsResetpath handles it. This makes that particular attack cost the group's money
rather than the attacker's. Bounded at one per epoch by the fence, but real.
group_messageis not precracked. It is the highest-volume grouptransaction and would add an account fetch per message; worth measuring first.