--- Day changed Thu Aug 16 2018
01:13 < dongcarl> andytoshi: I'm tackling the parsing stuff right now, jsyk
08:11 -!- savil[m]2 [savilmatri@gateway/shell/matrix.org/x-apauksymihwplyga] has quit [Remote host closed the connection]
08:20 -!- savil [savilmatri@gateway/shell/matrix.org/x-etxndewahlljdhqo] has joined #rust-bitcoin
08:38 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-fzavlczcqgreqcba] has joined #rust-bitcoin
18:06 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-fzavlczcqgreqcba] has quit [Quit: Connection closed for inactivity]
19:36 < BlueMatt> andytoshi: fyi: if you take this on other libs afl becomes faster (not sure if you've been using afl or honggfuzz) https://github.com/TheBlueMatt/rust-lightning/commit/eb84d6ea10a379d2705adcfa805f4d29a559c416
19:39 < andytoshi> been using honggfuzz
19:39 < andytoshi> thanks for the tip tho
19:40 < andytoshi> i'd accept such a PR for rust-bitcoin and rust-secp (i guess i should learn how to use afl tho to test it)
19:40 < BlueMatt> I'll do that eventually, I'm just backed up
19:41 < BlueMatt> just noting it in case you've been using afl
19:41 < andytoshi> ah thx
22:20 < ariard42> BlueMatt: updated logging PR, I can rework on it in the coming hours if you want to get it done asap
22:52 < BlueMatt> ariard42: oh, hey, you still here?
22:52 < BlueMatt> I will press merge in a few minutes if not
23:02 < ariard42> yes 
23:03 < BlueMatt> heyhey, if you want to fix the two comments here, that'd be cool: https://github.com/rust-bitcoin/rust-lightning/pull/91#pullrequestreview-147050187
23:03 < BlueMatt> also, the comment about not using log_error!() for things that are dos'able
23:03 < ariard42> i'm on it
23:04 < BlueMatt> I think maybe we'll want to rethink the log levels eventually but gotta think harder on that
23:04 < BlueMatt> like maybe we'll want a RemoteError level that indicates error, but that we may not want to write to persistent storage as an attacker can fill your disk
23:05 < ariard42> yes i had a look on all the others log_erros to know if they  were dos vectors
23:06 < ariard42> a RemoteError ? a one displayed but not written on disk ?
23:06 < BlueMatt> I mean I'd feel bad moving all the errors a remote peer can trigger into like Info or something like that
23:06 < BlueMatt> cause they clearly arent
23:07 < BlueMatt> like if a remote node sends us an error message, calling that Info is dumb
23:07 < BlueMatt> but also putting that in Error is dangerous
23:07 < ariard42> i see
23:07 < ariard42> on the implementation-side you can disallow logging past a given limit 
23:08 < ariard42> we can write this as a comment for logger implementers
23:08 < BlueMatt> yea, exactly, splitting it out so that Logger implementors can handle it appropriately (rate-limit it, ring-buffer it, etc)
23:09 < BlueMatt> or on platforms where logs are already ring buffers just log it
23:10 < BlueMatt> I'd, just kinda guessing, think that anything Info and up should be considered "safe" in that a remote peer cannot trigger an infinite amount of it
23:10 < ariard42> will add a warning commment
23:10 < BlueMatt> and then have a RemoteDOSError and RemoteDOSWarn or something like that,
23:11 < BlueMatt> or maybe all three? kinda sucks to have them all, but...
23:11 < ariard42> Hmmm not sure for the Infos considered as "safe"
23:11 < BlueMatt> ok, lets just call Warn and above safe, then
23:11 < BlueMatt> and we'll figure it out as we add logging everywhere
23:12 < ariard42> yes, maybe open an issue with the RemoteDOSError and RemoteDOSWarn ideas, need to think on it 
23:13 < BlueMatt> ok
23:14 < BlueMatt> so for now maybe just remove the log lines that are added?
23:14 < BlueMatt> and I'll go through and add a ton of shit
23:14 < ariard42> which lines?
23:14 < ariard42> was thinking that it's hard to have a distyinction
23:15 < ariard42> between RemoteDOSError and basic Error without perfclty knowing what can be triggered by a remote peer a priori
23:15 < BlueMatt> the log_error!(self, "Got error handling message: {}!", e.err); entries in Channel are definitely DoSable
23:15 < BlueMatt> well we do know what can be triggered by a remote peer in infinite quantity?
23:15 < ariard42> okay i remove all of them
23:16 < BlueMatt> the log_error!(self, "Got error handling message: {}!", e.err); in get_outbound_funding_created can stay an error, but the text may want to be more clear
23:16 < BlueMatt> same with funding_transaction_generated
23:17 < BlueMatt> the failures in forwarding are definitely DoSable
23:17 < BlueMatt> block_connected is fine (though, may want more clear text)
23:18 < BlueMatt> the ones in peer_handler are definitely DoSable
23:18 < BlueMatt> the Error decoding message, ignoring due to ln
23:18 < BlueMatt> d spec incompatibility. See https://github.com/lightningnetwork/lnd/issues/1407 one should be Debug
23:18 < BlueMatt> cause its not really an error we can handle, its more informative
23:18 < BlueMatt> or, at least, not our bug
23:18 < BlueMatt> plus it gets printed a ton
23:19 < BlueMatt> ok, I think thats all of them
23:19 < ariard42> okay just reading lnd issue
23:33 < ariard42> just reworking on making fields pub in Record and should be good
23:45 < ariard42> BlueMatt: updated, not sure for the text errors, as it's  normally included into e.err
23:50 < BlueMatt> ariard42: you have a few rebase errors from when you removed the log_errors where there are now unused-e warnings
23:50 < BlueMatt> where you renamed a _ to an err or e
23:51 < BlueMatt> at least in ChannelManager
23:51 < ariard42> aaahh sorry
23:51 < ariard42> oh yeah got it 
23:52 < BlueMatt> I think the Got error handling message: {}! in fail_htlc_backwards_internal is also remotely triggerable infinitely
23:52 < BlueMatt> not 100% sure, but...conservatism :p
23:53 < BlueMatt> same with claim_funds_internal
23:54 < ariard42> yeah was thinking to write warning comments "100% RemoteDoSError so have a hard protection and Error but maybe maybe dosable so have a weak protection" 
23:54 < ariard42> because it's to figure the execution path potentially triggable by a remote peer
23:55 < ariard42> *hard
23:55 < BlueMatt> otherwise looks good!
23:55 < BlueMatt> I mean I think generally a rule of "if its called on a path from peer_handler or an event that peers can generate, bad
23:56 < BlueMatt> if its generated in response to either a) receiving/sending money, or b) can only be triggered once per channel (ie results in channel closure), then good
23:56 < ariard42> yes but that s a good part of the code base
23:57 < BlueMatt> it is :/
23:57 < BlueMatt> but thats ok
23:57 < BlueMatt> I mean most users probably want mostly "sent money" "failed to send money" "received money" and "channel created/closed"
23:57 < BlueMatt> and why channel was closed
23:57 < BlueMatt> which covers a lot of potential errors