feat: dutydb

[mskrzypkows]

No description provided.

[mskrzypkows]

@claude review

[claude]

Claude finished @mskrzypkows's task in 4m 38s —— View job

---

Review — Pluto DutyDB (PR #415)

Overall: a solid, readable port of Charon's core/dutydb/memory.go. The high‑level structure, error variants and field layout match the Go source well, and the Notify+RwLock swap for Go's Mutex+query‑queue model is a sensible Rust idiom that I'd lean into. A few correctness / parity points are worth addressing before merging, and test coverage has some real gaps versus the Go suite. Comments grouped by severity.

---

1. Correctness / parity with Charon

(a) deadliner.add is awaited while holding the state write lock
memory.rs:219-224

pub async fn store(&self, duty: Duty, unsigned_set: UnsignedDataSet) -> Result<()> {
    let mut state = self.state.write().await;
    if !self.deadliner.add(duty.clone()).await {
        return Err(Error::ExpiredDuty);
    }

Go's MemDB.Store calls db.deadliner.Add(duty) synchronously under a sync.Mutex, so the lock is held for a non‑blocking instant. In Rust, Deadliner::add is async fn and the concrete Deadliner (crates/core/src/deadline.rs:258) does its own locking and channel send (tx.send(...).await). Holding the dutydb write lock across that .await blocks all readers/writers of the DutyDB on the deadliner's progress, which is a deadlock/contention foot‑gun (and worse than Go).

Recommend either:

• call deadliner.add(duty.clone()).await before acquiring the write lock, then acquire and proceed; or
• make Deadliner::add non‑async (Go's signature is sync).

(b) Dead clash check in store_agg_attestation
memory.rs:510-527

The key includes the tree‑hash root of the att data, and the "clash" check compares existing_data.tree_hash_root().0 != root. Because we just looked the entry up by that root, the two sides are always equal, so ClashingDataRoot is unreachable. This faithfully mirrors a bug already in memory.go:458-460, but worth flagging: this branch never fires and should either (i) be removed, or (ii) the key changed (e.g. slot only) so a real clash is detectable. Either is a behavior change vs. Go and warrants a separate decision.

(c) Aggregated attestation duty is always re‑inserted even when already present
memory.rs:521-524

} else {
    self.agg_keys_by_slot.entry(slot).or_default().push(key);
}
self.agg_duties.insert(key, agg.clone());

Matches Go (memory.go:462), but the .clone() on every duplicate store is wasted work — consider moving the insert into the else branch if Go's overwrite is not actually load‑bearing.

(d) await_* have no per‑call cancellation
memory.rs:289-307

Go's AwaitProposal(ctx, slot) returns ctx.Err() if the caller cancels. The Rust versions only honor the DB‑global CancellationToken. Callers can tokio::time::timeout(...) or abort the task to drop the future, but the API surface is narrower than Go's. I'd add a CancellationToken (or a tokio::time::Duration timeout) parameter to bring parity — otherwise it's easy for higher layers to leak tasks waiting forever.

(e) Expired duties are not drained when store returns early with an error
memory.rs:226-283

Same as Go (early return err in the switch skips the drain loop), so this is parity, but worth noting because a transient clash error will defer expiry processing until the next successful store. Not blocking.

(f) BuilderProposer and other unsupported duty types are still registered with the deadliner

deadliner.add(duty) runs before the switch, so a duty whose type later returns DeprecatedDutyBuilderProposer / UnsupportedDutyType ends up tracked by the deadliner and silently expires (delete_duty rejects unknown types again). Again, this is parity with Go, but it's a minor wart that has been carried over.

---

2. Concurrency model

(a) await_data thundering herd. memory.rs:341-367

notify_waiters() wakes every waiter on the channel, regardless of whether their specific key was the one stored. Each woken task then takes the read lock and does a hashmap lookup. For a cluster with many concurrent await_attestation calls in flight (one per committee), every Store will wake them all. Go's *Queries slice resolves only the entry whose key matches, with no spurious wakeups. Probably fine in practice given the scale, but worth being aware of — if it shows up as latency, a per‑key channel or a tokio::sync::watch keyed map is a clean fix.

(b) Notify::notified() registration pattern is correct. The pin! + enable() + lookup + select! ordering avoids the classic missed‑wakeup race. Nice.

(c) State.deadliner_rx: Option<...> is held inside State so that try_recv happens under the write lock. The Option/take() workaround stems from Deadliner::c() returning the receiver only once. It would read more naturally as a dedicated Mutex<Option<Receiver<Duty>>> field on MemDB (or simply on State) — keeping it in State works but the optional is awkward.

(d) MemDB::shutdown(&self) is idempotent via CancellationToken, which is an improvement over Go's "may only be called once" requirement. Worth mentioning in the doc comment.

---

3. API / style

• Naming collision. pluto_core::dutydb::MemDB and pluto_core::parsigdb::memory::MemDB are both reachable. Not a compile error, but consider DutyMemDB / ParSigMemDB to make imports unambiguous.
• State methods are sync but mutate &mut self — they implicitly assume the caller holds the write lock. Go names them *Unsafe for exactly this reason; a one‑line // caller holds the write lock doc on each method (or splitting State into its own private module) would help future readers.
• Doc comment on the commIdx=0 compatibility block memory.rs:466-468 is shorter than Go's. The Go version explains why both index variants are stored (post‑Electra default + buggy VCs); the Rust comment is a one‑liner. Worth porting the full rationale — this code looks pointless without the context.
• UnsignedDutyData::Proposal(Box<VersionedProposal>) — good call to box the variant; the other variants are much smaller.
• store_agg_attestation / store_sync_contribution differ in shape: agg writes to agg_keys_by_slot inside the else and to agg_duties outside; contrib writes both inside the else. Pick one style — the contrib version is the safer one (no overwrite when content matches by key).

---

4. Tests

The new suite covers shutdown, attester happy path, unsupported duty types, proposer, sync contribution, clashes for sync/blocks/proposer, and expiry. Compared to charon/core/dutydb/memory_test.go, the following are missing — I'd add at least the first three before merge:

• Aggregator end‑to‑end test (Go's TestMemDBAggregator) — await_agg_attestation and store_agg_attestation are completely uncovered.
• ClashingPublicKey — two different pubkeys for the same (slot, commIdx, valIdx).
• ClashingAttestationData (and the commIdx=0 source / target variants) — different phase0::AttestationData for the same (slot, commIdx).
• Await blocks then resolves: the mem_db_proposer test does this for proposals; consider replicating for attester / aggregator / sync.
• Shutdown wakes an already‑waiting await_* — current tests call shutdown before the await; the more interesting case is a waiter parked in select!.
• The clashing_sync_contributions test mutates signature to force a different tree‑hash root — it would be more explicit to construct two semantically distinct contributions, since "signature affects root" is only true today and depends on the SSZ schema.

The TestDeadliner helper uses std::sync::Mutex inside async fn add (memory.rs:651-655). The critical section is trivially short so this is fine, but parking_lot::Mutex or restructuring would make it impossible to misuse later.

---

5. Nits

• memory.rs:155-170: missing doc comment on State.
• memory.rs:172-184: the MemDB doc could explicitly say "functionally equivalent to charon/core/dutydb.MemDB" — there's a comment in the file header but not on the type.
• memory.rs:104-105: consider re‑exporting Result privately; many downstream callers will conflict with std::result::Result if they use pluto_core::dutydb::*.
• memory.rs:530-552: the temporary binding let inner = &contrib.0; could go away if SyncContribution had a Deref or a data() accessor — minor.

---

Summary

• One real concern: deadliner.add is awaited under the dutydb write lock (1a) — please move it outside or make it sync.
• Behavior matches Go closely; the small parity surprises (unreachable agg clash check, missing per‑call cancellation, deadliner add on rejected duties) are inherited from Charon — flag them in the PR description so we don't pretend they were intentional Rust choices.
• Add aggregator + clash tests to bring the suite in line with Go.

Nothing here blocks merge other than (1a) and the missing tests.

• Read PR diff and changed files
• Cross-reference Charon Go source (dutydb)
• Functional correctness review (parity with Go)
• Concurrency/locking review
• Rust style / API surface review
• Test quality review
• Post final review
  • Branch: dutydb