--- Day changed Tue Nov 27 2018
00:24 -!- midnightmagic [~midnightm@unaffiliated/midnightmagic] has quit [Ping timeout: 260 seconds]     
00:33 -!- midnightmagic [~midnightm@unaffiliated/midnightmagic] has joined #joinmarket     
00:40 < arubi> so cont.  I tried deliberately installing libsecp from source without ecdh and recovery, installed it system wide, then installed jm.  was still able to import jmbitcoin\coincurve from venv and functions like 'coincurve._libsecp256k1.lib.secp256k1_ecdh' seem to be defined
00:54 -!- undeath [~undeath@hashcat/team/undeath] has joined #joinmarket     
01:39 -!- undeath [~undeath@hashcat/team/undeath] has quit [Quit: WeeChat 2.3]     
02:11 < waxwing> does this line of thinking make any sense? let's say we want makers to be committed, we ask them to post a security bond (idea has been discussed before); problem is, that's a pretty icky privacy violation; you'd only want to use one such deposit, and it fingerprints you etc.
02:12 < waxwing> now, we know that the cryptonote-style ring sig allows you to generate a unique key image without revealing which ring member you are.
02:13 < waxwing> is there some variation on that idea that would allow a maker to post a 'key image' (or similar) for a utxo in such a way that it doesn't fingerprint them? i.e. they could post different ones at different times but you'd still recognize it? hmm can't seem to make sense of this.
02:13 < waxwing> and i also often forget when thinking about this: it's kinda blown out of the water by the fact that a utxo does not display the key involved, only the hash of the key :(
02:17 < waxwing> (btw cryptonote's "key image" concept is basically what we call here podle. more than that, but basically same concept)
02:19 < waxwing> i don't see any remotely feasible idea there, oh well.
02:24 < arubi> I think in the future we'll be seeing bare pubkeys in scripts rather than hashes (taproot etc.)
03:15 < waxwing> interesting point. also there's the whole zkpo-preimage thing. it's, i think getting pretty practical. but combining that with some kind of ring sig as well and .. well the whole set of properties i'm describing above doesn't look very feasible.
04:13 < belcher> the security bond fingerprints you the blockchain, but you could use joinmarket to mix the coins in and out of the bond
04:29 < waxwing> belcher, yeah, but meh. i don't think it'll sell.
04:30 < waxwing> could be wrong
04:58 -!- takamatsu [~takamatsu@unaffiliated/takamatsu] has quit [Ping timeout: 250 seconds]     
04:58 < waxwing> belcher, oh but the thing is, it's not so much about knowing someone's outputs as it is, if you want to keep using one security bond it fingerprints across sessions.
04:59 < waxwing> and unfortunately that's the bit that i have trouble imagining any crypto-shenanigans could solve.
04:59 < belcher> but its not linked with any eventual coinjoin transactions
04:59 < waxwing> yes sure
05:01 < waxwing> it's a weird grey area, we run makers for quite long periods, and we use some randomisation in the "privacy-enhanced" version ... i don't know how bad it is if you used say the same security deposit for several months, compared to how it is today.
05:01 < belcher> is DOSing by makers a problem then?
05:01 < belcher> iv done a lot of thinking about time-locked bonds for coinswap makers, because the two sides can easily DOS each other making them waste miner fees
05:02 < waxwing> you mean, if there's some punishment mechanism for misbehaviour?
05:03 < belcher> no i mean, what prompted you to think about bonds
05:10 < waxwing> belcher, oh; i'm just thinking about the whole 693 thing ... i know bonds don't really "solve" it but they would at least be something.
05:21 -!- takamatsu [~takamatsu@unaffiliated/takamatsu] has joined #joinmarket     
05:37 -!- takamatsu [~takamatsu@unaffiliated/takamatsu] has quit [Ping timeout: 250 seconds]     
05:43 -!- takamatsu [~takamatsu@unaffiliated/takamatsu] has joined #joinmarket     
05:55 < waxwing> so i tried to start #229 on my main JM setup, i did a reinstall but i'm getting an error from wallet-tool
05:55 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 272 seconds]     
05:56 < waxwing> line 849 of wallet_utils.py in get_wallet_cls : "No wallet implementation found for type 1"
05:56 < waxwing> Lightsword, undeath ^
06:00 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #joinmarket     
06:05 < qubenix> Is there any verification done on the packages pulled into the virtual env, or is joinmarket vulnerable to third party malware pulled in through pip? electrum is exploring this same question: https://github.com/spesmilo/electrum/issues/4874.
06:06 < waxwing> Lightsword, undeath that error is a result of this change, presumably just revert it: https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-142045c799b6f7c3a3b7b07358801a08R13
06:07 < waxwing> qubenix, i think the main thing we need to do is pin the dependencies. at least we don't have *too* many. see: https://github.com/JoinMarket-Org/joinmarket-clientserver/issues/193
06:07 < waxwing> feel free to propose other things, ofc
06:09 < qubenix> gotcha, of course i'll suggest any security enhancements i can think of.
06:16 -!- undeath [~undeath@hashcat/team/undeath] has joined #joinmarket     
06:38 < belcher> i had high hopes for using rust to write bitcoin projects but it seems the rust ecosystem has a similar thing like javascript where every project contains a thousand dependencies :\
06:44 < undeath> i guess that's a common problem with languages that don't have an extensive standard library
06:47 < belcher> c and c++ dont really have extensive standard libraries, above just the simple string manipulation stuff
06:49 < belcher> though i guess in c++ everyone uses boost and other really common libs
06:49 < belcher> i saw in js it seems to be common practice to have a dependency for a one-liner function
06:50 < undeath> yeah, it just happens that for c/c++ some larger libraries exist that solve a broad range of problem
06:50 -!- beIcher [~user@unaffiliated/belcher] has quit [Ping timeout: 246 seconds]     
06:57 < waxwing> does boost have leftpad though?
06:57 < waxwing> checkmate neckbeards
06:57 < undeath> lol     
06:57 < undeath> printf should have leftpad :P
06:59 -!- beIcher [~user@unaffiliated/belcher] has joined #joinmarket     
07:01 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 264 seconds]     
07:23 -!- belcher [~belcher@unaffiliated/belcher] has joined #joinmarket     
08:17 < arubi> printf /does/ have left pad :)
08:17 < arubi> ohh I just re-read the exchange, hehe
08:36 < waxwing> heh, in the pit:
08:36 < waxwing> <X-wanted> !swabsoffer 0 0000613 54444412c120 0 154 ~
08:36 < waxwing> <J5CXaao1qtqWijuT> !swreloffer 0 100000 114565112 0 0.00004 ~
08:36 < waxwing> <J5EFhUkDV1VhcZRR> !swabsoffer 0 100000 6573128376 100 300 ~
08:36 < waxwing> <X-wanted> ciao A11
08:36 < arubi> aw     
08:37 < waxwing> oh that's A11 not All
08:37 < undeath> it's not even a valid offer lol
08:37 < waxwing> right. people do that sometimes.
08:37 < waxwing> just manual fuzzing out of boredom i guess.
08:38 < arubi> undeath, just making sure if you saw my reply to you in #238
08:38 < undeath> btw, is it possible to send an invalid utxo in the podle commitment? that should make it possible to crash a maker
08:38 < undeath> arubi: ah, right. let me check
08:38 < arubi> thanks
08:39 < waxwing> undeath, why would it crash? we handle the case where query utxo returns None iirc
08:39 < waxwing> what we didn't handle is your 237 case, the non-standard script
08:39 < undeath> but we don't handle the case where utxo === "ggggg"
08:40 < undeath> arubi: that cmd returns no output
08:41 < arubi> now that is weird.  what's the size of the file?
08:41 < arubi> (the .so)
08:41 < belcher> they probably want to see what happens if they say so
08:41 < undeath> 64kb
08:41 < arubi> oof.  that can't be a correct libsec lib
08:42 < waxwing> undeath, i think you're right, you'll get an invalid int exception in query_utxo_set
08:42 < undeath> no, that one is the coincurve loader
08:42 < undeath> or a list index out of range or something like that
08:43 < waxwing> undeath, list index is very forgiving, txout[65:] on 'ggg' is just ''
08:43 < arubi> alright, invalid tx more important now.  when you have some time, pls check output of `objdump -T ....so`  (file is >500kb for me)
08:43 < undeath> ah, well, that's something :)
08:43 < waxwing> yes but we do int(txout[65:]) so no dice
08:43 < undeath> check objdump output for what?
08:43 < undeath> yes, exactly
08:44 < arubi> of the .so file
08:44 < undeath> but for what should I check the output?
08:45 < arubi> just want to see /what/ the output is
08:45 < arubi> the .so file is more than 500kb here so it being 64kb on your end is weird
08:45 < undeath> file format elf64-x86-64 and a bunch of DYNAMIC SYMBOL TABLE:
08:46 < undeath> again, the file you asked me to check is only the coincurve loader lib
08:46 < undeath> not secp256ki itself
08:46 < arubi> it still should contain a bunch of libsecp definitions
08:47 < undeath> yes, it does
08:47 < undeath> should I pastebin the whole output?
08:47 < arubi> so you're saying that it's fine that this same file is almost x10 larger on my end?
08:48 < arubi> yea, would help
08:48 < undeath> the coincurve loader is over 500kb for you?
08:48 < undeath> that's weird
08:48 < arubi> yes     
08:48 < undeath> static vs dynamic?
08:48 < undeath> mine is dynamic
08:48 < arubi> mine is as well
08:50 < undeath> https://gist.github.com/undeath/80d4885c093b3dca8d28517e6f7195b1
08:50 < arubi> all secp functions are undefined..  they should contain text
08:51 < undeath> is your lib dynamically linked against libsecp256k1?
08:51 < arubi> e.g. for me "0000000000014c80 g    DF .text  0000000000001170  Base        secp256k1_ecdh"
08:51 < undeath> because according to ldd mine is
08:51 < undeath> which then loads the system so
08:51 < arubi> mine isn't.  and I'm guessing you system's .so doesn't have ecdh
08:52 < undeath> exactly
08:52 < arubi> but I /do/ have a system .so that doesn't have ecdh (or recovery), and it didn't link against it
08:52 < undeath> that's weird
08:52 < arubi> do you have LD_LIBRARY_PATH set somewhere maybe?  or LDFLAGS?
08:53 < undeath> no     
08:54 < arubi> I'm lost.  does `grep IGNORE deps/coincurve-9.0.0/setup_support.py` return the patched-in 'if "_COINCURVE_IGNORE_SYSTEM_LIBSECP" in os.environ:' line ?
08:55 < undeath> yes     
08:55 < undeath> i'm pretty bad with gcc and linking stuff
08:56 < undeath> i have no idea why it would sometimes link statically and sometimes link dynamically to some lib
08:58 < arubi> depends on flags (and file availability),  can you change the shebang line in the install.sh script to '!#/bin/bash -x' re-run (choose to remove previous venv) and pastebin the output?
08:58 < undeath> k     
08:59 < arubi> oh it's going to be pretty long right, maybe remove the deps, jmvenv dirs manually and run while redirecting the output to a file
08:59 < undeath> running ./install.sh | tee install.log
09:00 < arubi> ah make sure to redirect 2>&1 after install.sh so the debug output is also in the file
09:00 < arubi> ./install.sh 2>&1 | tee ...
09:01 < undeath> heh, yes
09:01 < undeath> just noticesd
09:01 < undeath> well, here we go once again
09:03 < undeath> https://gist.github.com/undeath/d25ca6e617b3bd54144790283b880b33
09:08 < waxwing> looks good, no?
09:08 < undeath> well, yes, it looks good
09:08 < arubi> which OS are you using?  also it kinda looks like coincurve only builds the static libsecp... hm
09:08 < undeath> but i still don't get a working coincurve
09:08 < undeath> gentoo
09:10 < arubi> I'm rebuilding here.  wanna see if it builds the dynamic libs for me (I'm pretty sure it should)
09:12 < arubi> oh no it doesn't
09:12 < undeath> so there is no perceivable difference between our build logs?
09:13 < arubi> not that I can see no
09:13 < undeath> :/     
09:16 < arubi> do you have PKG_CONFIG_PATH set maybe?  
09:17 < arubi> I'll probably have to set up a gentoo docker (never used gentoo before)
09:18 < undeath> it's not really meant to be used on a quickly configured system
09:18 < undeath> not sure if there is a reliable docker container out there
09:19 < arubi> :(     
09:21 < undeath> oh, there does seem to exist a docker image
09:22 < arubi> I'll try it out.  will probably have to google all the commands for installing deps and such heh
09:22 < arubi> just like old times
09:23 < undeath> i can help you with that ;)
09:23 < arubi> awesome.  will ping if any issues
09:23 < undeath> but be aware that gentoo is a bit harder to use than most other distris
09:24 < arubi> trust me after having to use AIX and solaris at work, I'm happy that gentoo is linux at least :)
09:24 < undeath> lol     
09:25 < undeath> you can also install gentoo/bsd :D
09:25 < arubi> hahaha no thanks!
10:00 < arubi> 6bf7f7435857 / # vi
10:00 < arubi> bash: vi: command not found
10:00 < arubi> I can see this is going to be fun
10:00 < undeath> :D     
10:00 < undeath> nano may be installed
10:00 < arubi> yea there's nano..  savages :)
10:00 < undeath> emerge vim
10:00 < undeath> ;)     
10:00 < arubi> \o/  looks like it's going
10:16 -!- qubenix [~qubenix@66.172.11.228] has quit [Quit: quit]     
10:16 -!- qubenix [~qubenix@66.172.11.228] has joined #joinmarket     
10:20 < arubi> undeath, do you have libgmp headers installed?  just wanna know if I should install them or not
10:20 < undeath> i do
10:21 < arubi> alright, I don't see it emerge, I guess you installed from source?
10:21 < undeath> USE=gmp emerge libsecp256k1
10:21 < arubi> where do I put that?
10:21 < undeath> in your shell ;)
10:21 < arubi> oh sorry, dev-libs/gmp
10:22 < arubi> ahh I see :)
10:22 < arubi> for some reason I thought it was some .conf line
10:22 < arubi> emerge bash completion is really nice
10:23 < undeath> well, on a real system you would put that kind of stuff into a config file but for your purposes the env variables are enough
10:23 < arubi> cool.  I'm trying to document along the way.  would be nice to have a gentoo docker on travis
10:24 < undeath> heh, that's going to run for ages
10:24 < arubi> oh, well maybe if we put it on the first batch then it wouldn't affect the overall test times
10:25 < undeath> :D     
10:25 < arubi> (actually thinking about changing the order a bit, fedora\centos are taking longer and should start first prolly)
10:25 < arubi> well, on the first batch rather
10:25 -!- qubenix [~qubenix@66.172.11.228] has quit [Quit: quit]     
10:26 -!- qubenix [~qubenix@66.172.11.228] has joined #joinmarket     
11:31 -!- rdymac [uid31665@gateway/web/irccloud.com/x-xzuihnvuxaesxfpl] has quit [Quit: Connection closed for inactivity]     
11:35 < Lightsword> waxwing, reverted that
11:43 -!- GitHub41 [GitHub41@gateway/service/github.com/x-nmufblqtauwoopla] has joined #joinmarket     
11:43 < GitHub41> [joinmarket-clientserver] AdamISZ pushed 1 new commit to master: https://git.io/fpgBa
11:43 < GitHub41> joinmarket-clientserver/master 25ca6d2 AdamISZ: testing: allow auditing maker wallets in manual test
11:43 -!- GitHub41 [GitHub41@gateway/service/github.com/x-nmufblqtauwoopla] has left #joinmarket []     
11:49 < waxwing> Lightsword, thanks. waiting on undeath for that.
11:55 < waxwing> undeath, confirmed the crash bug you mentioned earlier; i can write a PR for it now, or would you like to do it
11:56 < undeath> if you fix that I have a bit more time for reviewing 229
11:56 < waxwing> ok doing it now
12:06 < waxwing> what's the "right" way (py3 compat included) of checking if a function arg is of a string type?
12:07 < waxwing> i don't technically need it for this fix but, just for the future
12:07 < undeath> well, ideally you do know that
12:08 < undeath> the function should only accept one thing and you should only call it with that
12:09 < undeath> besides that, isinstance(obj, (str, unicode)) would probably do the job
12:10 < waxwing> sure. but what other way (apart from that isinstance check) is there to enforce 'it should only accept one thing'?
12:11 < undeath> the most pythonic way would probably be hasattr(obj, encode)
12:11 < undeath> err, hasattr(obj, 'encode')
12:11 < Lightsword> the basestring check I think when using future
12:11 < Lightsword> that’s one of the areas things are tricky
12:12 < Lightsword> isinstance(obj, basestring)
12:13 < Lightsword> main issue is that differentiating between bytes and unicode is problematic until the entire codebase has been converted to py3 style
12:14 < undeath> Lightsword: does this return an int in py2 and py3? https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-c0cdf994c977bfa6e06d1a96bb41fb85R62
12:14 < Lightsword> I’m using the basestring check so that calls originating from jmclient still work
12:14 < Lightsword> undeath, should be int with future module I think
12:15 < Lightsword> it would normally return a string on py2 I think
12:15 < undeath> that means we could get rid of most struct.unpack('B', obj) calls?
12:15 < Lightsword> well…not sure…there’s some weirdness since jmclient isn’t converted yet
12:16 < undeath> well, either it works or it doesn't?
12:16 < undeath> would it work in some place and not in others?
12:16 < Lightsword> I think it sometimes depends on if the caller is converted or not
12:17 < Lightsword> basically there’s weirdness when passing py2 strings into converted functions that internally operate on unicode and bytes types
12:17 < undeath> those unpacks could be removed, right? https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-142045c799b6f7c3a3b7b07358801a08R131
12:19 < Lightsword> undeath, uh, think those are required
12:19 < undeath> why?
12:19 < undeath> those are explicitly set to bytes
12:21 < Lightsword> undeath, that’s within jmclient/jmclient/cryptoengine.py which hasn’t been converted to py3 style yet
12:22 < undeath> didn't you do that? you changed several things there
12:22 < undeath> and it was basically py3 style to begin with
12:22 < Lightsword> undeath, I minimally tweaked it to work with my jmbitcoin changes
12:23 < undeath> can you take a look at the file? i think that should work
12:24 < Lightsword> undeath, those aren’t bytes because I don’t have the future imports
12:24 < undeath> unicode_literals isn't enough for that?
12:24 < Lightsword> no, you need from builtins import *
12:24 < undeath> ah, I see
12:25 < Lightsword> unicode_literals means unprefixed strings are unicode
12:25 < undeath> ok, so it does nothing about bytes?
12:25 < Lightsword> if they are b prefixed without from builtins import * they are normal py2 strings
12:25 < undeath> ok     
12:25 < Lightsword> hence why I still need to convert them to integers
12:26 < undeath> or we just add the futures import and call it a day? :)
12:26 < Lightsword> ehhh…I was avoiding that…since adding those imports cause cascading issues
12:26 < undeath> I see
12:26 < Lightsword> and then you end up having to convert a lot more
12:27 < Lightsword> it’s really tricky to isolate the changes, that’s why it took me so long to convert jmbitcoin
12:27 < undeath> yeah, it really is a mess
12:27 < Lightsword> once I convert jmclient and everything else things should be easier
12:28 < Lightsword> the main issue is I can’t really rely on type checking because only part of the codebase understands bytes
12:29 -!- GitHub152 [GitHub152@gateway/service/github.com/x-qbufjodapipyyqrp] has joined #joinmarket     
12:29 < GitHub152> [joinmarket-clientserver] AdamISZ opened pull request #239: Handle invalid utxos in query_utxo_set (master...handle-invalid-utxos) https://git.io/fpgzs
12:29 -!- GitHub152 [GitHub152@gateway/service/github.com/x-qbufjodapipyyqrp] has left #joinmarket []     
12:29 < Lightsword> that’s why there’s still all those hacky regexes
12:29 < Lightsword> once I convert jmclient I should be able to remove those
12:30 < Lightsword> and just check for bytes vs strings since the whole codebase would then be using backported py3 types
12:31 < Lightsword> anyways if we can get that merged today I’ll start on jmclient conversions
12:31 < undeath> looking forward to getting rid of those regexes :)
12:32 < Lightsword> yeah, they are only still there as a temporary hack to deal with the mixed py2 and py3 types
12:32 < Lightsword> I really hate the py2 type system in general, it just feels like it’s loosly typed vs py3’s strong types
12:36 < undeath> Lightsword: what was the error that sometimes caused the SerializationTruncationError exceptions?
12:37 < Lightsword> undeath, it was when I had accidentially created corrupt transactions during developoment
12:37 < undeath> but it was triggered by some test?
12:37 < Lightsword> just bugs in my serialize() I think
12:38 < undeath> do you remember which one?
12:38 < Lightsword> I think I fixed the bugs
12:38 < undeath> yes     
12:38 < waxwing> was it the little endian thing?
12:38 < undeath> but since it was there i'd like to watch for potentially similar problems
12:38 < Lightsword> don’t think it was endian
12:39 < undeath> no, the endianness was a different problem
12:39 < Lightsword> I think it was related to how I was handling the hash type detection
12:40 < Lightsword> the regexes were unreliable for some test inputs, using pure length detection worked better since hashes are always fixed length
12:40 < undeath> ok     
12:40 < undeath> regexes not working sounds weird
12:41 < Lightsword> it was due a typing issue when using py2 strings
12:41 < undeath> eh :/
12:42 < Lightsword> basically had input and output going back and forth between py2 and py3 style code
12:46 < Lightsword> that was a super tricky issue to isolate lol
12:49 < undeath> according to the python documentation, the fmt argument for pack/unpack should be a str
12:49 < undeath> https://docs.python.org/3/library/struct.html?highlight=struct#examples
12:50 < Lightsword> undeath, that actually had to be bytes
12:50 < Lightsword> or else it would break py2 compat
12:50 < Lightsword> on some systems
12:50 < Lightsword> I think centos/fedora
12:50 < undeath> are you sure?
12:50 < Lightsword> yeah, pretty sure, it’s due to unicode_literals
12:51 < Lightsword> basically py2 doesn’t accept unicode for format strings
12:51 < Lightsword> well some versions of py2 do
12:51 < Lightsword> some don't
12:51 < undeath> but is the code even py3 compatible then?
12:51 < undeath> even the future lib is not perfect it seems
12:52 < Lightsword> https://python-future.org/stdlib_incompatibilities.html#struct-pack
12:52 < Lightsword> undeath, I think py3 can accept bytes or unicode
12:52 < Lightsword> if it can’t I’ll deal with it then
12:52 < Lightsword> “This was fixed in Python 2.7.7. Since then, struct.pack() now also accepts unicode format strings."
12:52 < Lightsword> I think centos and fedora were older than 2.7.7
12:53 < undeath> damnit, update your crap :/
12:53 < undeath> thanks for the link
12:53 < Lightsword> there’s going to be a number of changes when I actually get it running on py3, there’s other workarounds I can do at that point
12:54 < Lightsword> my current changes have been mostly just to shrink the diff
12:56 < undeath> thanks for adding the docstring to the serialize() method
12:57 < Lightsword> heh, I think waxwing wrote that
12:57 < Lightsword> in his first py3 conversion attempt
12:57 < Lightsword> I was copying some stuff out of that
12:57 < undeath> i was spending quite some time on reversing how the txobj was structured when implementing the new wallet
12:57 < undeath> ah, I see
13:02 < undeath> just to make sure, is the endianness correct here? https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR154
13:02 < undeath> i don't think we have any test that covers transactions with >255 outputs
13:02 < Lightsword> should be
13:03 < Lightsword> everyting should be little endian
13:03 < Lightsword> everything*
13:04 < undeath> yes, looks ok
13:09 < undeath> shouldn't _something_ happen is neither of those conditions is true? https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR152
13:10 < undeath> an exception should be appropriate
13:12 < Lightsword> undeath, I think those will always be one or the other since it’s a fixed length read here https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR75
13:12 < undeath> well, it _should_ be
13:12 < Lightsword> kk, I’ll add an else which throws an exception
13:13 < undeath> ping waxwing ^
13:16 < undeath> there isn't really any other sanity checking, so maybe that's intentional it's not erroring out
13:17 < undeath> but it just feels wrong
13:17 < Lightsword> pushed
13:20 < Lightsword> undeath, yeah, probably best to be defensive and throw an error there, but I think the code would error on deserialization first
13:20 < undeath> we don't always deserialize what we later serialize ;)
13:20 < undeath> we are build transactions after all
13:20 < undeath> *building
13:20 < Lightsword> yeah, true, guess it helps for checking internal assumptions
13:21 < Lightsword> this will get refactored shortly though regardless
13:22 < undeath> shouldn't this be unsigned? https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR166
13:28 < Lightsword> undeath, it is unsigned, see here https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-14344ea98bee8b31eb425ab3ecdccc1aR280
13:29 < undeath> then it should use <Q
13:29 < undeath> not <q
13:29 < Lightsword> eh? it uses struct.pack(b'<Q', x)
13:30 < Lightsword> or were you linking to o.write(struct.pack(b'<q', out["value”]))?
13:30 < undeath> yes     
13:31 < undeath> same for https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR139
13:32 < Lightsword> undeath, I had copied that from here https://github.com/petertodd/python-bitcoinlib/blob/master/bitcoin/core/script.py#L1010
13:33 < Lightsword> it’s probably fine to interpret either way
13:34 < undeath> they also encode the lock time as signed
13:34 < Lightsword> I guess I should probably just change all to unsigned for consistancy
13:35 < undeath> only locktime from looking tha bitcoinlib
13:35 < undeath> assuming that is correct
13:36 < undeath> it seems a bit random when looking only at the code what is signed vs unsigned
13:36 < Lightsword> yeah, most of these are things that should never be negative but are technically signed
13:38 < waxwing> undeath, sorry what's the ping exactly?
13:39 < undeath> should serialize() raise an exception if it encounters a tx hash that is not 32 bytes long?
13:39 < undeath> and if not, what should it do?
13:45 < Lightsword> undeath, now it’s all unsigned
13:48 < waxwing> undeath, i can't see a problem with having it raise. Because: there is nowhere where we are receiving from outside the app, a deserialized tx and then serializing it (and i can't currently see how/why we would, ever). so i think raise is good there.
13:48 < waxwing> the one we have to be careful about is deserialize, i guess.
13:48 < undeath> ok     
13:51 < Lightsword> think everything is good to merge now?
13:53 < undeath> i haven't even reviewed half of the modification in secp256k1_transaction.py and not secp256k1_main.py at all
13:53 < Lightsword> kk     
13:56 < undeath> i think 'i' could be >255 https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/229/files#diff-590754ac316dfffc53883e4a7606680fR263
13:57 < undeath> but i'm not sure at all
14:01 < waxwing> undeath, good point i was just asking about that (although in relation to different code); indeed tx indices are not limited like that. or wait, i was asking about outputs; i presume inputs are a similar story, except in practice much more difficult to make that very large than outputs.
14:01 < waxwing> but yeah hundreds of inputs must be a thing, at least in theory.
14:02 < waxwing> now with segwit it doesn't blow up quadratically, which is nice :)
14:02 < undeath> heh     
14:02 < waxwing> oh that's actually non segwit code so, heh again :)
14:07 < undeath> that probably does not make it illegal in a technical sense to have i>255
14:07 < waxwing> def should support > 255 unless i missed something big, yeah.
14:09 < waxwing> undeath, did you mean to highlight line 263 or 265 (the latter is the input index)
14:09 < undeath> oh, the lines moved because of the additional commits
14:09 < Lightsword> so I should use num_to_var_int there?
14:10 < undeath> yes, it should be line 265 with the current status
14:12 < Lightsword> oh, I guess that would actually be more of a read_var_int right?
14:13 < waxwing> but wait, where would we be calling signature_form or segwit_signature form where the index wasn't an integer?
14:13 < Lightsword> hmm, going to test with it removed
14:14 < waxwing> in segwit_signature_form i think it just assumes it's an integer
14:14 < waxwing> yeah just the wallet is calling, and it's getting the index from a for index, blah in blah i think
14:15 < waxwing> well from a .items() rather, but anyway, integer
14:16 < Lightsword> yeah, test pass without that, so removing it
14:16 < undeath> waxwing: is this a bug in the current codebase? https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/jmbitcoin/jmbitcoin/secp256k1_transaction.py#L299-L302
14:17 < undeath> in the second case the bytes of the sha256 are reversed, in the first they are returned un-reversed
14:18 < Lightsword> isn’t only the integer reversed?
14:18 < Lightsword> the hashcode
14:18 < undeath> in the first case, yes
14:18 < waxwing> yes i see what you mean. would have to check, but it looks like a bug to me too.
14:20 < waxwing> wait, no, is it? heh i'll have to check up on that.
14:21 < waxwing> you little endianise the hash code and shove it at the end of the transaction. i remember this being a slightly weird aspect of bitcoin.
14:21 < undeath> but would you only reverse the checksum in one case and not the other?
14:21 < undeath> that seems wrong
14:23 < waxwing> there's no checksum here, but i get what you're saying.
14:23 < undeath> sorry, hash
14:27 < waxwing> hmm interesting i checked the history and it was always like that, going back to when it was pasted in from pybitcointools. but i should stop being lazy and actually figure out whether it's right or wrong :)
14:28 < undeath> :D     
14:33 < waxwing> undeath, i'm pretty sure i know what's going on
14:33 < waxwing> and the problem is the function is poorly documented
14:33 < waxwing> the thing is, txhashing for signing is not the same as txhashing for txid
14:33 < undeath> and this is only for signing?
14:33 < waxwing> so it's using hashcode=None (i.e. default) to signal that we're calculating a txid; all the signing functions call it with hashcode=SIGHASH_ALL
14:34 < waxwing> (although they can use non-defaults too)
14:34 < waxwing> that's why it's only reversed in one case and not the other. i may not be perfectly correct, but it's definitely along those lines. i vaguely remember going through all this.
14:35 < undeath> this is horrible
14:35 < waxwing> yes, it is. i remember thinking how horrible it was when i was doing segwit. it was already a bit tangly and i made it more so, sorry about that :)
14:36 < waxwing> still, lots of motivation to rewrite it eh? :)
14:36 < undeath> :P     
14:37 < waxwing> yes, just checked on stackexchange, with txids you just reverse the bytes at the end. it's actually pretty clear really, no?
14:38 < waxwing> :p     
14:45 < Lightsword> so I guess deal with that later then?
14:45 < waxwing> yeah; for now, as long as you replicate the behaviour (which i believe you do), that's fine
14:45 < undeath> it's correct it seems
14:46 < undeath> yes, his PR doing that in the same way
14:51 < Lightsword> anything still need review?
14:54 < undeath> i will certainly make a comment in the pr once i'm done
14:54 < Lightsword> kk     
15:03 < undeath> changebase isn't used anymore, is it?
15:04 < undeath> oh, it's still used in jmclient
15:08 < Lightsword> undeath, I think I can kill it off, one sec
15:11 < Lightsword> pushed
15:14 < undeath> nice
15:20 < undeath> is encode/decode still used?
15:20 < Lightsword> in a few places
15:20 < Lightsword> I plan to eventually eliminate those but not quite yet
15:21 < undeath> ok     
16:11 < undeath> ok, review done for now, left a comment in the pr
16:18 < Lightsword> undeath, deserialize accepts untrusted input right?
16:19 < undeath> yes, that's what waxwing said earlier
16:19 < undeath> waxwing | the one we have to be careful about is deserialize, i guess.
16:20 < waxwing> it currently catches an index error, only
16:21 < waxwing> https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/jmclient/jmclient/maker.py#L111-L114
16:22 < undeath> ok, that can easily be extended to add the other exception
16:22 < waxwing> that may have made sense in an earlier version of the code, but doesn't now
16:23 < undeath> what happens if you try to pass invalid hex there?
16:23 < undeath> 'a' for instance
16:23 < undeath> or 'gggg'
16:24 < undeath> (also valid question for all other code paths that receive untrusted hex)
16:25 < Lightsword> I can just adjust that to catch serialization errors as well
16:25 < undeath> that should do the job for deserialize
16:26 < waxwing> yeah that's the only place we are taking in an untrusted serialized transaction
16:27 < Lightsword> kk, about to push up the change
16:27 < waxwing> there was actually a facility that belcher put together to allow makers to pass transactions between themselves for broadcast, that was pretty nifty, but for obscure reasons i couldn't put it in jmcs to begin with, and never fixed it (yet)
16:28 < Lightsword> pushed
16:28 < Lightsword> that all look good for merge?
16:28 < waxwing> i'll do it tomorrow Lightsword. Let me run it a bit more.
16:28 < Lightsword> kk     
16:34 < waxwing> Lightsword, you could squash though now, that'd be good
16:38 -!- undeath [~undeath@hashcat/team/undeath] has quit [Quit: WeeChat 2.3]     
16:49 < Lightsword> kk     
17:06 -!- AgoraRelay [~jmrelayfn@p5DE4A872.dip0.t-ipconnect.de] has quit [Ping timeout: 268 seconds]     
17:16 -!- AgoraRelay [~jmrelayfn@p5DE4ACA3.dip0.t-ipconnect.de] has joined #joinmarket     
18:02 -!- AgoraRelay [~jmrelayfn@p5DE4ACA3.dip0.t-ipconnect.de] has quit [Ping timeout: 246 seconds]     
18:16 -!- AgoraRelay [~jmrelayfn@p5DE4A9AB.dip0.t-ipconnect.de] has joined #joinmarket     
19:25 -!- beIcher [~user@unaffiliated/belcher] has quit [Ping timeout: 250 seconds]     
19:25 -!- beIcher [~user@unaffiliated/belcher] has joined #joinmarket