multi: unilateral-exit preparatory fixes and package-submission infra (1/5) - #260
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 introduces support for atomic parent+child transaction package submission (V3 package relay) across various chain backends, including LND and Esplora. It also enhances logging for durable actors and database transactions, adds timeouts to confirmation registrations to prevent hangs, and ensures VTXO metadata is populated early to avoid data races. Feedback focuses on improving the robustness of the new SubmitPackage methods by ensuring they consistently accept and respect context.Context for network calls, avoiding the use of http.DefaultClient in the test harness, and refining error handling when joining transaction results to avoid malformed error strings.
| SubmitPackage(parents []*wire.MsgTx, child *wire.MsgTx, | ||
| maxFeeRate *float64) (*btcjson.SubmitPackageResult, error) |
There was a problem hiding this comment.
The SubmitPackage method in the PackageSubmitter interface should accept a context.Context as its first parameter. This allows implementations to respect the caller's cancellation signal or timeout, which is especially important for network-bound RPC calls. The calling method LNDBackend.SubmitPackage already receives a context that should be passed down to this interface.
| SubmitPackage(parents []*wire.MsgTx, child *wire.MsgTx, | |
| maxFeeRate *float64) (*btcjson.SubmitPackageResult, error) | |
| SubmitPackage(ctx context.Context, parents []*wire.MsgTx, child *wire.MsgTx, | |
| maxFeeRate *float64) (*btcjson.SubmitPackageResult, error) |
| result, err := b.packageSubmitter.SubmitPackage( | ||
| parents, child, nil, | ||
| ) |
There was a problem hiding this comment.
Pass the ctx to the SubmitPackage call once the interface is updated to support it.
| result, err := b.packageSubmitter.SubmitPackage( | |
| parents, child, nil, | |
| ) | |
| result, err := b.packageSubmitter.SubmitPackage( | |
| ctx, parents, child, nil, | |
| ) |
| return fmt.Errorf("package not accepted: %s: %w", | ||
| result.PackageMsg, errors.Join(txErrors...)) |
There was a problem hiding this comment.
If result.PackageMsg indicates failure but txErrors is empty, errors.Join(txErrors...) returns nil. Formatting an error with %w and a nil value can lead to confusing output (e.g., package not accepted: ...: %!w(<nil>)). Consider checking if txErrors is non-empty before joining, or use a pattern similar to the one in lwwallet/esplora.go where the presence of errors is explicitly checked before formatting.
| func (s *BitcoindPackageSubmitter) SubmitPackage( | ||
| parents []*wire.MsgTx, child *wire.MsgTx, | ||
| _ *float64) (*btcjson.SubmitPackageResult, error) { |
There was a problem hiding this comment.
The SubmitPackage implementation should be updated to accept and use a context.Context for the HTTP request (using http.NewRequestWithContext). Additionally, it is recommended to avoid using http.DefaultClient as it lacks a timeout and uses shared state; instead, use a locally configured http.Client. Finally, the implementation should check resp.StatusCode (e.g., ensuring it is http.StatusOK) before reading the response body to handle HTTP-level errors like authentication failures gracefully.
| } | ||
| txHexes = append(txHexes, hex.EncodeToString(childBuf.Bytes())) | ||
|
|
||
| if err := b.esplora.SubmitPackage(txHexes); err != nil { |
There was a problem hiding this comment.
Pass the ctx to the SubmitPackage call to ensure the network request can be cancelled if the caller's context is done.
| if err := b.esplora.SubmitPackage(txHexes); err != nil { | |
| if err := b.esplora.SubmitPackage(ctx, txHexes); err != nil { |
| // SubmitPackage posts a package of raw transaction hex strings to the | ||
| // Esplora /txs/package endpoint for atomic package relay. Transactions must be | ||
| // ordered by dependency with parents first and child last. | ||
| func (c *EsploraClient) SubmitPackage(txHexes []string) error { |
There was a problem hiding this comment.
The SubmitPackage method should accept a context.Context and use it when making the POST request to the Esplora API (via http.NewRequestWithContext). This ensures that the network call respects the caller's timeout or cancellation signal.
| func (c *EsploraClient) SubmitPackage(txHexes []string) error { | |
| func (c *EsploraClient) SubmitPackage(ctx context.Context, txHexes []string) error { |
|
Claude finished @Roasbeef's task in 2m 9s —— View job PR Review
SummaryThe three commits are well-scoped and address distinct concerns. The VTXO metadata fix is correct and straightforward. The logging improvements are solid. The package submission infrastructure mostly looks good but has a handful of issues worth fixing — some of which Gemini already flagged. Commit 1 — VTXO metadata at confirmation (
|
84d72c1 to
2ce1d2d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84d72c1113
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // CommitmentTxID is the txid of the round's commitment | ||
| // transaction. Populated at confirmation time. | ||
| CommitmentTxID chainhash.Hash |
There was a problem hiding this comment.
This introduces CommitmentTxID/BatchExpiry/CreatedHeight on ClientVTXO, and SaveVTXOs now persists those values, but db.RoundPersistenceStore.dbVTXOToDomainVTXO still rebuilds ClientVTXO without assigning the new fields. As a result, callers using GetVTXO/ListVTXOs continue receiving zero metadata even after the write path was updated, which undermines the metadata-race fix for round-store readers.
Useful? React with 👍 / 👎.
| if result.PackageMsg != "success" { | ||
| return fmt.Errorf("package not accepted: %s: %w", | ||
| result.PackageMsg, errors.Join(txErrors...)) |
There was a problem hiding this comment.
If PackageMsg != "success" but no per-transaction errors are present, errors.Join(txErrors...) is nil and fmt.Errorf("...: %w", nil) emits %!w(<nil>) in the error string. That malformed error text can occur on package-level rejections without tx-level details and makes diagnostics/error matching brittle; branch to a non-%w error format when no tx errors were collected.
Useful? React with 👍 / 👎.
The round FSM saves VTXOs to the store before the VTXO manager sends VTXOCreatedNotification. Previously, CommitmentTxID, BatchExpiry, and CreatedHeight were left zero in the first save and filled in asynchronously by the manager's second upsert. This created a race: callers reading the VTXO between the two saves would see incomplete metadata. Fix: add CommitmentTxID, BatchExpiry, and CreatedHeight fields to ClientVTXO and populate them from the BoardingConfirmed event before SaveVTXOs. The first write is now complete, making the manager's upsert a harmless no-op.
Add a Log option to DurableActorConfig so actors can surface warning and error logs through the caller's logger instead of the context fallback. Log message type and delivery ID on Tell failures so silent nack loops become visible. Add WarnS calls to TransactionExecutor for begin, body, commit, and retry-exhaustion failures so DB transaction errors are observable without debug-level logging.
Add SubmitPackageRequest/Response to chainsource with backend forwarding. Add SubmitPackage to LNDBackend with optional PackageSubmitter interface (backed by bitcoind RPC). Add Esplora package relay support to lwwallet chain backend. Add PackageSubmitter field to LNDBackendFromLndClientConfig and daemon Config for wiring. Add 15-second timeout to LndClientChainNotifier's RegisterConfirmationsNtfn to prevent hangs under heavy block load. Add 10-second timeout to ConfActor's backend registration. Export GetLNDClientConn, BitcoindRPCUser, BitcoindRPCPass from the harness. Add BitcoindPackageSubmitter for itest package submission via direct bitcoind JSON-RPC. Export WalletKit() on BoardingBackend.
2ce1d2d to
8120a07
Compare
…-04-21-0 docs: nightly doc-gardening sweep 2026-04-21
2 participants
Summary
This is part 1 of 5 in a stacked split of #235. The original PR
bundled 15 commits across five distinct concerns into a single review.
This PR isolates the three preparatory commits so they can land
independently of the unroll subsystem itself.
Scope:
188bd32—multi: populate round metadata on ClientVTXO at confirmation. Fixes a VTXO-metadata race where callers readingbetween the round FSM's initial save and the VTXO manager's
follow-up upsert would see zero
CommitmentTxID,BatchExpiry, andCreatedHeight. Populates the fields fromBoardingConfirmedinthe first save; the manager's upsert becomes a no-op.
362588c—multi: improve durable actor and DB transaction failure logging. Adds aLogoption toDurableActorConfigso actors cansurface warn/error through the caller's logger. Logs message type
and delivery ID on Tell failures (makes silent nack loops visible).
Adds
WarnStoTransactionExecutorfor begin/body/commit/retryexhaustion.
c2e1d51—multi: add package submission and harness infrastructure. AddsSubmitPackageRequest/Responsetochainsourcewith backend forwarding. AddsSubmitPackagetoLNDBackend(via optionalPackageSubmitterbacked by bitcoindRPC). Adds Esplora package relay to
lwwallet. AddsPackageSubmittertoLNDBackendFromLndClientConfigand daemonconfig. 15s timeout on
RegisterConfirmationsNtfn, 10s onConfActorbackend registration. ExportsGetLNDClientConn/BitcoindRPCUser/BitcoindRPCPassand addsBitcoindPackageSubmitterfor itest package submission.Stack
unroll-01-prepunroll-02-planlib/recovery+unrollplanunroll-03-txconfirmtxconfirmactorunroll-04-coreunroll/unroll-05-wireSupersedes #235.
Authorship
All three commits are authored by @ellemouton; the cherry-picks preserve
the
Author:header.Test plan
go build ./cmd/...go vet ./...