Expose HTLC transaction locktime in BumpTransactionEvent::HTLCResolution

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.

[wpaulino]

While users could easily figure it out based on the set of HTLC descriptors included within, we already track it within the OnchainTxHandler, so we might as well expose it to users as a nice-to-have. It's also yet another thing they must get right to ensure their HTLC transaction broadcasts are valid.

[codecov-commenter]

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01⚠️

Comparison is base (31e78ff) 91.40% compared to head (99fff74) 91.39%.

❗ Current head 99fff74 differs from pull request most recent head 23e233b. Consider uploading reports for the commit 23e233b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@ Coverage Diff @@## main #2082 +/- ##
==========================================
- Coverage 91.40% 91.39% -0.01% 
==========================================
Files 102 101 -1 Lines 49563 49564 +1 Branches 49563 49564 +1 ==========================================
- Hits 45303 45299 -4 - Misses 4260 4265 +5 

Impacted Files	Coverage Δ
lightning/src/util/events.rs	35.27% <0.00%> (ø)
lightning/src/chain/package.rs	92.81% <88.88%> (-0.10%)	⬇️
lightning/src/chain/channelmonitor.rs	94.46% <100.00%> (-0.06%)	⬇️
lightning/src/chain/onchaintx.rs	92.46% <100.00%> (-1.42%)	⬇️
lightning/src/ln/monitor_tests.rs	97.85% <100.00%> (-0.35%)	⬇️

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Wait, aren't the anchor outputs 1-CSV? Doesn't that mean the locktime is implicitly the max(cltv, output_height + 1)?

[wpaulino]

The 1 CSV doesn't affect the transaction locktime though, only the height at which a spend is valid, but that's not an issue here since with anchors we only offer the HTLCs to claim once a confirmed commitment is presented. We can use a later locktime on HTLC transactions from a counterparty's commitment, which we can spend with only our signature. For our commitments, we need to maintain the same locktime the counterparty signed on.

[TheBlueMatt]

Right, this is changing the semantics of this function pretty substantially, and now really confusing - whereas it used to always be "the soonest time this can be confirmed" (which was correct because IIRC we had some code that looked at it in that light), its now that for all the transaction types exceptHolderHTLCOutput, which can be 0 or something else. That's pretty confusing, and IMO we should have consistent semantics across the function, or use something else for this particular use-case.

[wpaulino]

It seems like we can just reword the comment in absolute_tx_timelock then? The way it was previously intended to be used is no longer needed (this seems to have happened prior to me working on the codebase so I'm missing context). Its only use outside of anchors is to determine whether we should delay a new request for a future block, and using the to-be-crafted transaction's locktime there still seems correct – we should delay it until we're ready to broadcast it a block before its timelock. For the remaining output types, output_conf_height + 1 is a good default to discourage fee sniping, though we could do even better by using the current height instead.

[TheBlueMatt]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

[wpaulino]

Yea, I think so. let's just add more clarity. Happy to fix the fee sniping and use that too.

Done, also had to rebase to address a conflict.

[TheBlueMatt]

Thanks, nice to have anti-fee-sniping.