--- Day changed Tue Aug 07 2018
00:29 -!- pigeons [~pigeons@androzani.sysevolve.com] has quit [Ping timeout: 268 seconds]     
02:22 -!- undeath [~undeath@unaffiliated/undeath] has joined #joinmarket     
02:53 <@waxwing> undeath, 30 is OK by me. it's a very big change. obviously a smaller, more cleary defined set would be better, but I'm far less interested in that than I am in getting it done. i don't suppose anyone else will have a strong opinion either (although i could be wrong).
02:53 < undeath> I think I found a way to squash them in a better way
02:53 < undeath> problem with the 30 commits I had when I wrote that was that features were scattered all over
02:54 <@waxwing> understood
02:54 <@waxwing> i have a few more things i want to test but i'll wait till you're finished
02:54 < undeath> I now created a branch that, as a base commit, has the 8 or so files I newly created
02:54 <@waxwing> yg is running ok, looks like it had only one successful join so far but it seems fine
02:54 < undeath> then rebased on that one, which should make the history pretty clean
02:55 < undeath> now I can try to squash the remaining commits touching existing code in a meaningful way
03:13 <@waxwing> we'll doubtless need a few edits to joinmarket-qt.py. i just tried to load and got this error:
03:13 <@waxwing>   File "joinmarket-qt.py", line 1412, in syncWalletUpdate
03:13 <@waxwing>     self.updateWalletInfo()
03:13 <@waxwing>   File "joinmarket-qt.py", line 1424, in updateWalletInfo
03:13 <@waxwing>     t.updateWalletInfo(get_wallet_printout(self.wallet))
03:13 <@waxwing>   File "joinmarket-qt.py", line 1061, in updateWalletInfo
03:13 <@waxwing>     self.wallet_name = os.path.basename(self.mainwindow.wallet.path)
03:13 <@waxwing> exceptions.AttributeError: 'SegwitLegacyWallet' object has no attribute 'path'
03:14 <@waxwing> i'll be happy to make the changes needed after you've finished, although obviously i'll ask you for help to speed it up
03:14 <@waxwing> (i mean,to help if i don't understand something)
03:15 < undeath> I tried to test some things in -qt but obviously not enough
03:16 < undeath> if you volunteer to help with that, that would be wonderful!
03:16 <@waxwing> yeah np. meanwhile i just looked at my cmtdata/commitments.json and it only has a few lines. that's very suspicious
03:16 < undeath> -qt scares me a bit, I never liked gui code
03:16 <@waxwing> is there some place where it gets deleted/overwritten maybe in the new code? that "used" list is supposed to only ever grow.
03:17 <@waxwing> (unless user actually chooses to delete it, ofc)
03:18 < undeath> I didn't touch anything regarding the commitsments afaik
03:27 <@waxwing> yeah i'm just dumb, i forgot that cmtdata/commitments.json is a totally different thing from 'commitmentlist'
03:27 <@waxwing> (which used to be called blacklist)
03:37 <@waxwing> ok add-utxo functionality all seems to be working correctly.
03:58 <@waxwing> ok that bug is fixed by changing self.mainwindow.wallet.path to self.mainwindow.wallet._storage.path ... i guess one is not supposed to access underscore variables? (is it like a "private" variable suggestion? istr)
04:00 < undeath> yes     
04:02 < undeath> it seems weird to query the wallet_name from the wallet path, qt should have opened the file in the first place
04:02 < undeath> but I don't know how the surrounding code looks like
04:11 <@waxwing> yes. i can take a look at it later.
05:18 < undeath> I'm down to 17 commits now that look rather useful
05:20 < undeath> force-pushed
05:21 < undeath> there should be no changes to the previous branch but please verify with `git diff`
05:44 <@waxwing> thanks
05:44 <@waxwing> will be checking later, will be busy on and off today
05:49 -!- davex__ [~user@97-119-117-177.omah.qwest.net] has quit [Read error: Connection reset by peer]     
07:10 -!- lnostdal [~lnostdal@77.70.119.51] has joined #joinmarket     
10:15 -!- bitbee [~bitbee@138.197.209.248] has quit [Quit: Leaving.]     
10:36 -!- bitbee [~bitbee@138.197.209.248] has joined #joinmarket     
11:35 <@waxwing> so i did a git diff between the current PR 104 code i have (before your git adventures), and what you have there now, and it's really hard to read. because the new version is rebased on the latest code, the old version isn't. i think i saw maybe only a couple of lines diff related to your code (s.close()? in storage?)
11:38 < undeath> uh, there are changes in the diff?
11:38 < undeath> why didn't git show any to me? :/
11:39 <@waxwing> but wait, what are you comparing?
11:40 <@waxwing> i'm comparing the state of PR 104 before you did rebase and then squash, with after rebase and then squash
11:40 < undeath> you're on ca57a14d0ada2a4b49bebd5bdcaf1d7d5cc98352, right?
11:40 <@waxwing> i still have the former in a branch, so i created a new branch for the "after" state.
11:40 <@waxwing> then i did a git diff branch1..branch2
11:41 <@waxwing> c6cf7
11:41 < undeath> can you give me the full hash?
11:41 <@waxwing> c6cf17cda8df5a89cadfc2b1950cfa81633cd93a
11:42 <@waxwing> you remember, i started testing yesterday before you did all that stuff
11:42 <@waxwing> "ensure storage lock file.."
11:42 < undeath> oh, that was before the rebase
11:42 <@waxwing> umm that's what i said above ^ ;)
11:42 < undeath> i'm bad at reading :>
11:43 < undeath> aad918480d572a8df29efbd00b3b5aa91692bf72 is the last rebased, non-squashed commit
11:43 <@waxwing> so when you said earlier today "there should be no changes to the previous branch but please verify with `git diff" - what did you wanto me to compare against?
11:43 < undeath> I picked that from travis
11:44 < undeath> that one matches the code currently in the new-wallet branch
11:44 <@waxwing> i don't have that one i think
11:45 <@waxwing> no def not
11:45 < undeath> you can dl it from github https://github.com/undeath/joinmarket-clientserver/tree/aad918480d572a8df29efbd00b3b5aa91692bf72
11:45 < undeath> if you want to compare it locally
11:46 < undeath> git pull probably works somehow
11:46 <@waxwing> yeah but it's not what i reviewed, so i don't see much point. anyway nbd i can go through file by file again, the delta to what i reviewed is almost 100% not your code.
11:46 <@waxwing> it's just from the rebasing
11:46 < undeath> ok     
11:47 < undeath> you could probably export a patch from c6cf17 and compare it to a patch exported from the latest new-wallet commit
11:48 < undeath> that should be identical except maybe some merged lines
11:50 <@waxwing> i dont' understand that
11:50 <@waxwing> i'll try to patch up the Qt code now.
11:51 <@waxwing> after that i'll look at/test the electruminterface. still hoping for at least one more tester of current PR104 code.
11:53 < undeath> oh, right, electruminterface
11:53 < undeath> it's probably broken
11:54 <@waxwing> yes i expect so, no worries
12:03 < undeath> I tried comparing the state with the patch files but the stuff diff outputs is even harder to read than git diff
12:10 < undeath> oh, well, this kinda works
12:11 < undeath> on current new-wallet branch: git diff --cached -p --raw --minimal HEAD~17 > /tmp/new.diff
12:11 < undeath> on old new-wallet branch: git diff --cached -p --raw --minimal HEAD~107 > /tmp/old.diff
12:12 < undeath> and then you can compare old.diff with new.diff
12:12 < undeath> I used diffuse (gui) but diff would probably work as well
12:13 < undeath> there are some diff artifacts that differ because of different file checksums but that's all you should see
12:13 < undeath> different checksums because the old branch is missing the rebased commits
12:15 < arubi> undeath, did you get any conflicts when you pull rebased master?
12:16 < undeath> the only major difference is test_coinjoin.py, there are two different lines in setup.py and two lines "s.close()" in test_storage.py
12:16 < undeath> arubi: definitely in setup.py
12:16 < undeath> and test_coinjoin.py
12:17 < undeath> (in the new-wallet branch I had a vastly extended version of that)
12:17 < undeath> test_storage.py was probably auto-merged
12:18 < arubi> I was thinking maybe waxwing can create a copy of the original 104 branch he has locally, pull rebase on jmorg's master himself, then see the diffs between the current 104 and that
12:18 < arubi> it should contain only your fixes of the merge conflicts
12:18 < undeath> oh, that would be pretty easy
12:18 < undeath> good idea
12:19 < undeath> just make sure to copy over test_coinjoin.py from new-wallet into the merge when the conflict pops up, you don't want to merge that manually
12:24 -!- MaxSan [~user@185.9.19.107] has joined #joinmarket     
12:34 <@waxwing> sendpayment via Qt = OK (mainnet)
12:34 <@waxwing> i'll do a tumbler run with it on testnet also
12:35 < undeath> cool
12:35 <@waxwing> sorry i meant regtest. there is no testnet pit.
12:35 <@waxwing> in the past for big changes i've done mainnet full tumbler runs but it takes like a day, shrug.
12:36 < undeath> btw, I have two new commits for the new-wallet branch
12:36 <@waxwing> that's the name of the PR branch, is it
12:36 < undeath> one fixes a bug in fast sync and the other makes the lru cache in cryptoengine an actual lru cache
12:36 < undeath> yes     
12:37 <@waxwing> oh. still substantial changes? .. ok 
12:37 < undeath> I haven't pushed those yet and I will later need to squash them
12:37 < undeath> the fixes are about 6 lines each
12:38 <@waxwing> oh. don't worry. just push them, then i can include them in tests.
12:38 < undeath> ok     
12:38 < undeath> pushed
12:39 <@waxwing> oh; so that was needed to bump the most recently used to the top, right?
12:39 <@waxwing> in the cache i mean
12:39 < undeath> yep     
12:39 <@waxwing> ok. missed that :)
12:40 < undeath> I knew it was there, I just didn't care because it was still much faster than without
12:41 <@waxwing> understood
12:43 < undeath> the fix in fast sync is needed because while fixing the index retain thing _collect_addresses_gap was changed to not alter the wallet indices
13:11 <@waxwing> the "assert ask for password  not in kwargs" line in open_test_wallet_maybe conflicts a bit with the logic in joinmarket-qt.py. there, I use loadWalletFromBlockchain to open a regtest (with hexseed) or mainnet wallet with one call.
13:11 <@waxwing> can we just change it to assert that ask for password is either not there or not true?
13:12 < undeath> you mean assert 'ask_for_password' not in kwargs or not kwargs['ask_for_password']
13:12 < undeath> ?     
13:14 <@waxwing> i guess. but now something else is wrong.
13:14 <@waxwing> it's weird because test wallets are working fine on command line. oh well i have to take a break.
13:14 < undeath> I mean, sure, yeah, it just is an assert
15:21 <@waxwing> it's quite complicated to explain, but i believe i'm seeing an error in the fast sync version.
15:22 <@waxwing> well, it's very complicated. i'll have to test against the version before  your recent edit to see if it happened there.
15:23 < undeath> you can try to adjust a test case to trigger the problem if that's easier
15:24 <@waxwing> my best assessment for now is that it's wrongly setting the index to (the right index + gaplimit)
15:24 <@waxwing> it leads to a weird effect where the *non* fast sync reports an incorrect balance, but i've realised that it's caused by using the fast option in a previous run.
15:25 < undeath> must be some edge case because the generic tests are passing
15:25 <@waxwing> hmm. my best guess for now is no, it's not an edge case really.
15:26 <@waxwing> thing is, it's clear from 'displayall' : you should always see that every address is used until you hit the 'new' part. but that isnt' the case here. after the used there's a gap of 6 before the final change address used (which was used in a sendpayment run in Qt, which used fast sync)
15:27 <@waxwing> i only noticed this after regtest runs with the gui and it started "losing" coins. and yeah i could be wrong in various ways, it's pretty complicated. that's just my best assessment of what i'm seeing right now.
15:27 < undeath> if you have some failed cjs in between it's not unlikely at all to have gaps
15:27 < undeath> and an even number even more so
15:28 < undeath> maybe your gap limit is actually too low
15:28 < undeath> try with --gap-limit 30 or so
15:29 <@waxwing> yes i know, but .. they're marked used.
15:29 <@waxwing> yes, this is not the problem. it is very clear in regtest testing: i choose an address to send to in another mixdepth, resync and the coins are "gone".
15:30 < undeath> used = 'used' if k < unused_index else 'new'
15:30 <@waxwing> only when displayall do you see it's because there's a big gap created. anyway as i keep saying it's very complicated. i'll have to get into it tomorrow.
15:30 <@waxwing> believe me though, there *is* something wrong :)
15:30 < undeath> everything is 'used' unless it is 'new'
15:31 <@waxwing> yep     
15:31 <@waxwing> it is clearly creating a change address 6 (presumably not coincidence, gaplimit) further on than it should, though
15:31 <@waxwing> this is not failed coinjoins. i'm talking about something that just happened.
15:31 <@waxwing> (and didn't run yg after)
15:32 < undeath> I think I see what you are suggesting
15:32 <@waxwing> well cancel 'it's clearly...' .. nothing is that clear :) apart from that somethign is wrong, that much is kinda clear.
15:32 < undeath> but with the tests passing I'm not sure how it could happen
15:33 <@waxwing> incorrect balances being reported (on mainnet wallet), without the --fast setting, after doing a sendpayment with the Qt gui (which did use fast). and coins just disappearing when doing simple direct sends in regtest.
15:34 < undeath> the incorrect balances are even more weird
15:34 <@waxwing> are you doing tests using ygrunner yet? it does make it very easy to try it out.
15:34 < undeath> no, I wrote test_coinjoin for that :)
15:35 <@waxwing> yeah tests are great but they don't go end-to-end like this. ygrunner could easily be automated to do whole tumbler runs automatically, like i said i already had that working years ago but dropped it out because slow, but nevertheless there is definitely a place for this kind of thing.
15:37 <@waxwing> anyway there's nothing to argue, it's simply the case that *immediately* after doing that sendpayment, and before running yg, i got an incorrect balance report. no possibility of it being due to a gap from failed coinjoins.
15:37 < undeath> understood
15:38 < undeath> is -qt calling sendpayment or does it implement the functionallity itself?
15:38 <@waxwing> it does it itself, i.e. instantiates Taker
15:39 <@waxwing> you have a good point there .. at least i think. well it's late, i have to rest (heh, that's why i kept saying "it's complicated", it's totally frying my brain.)
15:39 <@waxwing> i'll work on it tomorrow for sure.
15:40 < undeath> ok :)
15:40 < undeath> rest well!
15:49 -!- undeath [~undeath@unaffiliated/undeath] has quit [Quit: WeeChat 2.1]     
17:27 -!- qubenix [~qubenix@185.213.152.162] has quit [Remote host closed the connection]     
18:15 -!- qubenix [~qubenix@116.206.228.203] has joined #joinmarket     
20:32 < grubles> anyone know when wasabi wallet coinjoin rounds happen? every 24 hours, right?
21:17 -!- arubi [~ese168@gateway/tor-sasl/ese168] has quit [Ping timeout: 250 seconds]     
21:18 -!- arubi [~ese168@gateway/tor-sasl/ese168] has joined #joinmarket