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: add config setting "wallet_fullrbf" #8821

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

Conversation

SomberNight
Copy link
Member

This adds a config variable "wallet_fullrbf", which (if enabled) lets the user ignore the BIP-125 signalling rules and try to RBF any tx.
Because of the need to connect to an electrum server that sets mempoolfullrbf=1, and the related tx-propagation and miner-hashrate-fraction issue (as also mentioned in the config var description added here), this is atm not exposed to the GUI.

related #8088 (comment)

@SomberNight SomberNight added the topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py label Jan 17, 2024
@ecdsa
Copy link
Member

ecdsa commented Jan 17, 2024

If a tx is a channel funding, it might be mutated by another instance of the same wallet, who does not know about that.
That is pretty much the only reason why we still use the RBF flag.

@ecdsa
Copy link
Member

ecdsa commented Jan 17, 2024

I do not like the idea of adding another user setting, without a clear explanation of the use case for it, and how important that use case is, compared to other use cases.

  • Are we trying to bump the fees of transactions created by other wallets and imported in Electrum?
  • Is that use case more important than the risk of losing funds from malleating channel funding transactions?

@SomberNight
Copy link
Member Author

If a tx is a channel funding, it might be mutated by another instance of the same wallet, who does not know about that.

Right. There is detailed discussion in #7072.

That is pretty much the only reason why we still use the RBF flag.

Well, tbh, I think we still need to distinguish between RBF (signalling) and non-RBF txs, because it is reliably possible to replace a signalling tx, which is probably not true for non-RBF.

the risk of losing funds from malleating channel funding transactions?

wallet.is_lightning_funding_tx() should provide some protection against that, which is also used in this PR. (more discussion in #7072 (comment))


use case for it

  • transactions created by other wallets and imported in Electrum (indeed as you correctly pointed out)
  • (double-spend) cancelling our own channel funding txs
    • because, as above, we are not signalling RBF for them
    • but perhaps we should just signal RBF for them instead
      • but if we do that, we might as well still have this PR, as the main reason against it is users accidentally bump_fee-ing funding txs

@spesmilo spesmilo deleted a comment from anysa282 Mar 11, 2024
@ecdsa
Copy link
Member

ecdsa commented Mar 12, 2024

note: if we decide that it is desirable to cancel channel funding transactions, then I think this should not be an option.
we could instead set the RBF flag on all channel funding tx.
we would still need a mechanism to prevent users from bumping the fee of a channel funding tx from another instance of Electrum.

AnwarSalam72

This comment was marked as spam.

@Sentini14

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants