--- Day changed Wed Jun 10 2020
00:14 -!- MasterdonX [~masterdon@103.39.132.190] has quit [Ping timeout: 265 seconds]     
00:17 -!- MasterdonX [~masterdon@66.115.175.41] has joined #bitcoin-core-pr-reviews     
00:55 -!- dr-orlovsky [~Dr_Orlovs@31.14.40.19] has joined #bitcoin-core-pr-reviews     
01:02 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
01:07 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 260 seconds]     
01:34 -!- slivera_ [~slivera@14-202-210-68.static.tpgi.com.au] has joined #bitcoin-core-pr-reviews     
01:36 -!- slivera__ [~slivera@103.231.88.26] has joined #bitcoin-core-pr-reviews     
01:37 -!- slivera [~slivera@103.231.88.10] has quit [Ping timeout: 258 seconds]     
01:38 -!- slivera_ [~slivera@14-202-210-68.static.tpgi.com.au] has quit [Ping timeout: 240 seconds]     
01:44 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
01:50 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 256 seconds]     
01:54 -!- peltre [sid268329@gateway/web/irccloud.com/x-fnyhlqregvovpeyf] has quit [Ping timeout: 256 seconds]     
01:55 -!- peltre [sid268329@gateway/web/irccloud.com/x-uilwmmlwtxvoszuv] has joined #bitcoin-core-pr-reviews     
02:31 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
02:40 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has joined #bitcoin-core-pr-reviews     
03:03 -!- Madie73Conroy [~Madie73Co@static.57.1.216.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews     
03:22 -!- Madie73Conroy [~Madie73Co@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 265 seconds]     
03:22 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Quit: ZNC - http://znc.sourceforge.net]     
03:24 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews     
04:08 -!- S3RK [~s3rk@47.246.66.112] has quit [Remote host closed the connection]     
04:32 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has quit [Ping timeout: 256 seconds]     
04:43 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has joined #bitcoin-core-pr-reviews     
04:50 -!- dr-orlovsky [~Dr_Orlovs@31.14.40.19] has quit [Ping timeout: 260 seconds]     
04:52 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
04:57 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 256 seconds]     
05:13 -!- KieraEnigma [52d08fc2@gateway/web/cgi-irc/kiwiirc.com/ip.82.208.143.194] has joined #bitcoin-core-pr-reviews     
05:18 -!- KieraEnigma [52d08fc2@gateway/web/cgi-irc/kiwiirc.com/ip.82.208.143.194] has quit [Quit: Connection closed]     
06:09 -!- slivera__ [~slivera@103.231.88.26] has quit [Remote host closed the connection]     
06:20 -!- troygiorshev [~troygiors@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews     
06:55 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has joined #bitcoin-core-pr-reviews     
07:23 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has quit [Ping timeout: 256 seconds]     
07:30 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
07:33 -!- seven_ [~seven@2a00:ee2:410c:1300:1d86:e4d9:cf41:9625] has joined #bitcoin-core-pr-reviews     
07:34 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 256 seconds]     
08:56 -!- lightlike [~lightlike@2a02:810d:b80:c2c:39dc:f255:f31e:6ac8] has joined #bitcoin-core-pr-reviews     
08:56 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has joined #bitcoin-core-pr-reviews     
08:57 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection]     
09:02 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews     
09:17 -!- Ravi_21M [6ad5aa39@106.213.170.57] has joined #bitcoin-core-pr-reviews     
09:23 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds]     
09:25 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews     
09:27 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews     
09:40 -!- figs [592e3e56@89.46.62.86] has joined #bitcoin-core-pr-reviews     
09:43 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews     
09:44 < jnewbery> we'll get started in 15 minutes
09:45 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews     
09:46 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Remote host closed the connection]     
09:53 -!- gimballock [b8a4b1b9@d-184-164-177-185.fl.cpe.atlanticbb.net] has joined #bitcoin-core-pr-reviews     
09:55 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews     
09:56 -!- SirRichard [~SirRichar@cpe-24-29-169-110.cinci.res.rr.com] has joined #bitcoin-core-pr-reviews     
09:57 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
10:00 < fjahr> #startmeeting
10:00 < fjahr> Hello everyone, welcome to this weeks PR review club. We are reviewing the Python implementation of MuHash3072 (#19105) which part of CoinStatsIndex (overview in #18000). Notes and questions are here: https://bitcoincore.reviews/19105.html.
10:00 < fjahr> I am drawing some comparisons to the C++ implementation in the questions but the focus of this session should definitely be on understanding how the algorithm works in the Python implementation. If you have any questions don't hesitate to ask, there can be multiple discussions happening in parallel.
10:00 < fjahr> feel free to say hi :)
10:00 < jnewbery> hi     
10:00 < kanzure> hi     
10:00 < troygiorshev> hi     
10:00 < gimballock> Hi     
10:00 < andrewtoth> hi     
10:00 < lightlike> hi     
10:00 < michaelfolkson> hi     
10:00 < raj_149> hi     
10:00 < vindard> hi     
10:00 < raj_149> hi     
10:01 < fjahr> who had a chance to review the PR? (y/n)
10:01 -!- MM77788811 [~MM7778881@195.206.104.107] has joined #bitcoin-core-pr-reviews     
10:01 < provoostenator> hi     
10:01 < Ravi_21M> hi     
10:01 < provoostenator> y     
10:01 < jnewbery> y     
10:01 < elichai2> hi     
10:01 < raj_149> y     
10:01 < emzy> HI     
10:01 < MM77788811> Hi     
10:01 < emzy> n     
10:01 < vindard> y     
10:01 < troygiorshev> n     
10:01 < lightlike> y     
10:01 < andrewtoth> n     
10:01 < nehan> hi     
10:01 < nehan> y     
10:02 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 265 seconds]     
10:02 < fjahr> Great. There were already some nice review comments. Let's get started with a warm-up question: What is the API of MuHash3072 in Python? Is it different from the C++ implementation in PR 19055?
10:02 < sipa> hi, y
10:02 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has quit [Quit: Leaving]     
10:03 < raj_149> insert, remove and digest?
10:04 -!- compress [~semicolon@178.162.222.42.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews     
10:04 < elichai2> that it doesn't overload the `/=` and `*=` operators and instead use named functions?
10:04 < fjahr> raj_149: yes, how about the constructors?
10:04 < fjahr> elichai2: yepp, that as well
10:05 -!- Smeier [8da18524@141.161.133.36] has joined #bitcoin-core-pr-reviews     
10:06 < sipa> look at the inputs to those insert/remove/*=//=
10:07 -!- gzhao408 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
10:08 < elichai2> That the C++ API accepts MuHash3072 objects and the python accepts raw data and then turn it into a MuHash3072?
10:08 -!- anom2084 [965000000@2405:8100:8000:5ca1::29c:4157] has joined #bitcoin-core-pr-reviews     
10:09 < fjahr> Yepp. And there is an empty default constructor in C++ which we don't have in Python.
10:09 < fjahr> Next Q: There is a numerator and a denominator property in the Python implementation. Can you explain how these are used in MuHash on a high level? Where are they in the C++ implementation?
10:09 < elichai2> isn't this an empty constructor though? https://github.com/bitcoin/bitcoin/blob/a6ee4c2ceee624d1d3ed1dfa4bd6f259139bb9d8/test/functional/test_framework/muhash.py#L63
10:10 < nehan> to keep running products in the numerator/denominator because inversing things is expensive. so inverse once at the end.
10:10 < fjahr> elichai2: Oh, ah, true :D
10:10 -!- anom2084 [965000000@2405:8100:8000:5ca1::29c:4157] has quit [Remote host closed the connection]     
10:11 < lightlike> numerator to add elements to the set, denominator to remove elements from the set
10:11 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has quit [Ping timeout: 258 seconds]     
10:11 < fjahr> nehan: lightlike: right, did you compare it to the C++ code?
10:12 < raj_149> fjahr: there isn't one in c++ it seems?
10:12 < troygiorshev> It looks like the c++ code doesn't current take advantage of this.  It computes the inverse every time we want to remove an element
10:12 < nehan> yes but i don't see where there are numerators/denominators in the C++
10:13 < sipa> the MuHash3072 object in the C++ code is lower level; the numerator/denominator is something higher-level code can do if it needs it
10:13 < elichai2> LOL. I only now realized that 386BYTES == 3072bits. the "hash384" confused me to think it uses some 384bits hash(sha384 or something hehe)
10:13 < sipa> one reason is that you may want to cache MuHash3072 objects for running hashes or so, after the inversion (so it's only 384 bytes and not 768)
10:14 < sipa> elichai2: no, 3072/8 = 384
10:14  * elichai2 🤦
10:14 < elichai2> ops yeah 384 not 386, typo
10:14 < troygiorshev> sipa: ah makes sense
10:14 < lightlike> yes, that took me also more time than it should have :-)
10:15 < sipa> but if that's not going to happen, perhaps the numerator/denominator (and also the SHA256'ing) should be integrated into MuHash3072 rather than forcing higher-level code to take care of that
10:15 < jnewbery> sipa: I haven't looked at the c++ code. When you say caching, do you mean that you're saving the 384 byte chacha digest for each block?
10:16 < sipa> jnewbery: i don't know?
10:16 < sipa> it depends on the use case
10:17 -!- MM77788811 [~MM7778881@195.206.104.107] has quit []     
10:17 < sipa> one could be that you'd precompute the muhash "effect" of every transaction in the mempool for example
10:17 < jnewbery> what do you mean by 'cache muhash 3072 objects for running hashes or so'?
10:17 < provoostenator> I think fjahr said in the C++ PR that only the sha256 hash is stored in an index
10:17 < jnewbery> ah ok
10:17 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has joined #bitcoin-core-pr-reviews     
10:17 < sipa> or per block, if you want to be able to roll forward/backward from that block
10:17 < provoostenator> See discussion here-ish: https://github.com/bitcoin/bitcoin/pull/19055#issuecomment-641996489
10:17 < fjahr> provoostenator: this is something different
10:18 < jnewbery> and currently the c++ implementation inverts every utxo spend rather than multiplying them all and then inverting once?
10:18 < sipa> jnewbery: iirc fjahr's code does one inversion per block
10:19 < fjahr> it's on the calculation for each block where I add new utxos and removed utxos separately and then do one division att the end
10:19 < fjahr> sipa: yes
10:19 < jnewbery> sipa fjahr: thanks
10:19 < fjahr> provoostenator: you asked about this in your first pass on the index as well
10:20 < sipa> as it is currently used it would be perfectly reasonable to have the numerator/denominator inside the MuHash3072 object - but if there was ever a desire to cache a full MuHash3072 object in a place where memory usage matters, this may be undesirable
10:21 < nehan> This is where the division is done per block: https://github.com/bitcoin/bitcoin/pull/18000/commits/1502a08b4a7ea8987271e87ad73715c565a916fc#diff-56c85843f4edd7acc62fbf7af80a67d1R197
10:21 < fjahr> nehan: thank you!
10:22 < fjahr> yeah, so instead of removing each undo instead they are added up and then removed together
10:23 < troygiorshev> sipa: IIRC one of the other incremental hashes had a much smaller state
10:23 < sipa> troygiorshev: ECMH has 32 bytes state
10:23 < troygiorshev> ECMH maybe
10:23 < sipa> well, 33
10:24 < elichai2> in storage or in memory? :P
10:24 < sipa> it's complicated
10:25 < elichai2> sipa: I asked troygiorshev hehe
10:25 < raj_149> are we planning on storing the the rolling hash to disk?
10:25 < fjahr> Maybe next question but keep going if the last part is still unclear to anyone! ChaCha20 is used in MuHash. What are the properties of that algorithm? What is ChaCha20 used for in particular in MuHash?
10:25 < sipa> in compressed form, 33 bytes; in affine coordinates, 64 bytes; in jacobian coordinates, 96 bytes; in denormalized jacobian representation, 120 bytes :p
10:26 < elichai2> :D     
10:26 < elichai2> still less than 384 bytes though 😅
10:26 < jnewbery> sipa elichai2: I think we're gettin a bit offtopic!
10:26 < sipa> sorry.
10:27 < raj_149> fjahr: its converting 32 bytes to 384 bytes firstly. Is there any other purpose its serving?
10:28 < fjahr> raj_149: yes, but why?
10:28 < raj_149> fjahr: because muhash numerator/denominator needs to be a 384 byte data?
10:29 < sipa> perhaps a useful question is: why not use ChaCha20 directly, without SHA256?
10:29 < jnewbery> sipa's original post here might be helpful: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-May/014337.html
10:30 -!- gzhao408 [49fcfb03@c-73-252-251-3.hsd1.ca.comcast.net] has quit [Remote host closed the connection]     
10:30 < jnewbery> Spcifically "As a result, MuHash modulo a sufficiently large safe prime is provably secure under the DL assumption. Common guidelines on security parameters [7] say that 3072-bit DL has about 128 bits of security."
10:31 < nehan> i think you need at least 3072 bits of security. 384 bytes is less than 512 bytes.
10:32 < raj_149> sipa: i am a little confused here, where exactly we are mixing chacha20 with sha256?
10:32 < provoostenator> Also if you transform 256 bits to 3072 bits, do you actually get 3072 bits of security? That always give me headaches.
10:32 < nehan> so if you were willing to use MuHash on 4096 bit integers you wouldn't need chacha
10:32 < sipa> nehan: i think you're confused, but i can't say where
10:32 < nehan> sipa: yeah probably :) i'm guessing
10:32 < sipa> nehan: maybe you're confusing bits with bytes?
10:32 < provoostenator> MuHash works with 256 bit blocks
10:33 < provoostenator> And doesn't compress
10:33 < sipa> what?
10:33 < provoostenator> Uhhh
10:33 < sipa> it deals with 3072 bit integers
10:33 < provoostenator> ChaCha
10:33 < jnewbery> raj_149: sha256 is used twice. We first hash the object using sha256 to a 32 byte digest, and then use that digest as the key in the chacha20 rounds
10:33 < lightlike> is there an attack scenario wrt the specific use case of UTXO statistics that we need to be secure against?
10:33 < nehan> My guess is that you *could* use MuHash with 4096 bit integers instead of 3072, but it would require extra storage space
10:34 < jnewbery> sha256 is then used once there's a muhash output for the entire set to reduce it from 384 bytes back down to 32 bytes
10:34 < sipa> nehan: and slower multiplication
10:34 < provoostenator> lightlike: not really, but if we ever want to use it for consensus stuff - e.g. UTXO commitments, then we do have to worry
10:34 < sipa> raj_149: the UTXO is hashed first with SHA256 (formerly truncated SHA512) to produce 32 bytes; those 32 bytes are expanded to 384 bytes using ChaCha20; those 384 bytes are interpreted as 3072-bit integers and multiplied together; the result of that multiplication is SHA256'd again to produce a 32-byte hash
10:34 < nehan> sipa: I also thought when you said "perhaps a useful question is: why not use ChaCha20 directly, without SHA256?" you meant SHA512.
10:35 < sipa> nehan: the SHA512 was replaced with SHA256
10:35 < sipa> after some benchmarking
10:35 < fjahr> sipa: will be :)
10:35 < sipa> ah     
10:35 < nehan> it's still 512 here: https://github.com/bitcoin/bitcoin/pull/19105/commits/400ac1cc9e7fb2983d4dced9512819c970ef9638#diff-832874419bea3a4c22ea70b000657e3eR55
10:35 < fjahr> didn't get around to push the new code
10:35 < jnewbery> ignore truncated SHA512 and just pretend we're saying sha256 :)
10:35 < sipa> sorry for confusing people, i should have checked
10:36 < troygiorshev> sipa: in you're question are you referring to the first usage of sha256 or the second?
10:36 < raj_149> sipa: just so that i understand it right, this is currently not the case with the python code right?
10:36 < jnewbery> raj_149: yes, this is the case with the python code
10:36 < sipa> ignore everything i said about SHA256; it's SHA512 for now
10:37 < raj_149> jnewbery: isn't chacha here already expecting a 32 byte data? its not hashing it anywhere.
10:37 < nehan> ok so ChaCha20 is a way of securely blowing up 256 bytes to 384 bytes
10:37 < fjahr> but trunkated to 256bits, which adds to the confusion
10:37 < troygiorshev> nehan: 32 to 384 yeah
10:37 < nehan> troygiorshe: oh right, thanks
10:37 < troygiorshev> nehan: or 256 to 3072 bits, whichever you'd prefer :)
10:38 < sipa> nehan: 256 *bits* to 384 *bytes*
10:38 < nehan> 32->384. that was my confusion :)
10:38 < sipa> yeah
10:38 < jnewbery> the data parameter in data_to_num3072 here: https://github.com/bitcoin/bitcoin/pull/19105/files#diff-832874419bea3a4c22ea70b000657e3eR53 should already by a 32 byte digest
10:38 < nehan> or I guess what the Python code says now is 64 truncated to 32 to 384
10:38 < sipa> the answer is just that ChaCha20 isn't a hash algorithm; it's an encryption algorithm, and it needs a random key to be secure (which we produce by hashing); it's actually encrypting all zeroes
10:39 < michaelfolkson> What was the motivation for using truncated SHA512 over SHA256?
10:39 < troygiorshev> 64 bit processors, right?
10:39 < sipa> michaelfolkson: this was discussed in the previous meeting on this topic, i think
10:39 < raj_149> jnewbery: ok got it, i thought its somewhere enforced in the function itself.
10:40 < sipa> but given that we're changing it to SHA256 maybe it isn't worth rehashing (haha)
10:40 < fjahr> discusion: https://github.com/bitcoin/bitcoin/pull/19055#issuecomment-640169988
10:40 < fjahr> michaelfolkson: ^
10:40 < troygiorshev> fjahr: thx
10:40 < michaelfolkson> Thanks
10:40 < jnewbery> I don't think it's worth discussing truncatedSHA512 -vs- SHA256 here. Just imagine it's a random oracle giving you a 32 byte output.
10:41 < fjahr> Ok, another question that was raised in an early review: What are your thoughts theStack’s suggestion to use Fermat’s little theorem in the code and the trade-offs involved?
10:41 < thomasb06> what are the ranges of 'a' and 'n-2'?
10:41 < andrewtoth> what about chacha20 making 384 bytes out of 32? Does it get any other inputs? If it's deterministic, won't the 384 bytes have the same security as 32 bytes?
10:42 < sipa> andrewtoth: they have 256 bits of entropy
10:42 < troygiorshev> fjahr: I agree with sipa's comment, saying "for those who understand this stuff, it's just as readable either way, and it's black magic for anyone who isn't familiar"
10:42 < nehan> fjahr: how often is modinv actually getting called? it seemed like once per digest, right? and the test only calls digest a few times.
10:42 < raj_149> fjahr: similar to stack's observation, i am getting one order of mag faster execution with fermet's theorem.
10:43 < nehan> raj_149: i think fermat's little theorem is supposed to be *slower*...
10:43 < sipa> raj_149: that's unexpected!
10:43 < jnewbery> I don't think performance matter too much in the test framework
10:43 < sipa> i was expecting the extgcd approach to be faster
10:43 < sipa> but agree that performance shouldn't matter
10:44 < nehan> i think it's less likely python's pow() will have a bug than this custom implementation of modinv so i'd go with pow :)
10:44 < thomasb06> if it doesn't overflow too
10:44 < troygiorshev> fwiw my testing agrees with TheStack
10:44 < raj_149> nehan: right, opposite, slower.. my bad..
10:44 < fjahr> nehan: yes but I am adding a more extensive test where it get's called more often. But in general I was just thinking of how we already are waiting for the ci environment. But it does not have too much of an impact, not comparable to a unnecessary sleep() or so
10:45 < nehan> i was thinking if you keep modinv you should add a unittest to ensure it's producing the same results as pow()
10:45 < sipa> nehan: that's a good argument - though it's the kind of algorithm that's unlikely to be wrong in a very small subset of cases
10:45 < jnewbery> one nice thing about the python implementation is that it's more accessible and readable for most people. I think key.py does a great job at documenting the algorithms: https://github.com/bitcoin/bitcoin/blob/6762a627ecb89ba8d4ed81a049a5d802e6dd75c2/test/functional/test_framework/key.py#L13
10:46 < fjahr> jnewbery: yeah, I think it's a good idea to use that one
10:46 < jnewbery> so having the same algorithm used as in the c++ implementation is useful because it acts as a teaching aid for people looking at the code
10:46 < nehan> jnewbery: good point!
10:46 < compress> jnewbery agreed
10:46 < raj_149> jnewbery: +1
10:46 < sipa> jnewbery: alternatively... a different algorithm in the tests means less likely that there's a bug repeated in both
10:47 < michaelfolkson> 2 tests
10:47 < jnewbery> (an argument against is that you want a completely different algorithm so you don't replicate a bug in the test code)
10:47 < jnewbery> sipa: +1
10:48 < fjahr> on a higher level we are comparing to the results of the c++ code so a incorrect modinv should pop up
10:48 < compress> so its a trade off of security/soundness vs readability / tech debt?
10:48 < nehan> so have multiple algorithms in the test and compare them
10:48 < fjahr> this brings me to my last question: How do you like the test to verify parity with the unit test of the C++ implementation? Would have done it differently? Should it be extended? Should it be moved to a different place?
10:49 < troygiorshev> are we really worried at all that modinv has been implemented incorrectly?  For something so standard, if we're worried that it's wrong we should also be worried about many other things
10:49 < michaelfolkson> I did want to understand what was going on in this PR that jnewbery referred to https://github.com/bitcoin/bitcoin/pull/18576
10:50 < michaelfolkson> We should be putting related unit tests in functional tests from now on?
10:50 < provoostenator> troygiorshev: I'm not worried modinv is implemented incorrectly in the Python library, but might be in our implementation. (though in this case I tested the result is the same)
10:50 < michaelfolkson> Is that the motivation?
10:52 < fjahr> michaelfolkson: not in the functional tests but into files of the functional test framework
10:52 < sipa> provoostenator: modinv isn't implemented in the Python library before 3.8 i think
10:52 < troygiorshev> fjahr: this seems like a great opportunity for fuzz testing
10:52 < elichai2> Maybe writing a list of test vectors can be nice
10:52 < lightlike> fjahr: it doesn't seem in the right place, because stuff tested in a functional tests should depend on a running node in some way - so I like jnewbery's suggestion to have python unit test.
10:52 < sipa> fuzz testing cryptographic code is extremely hard
10:52 < provoostenator> sipa: correct, I would just wait for that, and put a TODO until then
10:53 < fjahr> trygiorshev: yes, provoostenator has pointed this out as well
10:53 < troygiorshev> sipa: Ah I'm using the word too flexibly.  I just mean giving both the same random inputs
10:53 < troygiorshev> would only check for parity
10:54 < provoostenator> One thing you can do, if you have hundreds of test vectors, is to run a tiny percentage through the slower algoritm. But we only have 1.
10:54 < michaelfolkson> fjahr: And the motivation for this was that tests were unreliable? What was happening because the unit tests weren't in the files of the functional test framework?
10:54 < jnewbery> it's a shame that sipa wrote the c++ and python implementation. Ideally the python implemenation would be written by someone who isn't sipa and has never talked to sipa about muhash :)
10:55 < sipa> i volunteer jnewbery to rewrite it :)
10:55 < michaelfolkson> I need to get to understand the test framework better, it is regularly tripping me up
10:55 < sipa> (you're right)
10:55 < thomasb06> jnewbery: sipa: maybe one day...
10:56 < jnewbery> too late. I've already read your implementation so I can't write one that is uninfluenced by you
10:56  * troygiorshev scrolls up to see who responded "n" to "reviewed?"
10:56 < fjahr> michaelfolkson: it's a better way to do this style of testing of the the python implementations of stuff, its really a unit test and feels weird in the functional test files
10:57 < sipa> i think this is actually one part where MuHash has an advantage... apart from gluing SHA256/ChaCha20 together, the code is trivial in python (it's multiplying and dividing numbers modulo a prime)
10:57 < fjahr> I have added a proper functional test in a later part of the series now: https://github.com/bitcoin/bitcoin/pull/19145/commits/6013f04f319188ddf4926142291ced3242817e95
10:57 < provoostenator> Agreed, the Python version is very nice and short. Probably the first time I could wrap my head around it.
10:58 < fjahr> sipa: agree
10:58 < provoostenator> If you chuck the ChaCha20 and ModInv stuff in another file, it's even better
10:58 < fjahr> ok, one more minute
10:59 < jnewbery> I know it's very late in the meeting, but I thought real_or_random's comment here was very interesting: https://github.com/bitcoin/bitcoin/pull/19055#discussion_r438026185
10:59 < jnewbery> I haven't fully digested it, but it's something I plan to dig into
10:59 < provoostenator> Haha, digested
10:59 < nehan> oh -- i found the use of "set" confusing given that you can add the same value multiple times (at least this is what i recall from some other discussion) and get a different hash
11:00 < provoostenator> nehan: that confused me too. It's not a set. Unless you treat it very carefully.
11:00 < fjahr> yeah, it's a bit confusing because what you can do =/= what you should do
11:01 < sipa> jnewbery: haha, digested
11:01 < fjahr> I will improve docs certainly :)
11:01 < fjahr> #endmeeting
11:01 < nehan> thanks fjahr!
11:01 < andrewtoth> thanks fjahr!
11:01 < fjahr> went by way too fast again. Thanks everyone!
11:01 < troygiorshev> thanks fjakr!
11:01 < fjahr> r     
11:02 < troygiorshev> fjahr*
11:02 < jnewbery> thanks fjahr. Thanks sipa. Really interesting discussion
11:02 < fjahr> I really appreciate the feedback!
11:02 < emzy> thanks fjahr!
11:02 < lightlike> thank fjahr
11:02 < michaelfolkson> Time flies when you are having fun fjahr. Nice work
11:02 < compress> thanks fjahr
11:02 < fjahr> Thanks sipa!
11:02 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Quit: Leaving]     
11:02 < sipa> fjahr: yw, and thanks for hosting again
11:02 < figs> thanks everyone
11:02 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has quit [Ping timeout: 260 seconds]     
11:03 -!- gimballock [b8a4b1b9@d-184-164-177-185.fl.cpe.atlanticbb.net] has quit [Remote host closed the connection]     
11:04 -!- figs [592e3e56@89.46.62.86] has left #bitcoin-core-pr-reviews []     
11:06 < SirRichard> Thanks everyone, great discussion. Great to learn and follow along.
11:06 -!- SirRichard [~SirRichar@cpe-24-29-169-110.cinci.res.rr.com] has quit [Quit: SirRichard]     
11:09 -!- Ravi_21M [6ad5aa39@106.213.170.57] has quit [Remote host closed the connection]     
11:09 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has joined #bitcoin-core-pr-reviews     
11:12 -!- real_or_random [~real_or_r@173.249.7.254] has joined #bitcoin-core-pr-reviews     
11:17 -!- Smeier [8da18524@141.161.133.36] has quit [Ping timeout: 245 seconds]     
11:47 -!- lightlike [~lightlike@2a02:810d:b80:c2c:39dc:f255:f31e:6ac8] has quit [Quit: Leaving]     
11:56 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: ZNC 1.7.5 - https://znc.in]     
11:56 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews     
12:15 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!]     
13:36 -!- compress [~semicolon@178.162.222.42.adsl.inet-telecom.org] has quit [Ping timeout: 265 seconds]     
13:50 -!- troygiorshev [~troygiors@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Ping timeout: 246 seconds]     
14:20 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Remote host closed the connection]     
15:18 < jnewbery> The log for today's meeting is up: https://bitcoincore.reviews/19105.html and next week's notes and questions are also up: https://bitcoincore.reviews/18242.html
15:18 < jnewbery> it's quite a big PR next week, so you may want to give yourselves a bit more time to prepare
15:26 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
15:26 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has joined #bitcoin-core-pr-reviews     
15:31 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 260 seconds]     
15:31 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has quit [Quit: Leaving]     
15:39 -!- rafalcpp [~racalcppp@ip-178-214.ists.pl] has quit [Ping timeout: 256 seconds]     
16:33 -!- seven_ [~seven@2a00:ee2:410c:1300:1d86:e4d9:cf41:9625] has quit [Remote host closed the connection]     
16:33 -!- seven_ [~seven@2a00:ee2:410c:1300:1d86:e4d9:cf41:9625] has joined #bitcoin-core-pr-reviews     
17:21 -!- slivera [~slivera@103.231.88.26] has joined #bitcoin-core-pr-reviews     
17:52 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
18:05 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has joined #bitcoin-core-pr-reviews     
18:06 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has quit [Remote host closed the connection]     
19:55 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 246 seconds]     
21:19 -!- slivera [~slivera@103.231.88.26] has quit [Remote host closed the connection]     
21:20 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews     
21:21 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
21:23 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds]     
21:23 -!- vasild_ is now known as vasild     
21:36 -!- shesek [~shesek@unaffiliated/shesek] has quit [Read error: Connection reset by peer]     
21:36 -!- shesek [~shesek@185.3.145.28] has joined #bitcoin-core-pr-reviews     
21:36 -!- shesek [~shesek@185.3.145.28] has quit [Changing host]     
21:36 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
22:03 -!- S3RK [~s3rk@47.246.66.112] has quit [Remote host closed the connection]     
22:03 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
22:08 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 256 seconds]     
22:37 -!- ethzero [sid396973@gateway/web/irccloud.com/x-vitlyldkeacshzvi] has quit [Read error: Connection reset by peer]     
22:37 -!- illlicit_ [uid109953@gateway/web/irccloud.com/x-trorwzkbxbpaczlp] has quit [Read error: Connection reset by peer]     
22:38 -!- ethzero [sid396973@gateway/web/irccloud.com/x-xipppnvwjiuakmqk] has joined #bitcoin-core-pr-reviews     
22:38 -!- illlicit_ [uid109953@gateway/web/irccloud.com/x-jzwusbrquqajznan] has joined #bitcoin-core-pr-reviews     
22:38 -!- digi_james [sid281632@gateway/web/irccloud.com/x-ixyargxtdibxiafd] has quit [Read error: Connection reset by peer]     
22:38 -!- digi_james [sid281632@gateway/web/irccloud.com/x-doxceqxpvlfgpgsj] has joined #bitcoin-core-pr-reviews     
23:34 -!- S3RK [~s3rk@47.246.66.112] has joined #bitcoin-core-pr-reviews     
23:39 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: ZNC 1.7.5 - https://znc.in]     
23:39 -!- S3RK [~s3rk@47.246.66.112] has quit [Ping timeout: 256 seconds]     
23:39 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews