Send ChannelManager messages out-of-band to ensure ordered delivery by TheBlueMatt · Pull Request #217 · lightningdevkit/rust-lightning

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt · 2018-10-20T22:26:09Z

This fixes a rather glaring set of race conditions where we generate messages for a given channel in-order but then send them out-of-order due to the two independant message paths we have (events + direct returns). Sadly we give up a bit of effeciency here by handling all messages out-of-band and pushing them into a vec first. That said, the last commit gets to completely remove reentrancy from PeerHandler::read_event, which is super, super nice for client complexity in implementing PeerHandler, so overall I'm quite happy with it.

Based on #213 cause I dont want to rebase it but sadly the fact that a ton of tests had to have a bunch of diff (though should all still be pretty clearly identical in behavior) is gonna cause some pain for other PRs. I'm willing to wait a bit to hit merge for that reason and am happy to rebase as a few other things land, but would ideally like to get this in for 0.0.6 which should be middle/end of this week.

ariard · 2018-10-21T01:07:23Z

Well if tests are still identical in behavior it shouldn't be too much a hustle to rebase other current PRs, I mean at least with mines after a really quick skim on diffs. So when ready go ahead and get it merge for 0.0.6. Will review new commits tomorrow.

ariard · 2018-10-21T23:51:43Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

ariard · 2018-10-21T23:52:19Z

With head at b5cbc4e, I hit a crash with the fuzzer, but wasn't able to reproduce it twice (should learn honggfuzz I know will do it soon..)

Add retryable and error_code fields to PaymentFailed event Add RouteUpdate event ala lightningdevkit#217 containing msgs::HTLCFailChannelUpdate Move msgs::HTLCFailChannelUpdate from update_fail_htlc's return value to the payload of RouteUpdate fired inside fail_htlc_backwards_interanl Some fixes in decode_update_add_htlc_onion Move procesing onion failure to fail_htlc_backwards_internal Test for process_onion_failure

TheBlueMatt · 2018-10-23T18:04:56Z

After discussion, the crash in question was a race in resetting the rng state across threads between the dummy test and the breakage test, this is an independent issue that may have just been exacerbated here, but should be fixed separately (by commenting out the dummy test by default).

Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Drop channel_close_key from ChannelKeys

…nnel Drop channel_monitor_claim_key from ChannelKeys

This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt force-pushed the 2018-10-msg-resp-overhaul branch 2 times, most recently from f3e5cc6 to 36ae7b8Compare October 20, 2018 22:53

TheBlueMatt added this to the 0.0.6 milestone Oct 20, 2018

TheBlueMatt mentioned this pull request Oct 21, 2018
Fill out failure codes for onion packet #157
Closed

TheBlueMatt force-pushed the 2018-10-msg-resp-overhaul branch from 36ae7b8 to aa7539eCompare October 23, 2018 18:34

TheBlueMatt mentioned this pull request Oct 23, 2018
Add ChannelManager support for monitor update failure in one place #213
Merged

TheBlueMatt force-pushed the 2018-10-msg-resp-overhaul branch from aa7539e to cd43181Compare October 23, 2018 20:46

TheBlueMatt mentioned this pull request Oct 25, 2018
Fix and test update_add_htlc but disconnect pre-commitment_signed #222
Merged

TheBlueMatt force-pushed the 2018-10-msg-resp-overhaul branch from cd43181 to fbc42adCompare October 25, 2018 19:22

TheBlueMatt mentioned this pull request Oct 25, 2018
Implement and document Channel/ChannelManager (de)serialization #223
Merged

TheBlueMatt force-pushed the 2018-10-msg-resp-overhaul branch 4 times, most recently from df38048 to 470d9ccCompare October 26, 2018 19:04

Antoine Riardand others added 11 commits October 27, 2018 09:34

Generate Events from ChannelMonitor to indicate spendable ouputs
8c235d9
Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

Add a KeysInterface which provides keys from user
d33cb3c
Move ChannelKeys into keysinterface for generate a set of it from master_seed and change return type to panic on bogus data

Implement KeysInterface for KeysManager util
6d5dc6c

Integrate KeysInterface to ChannelManager with Arc
e361fed

Integrate shutdown_pubkey from KeysInterface in ChannelManager/Channel
32a9378
Drop channel_close_key from ChannelKeys

Integrate destination_script from KeysInterface in ChannelManager/Cha…
5180686
…nnel Drop channel_monitor_claim_key from ChannelKeys

Split Event, move MessageSendEvent push() inside channel_state lock
e397cb9

Send AcceptChannel responses out-of-band to ensure ordered delivery
608d517

Send funding_signed messages out-of-band to ensure ordered delivery
c962a27

Send announcement_signatures msgs out-of-band for ordered delivery
e382a7b

Send shutdown/closing_signed msgs out-of-band for ordered delivery
812f255

TheBlueMatt added 4 commits October 27, 2018 09:42

Send RAA/CS messages out-of-band to ensure ordered delivery
4342114

Respond to channel_reestablish out-of-band for ordered delivery
e2de49d

Send channel_reestablish out-of-band to ensure ordered deliver
249aa77

Avoid reentrancy of send_data from PeerHandler::read_bytes.
294ad32
This greatly simplifies clients of PeerHandler, and because almost all response messages have already been moved to process_events this doesn't change much effeciency-wise.

TheBlueMatt force-pushed the 2018-10-msg-resp-overhaul branch from 470d9cc to 294ad32Compare October 27, 2018 13:42

TheBlueMatt merged commit 3609ba5 into lightningdevkit:masterOct 27, 2018

jkczyz mentioned this pull request Jan 22, 2020
Remove peers_needing_send set from peer_handling #456
Closed