fix(marmot): handle being removed mid-commit without OOM (test 14)
When wn admin-removes A in interop test 14, A processed the proposal
locally — `tree.removeLeaf(A.leafIndex)` blanked her leaf and shrank
`tree.leafCount` past `myLeafIndex` — and then immediately walked into
BinaryTree.directPath(myLeafIndex, tree.leafCount)
at MlsGroup.processCommitInner. With `myLeafIndex >= leafCount`, the
left-balanced parent walk had no valid stopping point: each recursion
step doubled the candidate parent index, integer-overflowed past 2^31,
and either returned garbage or kept appending to the result list until
the JVM OOM'd. The OOM bubbled up as a `runBlocking` failure that
rolled back the whole commit — so A also stayed locally convinced she
was still a member, and the test timed out waiting for `not_member`.
Three layered fixes so the failure mode can't recur:
* `BinaryTree.parent`/`directPath`/`copath` now `require` an in-range
input. The root and any node ≥ nodeCount used to silently loop or
return garbage; now they throw `IllegalArgumentException` immediately.
This is defense-in-depth — a future caller passing an invalid index
gets a stack trace at the boundary instead of an OOM five frames
deep.
* `MlsGroup.processCommitInner` short-circuits the path-decrypt + epoch
advancement when the proposals in the commit just removed *us*. We
preserve the proposal-side tree mutations so the caller (and the
outer state machine) can observe that we're out, clear pending
proposals + sent keys, and return. Without this short-circuit the
function would derive a bogus all-zero `commit_secret`, fail
`confirmation_tag` verification, throw, and roll the snapshot back —
leaving us still convinced we were a live member.
* `MlsGroupManager.isMember` and a new `MlsGroup.isLocalMember()`
helper now return false once our leaf is null or past `leafCount`.
The post-Remove group entry still lives in `groups` so callers can
inspect the final tree, but every cli command (`group show`,
`message send`, etc.) sees `not_member` and returns the right error.
Also add a comprehensive `BinaryTreeTest` covering non-power-of-2
trees (3, 5, …, 32 leaves) and the boundary cases (root has no
parent, leafIndex ≥ leafCount must throw). The pre-existing tests
only exercised the 4-leaf example from RFC 9420 Appendix C; nothing
hit the `parentInRange` branch, which is exactly where the OOM lived.
Test 14's bash polling needed a small companion fix: it captured
amy's stderr through `2>&1` and fed the result to `jq`, but the
captured stream is interleaved with quartz `Log.d(…)` debug lines,
so the JSON parse always failed. Switch to a `grep` for the
`"error":"not_member"` literal — that signature only appears in
the JSON payload and survives the debug noise. Also tee the
captured output into `$LOG_FILE` so post-mortem logs include each
polling iteration's `[cli] ingest …` traces.
Marmot interop score: 11/16 → 14/16 (tests 9, 15 are unrelated MLS
issues — see follow-up).
https://claude.ai/code/session_013VYkpz8P1mPh9Ejxy9anhJ
This commit is contained in:
Claude
@@ -133,6 +133,18 @@ class MlsGroup private constructor(
|
||||
val leafIndex: Int get() = myLeafIndex
|
||||
val extensions: List<com.vitorpamplona.quartz.marmot.mls.tree.Extension> get() = groupContext.extensions
|
||||
|
||||
/**
|
||||
* True while our own leaf is still live in the tree.
|
||||
*
|
||||
* After `processCommit` applies a Remove proposal that targets us, the
|
||||
* group is intentionally kept in the manager so callers can inspect the
|
||||
* final tree state — but our leaf is null and `leafCount` may have
|
||||
* shrunk past `myLeafIndex`. Either condition means we can't propose,
|
||||
* commit, encrypt, or decrypt any further; surface that here so callers
|
||||
* get a clean answer instead of poking at private tree internals.
|
||||
*/
|
||||
fun isLocalMember(): Boolean = myLeafIndex < tree.leafCount && tree.getLeaf(myLeafIndex) != null
|
||||
|
||||
// --- Marmot admin helpers (MIP-01 / MIP-03) ---
|
||||
|
||||
/** Raw BasicCredential identity bytes of the member at the given leaf, or null. */
|
||||
@@ -1272,6 +1284,26 @@ class MlsGroup private constructor(
|
||||
newLeavesInCommit.add(applyProposalAdd(add))
|
||||
}
|
||||
|
||||
// If the proposals just removed *us*, there is no path-decrypt to do
|
||||
// and no confirmation_tag to verify against our (now bogus) commit
|
||||
// secret — we have no valid leaf and our copy of `commit_secret`
|
||||
// would be all-zeros, so the keyschedule would diverge from every
|
||||
// remaining member's. Without this short-circuit, the path-decrypt
|
||||
// block calls `BinaryTree.directPath(myLeafIndex, tree.leafCount)`
|
||||
// with an out-of-range index and OOMs the JVM (interop test 14).
|
||||
//
|
||||
// The proposals have already mutated the tree; preserve those
|
||||
// mutations on return so the outer state machine knows we are out
|
||||
// of the group. `pendingProposals` are cleared because they were
|
||||
// tied to the previous epoch.
|
||||
val removedSelf =
|
||||
myLeafIndex >= tree.leafCount || tree.getLeaf(myLeafIndex) == null
|
||||
if (removedSelf) {
|
||||
pendingProposals.clear()
|
||||
sentKeys.clear()
|
||||
return
|
||||
}
|
||||
|
||||
// Process UpdatePath
|
||||
var commitSecret = ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH)
|
||||
if (commit.updatePath != null) {
|
||||
|
||||
+11
-2
@@ -158,9 +158,18 @@ class MlsGroupManager(
|
||||
fun activeGroupIds(): Set<HexKey> = groups.keys.toSet()
|
||||
|
||||
/**
|
||||
* Check if we are a member of the given group.
|
||||
* Check if we are a (live) member of the given group.
|
||||
*
|
||||
* After processing a commit that removes us, the group entry stays in
|
||||
* [groups] so callers can still inspect the post-Remove tree, but our
|
||||
* own leaf has been set to null and the leaf count may have shrunk
|
||||
* past `myLeafIndex`. Either condition means we cannot encrypt any
|
||||
* more messages, propose anything, or decrypt future epochs — i.e.
|
||||
* we are functionally not a member, and surface as such here so
|
||||
* callers (`amy marmot group show`, the inbound processor) get a
|
||||
* clean `not_member` answer instead of a misleading `true`.
|
||||
*/
|
||||
fun isMember(nostrGroupId: HexKey): Boolean = groups.containsKey(nostrGroupId)
|
||||
fun isMember(nostrGroupId: HexKey): Boolean = groups[nostrGroupId]?.isLocalMember() ?: false
|
||||
|
||||
// --- Group Creation ---
|
||||
|
||||
|
||||
+43
-21
@@ -88,34 +88,40 @@ object BinaryTree {
|
||||
/**
|
||||
* Parent of a node in a tree with [nodeCount] total nodes.
|
||||
* Uses the left-balanced tree parent formula per RFC 9420 Appendix C.
|
||||
*
|
||||
* The root and any index ≥ [nodeCount] have no defined parent — the
|
||||
* left-balanced formula keeps walking up through virtual parents that
|
||||
* never come back into range, integer-overflows after ~31 doublings,
|
||||
* and either returns garbage or OOMs the call site that's storing the
|
||||
* result. Both modes are signs of a tree-accounting bug upstream
|
||||
* (e.g., calling directPath with a leafIndex past the post-Remove
|
||||
* leafCount); fail loudly here so callers can't silently corrupt
|
||||
* the rest of the commit-processing pipeline.
|
||||
*/
|
||||
fun parent(
|
||||
nodeIndex: Int,
|
||||
nodeCount: Int,
|
||||
): Int {
|
||||
// For left-balanced trees with non-power-of-2 leaves,
|
||||
// the parent might be beyond nodeCount. In that case,
|
||||
// the node's parent is the next valid ancestor.
|
||||
val k = level(nodeIndex)
|
||||
val b = (nodeIndex shr (k + 1)) and 1
|
||||
val p =
|
||||
if (b == 0) {
|
||||
nodeIndex + (1 shl k)
|
||||
} else {
|
||||
nodeIndex - (1 shl k)
|
||||
}
|
||||
|
||||
return if (p < nodeCount) {
|
||||
p
|
||||
} else {
|
||||
// Parent is out of range. This node is at the rightmost
|
||||
// edge of a non-full tree. Walk up through virtual parents
|
||||
// until we find one within range.
|
||||
parentInRange(p, nodeCount)
|
||||
require(nodeCount > 0) { "nodeCount must be positive, got $nodeCount" }
|
||||
require(nodeIndex in 0 until nodeCount) {
|
||||
"nodeIndex $nodeIndex out of range [0, $nodeCount)"
|
||||
}
|
||||
// The structural root of a left-balanced tree with `leafCount`
|
||||
// leaves sits at (1 << ceil(log2(leafCount))) - 1, derivable from
|
||||
// nodeCount via leafCount = (nodeCount + 1) / 2.
|
||||
val leafCount = (nodeCount + 1) / 2
|
||||
require(nodeIndex != root(leafCount)) {
|
||||
"nodeIndex $nodeIndex is the root of a $leafCount-leaf tree and has no parent"
|
||||
}
|
||||
return parentRec(nodeIndex, nodeCount)
|
||||
}
|
||||
|
||||
private fun parentInRange(
|
||||
/**
|
||||
* Inner walker that assumes [nodeIndex] has at least one valid parent
|
||||
* within [nodeCount]. Public callers go through [parent] which performs
|
||||
* the bounds check above.
|
||||
*/
|
||||
private fun parentRec(
|
||||
nodeIndex: Int,
|
||||
nodeCount: Int,
|
||||
): Int {
|
||||
@@ -127,7 +133,7 @@ object BinaryTree {
|
||||
} else {
|
||||
nodeIndex - (1 shl k)
|
||||
}
|
||||
return if (p < nodeCount) p else parentInRange(p, nodeCount)
|
||||
return if (p < nodeCount) p else parentRec(p, nodeCount)
|
||||
}
|
||||
|
||||
/** Root node index for a tree with [leafCount] leaves */
|
||||
@@ -141,11 +147,20 @@ object BinaryTree {
|
||||
/**
|
||||
* Direct path from a leaf to the root (excluding the leaf itself).
|
||||
* These are the parent nodes along the path from leaf to root.
|
||||
*
|
||||
* [leafIndex] must be in `[0, leafCount)`. Calling with `leafIndex >=
|
||||
* leafCount` is an upstream bug — most often a stale local index that
|
||||
* was valid before the tree shrank under a Remove proposal — and used
|
||||
* to OOM here because `parent()` would loop on the out-of-range start.
|
||||
*/
|
||||
fun directPath(
|
||||
leafIndex: Int,
|
||||
leafCount: Int,
|
||||
): List<Int> {
|
||||
require(leafCount > 0) { "leafCount must be positive, got $leafCount" }
|
||||
require(leafIndex in 0 until leafCount) {
|
||||
"leafIndex $leafIndex out of range [0, $leafCount)"
|
||||
}
|
||||
val nodeIdx = leafToNode(leafIndex)
|
||||
val n = nodeCount(leafCount)
|
||||
val rootIdx = root(leafCount)
|
||||
@@ -162,11 +177,18 @@ object BinaryTree {
|
||||
/**
|
||||
* Copath of a leaf: the siblings of each node on the direct path.
|
||||
* The copath determines which nodes need to receive encrypted path secrets.
|
||||
*
|
||||
* Same range contract as [directPath]: `leafIndex` must be in
|
||||
* `[0, leafCount)`.
|
||||
*/
|
||||
fun copath(
|
||||
leafIndex: Int,
|
||||
leafCount: Int,
|
||||
): List<Int> {
|
||||
require(leafCount > 0) { "leafCount must be positive, got $leafCount" }
|
||||
require(leafIndex in 0 until leafCount) {
|
||||
"leafIndex $leafIndex out of range [0, $leafCount)"
|
||||
}
|
||||
val nodeIdx = leafToNode(leafIndex)
|
||||
val n = nodeCount(leafCount)
|
||||
val rootIdx = root(leafCount)
|
||||
|
||||
@@ -23,6 +23,7 @@ package com.vitorpamplona.quartz.marmot.mls
|
||||
import com.vitorpamplona.quartz.marmot.mls.tree.BinaryTree
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertFails
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
@@ -160,4 +161,121 @@ class BinaryTreeTest {
|
||||
// Subtree of leaf 0 is just [0]
|
||||
assertEquals(listOf(0), BinaryTree.subtreeLeaves(0, 4))
|
||||
}
|
||||
|
||||
// ----- Non-power-of-2 tree shapes ----------------------------------------
|
||||
//
|
||||
// RFC 9420 Appendix C only worked example is the 4-leaf tree, but every
|
||||
// group with ≠ a power-of-2 members exercises the parentInRange branch in
|
||||
// BinaryTree.parent. Those branches were entirely uncovered before, and a
|
||||
// bug there (infinite recursion / infinite loop in directPath) is what
|
||||
// OOM'd amy when wn removed her in marmot-interop test 14.
|
||||
|
||||
@Test
|
||||
fun testRoot_nonPowerOfTwo() {
|
||||
// root = (1 << ceil(log2(n))) - 1
|
||||
assertEquals(3, BinaryTree.root(3)) // ceil(log2(3))=2, 2^2-1 = 3
|
||||
assertEquals(7, BinaryTree.root(5)) // ceil(log2(5))=3, 2^3-1 = 7
|
||||
assertEquals(7, BinaryTree.root(6))
|
||||
assertEquals(7, BinaryTree.root(7))
|
||||
assertEquals(15, BinaryTree.root(9))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testDirectPath3Leaves() {
|
||||
// 3 leaves, n=5, root=3:
|
||||
// 3
|
||||
// / \
|
||||
// 1 4
|
||||
// / \ \
|
||||
// 0 2 (leaf 2 sits at node 4; node 5 doesn't exist)
|
||||
// Node 4 is a leaf node here because the tree is not full — its
|
||||
// "parent slot" 5 would be ≥ nodeCount and is collapsed away by
|
||||
// parentInRange.
|
||||
assertEquals(listOf(1, 3), BinaryTree.directPath(0, 3))
|
||||
assertEquals(listOf(1, 3), BinaryTree.directPath(1, 3))
|
||||
assertEquals(listOf(3), BinaryTree.directPath(2, 3))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testDirectPath5Leaves() {
|
||||
// 5 leaves, n=9, root=7:
|
||||
// 7
|
||||
// / \
|
||||
// 3 8 ← leaf 4 collapsed up
|
||||
// / \
|
||||
// 1 5
|
||||
// / \ / \
|
||||
// 0 2 4 6
|
||||
assertEquals(listOf(1, 3, 7), BinaryTree.directPath(0, 5))
|
||||
assertEquals(listOf(1, 3, 7), BinaryTree.directPath(1, 5))
|
||||
assertEquals(listOf(5, 3, 7), BinaryTree.directPath(2, 5))
|
||||
assertEquals(listOf(5, 3, 7), BinaryTree.directPath(3, 5))
|
||||
assertEquals(listOf(7), BinaryTree.directPath(4, 5))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testDirectPathTerminatesForAllLeafCountsUpTo32() {
|
||||
// Property: every valid leaf in any tree size 1..32 has a directPath
|
||||
// that ends at root(leafCount) and has length == log2-ish. We don't
|
||||
// assert exact lengths — we just want to catch any future regression
|
||||
// where some (leafCount, leafIndex) triggers the parent() infinite
|
||||
// loop. Each call is wrapped in a generous timeout via the kotlin
|
||||
// test runner (default test timeout is fine: a non-terminating call
|
||||
// would OOM long before any test deadline).
|
||||
for (leafCount in 1..32) {
|
||||
val rootIdx = BinaryTree.root(leafCount)
|
||||
for (leafIndex in 0 until leafCount) {
|
||||
val dp = BinaryTree.directPath(leafIndex, leafCount)
|
||||
if (leafCount == 1) {
|
||||
assertTrue(dp.isEmpty(), "leafCount=1 has no path")
|
||||
} else {
|
||||
assertEquals(rootIdx, dp.last(), "directPath($leafIndex, $leafCount) should end at root")
|
||||
// copath must align with the directPath one-for-one
|
||||
assertEquals(dp.size, BinaryTree.copath(leafIndex, leafCount).size)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ----- Out-of-range inputs -----------------------------------------------
|
||||
//
|
||||
// Marmot-interop test 14 fails with a Java OOM because amy calls
|
||||
// `BinaryTree.directPath(myLeafIndex, tree.leafCount)` with `myLeafIndex
|
||||
// == tree.leafCount` (her own leaf was just removed and the tree shrank
|
||||
// past it). `parent()` then walks node indices ≥ nodeCount forever and
|
||||
// the result list explodes.
|
||||
//
|
||||
// These tests pin down that boundary so the failure becomes a clean
|
||||
// IllegalArgumentException instead of an OOM, AND so the MLS code path
|
||||
// that hits it has a deterministic regression.
|
||||
|
||||
@Test
|
||||
fun testDirectPathRejectsOutOfRangeLeafIndex() {
|
||||
// leafIndex == leafCount — the exact shape the post-Remove path hits.
|
||||
assertFails { BinaryTree.directPath(2, 2) }
|
||||
// leafIndex > leafCount.
|
||||
assertFails { BinaryTree.directPath(5, 3) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testDirectPathRejectsNegativeLeafIndex() {
|
||||
assertFails { BinaryTree.directPath(-1, 4) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testCopathRejectsOutOfRangeLeafIndex() {
|
||||
assertFails { BinaryTree.copath(2, 2) }
|
||||
assertFails { BinaryTree.copath(-1, 4) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testParentRejectsRootOrAbove() {
|
||||
// The root has no parent. Asking anyway is the symptom of a tree
|
||||
// accounting bug — surface it loudly instead of looping.
|
||||
val n = BinaryTree.nodeCount(4)
|
||||
assertFails { BinaryTree.parent(BinaryTree.root(4), n) }
|
||||
// Index strictly above nodeCount also has no defined parent.
|
||||
assertFails { BinaryTree.parent(n, n) }
|
||||
assertFails { BinaryTree.parent(n + 5, n) }
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user