--- Day changed Tue Oct 16 2018 10:06 < Blackwolfsa> @bluematt In regarding to https://github.com/rust-bitcoin/rust-lightning/pull/202#issuecomment-430030321 I think I understand the concern. But iterating over a mutable hashmap might cause them to mis or receive double messages. 10:07 < Blackwolfsa> I dont believe receive double announcements should be a problem, but missing an announcement message could potentially be. 10:10 < Blackwolfsa> Would it not be easier to track the amount of node's we are giving syncing information to? So we have some counter and a configurable limited maximum we can supply and a time. We either then ignore the routing sync or init message till the syncing counter reduces. 10:11 < Blackwolfsa> This combined with with properly set up tcp timeouts should be robust to withstand malicious syncing requests? 10:55 < stevenroose> andytoshi: > TBH, I don't actually understand this code, but if CI is happy it can't be worse than the old one.^^ What do you think @oli-obk? 10:55 < stevenroose> That's the spirit 13:09 < BlueMatt> Blackwolfsa: yea, cant use a hashmap, but we can just swap it for a BTree 13:09 < BlueMatt> then you can start iterating at a given element 13:32 < andytoshi> stevenroose: lol! 17:11 < Blackwolfsa> @Bluematt is there a way to see if the requesting peer emptied its buffer without going down to tcp level and looking at the ack message 17:11 < Blackwolfsa> I am trying to avoid go down to that level of code if we dont have to. 17:12 < BlueMatt> huh? every tcp implementation I'm aware of has a limited-sized buffer and prevents write()s until its been drained some 17:13 < BlueMatt> and we get that info back from the user's tcp stack via the return value of SocketDescriptor::send_data() 17:13 < BlueMatt> so we already track when our send buffer fills up 17:14 < BlueMatt> ie we should generate messages in response to PeerManager::write_event calls 17:15 < BlueMatt> at least when pending_outbound_buffer is empty 17:18 < Blackwolfsa> that could work 18:23 < stevenroose> hmm, andytoshi, quick word of advice again.. The Trezor HW wallet has two different protocols (new and old). I originally made a Protocol (with read, write, begin, end session) trait and two structs ProtocolV1 and ProtocolV2 to implement the respective protocols. Now to get the correct protocol in the client struct, it seems inevitable to make the client struct generic over .. 18:23 < stevenroose> That's a pity as the user of the struct shouldn't know anything about the two protocols 18:24 < stevenroose> cfr ICBOC would be as if HardDongle was generic over something internal 18:24 < andytoshi> i'd put them in an enum 18:24 < andytoshi> or a Box

