--- Log opened Thu Sep 17 00:00:21 2020 00:12 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 244 seconds] 00:50 -!- peter_ [~peter@185.114.3.24] has joined #rust-bitcoin 01:02 -!- jonatack [~jon@37.166.18.142] has joined #rust-bitcoin 01:21 -!- jonatack [~jon@37.166.18.142] has quit [Ping timeout: 240 seconds] 01:21 -!- jonatack [~jon@213.152.162.181] has joined #rust-bitcoin 03:18 -!- Dena12Trantow [~Dena12Tra@static.57.1.216.95.clients.your-server.de] has joined #rust-bitcoin 03:35 -!- jonatack [~jon@213.152.162.181] has quit [Ping timeout: 256 seconds] 04:19 -!- jonatack [~jon@37.166.18.142] has joined #rust-bitcoin 06:20 -!- jonatack [~jon@37.166.18.142] has quit [Read error: Connection reset by peer] 06:21 -!- jonatack [~jon@37.166.18.142] has joined #rust-bitcoin 06:23 -!- Kiminuo [~mix@141.98.103.116] has quit [Ping timeout: 272 seconds] 07:22 -!- jonatack [~jon@37.166.18.142] has quit [Read error: Connection reset by peer] 07:42 -!- Dena12Trantow [~Dena12Tra@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 256 seconds] 08:29 < BlueMatt> ariard: can you explain the fix at https://github.com/rust-bitcoin/rust-lightning/pull/649#discussion_r489813539 08:29 < BlueMatt> ariard: your proposed "fix" seems to just work around the issue by making the test no longer test the code? 09:00 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #rust-bitcoin 09:09 -!- Kiminuo [~mix@141.98.103.116] has joined #rust-bitcoin 09:14 < BlueMatt> ariard: ah! right, so thats exposing an actually super severe bug today (we create double-spends)! 09:15 < BlueMatt> ariard: take a look at https://github.com/TheBlueMatt/rust-lightning/commit/dd7546ea09b1dafc3ce7ebedb574a2fd3c2c1f14 09:27 -!- real_or_random [~real_or_r@173.249.7.254] has quit [Quit: ZNC 1.7.5 - https://znc.in] 09:29 -!- real_or_random [~real_or_r@2a02:c207:3002:7468::1] has joined #rust-bitcoin 09:52 < BlueMatt> ariard: oh, I see, its NOTABUG, it just generates one bogus tx before generating sane txn. 10:15 < BlueMatt> ariard: what do you think about getting rid of the rescanning stuff wholesale? 10:16 < BlueMatt> ariard: basically it would just add a requirement that, while scanning a block, if you find a match, you must *also* include any dependants of that transaction within the same block. 10:16 < BlueMatt> this is fine for bip 157/158, and with a remote filtering server its still pretty doable. 10:17 < BlueMatt> and it should only ever provide a trivial number of excess transactions we don't care about. 10:17 < BlueMatt> jkczyz: ^ 10:22 < jkczyz> BlueMatt: thanks for investigating 10:23 < jkczyz> I noticed the double-spend / bogus tx earlier when the same test failures were occurring https://github.com/rust-bitcoin/rust-lightning/pull/649#issuecomment-683514334 10:24 < BlueMatt> jkczyz: yea, i mean its...awkward. the test is way too specific (it tests that, given the channelmonitor has incomplete information so that generating bogus txn is the only option, that the channelmonitor will generate bogus txn) 10:25 < BlueMatt> obviously, long-term, we need to do transaction tests way more genericly (eg, "here's a list of outputs and a bag of transactions, make sure that the set of outputs is fully spent by valid transactions in the bag, but ignore invalid ones and double-spends") 10:25 < BlueMatt> but, also, do we want to ever support non-full information 10:25 < jkczyz> is the interim solution to relax the test checks? 10:25 < BlueMatt> well two approaches - its just those three tests, so its easy to just fix them to make them more robust 10:26 < BlueMatt> but the other option is to completely remove rescan support 10:26 < BlueMatt> which I think I'm leaning towards - its a lot of complexity to test it fully and I just don't know that users cant work around not having it. 10:27 < jkczyz> does that have any implications for using Electrum with chain::Filter? 10:27 < BlueMatt> yea, it puts additional requirements on an electrum-style server 10:27 < BlueMatt> namely, that it does the "must include spends of matched txn" thing. 10:28 < jkczyz> gotcha 10:28 < BlueMatt> which may mean you can't use an electrum server itself, but presumably could build something equivalent. 10:31 < jkczyz> is it correct to say removing rescan support means the same block must not be passed to block_connect more than once? 10:32 < BlueMatt> presumably, yea. i mean we could allow it, no harm in not, but only as long as it has the full data every time. 10:32 < BlueMatt> ugh, alright, let me just fix these tests to make them robust and we can decide this later 10:32 < jkczyz> sgtm 10:32 < jkczyz> gonna get some fresh air while I can but will circle back with you later 10:33 < BlueMatt> k 10:49 < ariard> BlueMatt: did you see my new comment about rescan ? I think we can drop it anyway post-anchor due to CSV 1 10:50 < ariard> BlueMatt: child shouldn't be spend in same block than channel parents 10:50 < BlueMatt> ariard: no, comment where? as for dropping it from csv, yes, that'd be a welcome change. 10:50 < BlueMatt> ah, now i think i understand your patch better - is that why it was written that way? 10:51 < ariard> BlueMatt: here https://github.com/rust-bitcoin/rust-lightning/pull/649#discussion_r490366300 10:51 < BlueMatt> oh just now. 10:51 < ariard> yes 10:52 < BlueMatt> hmmmm...what about funding + commitment? 10:52 < ariard> BlueMatt: wait what https://github.com/TheBlueMatt/rust-lightning/commit/dd7546ea09b1dafc3ce7ebedb574a2fd3c2c1f14 is about? 10:52 < BlueMatt> its somewhat of an edge case 10:52 < BlueMatt> ariard: it was just me looking at that test failing - i wasnt sure why the hell we were generating transactions which were consensus-invalid, but i see that it eventually gets it right, just generates nonsense transactions on the way. 10:53 < ariard> BlueMatt: right it's due to the rescan thing and aggregation 10:53 < ariard> BlueMatt: IIRC given you rescan you generate a) a justice for the parent then b) a justice for both parent and HTLC-child 10:54 < BlueMatt> but, while I want to jump and say we can rely on csv, given spec folks kept wanting to not have csvs if they could avoid them, i dunno if we can assume that'll be the end state 10:54 < BlueMatt> ariard: right, exactly that is what I'm seeing. 10:54 < ariard> BlueMatt: I would argue to keep them even ifthe long term 10:54 < ariard> due to less interference with tx-relay filter/carve-out obstruction 10:55 < BlueMatt> yes, well i kept trying to argue for that and i kept getting ignored. 10:55 < ariard> BlueMatt: okay so NOTABUG, just a weird transaction broadcast pattern due to the way our rescan logic is architectured 10:55 < BlueMatt> i think at some point everyone else got tired of us/me complaining that this shit is under-analyzed and just started moving on.... 10:55 -!- peter_ [~peter@185.114.3.24] has quit [Ping timeout: 260 seconds] 10:55 < BlueMatt> ariard: right, though definitely a bug in the test - the test actually *checks* that we generate that nonsense transaction :p 10:56 < ariard> lol and guess what finally SIGHASH_SINGLE was completely insecure ;) 10:56 < BlueMatt> heh, right....well.... 10:56 < ariard> BlueMatt: okay I wouldn't qualify this a bug more as our architecture leaking in test coverage 10:56 < ariard> but I see your point 10:56 < ariard> yes lightnign as a security culture issue, I know.. 10:56 < BlueMatt> right, i mean sure, whatever, either way the test is way too specific and should be toned down. 10:57 < BlueMatt> I'm playing with a better tx checking api for tests right now 10:57 < ariard> god sake, of course we shouldd have something to verify chain of transaction as expected 10:57 < ariard> I think we should just drop the rescan thing, then fix tests borken by this and move forward 10:58 < BlueMatt> yea, i mean thats one option, and its definitely the case that the rescan stuff makes testing all the cases.....hard 10:59 < BlueMatt> I'm just worried that we drop it, then find out that eg you cant point RL at an electrum server *and* upstream doesn't move towards CSV1 everywhere so we end up limited. 11:00 < ariard> I see your point we can bundle a descendants construction parser as pre-processing step for block_connected? 11:01 < ariard> I've already a comment calling to get the fitler out of block_connected, we should just a set of txn for which we know matching=true 11:02 < BlueMatt> its more than match=true, though, its (match=true || spends tx which matched) 11:02 < BlueMatt> and it may be happening on a remote server 11:02 < BlueMatt> like, I *think* if you use an electrum server today it works, *except* that you have to rescan 11:02 < ariard> yeaaah and electrum server aren't build for rescan, maybe 11:03 < BlueMatt> eh, you give it a new address/output script and it'll run back over the chain for you and give you the txn 11:03 < BlueMatt> so i think it would actually work pretty well. 11:04 < ariard> lol those servers as so easy to DoS 11:04 < ariard> but not the point here, should work if we document that rescanning is a requirement 11:04 < BlueMatt> yea...i mean they do have an address-index, though 11:05 < BlueMatt> so its not *that* expensive for them to give you a set of txn by address. 11:05 < ariard> or there is another way, we can just declare all the *potential* outputs to watch 11:05 < BlueMatt> ariard: hmm? no, whether to work with rescan *is* the point 11:05 < ariard> even before they materialize on-chain 11:05 < BlueMatt> i mean in theory we can, but thats a big set - its every commitment tx 11:05 < ariard> yes all transactiosn are counter-signed, so for a given state you know the HTLC outputs to potentially watch 11:06 < BlueMatt> and exactly in the electrum case its a bit absurd to send the server like 5 script_pubkeys for each commitment tx (the full set of outputs) 11:06 < ariard> it's 2 + N*HTLC_outputs * 2 (holder/counterparty) ? 11:06 < BlueMatt> probably? 11:06 < BlueMatt> something on that order, sure, but its a *lot* 11:06 < ariard> bad idea huge privacy leak on the balance 11:07 < BlueMatt> right. 11:07 < BlueMatt> so i think we have to decide to support rescan, or not 11:08 < ariard> I'm leaning to drop it 11:09 < BlueMatt> I think we should def support it if we can - the question is can we be confident that things work with it? 11:09 < BlueMatt> and that may be no? 11:09 < BlueMatt> like, we'd need test infra for it. 11:09 < ariard> that's a better seperation, the chain::Monitor impl build chain of matched transactions based on outputs hinted by the chain::Watch impl 11:10 < BlueMatt> i guess one point is theoretically the client *can* do rescan on their end manually 11:10 < BlueMatt> just match txn first, then rescan with the outputs 11:10 < BlueMatt> but its a ton more latency/work. 11:11 < ariard> the other solution is pre-computing then somehow as I was hinting above 11:11 < BlueMatt> ok, i think i agree with you, it just means you can never build a server with a ton of channels against an electrum server. 11:11 < ariard> please don't do this that would be insecure 11:12 < ariard> like a ton of channels with a lot of money 11:12 < ariard> I expect watchtower service to run against a full-node, even more than one 11:12 < BlueMatt> right, I dont think you should for 5 reasons, just noting that now you def cant :p 11:13 < ariard> it's more a layer question, the chain_processing layer does the parsing and return output, the chain layer access block data according to output (filters/blocks) 11:13 < BlueMatt> yea. 11:13 < BlueMatt> ok, yea, i think we agree, then. 11:13 < ariard> now the fact that your chain layer has to do rescaning is an implem question 11:14 < BlueMatt> jkczyz: unless you scream, I'm gonna go see if I can write a quick patch that does the same filtering logic on master, fix the tests for it, and we can move on :) 11:14 < ariard> if you want to boost latency build a tx/utxo index no? 11:14 < BlueMatt> yea, its just a question if you have an electrum server then you have internet latency to make requests 11:14 < BlueMatt> and for every block that has any txn you *always* have to rescan 11:14 < BlueMatt> instead of only sometimes 11:14 < ariard> in the future, we can provide pre-computed sets if we really want to boost perfs, but sounds like over optimization 11:14 < BlueMatt> but if you have very few transactions, thats probably ok. 11:15 < BlueMatt> and of course the preferred method would be to have the server do the "if it spends a matched txn, include it" logic. 11:15 < ariard> "if it spends a matched txn" right a child 11:15 < BlueMatt> right. 11:16 < ariard> I think we overall agree that a chain layer question and there is a lot of ways to optimize or access chain data so let's not stuck here 11:16 < ariard> just have the flexibility offered for different types of deployment but I think that's okay 11:16 < ariard> BlueMatt: no more coffee, back latter 11:16 < BlueMatt> k. I'm gonna work on a patch so that we require it at least in the tests. 11:37 < jkczyz> BlueMatt: works for me 11:37 < jkczyz> happy to review 12:13 -!- guest534543 [~mix@193.9.112.252] has joined #rust-bitcoin 12:16 -!- Kiminuo [~mix@141.98.103.116] has quit [Ping timeout: 260 seconds] 12:26 -!- peter_ [~peter@185.114.3.24] has joined #rust-bitcoin 12:36 -!- shesek [~shesek@164.90.217.137] has joined #rust-bitcoin 12:36 -!- shesek [~shesek@164.90.217.137] has quit [Changing host] 12:36 -!- shesek [~shesek@unaffiliated/shesek] has joined #rust-bitcoin 13:50 -!- guest534543 [~mix@193.9.112.252] has quit [Ping timeout: 240 seconds] 14:06 -!- belcher [~belcher@unaffiliated/belcher] has joined #rust-bitcoin 14:22 -!- Kiminuo [~mix@193.9.112.252] has joined #rust-bitcoin 16:07 -!- peter_ [~peter@185.114.3.24] has quit [Ping timeout: 260 seconds] 18:19 -!- wraithm [~wraithm@unaffiliated/wraithm] has quit [Ping timeout: 240 seconds] 18:19 -!- wraithm_ [~wraithm@unaffiliated/wraithm] has joined #rust-bitcoin 19:14 -!- hodlwave [~znc-admin@68.183.145.179] has left #rust-bitcoin ["leaving"] 22:20 -!- Kiminuo [~mix@193.9.112.252] has quit [Ping timeout: 272 seconds] 23:33 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 272 seconds] 23:39 -!- belcher_ [~belcher@unaffiliated/belcher] has joined #rust-bitcoin 23:42 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 246 seconds] 23:53 -!- wallet42__ [sid154231@gateway/web/irccloud.com/x-kglqdbvrlpwzgiep] has quit [Ping timeout: 240 seconds] 23:53 -!- prusnak [sid403625@gateway/web/irccloud.com/x-nficguliffcrpcdy] has quit [Ping timeout: 240 seconds] 23:53 -!- Isthmus [sid302307@gateway/web/irccloud.com/x-rhxfdwunesihigbv] has quit [Ping timeout: 260 seconds] 23:53 -!- elichai2 [sid212594@gateway/web/irccloud.com/x-jkeelzewuuuzwtst] has quit [Ping timeout: 240 seconds] 23:53 -!- Aleru [sid403553@gateway/web/irccloud.com/x-xqtgjdkzjabnbakl] has quit [Ping timeout: 244 seconds] 23:53 -!- jamesob [sid180710@gateway/web/irccloud.com/x-msxrcsfqcftqvufi] has quit [Ping timeout: 260 seconds] 23:55 -!- prusnak [sid403625@gateway/web/irccloud.com/x-fxisbvxyftpvqwzu] has joined #rust-bitcoin 23:56 -!- wallet42__ [sid154231@gateway/web/irccloud.com/x-kmodawzjplmhfcoq] has joined #rust-bitcoin 23:56 -!- Isthmus [sid302307@gateway/web/irccloud.com/x-yxshwdtkyhrjyuzz] has joined #rust-bitcoin 23:56 -!- elichai2 [sid212594@gateway/web/irccloud.com/x-emezvqtpooboqkzz] has joined #rust-bitcoin 23:56 -!- Aleru [sid403553@gateway/web/irccloud.com/x-eiqgrrqpwlwajdiw] has joined #rust-bitcoin --- Log closed Fri Sep 18 00:00:22 2020