mobile: prevent wallet stalls on external I/O #1131
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,77 @@ import ( | |
| "fmt" | ||
| "io" | ||
| "runtime/debug" | ||
| "time" | ||
|
|
||
| "github.com/lightninglabs/wavelength/sdk/wavewalletdk" | ||
| ) | ||
|
|
||
| const ( | ||
| // defaultReadTimeout bounds read-only wallet calls whose foreign host | ||
| // cannot supply context.Context through gomobile. Reads are safe to | ||
| // repeat after a timeout and must not inherit the daemon's entire | ||
| // lifetime. | ||
| defaultReadTimeout = 10 * time.Second | ||
|
|
||
| // defaultReceiveTimeout bounds invoice creation when a mobile host | ||
| // omits TimeoutSeconds. A timed-out request has an uncertain outcome: | ||
| // callers must reconcile Activity before deliberately creating another | ||
| // invoice. | ||
| defaultReceiveTimeout = 20 * time.Second | ||
|
|
||
| // maxReceiveTimeout prevents malformed host input from restoring the | ||
| // effectively unbounded behavior this mobile-only request field avoids. | ||
| maxReceiveTimeout = 5 * time.Minute | ||
| ) | ||
|
|
||
| // readContext derives the bounded context used by safe, repeatable mobile | ||
| // reads. The daemon-lifetime parent still cancels it immediately during Stop. | ||
| func readContext(parent context.Context) (context.Context, context.CancelFunc) { | ||
| return context.WithTimeout(parent, defaultReadTimeout) | ||
| } | ||
|
|
||
| // mobileReceiveRequest extends the SDK receive DTO with a mobile-only request | ||
| // deadline. The extra JSON field is backwards-compatible with older bindings, | ||
| // whose encoding/json decoder ignores it. | ||
| type mobileReceiveRequest struct { | ||
|
There was a problem hiding this comment. 🚫 Dismissed by @bhandras 🟡
Member
Author
There was a problem hiding this comment. Not worth changing. |
||
| AmountSat uint64 | ||
| Memo string | ||
| TimeoutSeconds int64 | ||
| } | ||
|
|
||
| // decodeReceiveRequest validates the mobile-only deadline and converts the | ||
| // wire request into the SDK DTO. Zero selects the bounded default so older | ||
| // hosts gain a deadline when they update only the native framework. | ||
| func decodeReceiveRequest(reqJSON []byte) (wavewalletdk.ReceiveRequest, | ||
| time.Duration, error) { | ||
|
|
||
| var mobileReq mobileReceiveRequest | ||
| if err := decode(reqJSON, &mobileReq); err != nil { | ||
| return wavewalletdk.ReceiveRequest{}, 0, err | ||
| } | ||
|
|
||
| timeout := defaultReceiveTimeout | ||
| if mobileReq.TimeoutSeconds < 0 { | ||
| return wavewalletdk.ReceiveRequest{}, 0, | ||
| fmt.Errorf("receive timeout seconds must not be " + | ||
| "negative") | ||
| } | ||
| maxTimeoutSeconds := int64(maxReceiveTimeout / time.Second) | ||
| if mobileReq.TimeoutSeconds > maxTimeoutSeconds { | ||
| return wavewalletdk.ReceiveRequest{}, 0, fmt.Errorf("receive "+ | ||
| "timeout seconds %d exceeds maximum %d", | ||
| mobileReq.TimeoutSeconds, maxTimeoutSeconds) | ||
| } | ||
| if mobileReq.TimeoutSeconds > 0 { | ||
| timeout = time.Duration(mobileReq.TimeoutSeconds) * time.Second | ||
| } | ||
|
|
||
| return wavewalletdk.ReceiveRequest{ | ||
| AmountSat: mobileReq.AmountSat, | ||
| Memo: mobileReq.Memo, | ||
| }, timeout, nil | ||
| } | ||
|
|
||
| // GetInfo returns the daemon readiness snapshot as JSON (wavewalletdk.Info). | ||
| func GetInfo() ([]byte, error) { | ||
| client, ctx, err := activeClient() | ||
|
There was a problem hiding this comment. 🟡
Member
Author
There was a problem hiding this comment. Fixed in |
||
|
|
@@ -73,10 +140,12 @@ func UnlockWallet(reqJSON []byte) ([]byte, error) { | |
|
|
||
| // Balance returns the wallet balance summary as JSON (wavewalletdk.Balance). | ||
| func Balance() ([]byte, error) { | ||
| client, parentCtx, err := activeClient() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ctx, cancel := readContext(parentCtx) | ||
| defer cancel() | ||
|
|
||
| bal, err := client.Balance(ctx) | ||
| if err != nil { | ||
|
|
@@ -107,19 +176,24 @@ func Deposit(reqJSON []byte) ([]byte, error) { | |
| return marshal(res) | ||
| } | ||
|
|
||
| // Receive opens a Lightning invoice receive. reqJSON accepts AmountSat, Memo, | ||
| // and an optional mobile-only TimeoutSeconds; the response is | ||
| // wavewalletdk.ReceiveResult. When the deadline expires, the outcome may be | ||
| // uncertain, so the host must reconcile Activity before retrying deliberately. | ||
| func Receive(reqJSON []byte) ([]byte, error) { | ||
| client, parentCtx, err := activeClient() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| req, timeout, err := decodeReceiveRequest(reqJSON) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(parentCtx, timeout) | ||
| defer cancel() | ||
|
|
||
| res, err := client.Receive(ctx, req) | ||
| if err != nil { | ||
|
There was a problem hiding this comment. 🟡 The deadline error is returned verbatim, so a host cannot separate "timed out, outcome uncertain, reconcile Activity first" from "rejected, no session created" without matching on an error string that crosses gomobile as free text; whether the SDK already tags deadline errors is not visible here, since
Member
Author
There was a problem hiding this comment. Fixed in |
||
| return nil, err | ||
|
|
@@ -175,10 +249,12 @@ func SendPrepared(reqJSON []byte) ([]byte, error) { | |
| // decodes to wavewalletdk.ListRequest; the response is the tagged-union | ||
| // wavewalletdk.ListResult. | ||
| func List(reqJSON []byte) ([]byte, error) { | ||
| client, parentCtx, err := activeClient() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ctx, cancel := readContext(parentCtx) | ||
| defer cancel() | ||
|
|
||
| var req wavewalletdk.ListRequest | ||
| if err := decode(reqJSON, &req); err != nil { | ||
|
|
@@ -305,10 +381,12 @@ func SweepWallet(reqJSON []byte) ([]byte, error) { | |
| // Status returns wallet readiness, balance, and pending counts as JSON | ||
| // (wavewalletdk.Status). | ||
| func Status() ([]byte, error) { | ||
| client, parentCtx, err := activeClient() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ctx, cancel := readContext(parentCtx) | ||
| defer cancel() | ||
|
|
||
| status, err := client.Status(ctx) | ||
| if err != nil { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,13 +5,19 @@ package swapwallet | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/lightninglabs/wavelength/rpc/swapclientrpc" | ||
| "github.com/lightninglabs/wavelength/rpc/wavewalletrpc" | ||
| "github.com/lightninglabs/wavelength/waverpc" | ||
| "google.golang.org/protobuf/encoding/protojson" | ||
| ) | ||
|
|
||
| // defaultCreditReadTimeout bounds optional remote credit enrichment during a | ||
| // balance read. The local satoshi balance remains useful when this call times | ||
| // out, so refresh paths must not wait on remote connectivity indefinitely. | ||
| const defaultCreditReadTimeout = 2 * time.Second | ||
|
|
||
| // Service implements the daemon-side WalletService gRPC handler. It is a | ||
| // thin facade: every method translates the proto request into typed internal | ||
| // calls (router, recv, history, runtime) and returns a normalized response. | ||
|
|
@@ -24,6 +30,8 @@ type Service struct { | |
| router *router | ||
| recv *receiver | ||
| history *history | ||
|
|
||
| creditReadTimeout time.Duration | ||
| } | ||
|
|
||
| // newService builds the Service handle given its composed dependencies and | ||
|
|
@@ -32,11 +40,12 @@ type Service struct { | |
| // pure wiring. | ||
| func newService(deps *Deps, runtime *Runtime) *Service { | ||
| return &Service{ | ||
| deps: deps, | ||
| runtime: runtime, | ||
| router: newRouter(deps, runtime), | ||
| recv: newReceiver(deps, runtime), | ||
| history: newHistory(deps, runtime), | ||
| creditReadTimeout: defaultCreditReadTimeout, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -558,7 +567,9 @@ func gapResponse(cursor int64) *wavewalletrpc.SubscribeWalletResponse { | |
| } | ||
|
|
||
| // fetchBalance is the shared helper that pulls the daemon's GetBalance and | ||
| // projects its richer breakdown onto the flat wallet shape. Optional credit | ||
| // enrichment has its own short deadline; when it is unavailable, the method | ||
| // still returns the authoritative local satoshi balance. | ||
| func (s *Service) fetchBalance(ctx context.Context) ( | ||
| *wavewalletrpc.BalanceResponse, error) { | ||
|
|
||
|
|
@@ -605,8 +616,13 @@ func (s *Service) fetchBalance(ctx context.Context) ( | |
| return resp, nil | ||
| } | ||
|
|
||
| creditCtx, cancel := context.WithTimeout( | ||
|
There was a problem hiding this comment. 🟡 A credit lookup that exceeds the new 2-second bound returns
Member
Author
There was a problem hiding this comment. Fixed in The bounded credit lookup still degrades to the authoritative local balance, but the fallback now emits |
||
| ctx, s.creditReadTimeout, | ||
|
There was a problem hiding this comment. 🚫 Dismissed by @bhandras 🟡
Member
Author
There was a problem hiding this comment. Not worth changing. The production path is |
||
| ) | ||
| defer cancel() | ||
|
|
||
| credits, err := s.deps.SwapService.ListCredits( | ||
| creditCtx, &swapclientrpc.ListCreditsRequest{ | ||
| Limit: 1, | ||
| }, | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
F3(Minor) — Doc says over-cap TimeoutSeconds is clamped, not rejected ·docs/wavewalletdk_mobile.md:57decodeReceiveRequestreturns an error whenTimeoutSecondsexceeds 300 (sdk/wavewalletdk/mobile/wallet.go:67), but this line tells host authors that positive values are "capped at five minutes", which reads as clamping — a host that sends600gets a failedReceive, not a 300-second one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
5183f572. The binding documentation now says values above five minutes are rejected, matchingdecodeReceiveRequestand its negative/excessive/overflow tests.