19:07:20 #startmeeting 19:07:20 Meeting started Mon Oct 26 19:07:20 2020 UTC. The chair is cdecker. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:07:20 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:07:32 Damn, forgot the command syntax 19:07:58 Uhm, nope that was correct, is there still a meeting running? 19:07:59 https://wiki.debian.org/MeetBot if anyone is curious 19:08:09 hum, strange, it should work 19:08:33 or did it break because last time I dc-ed in the middle of the meeting? 19:08:45 Ok, I'll try to check during the meeting 19:08:49 #link https://github.com/lightningnetwork/lightning-rfc/issues/805 19:09:02 should we try an endmeeting? 19:09:11 #endmeeting 19:09:17 bitconner needs to be done by the previous chair 19:09:21 ah 19:09:30 no, looks like the last meeting was ended properly (looking at the logs) 19:09:32 did the old meeting actually ever eeven end? can't recall 19:09:43 Ok, let's try again then 19:09:47 cdecker: Error: Can't start another meeting, one is in progress. 19:09:56 Nope, bot broken, ah well 19:09:59 Yeah, I rememeber reading logs. What is the name of the bot, can anyone rememeber? 19:10:30 I will be the bot and turn logging on, and ping aj to fix after. 19:10:46 thanks! 19:10:49 It was called lightningbot 19:11:01 Beep. MEETING STARTED. 19:11:01 the bot died? 19:11:04 lol 19:11:05 Anyway, back to the topic 19:11:08 lol 19:11:15 #topic Clarify bolt 4 #801 19:11:29 #topic BOLT 4: link to BOLT 1 for tlv_payload format #801 19:11:37 #link https://github.com/lightningnetwork/lightning-rfc/pull/801 19:12:03 I think this PR needs feedback from one of the pytools expert, clearly out of my league xD 19:12:09 It's changing a single name in the wire format and adds a clarification 19:12:22 yeh typo more or less, sgtm 19:12:29 lgtm 19:12:41 The clarification is ok, but the rename is likely to break some things on our end 19:12:54 Then again we pull in the spec in bulk and fix up in the same commit 19:12:55 the cost of auto generation ;) 19:13:03 So I think we're good too 19:13:47 OK, so he's a bit confused. Each tlv stream has its own name; they're a global namespace. Sure, type x tends to have x_tlvs, but increasingly we're dropping the _tlvs suffix (e.g. in offers I just removed it) 19:14:12 But I should comment on issue. 19:14:20 I think he's conbfused re tlv vs tlv_stream 19:14:27 so a tlv tuple vs a series of them 19:14:49 fwiw I don't think this really materially affects things, just more about consistent nomenclature 19:14:50 Exactly 19:15:11 Ok, should be easy to ack or nack 19:15:26 I don't really care enough either way :-) 19:15:58 same for me 19:16:04 Shall we just ack it then? 19:16:07 lol same 19:16:10 just did on the pr 19:16:11 merge! 19:16:11 I'm happy to replace tlvs -> tlv_stream ftw. BUt you can't reverse them. 19:16:26 totally fine with merging as-is and fixing nomenclature later in the overall spec 19:16:27 Hm? How do you mean rusty? 19:17:13 Ah just saw your comment on the PR 19:17:17 cdecker: his final comment is to change [`accept_channel_tlvs`:`tlvs`] to [`tlv_stream`: `accept_channel_tlvs`] 19:17:27 But tlv_stream is *not* a specific type. 19:17:40 (remember, format is TYPENAME: FIELDNAME 19:17:54 Yep, I remember the namespacing I opposed in the first place :-) 19:18:13 /shrug 19:18:54 It has the advantage of being precise? Anyway, we can imply the existence of foo_tlv_stream at the end of foo now, IIRC. 19:19:10 Ok 19:19:44 How about I update the tooling to accept `tlv_stream` and do a global sweep? 19:19:51 Sounds good 19:19:57 SGTM 19:20:06 #action rusty to try out the change and report success/failure 19:20:11 Moving on then 19:20:31 #topic Require to claim revoked local output in its own penalty tx post-anchor #803 19:20:36 #link https://github.com/lightningnetwork/lightning-rfc/pull/803 19:21:41 This is yet another mempool pinning attack found by our resident expert ariard :-) 19:21:57 concept ACK, just nits from a merge issue I think 19:22:52 Yeah, isn't this a subset of the known divide-and-conquer problem if you create a single penalty tx? 19:23:47 it's a bit different rusty, in this case they use the new flexibility in the 2nd level to possilby delay a sweep/resolution 19:24:03 Concept ACK also 19:24:07 but even if that 2nd level confirms, you still have the csv at that level and the revocation clause 19:24:19 so imo it's the other party just delaying the inevitable in a sense 19:24:50 so they block your mega spend for long enough, then maybe are able to sweep their output in isolation (tho that would require them to get past their own pinning?) 19:24:58 by the other party you mean the cheating party? 19:25:16 so delay long enough for their csv to expire, which _could_ be a long time, so it's better for you to just sweep that output first then do the rest 19:25:26 yeh cheating party 19:25:46 If I understood correctly this is about delaying and then replacing the penalty tx itself by weighing down the penalty by stuffing the HTLC-success/failure 19:25:50 as w/ most pinning stuff, imo there's a lot of caveats, but might as well be more defensive impl wise 19:26:23 So this is likely to have a large impact on punishments 19:26:24 roasbeef: right. There's an assumption that the "main output" (bad nomenclature, that's not used elsewhere in spec) is the most important, may not be true... 19:27:13 yeh so it's: you try to sweep the entire thing at once, but somehow (ordering, fees, mempool, etc, etc) they have this other large transaction tree, which "blocks" your actual penalty transaction 19:27:44 you can also react to it in real time kinda too, like if your penalty is taking a while, split it up and just sweep the main output then the rest 19:28:13 So, you have to be ready to split, and you have to do it based on delay, not based on onchain activity (before we worried about HTLC txs getting through). 19:28:26 mhmm, or just always start out w/ it being split 19:28:38 as before, you still need to handle partial 2nd level htlc confirmtaion and adjust accordingly 19:28:38 Yeah, that's what this PR is proposing 19:28:40 roasbeef: yeah, so make lots of tiny txs, because nobody cares... 19:29:02 Seems sensible enough to use either a splitting logic or not aggregate at all in the first place 19:29:06 yeh I mentioned that in the PR as well rusty, theen there's also the just move over most of their funds to miner's fees if you're still made whole after that 19:29:12 that's the simplest solution 19:29:37 roasbeef: s/so make/we make/ sorry... 19:29:48 but it's worth mentioning these subtleties for impl that start optimizing away from the simple choice of one tx per output sweep 19:29:58 Agreed 19:30:44 So, verdict is "LGTM, needs addressing the nits"? Sound good? 19:30:57 SGTM 19:30:59 sgtm 19:31:03 yeh base is good imo, needs some word smithing, room for expansion in this aerea later 19:31:19 #agreed looks good, needs addressing the nits 19:31:27 Ok, now on to the CVEs :-) 19:31:28 ack 19:31:38 weee 19:31:51 Shall we go through them individually? 19:32:08 sure 19:32:10 #topic Fail channel in case of high-S remote signature reception #807 19:32:10 #link https://github.com/lightningnetwork/lightning-rfc/pull/807 19:32:24 So high-S non-normalized signatures 19:32:52 set of changes in this PR lgtm, going further tho impls should start to attempt to possibly check mempool acceptance before accepting transactions 19:32:53 I'm wondering whether we should repeat this sentence everywhere (what's done in the PR) or have in Bolt 1 a section about signature validity that mentions this 19:33:09 Instead of that, mempool acceptance like roasbeef said 19:33:11 tho that can be kinda difficult tho, since policy rules can be addeed w/e and ppl can be on older versions of bitcoind that don't have those policy rules or w/e other node 19:33:37 it's harder for light clients too tho, assuming they haven't re-impl'd the "entire mempool" 19:33:56 also fwiw, Ithink the current bitcoind calls for mempool acceptance aren't package aware, so you can just test one transaction in isolation 19:34:04 I was tempted to just reference BIp62 but that only mentions low-s as a native source of malleability but doesn't make a prescription 19:34:05 t-bast: agreed, i think specifying it in one place is sufficient. also means we can start enforcing low-s on gossip msgs 19:34:08 that's a broader point tho, in this case it was the high-s 19:34:38 also fwiw, the way our fix was impl'd, if soeone did happen to have a high-s sig, it was convered automatically in the background before broadcast 19:35:20 c-lightning should never accept high-s signatures for anything because we use libsecp256k1 internally 19:35:36 same for eclair, we use libsecp256k1 19:35:47 mhmm in our case it was a re-impl for an optimization 19:35:53 betweeen our LN sig format and the bitcoind version 19:36:00 ok, was curious why indeed 19:36:05 so a version that operated on bytes, instead of using btcec which used big ints 19:36:07 Interesting 19:36:27 cgo + libsecp256k1 would not have been an option? 19:36:29 also in our case, we did full VM validation for co-op close, but only did normal ecdsa sig check for sigs/htlcs 19:36:41 * rusty fears anything reimplementing secp256k1... 19:36:48 cgo break cross-compilation 19:36:49 what I'm trying to say is that lack of libsecp wasn't the issue 19:36:57 Ouch, ok makes sense 19:37:03 it was manual conversion between 64-byte sigs and the bitcoin format 19:37:21 interesting, thanks for detailing that! 19:37:23 it could be an option, at the expense of portability 19:37:23 as the sigs themselves _are_ valid, and would have been mined 19:37:31 assuming relay 19:38:06 also this type of malleability kinda matters less post segwit, but can still be nice to have for certain mempool tracking stuff 19:38:16 Ok, everyone ok with the way the PR is formulated? Or shall we ask for a general validity section that is referred to from the appropriate sections? 19:38:50 Both options sound good to me, I'm fine with either one 19:38:58 cdecker: i think it's more forward looking to specify it once, then say all sigs in ln must me low-s 19:39:16 Yep, that'd also be my preference 19:39:49 Hehe, and rusty seems to like the explicit nature of the repeated statement 19:40:15 Yeah, people tend to read the part of the spec they're implementing. 19:40:28 link to the bip instead of the bitcoind PR tho? 19:40:41 Let's merge it this way, and then pull it back into a dedicated section at a later point once we have sufficiently many rules for them to be cluttered 19:40:51 sgtm 19:40:57 roasbeef BIP62 talks about low-s, but doesn't prescribe 19:41:06 So a link to it is unlikely to clarify 19:41:12 gotcha yeh, and that also lists a buncha other malleability vectors as well 19:41:45 #agreed cdecker to merge the PR and defer grouping the prescriptions to a later point in time 19:42:06 #topic Prevent preimage reveal collision while claiming onchain incoming HTLC #808 19:42:06 #link https://github.com/lightningnetwork/lightning-rfc/pull/808 19:42:52 roasbeef could you walk us through the attack vector? I have something like 3 different interpretations that don't overlap 100% 19:44:23 ok 19:44:31 cdecker: if you tried to route a payment through a node that had an invoice matching the forwarded htlc, lnd would claim the incoming channel if it went to chain 19:44:41 so for lnd we have two pre-iamge dbs: the ones we discovered and our own 19:44:58 you could amke us go to chain, and check our normal invoice db for the pre-image and settle it on chain 19:45:04 this is bad because the outgoing htlc is still live, so you're leap frogging the settlement 19:45:19 yep, so someone gets a pre-iamge early, and depending on the situation may be able to gain from that 19:45:33 the invoice that goes on chain actually needs to be the same amount as well, so you'd need to put up funds 1:1 19:46:02 it's solved by enforcing always outgoing before ingoing, or making sure we checked only the "general pre-image db" (discovered from routing forwarding) if we went on chain and were not the final hop 19:46:06 it just needs to be greater than the invoice amount 19:46:15 yeh 19:46:45 the fix we impl'd was the ordering check there, but also we plan on going back to also add the dependancy check as well as it's a more general solution 19:46:49 it's really hard to properly word this requirement without just explaining the attack 19:46:57 So the worst case scenario would be the attacked node out of pocket for a premature settlement of a forwarded payment? 19:47:11 lol yeh, it was also pretty subtle and took a few back n forths to actually narrow it down and attempt to categorize it 19:47:12 i think this could be simplified to: "MUST NOT reveal the preimage if there are active downstream HTLCs" 19:47:20 But shouldn't always claim all HTLCs that match known preimages make you whole again? 19:47:56 cdecker: so if this doesn't trigger the invoice being shwon as settled, the attakcer has bought a pre-iamge for the price of an on-chain transaction and routign fees, which may not actually be useful for anything depending on the application 19:47:57 cdecker: you pull on chain, mark the invoice settled, but since the downstream htlc is still active, the attacker settle downstream and take the funds back 19:48:08 yeh if it's is settled then that happens ^ 19:48:27 bitconner: it's not enough, it could be settled downstream but you still don't want to leak the preimage 19:48:29 it also requires the attacker to intercept an actual payment as well 19:48:55 making the payment_addr _mandatory_ eliminates the interception attack vector as they can't guess what it actually is to make us reveal the pre-image 19:49:09 imo it's easiest to imagine the attacker making a circular route. but the first hop is settled before the remaining n-1 19:49:10 which afaik, some nodes in the wild already require themselves with a small patch 19:49:44 once the first hop is settled, the node thinks the invoice is paid onchain, but the attacker then walks out the back door with all moolah 19:50:25 bitconner: when upstream takes the funds, won't that trigger you to resolve downstream payment? 19:50:28 yeh ideally they also need a direct chan to the victim as well, and some merchants these days hide their node behind another using a private channel to link the two, meaning it may have not been possible 19:50:48 rusty: so reverse the ordering: you settle the incoming before the outgoing 19:50:58 (assuming we're using the same terms here lol) 19:51:01 Ah, wait so the start scenario is the victim node V between attackers A and B. V issues an invoice, then A routes through A -> V -> B, but V considers the route-through as the payment? 19:51:16 cdecker: yes 19:51:19 cdecker: yes, rout-thru here meaning pulling on-chain 19:51:40 if V's invoice had the payment_addr then A+B couldn't guess that to provide a valid looking onion 19:52:06 also A+B may discover this by intercepting a normal payment to V, then somehow guessing it's for V to launch this other route to extract the pre-image 19:52:06 but, they can omit the payment secret entirely since no one requires it 19:52:06 Ah, I see. So there is just one payment that gets re-interpreted by the invoice DB when going on-chain, it's not really two HTLCs that collide at V (which is what I thought was the case) 19:52:28 ah yeh the "collision" nomenclature by antoine can be kinda confusing there 19:52:53 mhmm so it would need to be required as bitconner mentions, which is somehting we planned on doing anyway, and imo we've had enough time since we did the whole mpp/tlv revolution 19:52:53 Now it makes sense, thanks for the explanation! 19:53:01 yeah, the collision here refers to the malicious htlc's payment hash and the invoice payment hash 19:53:31 cccccclvtubhtjhvrkklrltkvjrbrdfdjbinlhtfvicc 19:53:37 noice 19:53:39 roasbeef: yeah, we're doing logging RN on blockstream store to see how many payments include the secret field. 19:54:07 we're thinking of making payment secrets required by default in 0.12 19:54:34 rusty: cool, I would assume most "mondern" wallets do at this point 19:55:00 would people welcome a pr that changes that to the recommended BOLT11 feature set? 19:55:06 roasbeef: yeah, you gotta think so.... 19:55:18 or, adds it, rather 19:55:32 end user implication here is that if they have a really old wallet (like 1 yr old at this point)? their payments may fail to certain destinations 19:55:38 cccccclvtubhtgujbggjcengcruiruunbektufnrvckt 19:55:43 but the invoice itself would also convey the requirement, so ideally they can get a nicer method 19:55:48 Well, there is something to be said for hitting users with the occasional incompatibility to keep them updating xD 19:55:52 the yubikey seems to agree here 19:55:57 heh true 19:56:03 subtle ack 19:56:36 bitconner: if the logs indicate ~everyone is upgraded, I'd like to do a c-lightning release with that to be sure to flush out more users. Then we can upgrade spec? 19:56:51 next lnd release is scheduled to drop mid-ish nov 19:57:00 Ok, going back to the PR in question, I think the wording could be improved slightly ("thou shalt not conflate payments destined for you and those that are just passing through") 19:57:05 the 0.12 that bitconner referenced 19:57:16 cdecker: ACK, that's exactly what should be meant 19:57:18 cdecker: but doesn't the dependancy ordering requireent suprecede that? 19:57:29 so "never pull the incoming htlc before the outgoing if the outgoing hasn't expired yet" 19:57:31 rusty: sure either one, i'm also okay with the spec leading the impl here 19:57:44 "Your software shall have no bugs, especially not ones which can lose money". Done. 19:58:05 šŸ˜‚ 19:58:09 roasbeef: I think that still leaves a hole, where you could reveal your preimage for less than the invoice amount 19:58:10 No I think it really should be that payments destined for you should be treated separately from those that are forwarded no more, no less 19:58:59 cdecker, yes that indeed was the fix on our end. we were falling back to the invoice database if we couldn't find the preimage in our "learned-forwarded-preimgae db" 19:59:00 Well, just re-reading your point roasbeef, yours implies mine, so either way is fine 19:59:40 Ok, so shall we bikeshed on the PR itself? I think it needs to be worded a bit more explicitly 19:59:59 separating the two indeed prevents this. main issue was that lnd thought it was the exit, so it wasn't considering that a downstream htlc existed. if they're properly isolated it goes away 20:00:02 I think it's not enough: A -> V -> B, A drops onchain, B fails the payment. The V -> B payment is settled, but V should still not reveal the preimage on-chain on the A -> B link 20:00:20 because the payment may be for less than the invoice amount 20:00:38 isn't that already specified re amount checking in the payload? 20:00:46 Yes, so separating the two is exactly what it should do: in your scenario it was forwarded so don't look in your invoice DB 20:00:49 I guess there's also kinda a layering thing here as well 20:01:14 cdecker: yep, indeed 20:01:38 Anyway, the easiest fix to all of this is "if you have the preimage, claim the HTLC independently of it being destined for you or not" that ought to teach them some respect 20:01:43 Agreed, clean separation of your own invoices and forwarded payments 20:02:00 (and don't forward it, be greedy) 20:03:20 sounds this like one needs some more work on the pr itself? 20:03:30 #agreed everyone to help make the formulation a bit more explicit 20:03:37 sgtm 20:03:48 sgtm 20:04:10 what happened to t-bast-unofficial? lol 20:04:15 #topic Make invoice's `s` flag mandatory ? #809 20:04:16 #link https://github.com/lightningnetwork/lightning-rfc/issues/809 20:04:21 the Real T-Bast took over 20:04:24 bitconner: I beat that loser 20:04:40 is -official the irc version of blue checkmark? 20:04:47 xD 20:05:10 Ok, last issue for today? 20:05:23 Nah, we can probably make it through the list 20:05:26 Just 2 more 20:05:43 So making `s` mandatory? 20:05:55 ah, perfect 20:05:55 And this one we already kinda discussed 20:06:25 Yep, so the verdict is that we are gauging support in the network, and reconvene as soon as we have conclusive results? 20:06:36 ACK 20:06:38 damn I loose the UTC game again :( 20:06:48 t-bast-official: yeah, want to report how many nodes indicate support and paste on issue? realCdecker can you grep store logs to find out how many have `s` in payments? 20:06:49 haha XD 20:06:59 #agreed More measurements needed to gauge the impact of making the flag mandatory 20:07:00 Am I the only non-real here? Iā€™m just a bot 20:07:02 ariard: haha, too late so we rejected all your PRs 20:07:13 rusty I have no access to the store logs (thankfully) 20:07:15 rusty: yep, sign me on to measure this 20:07:20 But I'll ask ops to pull the stats 20:07:26 realCdecker: thx! 20:07:32 nevermind I was mostly interested by channel spamming 20:07:58 tbh no matter what the numbers say i think we should do it 20:08:27 #topic Add a SECURITY.md #772 20:08:28 #link https://github.com/lightningnetwork/lightning-rfc/pull/772 20:08:38 ah here the context for this one 20:08:50 I mostly interested by getting some security.md out 20:09:02 it's a safety/ux tradeoff, but realistically no one should be running software that old 20:09:12 realBitconner, not sure we should. Hitting a couple of laggards on the head with incompatibility is one thing, if it's 50%+ it's a major issue 20:09:48 because while finding one of the CVE I did send a report to all of them even if it was a lnd issue (the high-S one) 20:10:18 as we don't have that much a gentleman coordination policy to avoid someone patching quickly stuff and thus revealing weakness in other implementation 20:10:18 on the bright side, most economically-relevant lnd nodes have likely been upgraded to 0.11 20:11:03 we should have more CVEs, they force people to update! 20:11:34 ariard, I like the proposal, and some more process around these things. However, I think we need to be a bit more specific 20:11:39 I like the idea of SECURITY.md, we should include some contact GPG keys directly in the repo too. I'll go. 20:11:40 I agree with ariard we should get a basic security.md out 20:12:02 And we can improve it as we go 20:12:15 For example point 3, informing upstream maintainers at the same time as the LN developers is dangerous 20:12:19 what I'm really worried is the kind of situation like the inflation CVE on core which was found by an external party and communicated to multiple core codebase maintainer 20:12:33 They may not coordinate with us, and inadvertently leak info about the vulnerability 20:12:39 in that kind of timeline you want to act fast and also coordinate with other 20:13:29 The same goes for point 5: the reporter should be discouraged from attempting to fix it and open a PR directly, despite it maybe being in their power 20:13:46 Opening a PR may put other implementations and the installation base at risk 20:14:38 So we definitely need to pin this down on two axes: who to inform, and what to do unilaterally 20:14:46 Keep it simple, and respectful. Get three contacts (one from LL, ACINQ and Blockstream), and we will handle contacting other implementations; don't make reporter do all the work. 20:15:21 Yep, we used to have the bitcoin-security list for these kinds of things, maybe that's a model as well 20:15:29 I agree with rusty and cdecker, make it as simple as possible for the reporter 20:15:54 +1 20:15:55 But do list things that the reporter must avoid doing at all cost 20:16:08 Remember the reporter is doing us a huge service, let's be as helpful as we can. 20:16:16 rusty: +1, will update the PR in consequence 20:16:23 #1 don't post on twitter 20:16:35 it would be great if each implementation can comment on the PR its security contact 20:16:38 Absolutely, having them run around gathering addresses is not good, let's have a single address and GPG key 20:16:39 realBitconner: lol 20:16:59 The address forwards to a lead on each team and we triage and coordinate from there 20:17:03 ariard: will do 20:17:11 realCdecker: people have vacations, so you want at least two. And sharing a GPG key is hard... 20:17:50 I think sending a report to 3 different GPG keys isn't that much a high barrier 20:18:05 i think having one gpg key per team. submit to the impl you know it works against. 20:18:06 and IIRC bitcoin-security have more than 2 people behind 20:18:24 Ok, but let's list the addresses and GPG fingerprints in the doc 20:18:38 ariard bitcoin-security was unencrypted for the most part 20:18:39 realCdecker: yes definitely 20:19:21 Ok, then the verdict is "simplify and precisify", sounds good? 20:19:33 ACK 20:20:03 sounds good 20:20:26 precisify šŸ¤“ 20:20:59 realCdecker: ah... but it's more for a threshold on receiver availability due to vacations/travels/... 20:21:23 #agreed Documenting the reporting process was accepted. The document needs to add contact details (e-mail and gpg keys) for the implementations and add details about what _not_ to do (report CVEs via the Twitter tag #StealMyMoney) 20:22:00 I think sharing an e-mail address and a GPG key in a team should be good, shouldn't it? 20:22:44 also should say not to use #HaveFunStayingPoor or Udi will RT 20:23:02 FWIW: 15EE 8D6C AB0E 7F0C F999 BFCB D920 0E6C D1AD B8F1 20:23:12 bitconner: love it 20:23:41 ACK, that fingerprint matches 20:24:09 Great, I think that concludes the official part of the meeting, now for wine and snacks 20:24:11 91FE 464C D751 01DA 6B6B AB60 555C 6465 E5BC B3AF 20:24:13 btw, going through the logs, I'm working on getting a libstandardness on the core-side to reduce future tx-standardness malleability vectors 20:24:19 snacks? 20:24:20 lnd pgp key^^ 20:24:22 o.O 20:25:02 #endmeeting! 20:25:07 #endmeeting