Funding_tx: add anti-fee sniping recommendation and check if final #1531
Conversation
lightning/src/ln/channelmanager.rs
Outdated
| @@ -2798,6 +2801,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
| /// not currently support replacing a funding transaction on an existing channel. Instead, | |||
| /// create a new channel with a conflicting funding transaction. | |||
| /// | |||
| /// Note to keep the miner incentives aligned in moving the blockchain forward, we recommend | |||
| /// the wallet software generating the funding transaction to apply anti-fee sniping as | |||
| /// implemented by Bitcoin Core wallet. See https://github.com/bitcoin/bitcoin/blob/e3c08eb620a2f317fc09fdf20969fa26f02afb91/src/wallet/spend.cpp#L612 | |||
There was a problem hiding this comment.
nit: wrap with <> to make it a "real" link (and add a \n). Also...man there needs to be a better link, maybe optech has one?
There was a problem hiding this comment.
Yeah I was thinking to point towards BIP326, though Taproot-only and I don't believe the nSequence will be really binding in practice for off-chain contract : https://github.com/bitcoin/bips/blob/master/bip-0326.mediawiki
Optech good one : https://bitcoinops.org/en/topics/fee-sniping/
lightning/src/ln/channelmanager.rs
Outdated
| { | ||
| let height = self.best_block.read().unwrap().height(); | ||
| // Transactions are evaluated as final by network mempools at the next block. | ||
| if funding_transaction.lock_time < 500_000_000 && funding_transaction.lock_time > height + 1 { |
There was a problem hiding this comment.
Maybe +2 or +3 to give a little bit of headroom?
There was a problem hiding this comment.
IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?
There was a problem hiding this comment.
Hmm, true. I guess I worry that if we reject hard at exactly +1 we'll fail to accept in some race conditions around block connection orders and the easy way to fix that is to break the anti-fee-sniping stuff by just subtracting one.
There was a problem hiding this comment.
Yeah, thinking more I would assume that your bitcoind is never behind as it should be the authoritative source of blocks for the other modules I think it's really hard to make assumption on the block connection orders between your wallet and LDK. So taking the +2 in case the wallet is faster than LDK. However, unless you have multiple and non-reconciliated (meeeeh...) block sources, your wallet shouldn't be faster than bitcoind-or-equivalent and not hit the headroom window (contrary what I was worried about in previous comment).
There was a problem hiding this comment.
If we rebroadcast funding transactions (e.g., every block), we could be a bit more forgiving here regarding the window. I wonder if this is something we'd want to do anyway outside of this context to ensure transaction propagation, as they could be dropped from mempools due to other higher fee transactions.
There was a problem hiding this comment.
Apart of the funding and closing transactions, I think we fee-bump and rebroadcast all our LN transactions in OnchainTxHandler. Of course, it would be really nice if we implement fee-bumping for those transactions suiting the user confirmation requirements (or to hedge against unreliable tx-relay peers). I think it's better implemented by us rather than BroadcastInterface as we might have view on the user UTXO set (at least after anchor output). My thinking on that has always been to wait to support dual-funding and splicing, as fee-bumping is part of the spec for both opening and close. That way we would have all our transactions scheduled by one unified component OnchainTxHandler (where it would become a trait and given access to ChannelManager, only of being a ChannelMonitor's struct for now).
Codecov Report
@@ Coverage Diff @@
## main #1531 +/- ##
==========================================
- Coverage 90.94% 90.93% -0.02%
==========================================
Files 80 80
Lines 43469 43503 +34
Branches 43469 43503 +34
==========================================
+ Hits 39533 39559 +26
- Misses 3936 3944 +8
Continue to review full report at Codecov.
|
ariard
force-pushed
the
2022-06-fee-sniping
branch
from
d5c8a4d to
2cae4ef
Compare
|
Updated at 2cae4ef with suggestions. Added |
lightning/src/ln/channelmanager.rs
Outdated
| { | ||
| let height = self.best_block.read().unwrap().height(); | ||
| // Transactions are evaluated as final by network mempools at the next block. | ||
| if funding_transaction.lock_time < 500_000_000 && funding_transaction.lock_time > height + 1 { |
There was a problem hiding this comment.
If we rebroadcast funding transactions (e.g., every block), we could be a bit more forgiving here regarding the window. I wonder if this is something we'd want to do anyway outside of this context to ensure transaction propagation, as they could be dropped from mempools due to other higher fee transactions.
ariard
force-pushed
the
2022-06-fee-sniping
branch
from
2cae4ef to
007e398
Compare
|
Updated at f99fa81, with review comment addressed.
Well to be propagated and included a transaction must be final no ? I think what is worrying are users calling our API with non-final transactions, and from now on we would reject them. At least with an error instead of a silent |
ariard
force-pushed
the
2022-06-fee-sniping
branch
from
007e398 to
f99fa81
Compare
ariard
force-pushed
the
2022-06-fee-sniping
branch
from
f99fa81 to
6e4a895
Compare
|
Updated at 6e4a895 with comment addressed. Test has been updated in consequence. |
If the funding transaction is timelocked beyond the next block of our best known chain tip, return an APIError instead of silently failing at broadcast attempt.
ariard
force-pushed
the
2022-06-fee-sniping
branch
from
6e4a895 to
69344fa
Compare
|
Updated at 69344fa with comments addressed. |
First commit checks if the funding transaction is final for propagation. If the nLocktime field has been set for any reason (e.g anti-fee sniping) and our bitcoind client is a bit behind, we may fail the p2p propagation in case of fast signatures exchange with our counterparty.
While this check is implemented the nearest from the user to allow correction, one alternative could be to make a requirement of
BroadcastInterface::broadcast_transactionto check for transaction finality, beyond our scope.broadcast_transactioncould be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within anAPIError::BroadcastFailure?Second commit invites funding transaction software to implement anti-fee sniping, I think a wallet best practice across the ecosystem to align the long-term incentives of the miners.