Funding_tx: add anti-fee sniping recommendation and check if final by ariard · Pull Request #1531 · lightningdevkit/rust-lightning

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

ariard · 2022-06-09T22:03:37Z

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_transaction to check for transaction finality, beyond our scope. broadcast_transaction could be modified to return a boolean, potentially signaling a transaction standardness issue, that we would propagate back to the user within an APIError::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.

TheBlueMatt

Sure, makes sense.

TheBlueMatt · 2022-06-09T22:05:35Z

 	///
+	/// 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


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?

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/

TheBlueMatt · 2022-06-09T22:05:53Z

+ {
+ 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 {


Maybe +2 or +3 to give a little bit of headroom?

IIRC Core is strict on the +1 ? If we give a headroom and the transaction is inside that window, we fail ?

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.

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).

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.

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-commenter · 2022-06-09T22:27:18Z

Codecov Report

Merging #1531 (f99fa81) into main (22dc964) will decrease coverage by 0.01%.
The diff coverage is 91.17%.

❗ Current head f99fa81 differs from pull request most recent head 69344fa. Consider uploading reports for the commit 69344fa to get more accurate results

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.04% <90.00%> (-0.13%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.34% <100.00%> (+0.01%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22dc964...69344fa. Read the comment docs.

ariard · 2022-06-09T23:06:03Z

Updated at 2cae4ef with suggestions. Added test_non_final_funding_tx to exercise new APIError.

wpaulino

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

wpaulino · 2022-06-10T00:30:46Z

+ {
+ 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 {


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 · 2022-06-10T21:53:19Z

Updated at f99fa81, with review comment addressed.

Not something to worry about yet, but we should make this very clear in the release notes, as I'm unsure of how prevalent the use of final transactions is across our users.

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 sendrawtransaction if you implement a really basic BroadcastInterface. Though yep, better to document that clearly in release notes.

ariard · 2022-06-13T19:17:17Z

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 · 2022-06-14T20:01:03Z

Updated at 69344fa with comments addressed.

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

TheBlueMatt reviewed Jun 9, 2022
View reviewed changes

wpaulino reviewed Jun 10, 2022
View reviewed changes

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/functional_tests.rs Outdated

wpaulino reviewed Jun 13, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs

Antoine Riard added 2 commits June 14, 2022 15:57

Check if funding transaction is final for propagation
2b7ef47
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.

Recommend funding_tx to apply anti-fee sniping
69344fa

wpaulino approved these changes Jun 14, 2022
View reviewed changes

TheBlueMatt approved these changes Jun 16, 2022
View reviewed changes

TheBlueMatt merged commit e533446 into lightningdevkit:mainJun 16, 2022

Uh oh!

Conversation

ariard commented Jun 9, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 9, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard commented Jun 9, 2022

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jun 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ariard commented Jun 13, 2022

Uh oh!

Uh oh!

Uh oh!

ariard commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ariard commented Jun 9, 2022

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-commenter commented Jun 9, 2022•
edited
Loading

ariard commented Jun 10, 2022•
edited
Loading

codecov-commenter commented Jun 9, 2022•
edited
Loading

ariard commented Jun 10, 2022•
edited
Loading