Enhance onchain transaction management #628
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Camillarhi
force-pushed
the
feat/tx-rebroadcast-fee-bumping
branch
from
39922cc to
c228760
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Just took an initial look and added a few high-level comments. I have yet to review any of the actual RBF logic changes.
However, as noted elsewhere, please don't make all the changes as single huge commit, but rather break the PR up in logical commits that all have descriptive commit messages outlining what the change is, why it's necessary, etc. For guidance you can have a look at https://cbea.ms/git-commit/
It would also make sense to not include the changes for #452 in this initial PR directly, but do it in a separate PR to keep the diff more manageable and reviewable.
bindings/ldk_node.udl
Outdated
| }; | ||
|
|
||
| dictionary AnchorChannelsConfig { | ||
| sequence<PublicKey> trusted_peers_no_reserve; | ||
| u64 per_channel_reserve_sats; | ||
| }; | ||
|
|
||
| dictionary RebroadcastPolicy { |
There was a problem hiding this comment.
Honestly not sure if we should expose this to the user at all, or if we should simply implement sane defaults, mod possibly allowing to disable auto-rebroadcast.
There was a problem hiding this comment.
Thanks for the feedback. I've set auto-rebroadcast to be enabled by default for reliability, but added a config option to disable it for users who need more control, while using the set sane default values.
Camillarhi
force-pushed
the
feat/tx-rebroadcast-fee-bumping
branch
3 times, most recently
from
a52b1e5 to
3523954
Compare
Camillarhi
force-pushed
the
feat/tx-rebroadcast-fee-bumping
branch
from
3523954 to
609d0a0
Compare
|
Thanks for the PR. |
| self.payment_store.remove(&payment_id)?; | ||
|
|
||
| self.payment_store.insert_or_update(payment_details)?; |
There was a problem hiding this comment.
I'm not fully sure about this flow: since with RBF we can’t guarantee which transaction will eventually confirm (usually the last one, but not always), removing the previous payment here might cause issues. What happens if the earlier tx ends up confirming instead of the latest — would the wallet balance be deducted while the store still shows the payment as pending? If so, could this lead to a confusing UX for the user?
There was a problem hiding this comment.
Excellent catch. You're right to be concerned about the potential for state inconsistency.
However, the update_payment_store method acts as a reconciler that syncs the payment store with the actual state of the BDK wallet. Since the payment store is designed to be a reflection of the wallet's state.
Introduces a `RebroadcastPolicy` to manage the automatic rebroadcasting of unconfirmed transactions with exponential backoff. This prevents excessive network spam while systematically retrying stuck transactions. The feature is enabled by default but can be disabled via the builder: `builder.set_auto_rebroadcast_unconfirmed(false)`. Configuration options: - min_rebroadcast_interval: Base delay between attempts (seconds) - max_broadcast_attempts: Total attempts before abandonment - backoff_factor: Multiplier for exponential delay increase Sensible defaults are provided (300s, 24 attempts, 1.5x backoff).
Add `Replace-by-Fee` functionality to allow users to increase fees on pending outbound transactions, improving confirmation likelihood during network congestion. - Uses BDK's `build_fee_bump` for transaction replacement - Validates transaction eligibility: must be outbound and unconfirmed - Implements fee rate estimation with safety limits - Maintains payment history consistency across wallet updates - Includes integration tests for various RBF scenarios
Camillarhi
force-pushed
the
feat/tx-rebroadcast-fee-bumping
branch
from
609d0a0 to
8b7cf78
Compare
Thanks for the review! I've split the PR into two separate commits as suggested |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
This PR enhances on-chain transaction management:
Rebroadcast/bumping of on-chain wallet transactions
Handle RBF'd Pending payments
Changes
Related Issues