unroll: keep restore failures retryable, restore via Ensure (#381) - #443
Closed
ellemouton wants to merge 1 commit into
Closed
unroll: keep restore failures retryable, restore via Ensure (#381)#443ellemouton wants to merge 1 commit into
ellemouton wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the unroll registry by ensuring that transient failures during actor restoration do not permanently strand recovery jobs. It introduces inline restoration in handleEnsure for dormant non-terminal records and modifies restoreNonTerminal to leave records in a retryable state rather than marking them as failed. Feedback highlights a critical data race in the restoreNonTerminal method and suggests ensuring the pending map is correctly updated during inline restores to maintain state consistency.
| ) | ||
| } | ||
|
|
||
| r.active[req.Outpoint] = child |
There was a problem hiding this comment.
When restoring a non-terminal record inline, it should be added to the r.pending map. This ensures that the Trigger information is preserved when the child eventually terminates (as handleTerminated relies on r.pending to carry over historical fields not present in the termination message) and avoids redundant store lookups in handleGetStatus.
Suggested change
| r.active[req.Outpoint] = child | |
| r.active[req.Outpoint] = child | |
| r.pending[req.Outpoint] = cloneRegistryRecord(*existing) |
Member
Author
There was a problem hiding this comment.
Done — when handleEnsure restores an existing non-terminal record inline, r.pending is now mirrored alongside r.active using cloneRegistryRecord(*existing). I also applied the same mirror to handleRestoreNonTerminal so freshly-restored records benefit on boot. New SHA: f861c42.
| // on every subsequent boot and handleEnsure would short-circuit on the | ||
| // terminal record. For a VTXO that is in unilateral_exit and near | ||
| // expiry, that translates into locked or lost funds — see issue #381. | ||
| func (r *registryBehavior) restoreNonTerminal(ctx context.Context) error { |
There was a problem hiding this comment.
The restoreNonTerminal method accesses and modifies r.active and r.pending directly on the behavior, which is also accessed by the actor's goroutine in Receive. This creates a data race. Since handleEnsure now also performs restoration, the risk of concurrent access to these maps is increased. This method should be implemented as a message sent to the actor to ensure serialized access within the actor's goroutine.
Member
Author
There was a problem hiding this comment.
Good catch — confirmed the race: NewUnrollRegistryActor calls Start() before the boot path issues RestoreNonTerminal, so handleEnsure / handleGetStatus messages from the chain resolver or RPC layer can already be in flight on the actor goroutine while restoreNonTerminal mutated r.active and r.pending directly from the caller goroutine.
Fix: introduced an internal restoreNonTerminalMsg (sealed via registryMsgSealed) and a restoreNonTerminalResp carrying any error string. The public UnrollRegistryActor.RestoreNonTerminal now Asks the registry actor with this message instead of touching the behavior directly, so the actual restore work runs inside Receive on the actor goroutine and is serialized against every other Receive turn. The body was renamed restoreNonTerminal -> handleRestoreNonTerminal to match the existing handler naming convention.
Added TestRegistryRestoreNonTerminalDispatchedThroughActor which exercises a concurrent RestoreNonTerminal + GetStatus from two goroutines; -race is the actual guard. Full unroll suite passes under go test -race -count=1. New SHA: f861c42.
The registry used to mark each non-terminal job PhaseFailed when
RestoreNonTerminal could not spawn or resume its child actor, and
handleEnsure short-circuited on any existing record. Resume can
fail for transient external dependencies — chain backend
SubscribeBlocks / RegisterSpend through ChainSource, or a flaky
DB. Once the record was marked terminal, ListNonTerminalRecords
skipped it on every subsequent boot, turning a transient outage
during daemon start into a permanent shutdown of the recovery
path for that VTXO. For a VTXO that is in unilateral_exit and
near expiry, that translates into locked or lost funds.
This commit makes restore failures non-sticky on two paths:
1. restoreNonTerminal no longer calls MarkTerminal on spawn /
resume failure. The durable record stays non-terminal, is
logged, and gets retried on the next daemon restart and on
any in-boot EnsureUnrollRequest for the same outpoint.
2. handleEnsure detects "non-terminal store record, no active
child" — the signature of a previous restore that did not
wire up — and attempts an inline restore via a shared
tryRestoreOne helper. Success returns Created=false with
the historical ActorID; failure surfaces the error so the
caller can retry.
Three new tests lock the behavior in:
- TestRegistryRestoreFailureLeavesRecordRetryable:
first RestoreNonTerminal fails on resume, record stays
non-terminal, a second RestoreNonTerminal with a healthy
spawn succeeds.
- TestRegistryEnsureRestoresFailedNonTerminalRecord:
a fresh EnsureUnroll on an unrestored non-terminal record
triggers inline restore.
- TestRegistryEnsureRetriesAfterInlineRestoreFailure:
a failed inline restore inside Ensure does not strand the
job; the next Ensure retries.
Closes #381.
ellemouton
force-pushed
the
fix/381-restore-failure-disables-unroll
branch
from
May 15, 2026 03:06
e1da389 to
f861c42
Compare
ellemouton
marked this pull request as ready for review
May 15, 2026 03:18
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Member
Author
|
Superseded by consolidated PR #459. Closing to reduce CI load. |
ellemouton
added a commit
that referenced
this pull request
May 15, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 19, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 19, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 19, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 19, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 19, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 19, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 19, 2026
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
ellemouton
added a commit
that referenced
this pull request
May 22, 2026
PR #443 closed the post-registration impersonation gap by recording the SHA-256 fingerprint of the TLS leaf certificate observed when a mailbox identity first passed Schnorr verification, and gating Pull and AckUpTo on that fingerprint. It left a registration-time hijack open: the Schnorr signature on the first Send covers the mailbox identity and envelope contents but not the TLS leaf public key. An attacker who captured or MITM'd a victim's signed first Send could replay the same envelope across a different TLS session backed by a leaf the attacker controls; the server would Schnorr-verify the envelope, bind the attacker's fingerprint to the victim's mailbox ID, and the post-#443 fingerprint defense would then happily authorize the attacker on Pull / AckUpTo. TLS certs in this deployment use P-256 while mailbox keys use secp256k1, so the two key materials cannot be identical — the link between them has to be a secp256k1 signature over the TLS pubkey, not key reuse. HandleUnknownClient now extracts the SubjectPublicKeyInfo of the TLS leaf the gRPC transport actually saw, looks for an x-mailbox-tls-bind-sig header on the envelope, and verifies that header as a Schnorr signature, by the Schnorr-verified mailbox key, over a BIP-340 tagged digest of (senderPubKey || leafSPKI). SPKI rather than the raw key bytes is used so the binding commits to the curve and algorithm identifier in addition to the key material. Only after this verification does the existing mailboxTLSBindings.Bind run. The SPKI fed into the digest comes from the TLS PeerCertificates exposed by gRPC, never from any envelope field, so a malicious client cannot supply both the SPKI it claims to have signed and a matching signature. To avoid locking out pre-#448 clients during the upgrade window, verification runs in soft mode by default: a missing header logs a warning and Bind proceeds, so existing fleets can roll forward without coordinated downtime. A new MailboxConfig.RequireTLSBindingSig flag flips the requirement to hard once the fleet has upgraded; a malformed or wrong-key binding sig is always rejected even in soft mode because a legacy client would omit the header outright, never emit garbage. A future release will flip the default and a subsequent release will remove the soft path entirely. Send is the only first-contact entry point in the bridge — no sibling registration RPC needs the same treatment. Closes #448.
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.
Closes #381.
Summary
unroll/registry.RestoreNonTerminalmarked any recordPhaseFailedwheneverspawnorresumefailed.resumecan fail on transientChainSource/ DB issues at daemon boot (e.g.SubscribeBlocks,RegisterSpend). BecauseListNonTerminalRecordsskips terminal records andhandleEnsureshort-circuited on any existing record, a single transient failure permanently disabled unroll recovery for that VTXO — near-expiry VTXOs would risk locked/lost funds.Fix
tryRestoreOnehelper forspawn+resume.restoreNonTerminalno longer marks records terminal on failure — it logs and continues, leaving each record retryable on the next boot or nextEnsureUnroll.handleEnsure: when an existing store record is non-terminal but no active child exists, it callstryRestoreOneto attempt an inline restore. Terminal records still short-circuit so the recorded sweep txid / fail reason (dedup invariant) is preserved.The critical-expiry chain-resolver path (
darepod/server.go:4272-4280) already invokesEnsureUnroll, so a near-expiry VTXO automatically retries restoration after a transient backend outage.Test plan
TestRegistryRestoreFailureLeavesRecordRetryableTestRegistryEnsureRestoresFailedNonTerminalRecordTestRegistryEnsureRetriesAfterInlineRestoreFailuremake lint-native— 0 issuesgo test ./unroll/ -count=1— pass