--- Day changed Sun Nov 25 2018
01:03 -!- rdymac [uid31665@gateway/web/irccloud.com/x-skqpegrbhmbvrwsu] has quit [Quit: Connection closed for inactivity]     
01:19 < Lightsword> waxwing, why do you call reactor.stop() here? https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/7b201b69dd0a2ed5979aa5d0c0bd52fa59147721/jmclient/test/test_client_protocol.py#L81
03:12 -!- undeath [~undeath@hashcat/team/undeath] has joined #joinmarket     
03:44 < waxwing> undeath, Lightsword currently this one is cropping up in test_wallet.py: https://0bin.net/paste/mgpdTBxHkNh4Yg4P#nvG+d3eEnStHlcWhUsJWw6uZti8WV2ChLvqYmu7CUme
03:45 < waxwing> Lightsword, hmm, well it was presumably stopping on a failure, but ... i'll have to try to remember (also there's a reason i actually wrote about this test in the docs/TODO.md, i never figured out how to use twisted.trial in a way that made much sense...)
03:45 < Lightsword> waxwing, you need to update bencoder
03:45 < waxwing> Lightsword, ah gotcha, we've got that in the dependencies now have we?
03:45 < Lightsword> needs to be version 2.0.0 or higher
03:46 < Lightsword> it gets correct version if you rebuild
03:46 < Lightsword> or update
03:46 < undeath> https://github.com/whtsky/bencoder.pyx/pull/41
03:46 < undeath> :)     
03:47 < Lightsword> yep     
03:51 < waxwing> ok so my pip doesn't see any version but 0.1.0 and 0.2.0
03:51 < waxwing> and i'm now trying a clean ./install.sh --develop and getting this error:
03:51 < waxwing> build/temp.linux-x86_64-2.7/_libsecp256k1.c:493:28: fatal error: secp256k1_ecdh.h: No such file or directory
03:52 < undeath> what command did you run to update bencoder?
03:53 < waxwing> i tried pip install --upgrade bencoder .. oh, maybe i used the wrong name :)
03:53 < waxwing> that often happens
03:53 < undeath> yes :P
03:53 < undeath> it's bencoder.pyx
03:53 < undeath> eventually you will install typesquatting malware :P
03:54 < waxwing> right. the other error with install is presumably about coincurve? i noticed there was some issue recently on the coincurve repo, not sure if related
03:54 < waxwing> undeath, yes, good point
03:54 < undeath> yes, the other error looks like a problem with coincurve
03:55 < undeath> although it seems weird to randomly pop up since we are always compiling the same version
04:03 < Lightsword> yeah,m it’s 0.2.0 I think
04:04 < undeath> Successfully installed bencoder.pyx-2.0.0
04:04 < Lightsword> ah, right
04:05  * Lightsword should probably go to bed
04:05 < Lightsword> you need libgmp
04:05 < undeath> can never go wrong with going to bed
05:06 < waxwing> Lightsword, the "TODO .. crap out" thing: when i was writing that up I think (pretty sure) I was worried that native segwit spends wouldn't work due to how the parsing routines would handle a null in the scriptSig.
05:06 < waxwing> but it's fine. so that is stale, in as much as that worry didn't materialize (and i've just verified now by looking at native segwit spend deser, it works OK)
06:36 < waxwing> yeah works fine in new and old versions. they read the 00 correctly for 0 length and do what they should.
06:42 < waxwing> Lightsword, it occurs to me that it'd be good if you pushed the updates as separate commits and only squashed at the end it'd be easier; that way i can see both the diff of the new change and the overall change, can help with reviewing.
08:33 -!- rdymac [uid31665@gateway/web/irccloud.com/x-yvodzhsqwslaynxh] has joined #joinmarket     
08:52 < waxwing> undeath, Lightsword so this def works in py2 as well as py3? https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR406
08:52 < waxwing> i'm wondering why i had the alternative for py2 if it worked anyway
08:56 < undeath> maybe it just grew organically?
08:57 < undeath> I can't see why it would not work
08:58 < waxwing> yeah i'm getting a bit lost trying to figure out how it is/isn't tested. i'll keep at it. as to why it's there, i presumably had a reason; even if i was wrong.
08:59 < undeath> the x variable in that code has become a little redundant
09:00 < waxwing> heh     
09:07 < waxwing> yeah it's fine like that. i prob just made an error in testing and assumed it was a py2/py3 thing.
09:17 < waxwing> found a DOS against takers (obv not a huge deal, dos-ing takers is basically SOP in JM :) ... https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/jmbitcoin/jmbitcoin/secp256k1_transaction.py#L642 
09:17 < waxwing> address_to_script can throw with an invalid address
09:18 < waxwing> i mean, i guess the bug is in taker.py because there's a missing validate_address call i think.
09:19 < undeath> taker crashes can be annoying but not as bad as maker crashes
09:19 < undeath> there are probably a couple more ones for both sides :/
09:20 < waxwing> yeah. i'll PR this; it should be independent. i was looking at address_to_script, and noticed it doesn't catch. most calls to it are using our own addrs, but this was the exceptional case i found.
09:20 < waxwing> (independent of the current py3 stuff i mean)
09:22 < undeath> i can add it to #205
09:22 < waxwing> undeath, sorry don't quite get it; add what? don't you think we should merge a patch now?
09:23 < undeath> oh, it's also in the current code base?
09:23 < waxwing> yes, as i said, it's independent of this py3 stuff
09:24 < undeath> ah, was thinking I might have screwed up in #205, which also uses address_to_script
09:28 < waxwing> i guess my flippant comment "obv not a huge deal, dos-ing takers is basically SOP in JM" is really a bit off, since this prevents complete-with-subset (which is heavily used today in my experience)
09:28 < undeath> but dos'ing a taker does not take away liquidity from the market at least
09:28 < waxwing> right it's way less damaging for sure
09:40 < undeath> i think you can dos a maker when you supply an authorizing tx with an output that cannot be converted to an address
09:41 < undeath> you query the tx at https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/efe2d41d34380c8c075aaff68be33e165fba14c3/jmclient/jmclient/maker.py#L60
09:42 < undeath> which assumes there exists at least one address for each input: https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/db62223ecdd5d3b81f6f1ad2b06833871a9e25d3/jmclient/jmclient/blockchaininterface.py#L842
09:44 < undeath> err, for each output
09:52 < undeath> we really should just wrap each message handler for makers into an exception handler so it never crashes
10:22 -!- belcher [~belcher@unaffiliated/belcher] has quit [Quit: Leaving]     
10:30 < Lightsword> waxwing, the future builtins change behavior to py3
10:35 < waxwing> Lightsword, ah! ofc, thanks
10:36 -!- GitHub191 [GitHub191@gateway/service/github.com/x-gtisppguieoladjo] has joined #joinmarket     
10:36 < GitHub191> [joinmarket-clientserver] AdamISZ opened pull request #235: Validate maker destination addresses (master...validate-maker-address) https://git.io/fpEsr
10:36 -!- GitHub191 [GitHub191@gateway/service/github.com/x-gtisppguieoladjo] has left #joinmarket []     
10:36 < Lightsword> waxwing, btw you should never call reactor.stop from tests
10:37 < Lightsword> https://twistedmatrix.com/documents/current/core/howto/testing.html#leave-the-reactor-as-you-found-it
10:37 < waxwing> Lightsword, yeah but the twisted.trial thing is different. but i'm not going to try to figure it out now, if you want to make changes pls do :)
10:38 < Lightsword> waxwing, you use twisted trial for some tests
10:38 < waxwing> yes that's what i mean, i used it but i never figured out a *good* way to use it.
10:38 < waxwing> i mentioned this pain point in the TODO.md doc, i marked it as a 'this really needs to be fixed, it's terrible' :)
10:39 < Lightsword> ideally would probably just switch the entire project to trial
10:39 < Lightsword> but probably best to do that after converting to py3
11:32 < waxwing> undeath, the code you linked above re: makers checking addresses is from your PR, right. i don't think there's a problem in the current master.
11:32 < waxwing> or at least, i don't think *that* specifically is a problem :)
11:33 < waxwing> i.e. https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/jmclient/jmclient/maker.py#L83
11:34 < waxwing> so, what's the story with outputs with no address, in bitcoin rpc? i guess it's just like, if the scriptpubkey is not one of the standard templates, no address is displayed?
11:34 < waxwing> only of minor interest, doesn't really affect what we should do
11:50 < Lightsword> waxwing, so my thoughts for how things should eventually be refactored is to move away from the various custom parsers and use python-bitcoinlib, do you think that would be the best strategy?
11:55 < waxwing> Lightsword, i guessed you missed me saying that a couple of days ago :)
11:56 < waxwing> not that it doesn't have its caveats of cours
11:56 < waxwing> e     
11:56 < Lightsword> yeah, forgot exactly sure how much of that was discussed
11:57 < Lightsword> I don’t really want to attempt that until py2 support is dropped however
11:57 < waxwing> see this note from ages ago: https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/docs/TODO.md#bitcoin
11:58 < Lightsword> a lot of these ugly regex checks and other type conversion checks can be replaced easially once we are py3 only since types in python3 are much more explicit than python2
12:02 < Lightsword> tracing the type conversion issues is super time consuming
12:09 < Lightsword> waxwing, one potential issue https://github.com/petertodd/python-bitcoinlib/issues/192
12:10 < waxwing> hmm, yeah. interesting. thing is, it was never going to give us everything as-is, anyway. we still need bip32 stuff and i think realistically we need a libsecp binding.
12:10 < waxwing> https://github.com/petertodd/python-bitcoinlib/pull/185
12:12 < Lightsword> yeah, the signing stuff is probably still fine to use coincurve with(although probably we could do with refactoring there)
12:14 < waxwing> Lightsword, anyway, grateful to you for the effort so far, it seems to me that the PR does the job :) Haven't *quite* finished reviewing (was most of the way through *transaction but then got sidetracked finding that bug.)
12:18 < Lightsword> yeah, it’s tricky work overall, this is def one of the toughest projects I’ve attempted to convert from python2 to python3, at least the test suite is fairly extensive, would be way harder without that
12:19 < Lightsword> there’s just so many internal assumptions that cause cascading issues that require lots of manual tracing
12:26 < waxwing> Lightsword, re that \x00 thing (no script) it's fine without it (i tested), but it doesn't matter.
12:27 < Lightsword> yeah, I was probably being overly defensive there, I was hitting some detection bugs though so was trying to explicitely handle edge cases
12:29 < waxwing> Lightsword, did you write that or is it a copy-paste? https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR548
12:29 < waxwing> i only ask because i vaguely remember writing that :)
12:29 < waxwing> oh duh it's here https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fL483
12:29 < Lightsword> heh, yeah
12:30 < waxwing> ah i see, another one of those 'future' things, so old ver not even needed
12:31 < Lightsword> yeah
12:31 < waxwing> json_is_base and json_changebase can be removed right. wait did i write that somewhere?
12:31 < Lightsword> I think they can be, will give it a try
12:31 < waxwing> yes i remember now, i tested it this morning but only wrote it in my notes, not the PR
12:41 < Lightsword> pushed some cleanup changes just now
13:25 -!- lnostdal [~lnostdal@77.70.119.51] has quit [Quit: https://www.Quanto.ga/]     
13:28 < undeath> waxwing | so, what's the story with outputs with no address, in bitcoin rpc?
13:28 < undeath> it doesn't really matter if we need it if it can be used to crash our application :P
13:30 < undeath> so yes, if a tx is used that has script outputs which cannot be encoded as address it will likely crash a maker
13:31 < undeath> i'm revisiting the idea of creating an exception wrapper for the twisted commands
13:31 < undeath> i think i found a way to do it
13:34 -!- lnostdal [~lnostdal@77.70.119.51] has joined #joinmarket     
13:49 < waxwing> undeath, i just meant if you look at the code in master (i linked above) it checks equality of the ['address'] entry; so i guess the question is whether that entry exists in the dict or not. i presumed it did, i can check.
13:52 < undeath> bitcoind can return an empty list in which case blockchaininterface.py would cause an IndexError exception
13:54 < waxwing> return an empty list where?
13:56 < undeath> for the 'addresses' key in the result of the 'gettxout' rpc call
13:57 < waxwing> well in that case, yep, query_utxo_set indexerrors
14:00 < undeath> why do some twisted commands always return {'accepted': True} and others use True or False?
14:02 < waxwing> that was definitely something i was unsure about; is the 'accepted' value telling the sender that 'ok message received' or 'i actually accept this message'. (and i struggle to remember anyway). feel free to correct it if needed.
14:03 < waxwing> finding it really hard to keep up right now (in case it wasn't obvious!). for like a year nobody looked at this code (and i was asking a lot). now there's like 5 massive changes going on at once :)
14:03 < undeath> well, I was kinda confused myself
14:03 < waxwing> in case it isn't clear, that isn't a complaint, it's more an excuse :)
14:04 < undeath> i'm writing an error wrapper right now and tried to simply return {'accepted': False} in error cases
14:04 < undeath> but for the specific method i "crashed" it caused joinmarket to exit
14:04 < undeath> even though the exception was properly caught
14:05 < undeath> so I changed it to {'accepted': True} and it worked as expected
14:05 < undeath> but I see other methods return {'accepted': False} in error cases
14:06 < undeath> so I have no idea how it's supposed to work
14:06 < undeath> sorry :)
14:06 < waxwing> i can see a couple of 'False' cases where it looks like a violation of the primitive state machine.
14:06 < waxwing> why apologise?
14:08 < waxwing> i can spend a bit of time on this query_utxo_set thing to reproduce the error, or are you doing that already undeath ?
14:08 -!- belcher [~belcher@unaffiliated/belcher] has joined #joinmarket     
14:08 < undeath> i'm writing the generic error handler so it doesn't matter
14:09 < undeath> if you want to fix that specific error I can do that later
14:09 < waxwing> ok.     
14:09 < undeath> in fact i'm only seeing one method that returns False
14:10 < undeath> which is on_JM_FILL_RESPONSE
14:10 < waxwing> no there's a couple more
14:11 < undeath> where?
14:11 < undeath> i only see commands in jmclient/client_protocol.py
14:11 < waxwing> it's like it says in checkClientResponse, any failure is considered critical, it's saying 'if you get a False we're in an inconsistent state, shut down
14:12 < undeath> ah, in daemon_protocol are others
14:12 < undeath> ok, so in case of "False" we stop reactor because something critical happened?
14:13 < undeath> or rather, we disconnect the client
14:13 < undeath> so it's more of the nature of some internal thing went totally wrong?
14:13 < waxwing> yes that seems to have been my thinking.
14:14 < undeath> ok, thanks
14:16 < undeath> so in case some external data triggers an error we probably always want to return True
14:16 < waxwing> yes, unless we change the logic of how we respond in the callback i guess.
14:16 < undeath> nah, don't let us get there :)
14:45 -!- GitHub161 [GitHub161@gateway/service/github.com/x-urtulypccyizvinp] has joined #joinmarket     
14:45 < GitHub161> [joinmarket-clientserver] undeath opened pull request #236: generic exception handler (master...prevent-maker-crash) https://git.io/fpE4r
14:45 -!- GitHub161 [GitHub161@gateway/service/github.com/x-urtulypccyizvinp] has left #joinmarket []     
14:48 < undeath> most of that code is actually documentation for once
15:07 -!- GitHub182 [GitHub182@gateway/service/github.com/x-gdzdjzdqepfkyvoq] has joined #joinmarket     
15:07 < GitHub182> [joinmarket-clientserver] undeath opened pull request #237: fix possible crash on tx with odd scripts (master...fix-odd-script-crash) https://git.io/fpEBs
15:07 -!- GitHub182 [GitHub182@gateway/service/github.com/x-gdzdjzdqepfkyvoq] has left #joinmarket []     
15:22 -!- rdymac [uid31665@gateway/web/irccloud.com/x-yvodzhsqwslaynxh] has quit [Quit: Connection closed for inactivity]     
16:36 -!- undeath [~undeath@hashcat/team/undeath] has quit [Quit: WeeChat 2.3]     
17:08 -!- AgoraRelay [~jmrelayfn@p54866A35.dip0.t-ipconnect.de] has quit [Ping timeout: 268 seconds]     
17:16 -!- AgoraRelay [~jmrelayfn@p5DE4AC1B.dip0.t-ipconnect.de] has joined #joinmarket