Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: RBF batch payments manager #9298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ecdsa
Copy link
Member

@ecdsa ecdsa commented Nov 12, 2024

note: this is not "reorg safe", meaning that there is no guarantee that all payments will end up in in the blockchain.
However, I believe this is "double send safe", meaning that we will never send a payment twice.

@ecdsa ecdsa marked this pull request as ready for review November 12, 2024 10:20
@ecdsa ecdsa force-pushed the batch_payments_manager branch from b71b11f to 45c7543 Compare November 14, 2024 09:21
@ecdsa ecdsa force-pushed the batch_payments_manager branch 13 times, most recently from a48e7ca to 5c7a247 Compare December 4, 2024 10:34
@ecdsa ecdsa added this to the 4.6.0 milestone Dec 4, 2024
@ecdsa ecdsa force-pushed the batch_payments_manager branch 13 times, most recently from 3bdbf87 to 3ad4406 Compare December 10, 2024 09:24
@ecdsa
Copy link
Member Author

ecdsa commented Feb 21, 2025

File "/home/user/code/electrum-fork/electrum/wallet.py", line 1879, in make_unsigned_transaction
base_tx = self.get_unconfirmed_base_tx_for_batching(outputs, coins)

This does not correspond to the code in the batch_payment_manager branch.
get_unconfirmed_base_tx_for_batching is not used anymore (and should probably be removed)

@f321x
Copy link
Member

f321x commented Feb 21, 2025

Can confirm it doesn't happen on this branch, only on master.

@ecdsa ecdsa force-pushed the batch_payments_manager branch 2 times, most recently from 1b9218c to be6f9bb Compare February 21, 2025 11:43
@ecdsa ecdsa force-pushed the batch_payments_manager branch from 9f589b0 to ccb0cc3 Compare February 23, 2025 09:10
@ecdsa ecdsa marked this pull request as ready for review February 23, 2025 09:27
@ecdsa
Copy link
Member Author

ecdsa commented Feb 24, 2025

Note: This PR does not work well with fee settings.

When we change the fee settings in ConfirmTxDialog, this side effects the fee settings in config, which is in turn used by TxBatcher. I think we should use a FeeEstimator object that holds fee settings and provides fee estimates, without committing the settings to config.

@ecdsa ecdsa force-pushed the batch_payments_manager branch from ccb0cc3 to 3325ca1 Compare February 26, 2025 10:58
@ecdsa ecdsa force-pushed the batch_payments_manager branch 12 times, most recently from 39e40c0 to 7918670 Compare March 7, 2025 14:00
@ecdsa ecdsa force-pushed the batch_payments_manager branch 4 times, most recently from 2a231a5 to 88a16f1 Compare March 11, 2025 12:50
The class TxBatcher handles the creation, broadcast and replacement
of replaceable transactions. Callers (LNWatcher, SwapManager) use
methods add_payment_output and add_sweep_info. Transactions
created by TxBatcher may combine sweeps and outgoing payments.

Transactions created by TxBatcher will have their fee bumped
automatically (this was only the case for sweeps before).

TxBatcher manages several TxBatches. TxBatches are created
dynamically when needed.

The GUI does not touch txbatcher transactions:
  - wallet.get_candidates_for_batching excludes txbatcher
    transactions
  - RBF dialogs do not work with txbatcher transactions

wallet:
  - instead of reading config variables, make_unsigned_transaction
    takes new parameters: base_tx, send_change_to_lighting

tests:
  - unit tests in test_txbatcher.py (replaces test_sswaps.py)
  - force all regtests to use MPP, so that we sweep transactions
    with several HTLCs. This forces the payment manager to aggregate
    first-stage HTLC tx inputs. second-stage are not batched for now.
@ecdsa ecdsa force-pushed the batch_payments_manager branch from 88a16f1 to 94f4b0b Compare March 11, 2025 13:08
Comment on lines +3351 to +3356
tx = self.create_transaction(
inputs=[txin],
outputs=[],
password=None,
fee_policy=FixedFeePolicy(0),
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no outputs and zero fee? what is this supposed to do?
and in any case, maybe we should make make_unsigned_transaction enforce there is at least one output (that's a consensus rule btw)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given no outputs, make_unsigned_transaction will send all the coins to a change address.
Zero fee: this is a future transaction, its purpose is only to provide feedback to the user.
A fee will be added when the actual tx is broadcast.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given no outputs, make_unsigned_transaction will send all the coins to a change address.

Ah right... ok, makes sense. Could you add a comment "this will send all the coins to a change address"?

Note that create_transaction tries to sign this by default. Is that intentional in this case? (with zero fee?)
(indeed we should just kill wallet.create_transaction)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants