--- Day changed Fri Oct 26 2018 00:00 < BlueMatt> Blackwolfsa: well we dont know what kind of threading the client has 00:00 < BlueMatt> most likely the client has a single socket handling thread 00:00 < BlueMatt> so it would block all peers 00:00 < BlueMatt> but if they have one thread per peer then it wouldnt 00:00 < BlueMatt> but no one writes one-thread-per-connection applications anymore cause its needlessly high overhead 00:26 < ariard> refactor #214 with most of review comments, still on local_delayedkey case, will let apart SigsMode (watchtower) for now 00:40 < ariard> BlueMatt: hmmm instead of passing per_commitment_secret in provide_latest_local_commitment_tx_info why not update monitor key storage with it each time there is a new commitment? 00:41 < ariard> doesn't feel it's the right way to mix secret key with LocalTx, even if this key is useless if you don't have access to base key 00:43 < BlueMatt> ariard: fair enough, though still no reason to have a second function call/variable - you can also eg change latest_local_commitment_tx from an Option to an Option<(LocalTx, PrivateKey)> or something like that 00:46 < ariard> yes trying something similar right now 01:59 < BlueMatt> ariard: I'm around for another bit if you want any more rounds of review on 214, btw 02:01 < ariard> still hacking on it 02:01 < ariard> BlueMatt: need your take on this https://github.com/rust-bitcoin/rust-lightning/pull/214#discussion_r228375165 02:03 < BlueMatt> ariard: yea, I guess for the KeysManager default, sure 02:03 < BlueMatt> returning both seems reasonable to me 02:04 < BlueMatt> ariard: re: KeysInterface-vs-KeysManager - it looks to me like KeysInterface wants to be a "general interface for getting keys" which is implemented by KeysManager which the user could chose to use, or they could do their own 02:05 < BlueMatt> ariard: the docs, however, dont reflect that split/flexibility 02:05 < BlueMatt> am I misreading the intended design or are the docs just a bit out-of-date/confusing? 02:05 < BlueMatt> eg KeysInterface lists derivation paths, but that looks to me like a KeysManager-specific implementation detail (ie should be documented on KeysManager, not KeysInterface0 02:05 < BlueMatt> ) 02:06 < ariard> well the doc is a bit out-of-date I think, we clean it when done 02:07 < BlueMatt> kk 02:07 < ariard> but yes KeysInteface is a "general interface for getting keys" agree, and derivation path sould be in doc KeysManager 02:11 < BlueMatt> kool 02:18 < ariard> BlueMatt: and it's compile! just need to delete some warnings, update tests, push next round in 10min max 02:18 < BlueMatt> kk, awesome 02:30 < ariard> hmm shit breaking a lot of tests 02:43 < ariard> I mean it's the writer stuff, due to adding fields 02:56 < ariard> BlueMatt : #214 updated with per_commitment_point in KeyStorage, will update the docs and find a workaround for writer tomorrow 03:04 < BlueMatt> ariard: cool! looks pretty good. if you have time to write like one basic test to make sure you get the events back I'd appreciate it - in the rush towards 0.0.6 I dont mind too much if its just something to get a tiny bit of line coverage, but the difference between no test and one really shitty one is pretty big 03:06 < BlueMatt> obviously also have to plumb through all the KeysInterface stuff giving ChannelManager a reference/Arc to one and using that to create ChannelKeys and pass those in to Channels? 03:07 < BlueMatt> in any case any more progress you can make would be appreciated, I'll probably take whatever you have tomorrow and just do whatever I want to it and hit merge so I can push 0.0.6 tomorrow either way 03:10 < BlueMatt> anyway, I'm off to bed 03:13 < ariard> me too, noted will try to do the more tomorrow morning if I've time before you take it forward 03:33 < ariard> well got the writer framework so fix it :) will focus on writing tests and plumbering to connect the dots at wake up 06:05 < Blackwolfsa> @BlueMatt #169 now has more complete comments, is there any outstanding comments that you think is nit clear enough 08:30 < Blackwolfsa> With #202 you want me to put that whole thing in a closure with a worker thread rather? 10:56 < stevenroose> apparently the 65-byte serialization format for recoverable signatures Trezor is using is compatible with Core's. I't something with the recovery id is the first byte after subtracting a constant value.. 11:01 < stevenroose> https://github.com/bitcoin/bitcoin/blob/master/src/key.cpp#L252 11:03 < stevenroose> I think it's worth it to make a encoder/decoder for this format in RecoverableSignature.. Question is what to name it :D It's a Core custom thing 11:09 < stevenroose> there is a fn from_compact(secp: &Secp256k1, data: &[u8], recid: RecoveryId) -> Result 11:10 < stevenroose> which is the libsecp256k1 compact, not the bitcoin core compact :| 12:03 < stevenroose> andytoshi: question: in an unsafe block, when you do `[0; 64].as_mut_ptr()`, would that work with a slice as well? f.e. `[0; 65][1..].as_mut_ptr()`? 12:31 < stevenroose> https://github.com/rust-bitcoin/rust-secp256k1/pull/74/files 13:07 < ariard> BlueMatt: okay integrate KeysInterface to ChannelManage but changed current derivation scheme for get_channel_keys in KeysManager to avoid ChannelManager to keep a mutable reference to it, so for now master_channel_key is /4' 13:11 < ariard> working to integrate it with Channel and ChannelMonitor 13:49 < ariard> BlueMatt: okay just push commits for destionation_script/shutdown_pubkey integration, should pass actual tests 13:49 < ariard> AFK for a couple of hours, will be back to write SpendableOutputs event tests 14:20 < andytoshi> stevenroose: yes, as_mut_ptr() will work on slices 14:21 < andytoshi> i need to think about adding extra ergonomic support for core's recoverable signatures, that API is a blight and hopefully we can forget it exists in the coming months 14:24 < BlueMatt> Blackwolfsa: no? rust-lightning *should not* spawn threads 14:25 < BlueMatt> Blackwolfsa: if you store where you are in sending the updates, when you get a write_event (indicating that the write buffer for that peer has available space) you can then generate additional messages and send them down the pipe 14:26 < BlueMatt> Blackwolfsa: 169 is probably ready, I'll take it post-0.0.6 14:30 < BlueMatt> ariard: cool, taking a look at the current pr now 14:35 < andytoshi> ah stevenroose if the encoded depends on compressedness of the key, it can't go in rust-secp ... rust-secp does not have a notion of "compressed" and "uncompressed" keys, those are just different serializations of the single PublicKey type 14:35 < andytoshi> i'd be OK with adding this to rust-bitcoin, behind a feature gate 14:54 < BlueMatt> ariard: I'm working on rewriting bits of 214, fyi 15:01 < stevenroose> andytoshi: yeah I noticed as I was implementing that that dependency on the key compressedness was quite bad 15:01 < stevenroose> I will just have a custom parse function in my lib 15:02 < stevenroose> (both trezor and bitcoindrpc will need it I guess) 15:04 < andytoshi> sounds good 21:09 < ariard> BlueMatt: reviewing #225, is there any changes that I should look on specifically ? 21:21 < BlueMatt> ariard: not really, its mostly just simplifications of stuff, there really aren't any material changes there 21:22 < BlueMatt> really almost all just removing some stuff I thought was overkill, fixing a few docs, etc 21:22 < BlueMatt> and moving a few things around 21:22 < BlueMatt> its not a ton of changes, though the diff might not be tiny 21:22 < BlueMatt> note that you may want to try git diff-tree -U10 YOUR_PR_COMMIT_HASH MY_PR_COMMIT_HASH 21:23 < BlueMatt> to see full set of changes (though it'll include a rebase) 21:26 < ariard> cool seeing the changes, good to get out WalletInreface, more flexibility for the user wallet 21:27 < ariard> maybe I would like to do update some docs, doing it on your tree or my tree with rebase of yours ? 21:27 < ariard> and add test 21:31 < BlueMatt> probably do it on top of #223 21:31 < BlueMatt> otherwise I have to rebase the whole tree 23:45 < ariard> BlueMatt: do we test anywhere that we're generating appropriate commitment number ? 23:46 < ariard> I mean in Channel::commitment_signed, cur_local_commitment_transaction_number, after update should be same as new commitment tx's commitment number right ?