Repository navigation
fix: only fee-bump transactions the wallet broadcast - #1140
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Pins LDK Node 0.7.0-rc.78, so bumpFeeByRbf refuses transactions the wallet has no broadcast record for, and maps the new NodeError.TransactionNotBroadcastByWallet case in AppError. Reviewed 7477475, full tier (dependency pin). Build skipped: iOS does not build on Linux, so this review is reasoned from the code, the pinned release and CI.
What I checked, and 5 candidates I ruled out
Read in full: Bitkit/Utilities/Errors.swift (the NodeError mapping), CoreService.boostOnchainTransaction, LightningService.bumpFeeByRbf / accelerateByCpfp / send, OnchainSendAttemptService.prepareOnchainSend
Pin: Package.resolved revision 1976754 is the commit v0.7.0-rc.78 points to, it contains synonymdev/ldk-node#129 (merge e794b35 is an ancestor), and the release's LDKNodeFFI.xcframework.zip hashes to the Package.swift checksum 2a003f8a…f285.
Upstream delta rc.77..rc.78: #129 only, plus the release commit. All three wallet broadcast routes record the txid before dispatching: send_to_address, PreparedOnchainSend::broadcast, and the RBF/CPFP replacements.
CI: SwiftFormat, validate and detect-changes green. Run Tests, Run Integration Tests and build-local were still pending when I reviewed.
Ruled out
- Bitkit sends that skip the record: both on-chain send paths in the app (
sendToAddressWithBroadcastResult/sendAllToAddressWithBroadcastResult, andprepareSend*thenprepared.broadcast()) go through recording routes. Hardware-wallet sends are already gated out of Boost byisHardwareWallet. PersistenceFailedfrombroadcast()misclassified by the Paykit "definitely not sent" check: ldk-node returns it before dispatch when the record write fails, so classifying it as not sent is correct.- Exhaustive
NodeErrorswitches elsewhere: every other switch overNodeErrorhas adefault.Errors.swiftis the only exhaustive one, and the diff covers it. - New VSS dependency on the send path: the record goes to the node's existing KV store, which
persist_all()already writes before every send, so sends gain no new failure mode. - Unconfirmed sends from before the upgrade cannot be boosted: deliberate, and listed under Out of Scope in the PR.
Merge confidence: 4/5, no findings, but the unit, integration and build jobs had not finished and manual tests 1 to 3 have not run on a device.
|
Pushed 251f596: adds |
|
Pushed f0bca98: |
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no HIGH or MEDIUM.
- Every legitimate bump still works: RBF on your own sends (all app sends go through the prepared broadcast, including recovery re-preparation), CPFP on received transactions, and bumps after a restore, since the broadcast record lives in the VSS store.
- Hardware-wallet sends and transfers stay excluded from Boost.
- A counterparty-crafted RBF is refused with nothing queued.
- Sends made before the upgrade (v3.0.0 ships ldk-node rc.74) can no longer be bumped, which the PR body acknowledges.
Performance-relevant: ldk-node rc.78 adds a synchronous VSS write (record_broadcast_txid) to every send and boost. That is one extra round trip normally, and up to 180 s of retries when VSS is degraded.
Device gate (partial): not run yet. Pending: boost/rbf-own-send.xml and boost/cpfp-receive.xml, and timing a send against master on the same network.
jvsena42
left a comment
There was a problem hiding this comment.
Approving. The code scan had no HIGH or MEDIUM, and the device gate passed.
Device gate: f0bca98, iPhone 17 simulator, regtest on staging, compared with master 68bede6.
boost/rbf-own-send.xml: pass. The fee went from 143 to 422, the TXID changed, RBFBoosted is listed, and it confirmed after mining.boost/cpfp-receive.xml: pass. The Boost Fee was -2 190, the receive kept its TXID, CPFPBoosted is listed, and both confirmed after mining.- The success toast came after an 11 s sync and wasn't captured; the log confirms it with
Boost transaction completed successfully. - The Boosting status wasn't seen because a shared staging block confirmed the parent first.
- The success toast came after an 11 s sync and wasn't captured; the log confirms it with
- Negative check: an incoming unconfirmed transaction offers only CPFP, and an own send offers only RBF.
- Perf: "Created new transaction" to "Successfully broadcast transaction" had a median of 112 ms against 59 ms on master, 6 sends each. The extra ~53 ms is the new ldk-node rc.78 broadcast-record write, and Android showed +65 ms on synonymdev/bitkit-android#1706. One run here took 2.9 s at that step with no log line in the gap.
Twin: synonymdev/bitkit-android#1706
Refs: synonymdev/ldk-node#129
This PR updates LDK Node to
0.7.0-rc.78, which contains synonymdev/ldk-node#129, so Boost only replaces outgoing transactions this wallet sent.Description
0.7.0-rc.78inproject.pbxprojandPackage.resolved. After it,bumpFeeByRbfreplaces only transactions the wallet built and broadcast. Any other transaction ID returns the newNodeError.TransactionNotBroadcastByWalletbefore a replacement is built.NodeError.TransactionNotBroadcastByWalletinErrors.swiftto "No broadcast record for this transaction". TheNodeErrorswitch there is exhaustive, so the new case is required to compile.Out of Scope
Bitkit/Views: a dedicated Boost error message. The new error reaches the user through the existing LDK Node error text.Design
N/A — no UI changes.
Preview
N/A — no user-visible UI change.
QA Notes
Setup
Regtest with on-chain funds, on a build of this PR.
Journeys
journeys/boost/rbf-own-send.xml— boost an unconfirmed send this wallet made: the replacement keeps the amount, raises the fee, gets a new TXID and confirms — passed at f0bca98 (@jvsena42's run)journeys/boost/cpfp-receive.xml— boost an unconfirmed incoming payment: a Boost Fee child appears, the receive keeps its TXID and both confirm — passed at f0bca98 (@jvsena42's run)Manual Tests
0.7.0-rc.77with an existing wallet and channels → the node starts, channels reconnect and balances match — upgrade path not in CapabilitiesAutomated Checks
0.7.0-rc.78resolved from a clean package cache; it builds, including the newNodeError.TransactionNotBroadcastByWalletcase.Gaps