oor+vtxo: activate VTXO actors for OOR receives, add ChainDepth plumbing - #173
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the handling of Virtual Transaction Outputs (VTXOs) by resolving a critical gap in the Out-of-Round (OOR) receive process. Previously, OOR-received VTXOs were persisted but not fully integrated into the system's active management, leading to bypassed lifecycle events. This change ensures that all VTXOs, regardless of their origin, are properly monitored and managed. Additionally, it introduces a robust mechanism for tracking ChainDepth, providing a more granular and accurate understanding of a VTXO's lineage and its proximity to an on-chain commitment, which is foundational for future policy implementations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This PR is well-structured and effectively addresses the OOR receive lifecycle gap. The introduction of ChainDepth is a good step for future policy decisions, and the code is clear. The addition of extensive tests, including a system test, is commendable. I found one potential data race issue in the new SetVTXOManager method, which I've commented on. Otherwise, the changes look solid.
| func (a *RoundClientActor) SetVTXOManager(ref actor.TellOnlyRef[VTXOManagerMsg]) { | ||
| a.cfg.VTXOManager = ref | ||
| } |
There was a problem hiding this comment.
This function modifies a.cfg.VTXOManager on a potentially running actor, but there's no synchronization to prevent data races. The RoundClientActor is started within initRoundActor before this method is called in darepod/server.go. If the actor's message loop reads a.cfg.VTXOManager at the same time this function writes to it, a data race will occur.
To fix this, you should use a mutex to protect access to a.cfg.VTXOManager. Assuming RoundClientActor has a mutex (e.g., a.mu), you should lock it in this function and also around any reads of this field. For example:
func (a *RoundClientActor) SetVTXOManager(ref actor.TellOnlyRef[VTXOManagerMsg]) {
a.mu.Lock()
defer a.mu.Unlock()
a.cfg.VTXOManager = ref
}Remember to also protect the reads in the actor's message loop.
| func NewOORClientActor(cfg ClientActorCfg) *OORClientActor { | ||
| wireVTXOManagerNotifier(&cfg) | ||
|
|
35f546f to
f91f08e
Compare
|
@claude review this |
|
Claude finished @ellemouton's task in 3m 35s —— View job Code Review
Overall this is a well-structured PR. The OOR → VTXO actor lifecycle fix is clean, the Confirmed Issue: Data Race in
|
912e081 to
6d27ce9
Compare
Teach the OOR actor to forward durably materialized incoming VTXOs into the VTXO manager. This keeps OOR on the existing persist-then-notify architecture while avoiding a second store write. The manager gains a dedicated message for already-persisted descriptors, and focused unit tests cover the new notifier wiring.
Start the VTXO manager actor during daemon startup and connect both round completion notifications and OOR incoming materialization to it. The round actor now supports late manager wiring so startup can bring the manager online after the round actor is registered. Tests cover the late binding path so the runtime wiring stays explicit and safe.
Add a systest that drives an incoming OOR receive through materialization and asserts the VTXO manager activates the new VTXO actor. This covers the runtime wiring path that unit tests do not fully exercise, including persistence, manager notification, and actor registration in the live system graph.
Introduce ChainDepth as a first-class field on vtxo.Descriptor and oor.IncomingVTXOMetadata, distinct from the existing TreeDepth. TreeDepth tracks position within the VTXT (virtual transaction tree), while ChainDepth counts OOR checkpoint hops from the last on-chain commitment. Round-created VTXOs explicitly set ChainDepth to 0. This is Part 2 of issue #124: the field is carried through the domain types so that persistence and RPC layers can expose it in follow-up commits.
Add migration 000005 with a chain_depth column (INTEGER NOT NULL DEFAULT 0) to the vtxos table. Existing rows read as 0, which is the correct value for round-created VTXOs and the safe default for historical OOR VTXOs with unknown lineage. Thread the field through both store implementations: - VTXOPersistenceStore (OOR path): maps Descriptor.ChainDepth on insert and read. - RoundPersistenceStore (round path): explicitly sets ChainDepth to 0. The InsertVTXO ON CONFLICT clause preserves existing chain_depth when the incoming value is 0, matching the pattern used for tree_depth and batch_expiry.
Add chain_depth to the daemon VTXO message (field 10) and the indexer VTXOInfo message (field 15). The daemon RPC server maps Descriptor.ChainDepth into the new proto field so ListVTXOs callers can inspect the OOR hop count. This enables future tooling and metadata resolvers to consume chain depth without another wire-format break.
Set ChainDepth to 2 in the systest incoming metadata fixture and assert the persisted descriptor retains the value. This proves the field flows through the OOR receive path and database round-trip in the full actor-system integration test.
3c1896f to
a4489c8
Compare
…nagerMsg Replace the panic() in mapRoundVTXOManagerMsg with compile-time type assertions that guarantee all round.VTXOManagerMsg implementors also satisfy vtxo.ManagerMsg. This eliminates the runtime panic risk while keeping the type assertion infallible.
Add an early validation check rejecting negative ChainDepth values in the incoming VTXO descriptor builder. ChainDepth represents OOR checkpoint hop count and is semantically non-negative.
Update CLAUDE.md and AGENTS.md for vtxo, oor, darepod, and db packages to reflect new VTXOsMaterializedNotification message flow, ChainDepth field on Descriptor, actor startup ordering invariants, and migration 000005 chain_depth column.
a4489c8 to
ef534be
Compare
oor: fix flaky TestOORServerRejectsTamperedFinalizeSignature
tap-sdk PR #173 adds the CallerSigned signing-plan variant needed to classify lnd funding inputs on caller-funded anchors.
tap-sdk PR #173 (caller-signed anchor inputs) merged.
tap-sdk PR #173 adds the CallerSigned signing-plan variant needed to classify lnd funding inputs on caller-funded anchors.
tap-sdk PR #173 (caller-signed anchor inputs) merged.
tap-sdk PR #173 adds the CallerSigned signing-plan variant needed to classify lnd funding inputs on caller-funded anchors.
tap-sdk PR #173 (caller-signed anchor inputs) merged.
tap-sdk PR #173 adds the CallerSigned signing-plan variant needed to classify lnd funding inputs on caller-funded anchors.
tap-sdk PR #173 (caller-signed anchor inputs) merged.
Partial progress on #124 (Parts 1 and 2)
Summary
This PR addresses the OOR receive lifecycle gap from #124 and lays the
data-model groundwork for chain-depth-aware exit policy.
It does two things:
the DB". After an incoming OOR VTXO is materialized, the OOR flow now
notifies
VTXOManager, which spawns a live VTXO actor for thatdescriptor.
ChainDepthtracking to VTXOs.ChainDepthisdistinct from
TreeDepth: it represents the number of OOR checkpointhops between a VTXO and the most recent on-chain commitment.
What This PR Covers
Part 1: OOR receive now activates VTXO actors
Before this change, incoming OOR VTXOs were written to the
vtxostable butnever forwarded to
VTXOManager, so no VTXO actor was spawned.This PR changes that by:
VTXOManagerthe same manager
Result:
are no longer bypassed for received OOR VTXOs
Part 2: persisted
ChainDepthplumbingThis PR also introduces
ChainDepthon VTXOs and threads it through:vtxo.DescriptorRound-created VTXOs explicitly default to
ChainDepth = 0.What This PR Does Not Cover
This PR does not complete the full production metadata wiring needed to
source authoritative non-zero incoming OOR
ChainDepth.That gap already existed before this branch:
darepod/server.gostill does not wire the fullincoming metadata resolver path
should eventually provide incoming OOR lineage metadata, including chain
depth
So while this PR makes the model, storage, and APIs ready for
chain-depth-aware behavior, it does not claim to fully complete Part 2
end-to-end in production.
This PR also does not implement Part 3 from #124:
future work
Why This Split Still Makes Sense
Even without the final production metadata resolver wiring, this PR is still
valuable on its own because it:
chain-depth-aware policy lands
ChainDepthwherever authoritativemetadata is supplied, without inventing values from
TreeDepthTesting
Covered by:
ChainDepthpropagation and round-trip persistenceinjected
ChainDepthsurvives materialization/persistenceCommands run:
make unit pkg=oor,make unit pkg=vtxo,make unit pkg=db,make unit pkg=darepodgo test -tags systest ./systest -run TestOORIncomingMaterializationSpawnsVTXOActor -count=1make lintFollow-up Work
Remaining follow-up after this PR:
darepod/server.goChainDepthfrom the receive metadatapath
ChainDepthinstead of overloadingTreeDepth