18:24 < andytoshi> this isn't high-perf code, we can waste some vtables 18:25 < stevenroose> I considered enum 18:25 < stevenroose> Oh you mean enum ProtocolImpl{ V1(ProtocolV1), V2(ProtocolV2)}? 18:26 < stevenroose> Or just a Box aha that's probably the best way.. That's what like interface references actually do in other languages I guess 18:27 < andytoshi> yeah 18:27 < stevenroose> Hmm, a box still makes it generic, no? 18:27 < andytoshi> yes, but that's invisible to the user 18:28 < andytoshi> the client struct would have some field with this 18:28 < andytoshi> but the user wouldn't know how it was constructed 18:28 < stevenroose> but wait, that's the same when it's a field, no? 18:28 < andytoshi> oh, no, not a Box 18:28 < andytoshi> a Box 18:28 < stevenroose> but Protocol is a trait 18:28 < andytoshi> which is Box in newer versions of rust 18:28 < andytoshi> yes 18:29 < stevenroose> that gives me "wrong number of type arguments: expected 1, found 0" 18:29 < stevenroose> same with dyn 18:30 < andytoshi> oh, the trait is parameterized? 18:30 < andytoshi> why is the trait parameterized? 18:30 < stevenroose> oh yeah it is 18:31 < stevenroose> But it's probably better if it isn't :) 18:32 < stevenroose> Is it bad practice to have every method in a trait be generic over the same argument? 18:33 < andytoshi> i don't know, it has never occured to me to do that 18:33 < andytoshi> i don't understand your design 18:33 < stevenroose> https://gist.github.com/stevenroose/fe0da721c7e7918476784fdba15a28f7 18:33 < stevenroose> So the transport is what actually pushes stuff to the HID handle, which is kept in the client. This is because in principle, the protocol supports HID WebUSB and another third transport. 18:34 < andytoshi> the transport should be an associated item 18:34 < stevenroose> I'm only implementing HID, but not excluding others. And they (python impl) also use the protocols on top of the transport 18:34 < andytoshi> not a parameter 18:34 < stevenroose> That would make sense probably, yes. 18:40 < stevenroose> andytoshi: "the trait `protocol::Protocol` cannot be made into an object" ("note: method `write` has generic type parameters") I never used Box before tbh 18:42 < andytoshi> oh, yeah, as written this is not object-safe 18:42 < andytoshi> that's kinda annoying.. hmm 18:42 < stevenroose> Rust book: "Trait objects are both simple and complicated:" :| 18:43 < andytoshi> can you put your messages into a giant enum? 18:43 < andytoshi> instead of using a trait? 18:43 < stevenroose> I could just pass the bytes 18:43 < stevenroose> And serialize before the protocol.. 18:44 < stevenroose> Makes the protocol protobuf-indepenent (except for MessageType), but pushed upstream the encoding and decoding 18:45 < andytoshi> i would put the specific messages that trezor uses into some sort of enum/struct 18:46 < stevenroose> that enum is MessageType. But it just had the type IDs, no reference to the actual Rust type associated 18:46 < andytoshi> i'd pull the protobuf message into there 18:47 < stevenroose> Well the existing one is autogenerated 18:48 < stevenroose> I'm not sure I understand your suggestion 18:48 < andytoshi> maybe i don't understand the protobuf api. but i don't see why any of these needs to be parameterized 18:48 < stevenroose> Like a giant enum ProtoMessage { with f.e. Ping(protos::PingMessage), ... } 18:49 < stevenroose> And then implement encode and decode? 18:49 < andytoshi> yeah 18:49 < andytoshi> rust-bitcoin's network messages should also work this way .. i think they're close 18:49 < stevenroose> It's parametrized because every protobuf message struct implements protobuf::Message to do encoding and decoding 18:50 < stevenroose> Yeah the difference with rust-bitcoin's is probably that all the work for it to work out of the box is already done by the protobuf people. So redoing that is kinda reinventing the weel. 18:51 < stevenroose> There is a protobuf::parse_from_bytes(bytes: &[u8]) -> Result f.e. 18:51 < andytoshi> i'd use a Box then 18:51 < stevenroose> And if you parametrize with protos::PingMessage (which is not framework, but generated), then it works 18:52 < stevenroose> Also, exactly why is the Box unsafe if Protocol has a parametrized method? 18:55 < stevenroose> https://stackoverflow.com/a/42620873/749521 I guess 18:56 < stevenroose> Oh didn't know &Trait was possible. That would work for the messages, no? 18:57 < andytoshi> it's "object-unsafe", which is a bizarre term that simply means that you can't uniquely specify a vtable for it 18:57 < andytoshi> yep, you can use &Trait if you're willing to deal with lifetimes 18:57 < andytoshi> oh, actually there probably won't be any lifetime issues here, you're just borrowing for the function 18:58 < stevenroose> That owrks for writing, for reading (where type context is needed) it doesn't. 18:58 < stevenroose> I need to return the message there. Can return a Box, but can't parse the message without having the MessageType -> struct type mapping after all 18:58 < stevenroose> grmbl 19:00 < stevenroose> yeah I can do the enum thing 19:14 < stevenroose> Hehe, some Vim regexing and the enum is already there :D 19:16 < andytoshi> very nice 19:21 < stevenroose> I realized that I need the enum to have indices both ways (type id -> enum value and enum value -> type id). Really not a fan. Still considering having the protocol just handle Vec and do the encoding/decoding above that 19:49 < BlueMatt> ariard: ping 19:50 < BlueMatt> ariard: see https://github.com/rust-bitcoin/rust-lightning/issues/81#issuecomment-430375705 20:49 < ariard> BlueMatt: hey,i'm available in one hour and half to talk about it 22:29 < ariard> BlueMatt: I spotted the 0.0.6 well I've started just few lines and notes, still looking for a key derivation scheme, if you have one in head tell me I'll implement it 22:30 < ariard> seems lnd have crafted their own, called aezeed 23:10 < BlueMatt> yea, we def dont want some nonstandard bullshit 23:10 < BlueMatt> ideally the user could specify it 23:10 < BlueMatt> after all, the user is the wallet, not us 23:11 < BlueMatt> doing simple trees of HD keys from an HD key the user gives us seems like a reasonable approach to me, though I haven't thought hard about it 23:11 < BlueMatt> I figure they give us some HD root key of some kind (or maybe HD root key + base derivation) and then we just derive from there 23:11 < BlueMatt> that way we can talk about keys by their derivation path from the root when we tell the user certain keys which they should watch the chain for 23:50 < ariard> < BlueMatt> that way we can talk about keys by their derivation path from the root when we tell the user certain keys which they should watch the chain for // Okay so ChannelMonitor should notify back LN-not-a-wallet-manager when spotting one of its key on chain, and then this last one pass derivation path to user wallet ? 23:50 < ariard> I mean the keys corresponding to protocol exit points like channel_monitor_claim_keys or shutdown 23:51 < ariard> And for this ones should we derive them as well ? Or let the user decide with an optional field ? 23:51 < ariard> nevermind for aezeed, in fact it's a bip39 replacement, so not our concern 23:56 < BlueMatt> I think we should derive all the keys, but in tree form, so eg user gives us an HD key and says "derive everything from "m/1/2" so then we may derive Channel A as "m/1/2/1" and each entry in ChannelKeys as a subkey of that so "m/1/2/1/1-8" 23:57 < BlueMatt> but, indeed, for things from a non-cooperative close we have to tell the user the key 23:58 < BlueMatt> I guess we cant really derive those from HD cause they may mix with remote data 23:58 < BlueMatt> oh, I guess a subkey can just be a master key to us? 23:59 < BlueMatt> ok, yeaaaa, soooo