Expose next_channel_id in PaymentForwarded event

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased

[atalw]

Closes#1391

There is a minor refactor included in this PR. The type of pending_monitor_events in chainmonitor has been changed to HashMap<OutPoint, Vec<MonitorEvent>>. This associates events with an outpoint, thereby removing the dependency in the MonitorEvent enum to store the outpoint separately. One doubt I had is if the order of pending events matters. If it does, this approach will not work.

[codecov-commenter]

Codecov Report

Merging #1475 (44ec53c) into main (637fb88) will increase coverage by 0.00%.
The diff coverage is 93.84%.

❗ Current head 44ec53c differs from pull request most recent head 1ae1de9. Consider uploading reports for the commit 1ae1de9 to get more accurate results

@@ Coverage Diff @@## main #1475 +/- ##
========================================
Coverage 90.88% 90.88% ========================================
Files 75 76 +1 Lines 41474 42081 +607 Branches 41474 42081 +607 ========================================
+ Hits 37695 38247 +552 - Misses 3779 3834 +55 

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	68.18% <ø> (+7.07%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.54% <ø> (-0.02%)	⬇️
lightning/src/util/events.rs	33.33% <0.00%> (-0.24%)	⬇️
lightning/src/util/test_utils.rs	77.96% <ø> (-4.46%)	⬇️
lightning/src/chain/chainmonitor.rs	97.91% <100.00%> (+0.27%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.76% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	84.79% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_tests.rs	97.09% <100.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	99.23% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs	100.00% <100.00%> (ø)
... and 35 more

---

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 637fb88...1ae1de9. Read the comment docs.

[tnull]

Thanks for having a look at this! However, I'm not too sure if the change actually warrants this kind of refactoring?
IIUC, I also find it somewhat confusing that the OutPoint key would really be used in different ways semantically, i.e., some events would actually monitor for on-chain changes there, and some, such as the PaymentForwarded only would use it to basically remember a variable temporarily. Is this correct?

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.
As the PaymentForwarded event rather tells us something about the immediate neighbor nodes from which a payment came/to which we forwarded, I'd find a terminology along the lines of incoming_channel_id/outgoing_channel_id or prev_channel_id/next_channel_id more intuitive.

[atalw]

Using the current approach all events are linked to an OutPoint from a single source. This seemed like a cleaner solution for solving the problem of getting the OutPoint to claim_funds_internal(). The other option was to add a field in the HTLCUpdate struct (which is what I initially did). That way we wouldn't need to refactor but it seemed out of place there and led to more boilerplate code.

You're right about the different semantic use of OutPoint for different enum fields but that would happen in the other approach too. Not sure how to avoid that.

Initially, I faced the confusion of nomenclature as well but I rationalized it because a node only knows about the prev/next nodes in the route. So for it the source and destination are the prev and next node, though I see how it is confusing. I added documentation to clarify what source/sink mean but if we still feel the naming is unintuitive, I'll update it.

[TheBlueMatt]

However, I'm not too sure if the change actually warrants this kind of refactoring?

Generally we've been pretty open to this kind of refactoring in the past - its relatively trivial, most existing users don't implement this interface directly so won't even see it, but even if they do they won't have any trouble adapting to it. Its certainly a lot cleaner than changing the serialization format, IMO.

Also, not sure if it is just me, but every time I read
source/sink or source/destination pairs in a payment / HTLC context, my first assumption is that they refer to the payment endpoints, i.e., the payment origin / the (final) payment destination.

Hmm, fair enough. I'd suggested it as an alternative to "source+destination" which has that problem even worse, but I like your prev/next or incoming/outgoing suggestion.

[atalw]

So I'll do a couple of things

1. Update the return type to Vec<(OutPoint, Vec<MonitorEvents>)>
2. Revert the TLV changes (add OutPoint back to enum fields)
3. Rename channel fields to prev/next

Thanks for the feedback and being patient with me! It really helps especially because I'm new to FOSS, Bitcoin, Lightning, and Rust.

[TheBlueMatt]

A few nits but this looks good to me 🚀

[valentinewallace]

This looks good! Think I'm ACK after remaining feedback is addressed

[atalw]

Rebased