Provide additional parameters to sign_remote_commitment

[devrandom]

Provides:

• redeem scripts
• remote per-commitment point

This will allow policy checks in a future external signing component.

[ariard]

ACK 164af7f

Thanks for working on this! It's great to let signer verify than remote commitment transactions are paying to_remote + htlc outputs with the right user-owned pubkeys. Concerning checking remote per_commitment_point, beyond script correctness, is there any further policy check which can be implemented, like curve point selection attacks this may alleviate ?

[devrandom]

Thanks for working on this!

👍

We wrote this for context: https://gitlab.com/ksedgwic/liposig/-/wikis/Lightning-Policy-Signing-PoC

... Concerning checking remote per_commitment_point, beyond script correctness, is there any further policy check which can be implemented, like curve point selection attacks this may alleviate ?

It's important to check that revocationpubkey in the script is correctly derived from our revocation_basepoint and the remote per_commitment_point. Otherwise, the remote may either take the funds, or we may be prevented from applying the penalty tx.

Also, as you say, there may be special curve points that also result in similar penalty tx attacks, but I haven't looked at detail. My initial impressions: a remote per-commit point not on the curve will be bad, since I think you won't be able to sign the penalty tx. A remote per-commit at infinity will actually be in your favor, as you'll be able to sign right away.

[ariard]

We wrote this for context: https://gitlab.com/ksedgwic/liposig/-/wikis/Lightning-Policy-Signing-PoC.

That's really cool work, I do think too than pairing incoming/outgoing HTLC by hash and only allowing balance increase at preimage providing is a must for routing nodes. Reading your proposal, there is at least 2 issues :

• you should check that the incoming channel tx hasn't been revoked
• what's about your LN node submitting update_fee ? You need a trusted fee oracle too

Further, the security model of LN being act-upon-some-transactions-broadcast, you have to trust than at least one of your monitoring backend (LN node/watchtowers) is going to act. I've few notes on LN & hardwares wallets including channel monitoring and vector of attacks, will try to publish them on the ml soon.

It's important to check that revocationpubkey in the script is correctly derived from our revocation_basepoint and the remote per_commitment_point. Otherwise, the remote may either take the funds, or we may be prevented from applying the penalty tx.

Ofc! That what I meaned by script correctness, pubkeys being part of them, and yeah you should verify than remote per-commit point is on the curve but not sure if it's really exploitable given it would mess remote htlc pubkeys too, worst case is stalling the channel ?

[TheBlueMatt]

Awesome, looks good. Should make the enforcer a bit tighter, though, I think, so that we dont break the caller side in the future.

[TheBlueMatt]

Then it should probably be a vec containing pairs, not two vecs.

[devrandom]

The outputs vec is buried inside the Transaction object, so I assume this has to wait until we move to tx construction in the signer.

[devrandom]

Unless you're thinking of moving the redeem script vec into Transaction?

[devrandom]

@ariard we will incorporate your feedback into our list of policy controls here: https://gitlab.com/ksedgwic/liposig/blob/master/docs/policy-controls.md

Regarding fees, probably makes sense to incorporate some absolute fee limit (e.g. 5% of total channel value) and another limit via a fee oracle, to reduce trust required in the oracle.

[TheBlueMatt]

Two minor comments, plus the one point above and then I think this is good to go IMO.

[TheBlueMatt]

nit: Looks like moving script to here is used, so needless hunk.

[devrandom]

Perhaps I'm missing something because Rust is new to me.

I pulled the argument out into a variable for readability. Are you saying that it's more efficient to have the expression inline in the next line? I did an experiment as follows, and got identical compiler output with rustc -O:

#[derive(Debug)]
pub struct Obj(u8);

fn bla() -> Obj {
    return Obj(5);
}

fn main() {
    let script = bla();
    println!("hello world, {:?}", script);
//    commenting the above and uncommenting below results in the same compiler output
//    println!("hello world, {:?}", bla());
}

[TheBlueMatt]

No, sorry, not commenting about effeciency at all here, just noting that the hunk isn't doing anything. I usually point out such things as its otherwise awkward in the same commit and I didn't automatically assume it was for readability. Doesn't matter too much either way.

[devrandom]

OK, so readability improvements in separate commits, so the intention is more transparent? or attach a PR note to the hunk?

[devrandom]

the one point above

Are you referring to the comment "Then it should probably be a vec containing pairs, not two vecs."? I had a question in reply to that.

[TheBlueMatt]

Oops, sorry, it was marked resolved so guess it got hidden, the comment in the enforcement block:

"Oops, sorry, would be better to put this in EnforcingChannelKeys since otherwise we're just needlessly slowing down InMemoryChannelKeys when we "know" that it is fine (mostly cause EnforcingChannelKeys tests it in fuzz targets :p)."

[devrandom]

Ah, not sure why I resolved that one. Fixed.

[devrandom]

Should I squash?

[TheBlueMatt]

Oops, I'm really sorry, I really didn't turn my brain on the last two review rounds :(. So I dont think there's any actually new information being passed here - it can all be calculated from other info provided already (though we should probably expose get_revokeable_redeemscript publicly and better document this fact):

• The remote_per_commitment_point is actually already available as keys.per_commitment_point (since keys is, itself, a per-commitment object),
• The HTLC redeemscripts are to be calculated using chan_utils::get_htlc_redeemscript() which will generate it from the deconstructed HTLCOutputInCommitment values passed in (and are already being done so to generate the HTLC transaction signatures),
• The to_remote_output can be calculated from keys and to_self_delay.

I'd generally rather avoid passing in duplicative information (it can be massaged by any hardware-wallet-supporting client implementations of ChannelKeys.

You can see this by the tests passing with the following commit: TheBlueMatt@60858cc

[devrandom]

Oh, that's great actually. It means we already have the desired architecture for moving forward.

I think #444 is still needed though, so will rebase that on master.

[devrandom]

That said, I see that the remote TxCreationKeys structure has a bunch of per-tx derived keys. I think it would make sense to change this to pass in just one item:

• remote per_commitment_point (per tx)

and these should be passed-in during channel creation:

• remote delayed_payment_basepoint
• remote htlc_basepoint

Everything else can be derived in the signer.

That would actually restore some changes in this PR, but not the redeem script stuff.

[TheBlueMatt]

Yep, I think that would be great... one way we may want to do that without performance loss for the InMemoryChannelKeys would be to instead just make the computed keys only visible within the crate and expose the per_commitment_point only for public consumption.

…

> On Jan 15, 2020, at 15:58, Dev Random ***@***.***> wrote:  That said, I see that the remote TxCreationKeys structure has a bunch of per-tx derived keys. I think it would make sense to change this to pass in just one item: remote per_commitment_point (per tx) and these should be passed-in during channel creation: remote delayed_payment_basepoint remote htlc_basepoint Everything else can be derived in the signer. That would actually restore some changes in this PR, but not the redeem script stuff. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.