--- Day changed Sun Aug 05 2018
01:51 -!- undeath [~undeath@unaffiliated/undeath] has joined #joinmarket     
03:52 <@waxwing> undeath, how do i install the argon2 package properly? i'm getting a not found on low_level
03:52 <@waxwing> i guesss there's a binding or something?
03:53 <@waxwing> i realise that the right thing to do is run installation from scratch but i wanted to run tests on my own current environment.
03:53 < undeath> I think I simply used pip
03:53 <@waxwing> hmm yeah that's what i did
03:53 < undeath> maybe a version thing?
03:53 <@waxwing> AttributeError: 'module' object has no attribute 'low_level'
03:53 < undeath> let me check
03:54 <@waxwing> Version: 0.1.10 according to `pip show` here
03:57 < undeath> 0.1.10?
03:57 < undeath> argon2-cffi (16.3.0)
03:57 < undeath> that's what it says for me
03:57 < undeath> did you install a wrong package?
03:58 <@waxwing> ah i guess that must be it, yes
03:58 <@waxwing> 8yLKunsg7y_s
03:58 <@waxwing> oops
03:59 <@waxwing> https://github.com/flamewow/argon2_py
03:59 <@waxwing> argon2-cffi got it
03:59 < undeath> if in doubt check the setup.py ;)
04:01 <@waxwing> yeah lazy, sorry :)
04:01 <@waxwing> it's a generally slightly icky feature though, that module names are not only different from package names, but you can get conflicts like that. oh well, i'll be non-lazy next time.
04:03 < undeath> yeah, ideally each package would have a distinct module name
04:06 <@waxwing> ok tests passing, i'll start reading and maybe trying out a few manual tests as i go along (and asking Qs of course!)
04:07 <@waxwing> have you thought about rebasing yet? i guess it's going to be a complete pain
04:07 < undeath> I have rebased a week or so ago
04:07 <@waxwing> oh! great, so we should be more or less fine
04:07 < undeath> two weeks rather
04:10 < undeath> I have done a couple successful maker cjs with commit 
04:10 < undeath> 7203312
04:10 <@waxwing> great
04:10 < undeath> though at that commit non-segwit had a bug, I've not been able yet to retest that
04:11 <@waxwing> lol "SegwitLegacyWallet" .. funny because it does actually make sense.
04:11 < undeath> :)     
04:11 < undeath> hope we can switch to native sw soon
04:22 <@waxwing> so is random.SystemRandom() basically the same as os.urandom()? (in terms of being cryptographically secure?)
04:22 <@waxwing> also i wonder whether it isn't better to have the default be True on crypto...secure there
04:23 < undeath> yes, it's os.urandom
04:24 < undeath> there are not many places that really need cs-random data but it would be safer, yes
04:27 < undeath> the only place cs-random is used is for wallet entropy creation
05:21 <@waxwing> tx['category'] == 'receive' ; is that guaranteed to pick up all the wallet's used addresses?
05:22 <@waxwing> https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/104/files#diff-79627d21bc41aff521fe45af1ee3968aR495
05:24 < undeath> it's what the old implementation used, too
05:24 < undeath> I don't know how reliable it is
05:24 <@waxwing> oh, did it? i'll have a look
05:24 < undeath> for me it always seemed to work
05:24 < undeath> see line 582/583 in the diff
05:26 <@waxwing> ah yes, i didn't remember that because it was part of what was there before i edited it. but yeah i was always a bit unsure about that. probably i'm getting confused, though, between the output of `listaddressgroupings` and `listtransactions`
05:26 <@waxwing> yeah it must be correct
05:47 -!- Sentineo [~Undefined@unaffiliated/sentineo] has quit [Quit: .]     
05:47 <@waxwing> how will _get_used_indices function in the case where there was no index_cache; wouldn't the _script_map be empty?
05:48 <@waxwing> just thinking out loud here, trying to work out the details in sync_addresses on startup
05:54 < undeath> we make sure _script_map is populated in _collect_addresses_init
06:00 <@waxwing> ok, i think i got that
06:01 <@waxwing> one important nuance: if the index is set in index_cache but the addresses havent' been used up to that point, we should still respect the index_cache value, because those unused addresses may have been given (to a Taker, at any rate) in a negotiation of a failed tx and we don't want to use them again.
06:02 < undeath> that's what happens in _get_used_indices: indices[md][internal] = max(indices[md][internal], index + 1)
06:03 <@waxwing> great, yeah i remember that line :)
06:05 < undeath> oh, wait, does it?
06:06 <@waxwing> i was just about to ask
06:06 <@waxwing> that specific line is going to set to the highest used address right
06:06 < undeath> yeah
06:07 < undeath> it never concerns itself with data not created by itself
06:07 <@waxwing> there was a line in the old code that was basically max(highest index found,, index cache)
06:07 < undeath> I think I missed to implement that
06:08 <@waxwing> joinmarket wallets are extremely annoying about this. gaps galore.
06:10 <@waxwing> you're already way ahead of me but just for ref, here, from master: https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/jmclient/jmclient/blockchaininterface.py#L627-L629
06:11 < undeath> I think I wanted to implement that but eventually forgot while writing the other code
06:12 < undeath> _collect_addresses_init makes sure to keep the old index but later on it's simply overwritten
06:17 < undeath> good catch :)
06:46 -!- lnostdal__ is now known as lnostdal     
06:54 <@waxwing> so get_new_addr (which now calls get_new_script) doesn't import the address any more.
06:55 <@waxwing> it's rather sad but i'm still struggling to even understand the logic, fully, of the very first step in sync_addresses :) you gather all the addresses up to the last used (as per index_cache), plus the gap limit. then reset the index to the last used.
06:56 <@waxwing> (talking about _collect_addresses_init)
07:00 < undeath> yes, not importing the address when creating it isn't really optimal but have the wallet concern itself with the blockchaininterface and other external details even less so
07:01 <@waxwing> ok, gotcha. so it does it elsewhere i guess. haven't read that part yet.
07:02 < undeath> maker + taker import the address when generating it
07:02 <@waxwing> sure, fine
07:03 < undeath> it shifts the burden of importing the address to the caller of wallet.get_new_*
07:05 < undeath> you got the logic for _collect_addresses_init right
07:06 < undeath> after that it checks which of the gathered addresses have been used to set (soon: increase) the indices as needed
07:07 < undeath> if some index needed increasing we know the sync is not yet finished and request one more iteration
07:08 <@waxwing> so your end goal there is to have the final status be: we have the latest used address as index, and we have imported gap-limit more than that. is that right.
07:09 < undeath> the last part is more of an unintended side effect but it doesn't hurt anything
07:10 < undeath> also, it's the next unused index, not the last used one
07:11 < undeath> but that's mainly relevant for how the wallet handles things internally
07:24 <@waxwing> so this will be skipped if the address is not in _script_map right: https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/104/files#diff-79627d21bc41aff521fe45af1ee3968aR528
07:27 < undeath> yes, that's what wallet.is_known_address() does
07:27 < undeath> just pushed a fix for the retain index bug
07:41 <@waxwing> yeah i believe it's functionally the same as the old version (checks up to existing index cache, plus gap limit).
07:41 < undeath> there actually is a pretty serious flaw right now that still needs fixing
07:42 < undeath> the old implementation was using a dedicated "gap limit" for syncing
07:42 < undeath> which was set to 50 or so
07:42 < undeath> my new implementation currently uses the wallet.gap_limit which is I think 6 by default
07:42 < undeath> so you may end up needing way more iterations
07:43 < adlai> undeath: got a better improvement suggestion than "use exponential backoff or assume that the human will change the hardcoded value manually in case of too much gapping"?
07:43 < undeath> I think the hardcoded gap limit worked fine
07:43 < undeath> wouldn't mind adding that again
07:44 < undeath> I only really noticed after I had implemented it the way it is now
07:52 <@waxwing> not sure which variable you mean; maybe addr_req_count (20)?
07:53 < undeath> yeah, that one
07:57 <@waxwing> yeah, that new commit seems to do the trick
07:58 <@waxwing> even with addr_req_count, if there's an address used that's past index_cache + gap limit, it won't get picked up (in the old code)
07:58 <@waxwing> uhh .. well not *exactly*
07:59 <@waxwing> https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/jmclient/jmclient/blockchaininterface.py#L591-L593
08:00 <@waxwing> it breaks out at any point if there are gaplimit (i.e. 6 by default) unused and we're past the point of the index_cache
08:00 < undeath> oh, so the behaviour isn't that different now?
08:01 <@waxwing> i believe so
08:02 <@waxwing> but it's damn tricky.
08:06 <@waxwing> note the 'fast' version was insisting on finding all: https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/jmclient/jmclient/blockchaininterface.py#L500-L501
08:07 <@waxwing> i haven't yet fully read the new version of that
08:07 < undeath> that's still happening
08:08 < undeath> the check is just the other way round. we populate a set and remove every address we have found until it's empty
08:08 <@waxwing> yeah actually i had half read it, and rechecking now after edit, yeah i think it makes sense to me
08:31 <@waxwing> so about the stuff in this section: https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/104/files#diff-142045c799b6f7c3a3b7b07358801a08R122
08:31 <@waxwing> are these key types something known?
08:32 <@waxwing> is it basically that you're saying, that if someone wants to import, they have to specify a key type within a set defined in JM.
08:33 < undeath> actually wallet ignores key_type
08:34 < undeath> it seemed like a good idea when implementing cryptoengine but during my testing in the last weeks I realised it doesn't work in most cases
08:35 <@waxwing> yeah it's fine. i'd actually like import to be disabled by default tbh.
08:35 < undeath> if key_type was successfully detected and the user didn't overwrite it on cli it'll use that
08:35 <@waxwing> so trying out against /ygrunner now, one thing i noticed is wallet-tool displaying only 1 mixdepth by default
08:35 < undeath> otherwise it'll either use the wallet's native key type or what the user supplied
08:36 < undeath> using what command?
08:37 <@waxwing> `python wallet-tool.py hexseed`
08:37 <@waxwing> just -m 5 and it's fine
08:37 < undeath> uh, never used/tested/looked at hexseed
08:37 < undeath> or only briefly
08:38 <@waxwing> also just tried direct send (-N 0), it worked fine but the prompt displayed "sending 200000000" instead of "sending "50000000" (it displays the utxo total value, instead of the sending amount)
08:38 <@waxwing> "worked fine" means the correct number of coins were sent, just the prompt showing the wrong value
08:39 < undeath> hexseed is not an option in wallet_utils.py?
08:40 <@waxwing> it'll be this edit here: https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/104/files#diff-7ab6abfbc74fdc5e00dc2d4b03a40383R83
08:40 <@waxwing> sorry i'll come back to the hexseed thing :)
08:40 <@waxwing> yeah it's just in regtest you can enter a hex encoded seed in place of a wallet. it seems to be working fine.
08:40 < undeath> oh, yeah, I was wondering about that just today
08:41 < undeath> I have no idea why I did that because it actually seems it would even fail for non-sw
08:41 < undeath> but that shouldn't change anything about the value
08:42 <@waxwing> ok let me think about it a minute
08:42 < undeath> it just makes sure to set the amount to None in non-sw mode because that's how jmbitcoin.sign works
08:43 < undeath> I probably intended to replace that with wallet.sign_tx but forgot to change that line I suppose
08:45 <@waxwing> you need to use something like amt, not amount
08:45 <@waxwing> because "amount" is the sending amount the user specified at the start, which will be displayed in the check
08:45 <@waxwing> whereas "amt" is just there for segwit utxo signing
08:45 < undeath> oh, I overwrote a variable in the same scope?
08:45 <@waxwing> yeah that's all it is
08:46 < undeath> I see
08:46 < undeath> so it's bad in two ways
08:47 <@waxwing> i didn't get what the other problem was?
08:48 <@waxwing> if it was non-segwit, we used amount=None to flag it
08:48 < undeath> yes, but not after my edit there
08:48 <@waxwing> oh, right
08:48 < undeath> which was just brainfart
09:13 < undeath> changed that code to use wallet.sign_tx
09:18 <@waxwing> fixed, thanks
09:44 < arubi> some updates about secp-py and libsecp, I think I got it to work well enough for a PR (still waiting on centos and fedora to finish running).  osx will have to be tested by someone though, because I can't get the tests to execute on travis
09:46 < arubi> another thing that came up, in our current pip install process, we use wheels for the dependencies (binary releases), not building from source.  is building these python dependencies from source something that we'd want to do?
09:48 < arubi> if so, we'll need openssl source release.  either re-enable the flow in install.sh, or use the package available from the OS package manager
09:48 < arubi> it's pretty easy to achieve on linux, but osx might not be as straight forward (although also not too bad)
10:24 < arubi> #180 .  appreciate testing from macos \ arm folks
11:05 <@waxwing> undeath, i like the caching feature in the bip32 thing, have you seen it speeding things up? i don't know whether syncing'd be bound by doing rpc calls or address generation, guess it depends on various things
11:07 <@waxwing> i don't know that it makes sense to put calls to a btc library into a module called 'cryptoengine', but that's of limited interested. it's just a layer over the lib at the moment.
11:37 < undeath> the caching greatly improves speed. see my comment in the pr. " it did improve the runtime of test_wallet.py by about 50% for me"
11:38 <@waxwing> oh, great :) yes i *thought* i already noticed it in regtest, but toy example. yes, i couldn't be certain, but i'd have expected it'd be quite significant.
11:38 < undeath> the idea with the cryptoengine is that you have this one class/object that you can easily swap out to support different implementations
11:39 < undeath> for now it's only btc p2pkh/p2sh-p2wpkh but it would be pretty easy to eg support ltc
11:39 < undeath> and all code you have to touch in jmclient would be the cryptoengine
11:39 <@waxwing> oh; "crypto" .. :)
11:41 < undeath> you mean coinengine would be a better name? :)
11:41 <@waxwing> there's even like a website cryptoisnotcurrency.com or something
11:42 < undeath> well, it is a layer for the crypto parts
11:42 <@waxwing> i bikeshed with the best of them but i won't start on this one :)
11:43 -!- MaxSan [~user@185.9.19.107] has joined #joinmarket     
11:43 <@waxwing> i wonder if pyaes handles PCKS7 padding of CBC properly
11:45 < undeath> why wouldn't it?
11:47 <@waxwing> i've found others that don't spread here and there.
11:50 <@waxwing> https://github.com/btcsuite/btcd/pull/1108 may be of interest (but really it isn't that interesting ... afair roasbeef told me that code isn't actually used anywhere, in that repo)
11:53 <@waxwing> nope, looks like they make the same mistake, hang on i'll find it on github
11:53 < undeath> lol, spot on
11:53 < undeath> yep     
11:53 < undeath> just checked
11:54 < undeath> https://github.com/ricmoo/pyaes/blob/master/pyaes/util.py
11:55 < undeath> I also check the magic in the first eight bytes, so wrongly accepted passwords should be very unlikely still
11:55 <@waxwing> yeah. well if you read the link in that PR you can see it's a corner case
11:56 <@waxwing> right, it's hard to get the "i accept garbage" case, and even then it requires a special situation for that to matter
11:56 <@waxwing> but it's still quite annoying that this mistake is everywhere, oh well.
11:56 < undeath> very true
11:56 < undeath> especially since this is not a hard to implement problem
11:56 <@waxwing> i mean hopefully TLS implementations are not doing this stuff ...
11:57 < undeath> i wouldn't be surprised if some coders just used others' code for reference
11:57 < undeath> someone started the mess and now it's everywhere
12:00 <@waxwing> exactly, that's my deduction
12:01 <@waxwing> "full stackexchange" developers :)
12:02 < undeath> lol     
12:11 <@waxwing> ok reviewed storage now, all looks very good, left a couple of comments in PR
12:22 < undeath> thank you :)
12:22 < undeath> one thing I noticed in the old implementation is that it added a fairly big padding to disguise the length of the mnemonic extension
12:23 < undeath> right now storage only adds the little padding required by aes
12:24 < undeath> although with the whole wallet data being encrypted as a whole it's not as easy to deduct anything from the encrypted size
12:26 <@waxwing> yes we had a rather convoluted discussion about that. it'll probably be in my IRC logs.
12:26 <@waxwing> belcher, might remember
12:28 < undeath> it would be pretty much trivial to add
14:35 -!- rdymac [uid31665@gateway/web/irccloud.com/x-sezswkmhcfxnozqx] has joined #joinmarket     
15:31 -!- undeath [~undeath@unaffiliated/undeath] has quit [Quit: WeeChat 2.1]     
18:40 -!- instagibbs [~instagibb@pool-100-15-122-172.washdc.fios.verizon.net] has quit [Ping timeout: 240 seconds]     
18:41 -!- instagibbs [~instagibb@pool-100-15-122-172.washdc.fios.verizon.net] has joined #joinmarket