Drop system clock calls for PendingHTLCsForwardable events. by TheBlueMatt · Pull Request #351 · lightningdevkit/rust-lightning

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt · 2019-07-18T22:15:52Z

Instead, return a Duration and let the user do the work of waiting.
This is one of only a handful of steps to make us
mostly-syscall-free, at least enough to run in WASM according to
elichai.

1 extra commit to remove an entirely-useless field in channel that relied on Instant::now().

Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai · 2019-07-18T22:29:50Z

Cool :)

elichai · 2019-07-18T23:32:42Z

It's still calling SystemTime in the Key interface (https://github.com/rust-bitcoin/rust-lightning/blob/master/src/chain/keysinterface.rs)

ariard

ACK e2a9ed7, comments are already there API design issues.

That's cool, user can still hack its own monotonic counter if he can't rely on time syscall and wrappers

ariard · 2019-07-19T00:04:41Z

 /// See AwaitingRemoteRevoke ChannelState for more info
 enum HTLCUpdateAwaitingACK {
-	AddHTLC {
+	AddHTLC { // TODO: Time out if we're getting close to cltv_expiry


Just to be sure, timeout here is only relevant if you are last peer in the payment route, don't have the preimage and can initiate the canceling walk back?

And furthermore, I've a doubt where the cltv_expiry field of AddHTLC should be used in our API, I mean we have fail_htlc_backwards, which is public but how API consumer is leaded to be aware of delay ?

These are HTLCs which we accepted for relay but which we have not yet added to the outbound channel (as we were waiting for a remote revoke_and_ack at the time we received them, or were otherwise unable to forward them). AFAIR, we will never fail these HTLCs backwards as the ChannelMonitor isn't even aware of them at this time.

Okay gotcha, not implemented yet, it's going with other TODO in Channel::update_add_htlc, need to pass height in process_pending_htlc_forwards I guess

I think we can just fail them backwards if they're still waiting to be forwarded as the CLTV starts to get close in a hook from block_connected.

TheBlueMatt · 2019-07-19T00:54:32Z

@elichai right, I'm aware, I have an alternative to #352 which also drops most of the rng usage inside the library and fixes that then coming up soon.

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

TheBlueMatt added 2 commits July 18, 2019 18:15

Drop time_created in HTLCAwaitingACK::AddHTLC (we can use the CLTV)
0dfd20b

Drop system clock calls for PendingHTLCsForwardable events.
e2a9ed7
Instead, return a Duration and let the user do the work of waiting. This is one of only a handful of steps to make us mostly-syscall-free, at least enough to run in WASM according to elichai.

elichai mentioned this pull request Jul 19, 2019
Replace randomness source #352
Closed

ariard approved these changes Jul 19, 2019
View reviewed changes

TheBlueMatt merged commit fbd58b4 into lightningdevkit:masterJul 19, 2019

Uh oh!

Conversation

TheBlueMatt commented Jul 18, 2019

Uh oh!

elichai commented Jul 18, 2019

Uh oh!

elichai commented Jul 18, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Jul 18, 2019

Uh oh!

elichai commented Jul 18, 2019

Uh oh!

elichai commented Jul 18, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Jul 18, 2019

Uh oh!

elichai commented Jul 18, 2019

Uh oh!

elichai commented Jul 18, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elichai commented Jul 18, 2019•
edited
Loading

elichai commented Jul 18, 2019•
edited
Loading

elichai commented Jul 18, 2019•
edited
Loading

elichai commented Jul 18, 2019•
edited
Loading