--- Day changed Wed Jul 03 2019 00:11 -!- ccdle12 [~ccdle12@cpc139350-aztw33-2-0-cust310.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews 01:12 -!- setpill [~setpill@unaffiliated/setpill] has joined #bitcoin-core-pr-reviews 01:38 -!- michaelfolkson [~textual@46.233.77.216] has joined #bitcoin-core-pr-reviews 01:40 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 02:20 < jnewbery> michaelfolkson pointed out that the date on https://bitcoin-core-review-club.github.io/15443.html was wrong. It previously said July 5th. The review club is *today* as normal. I've corrected the page. 02:41 -!- michaelfolkson [~textual@46.233.77.216] has quit [Remote host closed the connection] 06:40 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-nueclfyykucknpyf] has joined #bitcoin-core-pr-reviews 07:37 -!- hebasto [~hebasto@95.164.65.194] has joined #bitcoin-core-pr-reviews 07:55 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews 08:23 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 08:29 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 08:57 -!- setpill [~setpill@unaffiliated/setpill] has quit [Quit: o/] 09:09 -!- MarcoFalke [~none@198.12.116.246] has quit [Read error: Connection reset by peer] 09:28 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving] 09:28 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews 09:29 -!- jonatack [d598a1a5@213.152.161.165] has joined #bitcoin-core-pr-reviews 09:50 -!- lightlike [~lightlike@2001:16b8:574c:a100:4479:f4b6:cda7:e67b] has joined #bitcoin-core-pr-reviews 09:51 < jnewbery> we'll get started in about 10 minutes 09:52 -!- JulioBarros [~juliobarr@97-115-0-127.ptld.qwest.net] has joined #bitcoin-core-pr-reviews 09:57 < sosthene> Hi all 09:58 < digi_james> Hello :) 10:00 < jnewbery> hi! 10:00 < lightlike> hi! 10:00 < ariard> hi! 10:00 < jonatack> hi! 10:00 < ccdle12> hi! 10:01 < jnewbery> whilst we give everyone a couple of minutes to get here, I suggest we all review the notes and questions at https://bitcoin-core-review-club.github.io/15443.html 10:01 -!- drbrule [~user@cpe-72-226-106-116.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 10:01 < amiti> hi! 10:02 < jnewbery> This week's PR is adding new tests. There are no changes to the C++ code. 10:02 < jnewbery> I don't think we've covered a test-only PR yet, which is a bit of an oversight, since they're a good place to start contributing 10:03 < jnewbery> I thought this one was interesting because it gives us a chance to look at output script descriptors, which are going to be important in the Bitcoin Core wallet 10:04 < jnewbery> Before we start with the *Questions* section of https://bitcoin-core-review-club.github.io/15443.html, did everything make sense in the *Notes*? Did anyone have any questions about those points? 10:04 -!- afigs [48224bde@72.34.75.222] has joined #bitcoin-core-pr-reviews 10:05 < jnewbery> ok, great. digi_james: I know you had some questions about solvability and spendability this week. Can you state what your question was and what you learned? 10:05 < sosthene> I wonder when the descriptors were added? I don't remember seeing it before upgrading to 0.18, but maybe I wasn't paying attention 10:06 < jnewbery> sosthene: look at the docs here: https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md . The first RPC to add support for descriptors was scantxoutset in 0.17 10:06 < digi_james> jnewbery: a descriptor is solvable if the output script and sigscript (minus signatures) can be derived from the output descriptor. 10:07 < digi_james> jnewbery: a descriptor is spendable if the signature can also be derived if the private key is also known. 10:08 < jnewbery> digi_james: that's right. So if the descriptor allows us to derive the scriptPubKey but not the scriptSig to spend it, then it's not solvable 10:08 < digi_james> So for example, a addr(P2SH_addr) is not solvable because the P2SH preimage is not known. 10:08 < jnewbery> exactly 10:09 < jnewbery> these definitions of solvable / spendable were originally used in the wallet 10:09 < jnewbery> ok, next question: What type of testing is used in Bitcoin Core? 10:10 < sosthene> unit and functional 10:11 -!- hugohn [~textual@c-73-223-69-29.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 10:11 < jnewbery> sosthene: yeah, that's mostly it. I believe there's also some fuzz testing and property-based testing (https://github.com/bitcoin/bitcoin/pull/12775) 10:11 < lightlike> I've also seen code for fuzzing. Is this used continually? 10:12 < jonatack> (plus a bit of linting in the CI and an optional serving of fuzzing that can use more data and examples) 10:12 < jnewbery> lightlike: I've never used fuzz testing myself. I'm pretty sure it's not used in the travis CI. 10:13 < drbrule> Is the testing described in the /doc folder? Wasn't sure if it's documented or just self-documented. 10:13 < sosthene> sorry can we explain what's fuzz testing? 10:13 < jnewbery> ok, so this PR adds functional tests. What are the advantages/disadvantages of that for this testing? 10:13 < jnewbery> ariard: mind taking the question about fuzz testing? 10:14 < fjahr> functional test are slow since they test the full stack 10:14 < ccdle12> jnewbury: functional tests simulate behaviour of an end user making rpc calls 10:14 < jonatack> Fuzzing: see doc/fuzzing.md 10:15 < jnewbery> jonatack: thanks 10:15 < ariard> sosthene: basically you throw millons of random-generated inputs into full-node components to discover crashs or bugs 10:15 < jnewbery> other test documentation: 10:15 < jnewbery> - unit - https://github.com/bitcoin/bitcoin/blob/master/src/test/README.md 10:15 < jnewbery> - functional/integration - https://github.com/bitcoin/bitcoin/blob/master/test/README.md 10:16 < ariard> it may be somehow semi-structurated to incrase bugs discovering success 10:16 < lightlike> also https://en.wikipedia.org/wiki/Fuzzing for a general overview 10:16 < drbrule> jnewbury: Thank you. 10:16 < ariard> s/structurated/structured 10:16 < jonatack> also: test/functional/README.md 10:17 < sosthene> ariard: got it, thanks 10:17 < jnewbery> fjahr: ccdle12: yeah, that's right. Functional tests spin up one or more full running instances of the bitcoind node, so they test the full functional behaviour, but are much slower to execute 10:18 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 10:18 < jnewbery> any thoughts about whether this functionality should be tested with unit or functional tests? 10:19 < fjahr> the rpc should be tested but especially the different success paths should be unit tests 10:19 < fjahr> as ccdle12 noted funcitonal is about the interaction of the user, so it should test for responding with the correct error messages etc. 10:20 < ccdle12> maybe unit for the core logic? the descriptors seem to be almost "standalone" meaning the rpc calls don't touch any other part of the codebase/affect other parts of the running daemon 10:20 < jnewbery> fjahr: that was my initial reaction too. Why is getdescriptorinfo a good candidate for unit tests? 10:21 < jonatack> it has few dependencies and can be tested in isolation 10:21 < hugohn> because it doesn't require external dependencies 10:21 < jnewbery> ccdle12: right. Because it's a pure utility, it doesn't need any setup with the rest of the node. 10:23 < jnewbery> I'm definitely not a concept NACK (some tests are better than no tests), but I think it's something to be aware of 10:23 < jnewbery> my feeling is that we rely on functional tests a bit too much in Bitcoin Core where sometimes unit tests might be more appropriate 10:23 < lightlike> jnewbery: but then we probably wouldn't test getdescriptorinfo directly, just stumbled today over a comment (https://github.com/bitcoin/bitcoin/blob/1381ddbcfcb6429b1327fd3db91ef97d8603aef9/src/test/setup_common.cpp#L74) stating that RPC tests should be moved to functional. 10:24 < jnewbery> oh, interesting 10:25 < hugohn> RE: disadvantages of functional/integration testing. If I might add, besides the fact that it's slow, IMO it might not be possible to have full 100% test coverage with functional/integration testing (unlike unit testing), because your functional tests usually test things in very specific orders and not all the possible ways the components can react. So it's helpful but, shouldn't be relied on exclusively. 10:25 < hugohn> *can interact 10:26 < jnewbery> lightlike: there's definitely room for both. I disagree that all RPC testing should be done using the functional test suite 10:26 < jnewbery> especially for pure utility functions like `getdescriptorinfo` 10:26 < jnewbery> but opinions vary! 10:27 < jnewbery> Any thoughts about Sjors comment here: https://github.com/bitcoin/bitcoin/pull/15443#pullrequestreview-205311643 ? 10:27 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 246 seconds] 10:27 -!- afigs [48224bde@72.34.75.222] has quit [Remote host closed the connection] 10:29 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 10:29 < jnewbery> ok, any other questions about the code itself? 10:29 < digi_james> I was confused about sjors comments. 10:30 < digi_james> descsum_create is part of the test framework, no? 10:30 < sosthene> do we have to use functional testing to test the output of a rpc call or can it be done with a unittest? 10:30 < lightlike> I haven't understood why does a (second) python implementation for checksum has to exist (and then needs to be tested). 10:30 < digi_james> I suppose we could pass the checksum as part of the input rather than generate it via a test function. 10:30 -!- hebasto [~hebasto@95.164.65.194] has quit [Remote host closed the connection] 10:31 < jnewbery> digi_james: yes, descsum_create is part of the functional test framework 10:32 < jnewbery> These tests call `getdescriptorinfo` to get the checksum from the bitcoind node, and also call descsum_create() to get the checksum from the python implementation 10:32 < jnewbery> checking that the two implementations of the checksum match is a good way to test 10:33 < jnewbery> so that's an argument for doing this as a functional test: it forces us to use implementations in two different languages 10:33 < digi_james> Why arent the checksums hardcoded in the test? 10:33 < digi_james> I see... 10:33 < jnewbery> digi_james: they could be, but having a python implementation that can be re-used makes it easier to add more tests in future 10:34 < jnewbery> but also, where would you get the hard-coded values from if you didn't implement the checksum in the test code? 10:34 < fjahr> So there are tests for descsum_create() as well? 10:34 < digi_james> jnewbery: yup that makes sense to me now ... 10:35 < jnewbery> fjahr: I don't think so. We generally don't have tests for the test code. 10:35 < jnewbery> The tests test the code and the code tests the test 10:35 < fjahr> so if we introduce logic errors in both implementations they would not be noticed 10:35 < hugohn> that's def an interesting approach. introduce new logic in the test code itself... 10:36 < hugohn> yeah... you're testing for consistency not necessarily accurary 10:36 < jnewbery> one way to test the test code is to somehow break the node implementation and verify that the test starts failing 10:37 < jonatack> fjahr: with test code I think we depend a lot on review 10:37 < jonatack> (Sjors comment seems good to me... I have a few general questions regarding writing/reviewing tests, when it's time) 10:37 < digi_james> I suppose Sjors is suggesting to test a test :) 10:37 < lightlike> jnewbery: but doesn't this reasoning apply to basically everything (e.g. all kind of cryptographic functions)? I'm not sure why it is worth reprogramming function in python just for testing purposes and then keep it in the codebase. 10:38 < lightlike> (as an initial proof of concept, of course) 10:38 < fjahr> I think having some hardcoded values to test against would be good to make regressions explicit 10:38 < jnewbery> digi_james: one way to test the test code would be to use python's unittest module 10:39 < jnewbery> but in most cases, I don't think that's worthwhile 10:39 < digi_james> :) 10:40 < jnewbery> lightlike: yes, I think it's useful to have python implementations of cryptographic functions in the test framework. See https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/key.py for example 10:42 < fjahr> I guess another way to deal with it is to never change the python and the c++ implementation in the same PR 10:42 < fjahr> but not sure how that could be enforced 10:42 < jnewbery> any other questions about the code in https://github.com/bitcoin/bitcoin/pull/15443/files 10:42 < jonatack> Could use some logging :p 10:43 < jnewbery> yeah, logging would be good! 10:44 < jonatack> perhaps separate into subtests too 10:44 < jnewbery> fjahr: I wonder if https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md#examples should be updated to include the checksums so they can be used as test vectors for other implementations 10:45 < digi_james> I wonder why the test vectors did not include unsolvable and private keys = true 10:45 -!- Zenton [~user@unaffiliated/vicenteh] has quit [Ping timeout: 272 seconds] 10:46 < digi_james> Or the vectors in descriptor.md should be updated with such, as fjahr suggest 10:46 < fjahr> yeah, that would make sense to me 10:46 < jnewbery> digi_james: I think because that's mainly a Bitcoin Core wallet concept 10:47 < jonatack> A general question, if ok, about test assertion argument order: 10:47 < jonatack> In some test frameworks there is a convention that can be useful for test error reports to make better sense. 10:47 < jonatack> In Bitcoin Core it looks like there is an implicit convention of assert_bla(actual, expected)...? 10:47 < jonatack> I didn't see it stipulated in the testing docs... perhaps I overlooked it. 10:47 < jonatack> Is there a strict (or loose) convention? 10:48 < jnewbery> jonatack: the convention is to use the assert_bla(actual, expected) rather than assert actual == expected 10:48 < jnewbery> because assert_bla(a,e) will print out a and e if they don't match 10:49 < jnewbery> whereas assert a == e will just fail with no helpful logging 10:49 < jonatack> agreed... my question pertains to the arg order 10:50 < jnewbery> oh right. I don't think there's a convention 10:50 < jnewbery> I think we have assert_greater_than() but not assert_lesser_than(), so the order depends on which you want to be greater for that one :) 10:50 < jonatack> it looks like actual, expected in practice 10:51 < jonatack> but unsure how hard or soft that is 10:51 < jonatack> for assert_equal in any case :) 10:51 < jnewbery> yeah, I think I've mostly seen actual, expected 10:51 < jonatack> Another question about running linters before pushing test commits 10:52 < jonatack> I could bit by not doing that this morning 10:52 < jonatack> got bit by not running linters before pushing 10:52 < jonatack> which do you run... 10:53 < jonatack> I automated lint-python.sh and lint-python-deadcode.sh 10:54 < jnewbery> I generally don't run them locally 10:54 < jnewbery> travis tells you pretty quickly when they fail 10:54 < lightlike> also a general question: does anyone know if there is a debug.log generated for unit tests (where the LogPrintf() go to)? 10:55 < jnewbery> lighlike: I don't know about debug.log for unit tests, but you can run with --log_level=all to get more verbose output: https://github.com/bitcoin/bitcoin/blob/master/src/test/README.md#running-individual-tests 10:56 < jonatack> jnewbery: thanks 10:56 < jnewbery> ok, 5 mkinutes until the hour. Any final questions before we wrap up? 10:56 < digi_james> jnewbery: Is there an formal/informal roadmap for descriptors? Whist there are very neat, I wonder if they could exist in PBSTs? I suppose they will play a larger role in wallets managing taproot paths? 10:56 < jnewbery> Also, any requests for PRs to cover next week and beyond? 10:57 < digi_james> Internal node/wallet interfaces? 10:57 < jnewbery> digi_james: we want the wallet to move entirely to descriptors. There's a project tracking that here: https://github.com/bitcoin/bitcoin/projects/12 10:57 < jnewbery> as for the specification of the language itself, yes, it would need to be extended to support taproot 10:58 < jnewbery> I'd also recommend watching the talk on miniscript here: https://www.youtube.com/watch?v=XM1lzN4Zfks&feature=youtu.be which is related 10:58 < digi_james> jnewberry: Ah thank you, I will have a look at the project, awesome! 10:59 < jnewbery> alright, let's wrap it up there. Thanks everyone! 10:59 < ariard> I need to think more about it but I think a lightning-prefix could be useful too 10:59 < hugohn> thanks jnewbery 10:59 < lightlike> thanks! 11:00 < digi_james> Thanks jnewbery, and everybody else! 11:00 < jonatack> Thanks jnewbery and everyone! 11:10 -!- hugohn [~textual@c-73-223-69-29.hsd1.ca.comcast.net] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 11:19 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving] 12:39 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 268 seconds] 12:40 -!- jonatack [d598a1a5@213.152.161.165] has quit [Remote host closed the connection] 12:44 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 12:47 -!- JulioBarros [~juliobarr@97-115-0-127.ptld.qwest.net] has quit [] 13:21 -!- drbrule [~user@cpe-72-226-106-116.nyc.res.rr.com] has quit [Quit: Lost terminal] 14:35 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 14:39 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 246 seconds] 14:44 -!- ccdle12 [~ccdle12@cpc139350-aztw33-2-0-cust310.18-1.cable.virginm.net] has quit [Remote host closed the connection] 14:46 -!- ccdle12 [~ccdle12@cpc139350-aztw33-2-0-cust310.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews 15:09 -!- ccdle12 [~ccdle12@cpc139350-aztw33-2-0-cust310.18-1.cable.virginm.net] has quit [Remote host closed the connection] 15:15 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 15:15 -!- lightlike [~lightlike@2001:16b8:574c:a100:4479:f4b6:cda7:e67b] has quit [Quit: Leaving] 15:20 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 245 seconds] 15:45 -!- Zenton [~user@unaffiliated/vicenteh] has quit [Ping timeout: 245 seconds] 15:55 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 16:25 -!- insolace [~insolace@mobile-166-137-176-223.mycingular.net] has joined #bitcoin-core-pr-reviews 16:32 -!- insolace [~insolace@mobile-166-137-176-223.mycingular.net] has quit [Remote host closed the connection] 16:34 -!- insolace [~insolace@mobile-166-137-176-223.mycingular.net] has joined #bitcoin-core-pr-reviews 16:46 -!- insolace [~insolace@mobile-166-137-176-223.mycingular.net] has quit [Remote host closed the connection] 16:52 -!- insolace [~insolace@mobile-166-137-176-223.mycingular.net] has joined #bitcoin-core-pr-reviews 16:59 -!- insolace [~insolace@mobile-166-137-176-223.mycingular.net] has quit [Remote host closed the connection] 17:10 -!- hugohn [~textual@50-1-44-2.fiber.dynamic.sonic.net] has joined #bitcoin-core-pr-reviews 17:14 -!- hugohn [~textual@50-1-44-2.fiber.dynamic.sonic.net] has quit [Client Quit] 17:16 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 17:20 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 268 seconds] 17:36 -!- sosthene [~sosthene@88.191.20.124] has quit [Ping timeout: 272 seconds] 17:37 -!- sosthene [~sosthene@88.191.20.124] has joined #bitcoin-core-pr-reviews 18:03 < wumpus> jnewbery: I think the reasoning is that RPC is a separate 'subsystem' from whatever is being tested (it needs to be specifically initialized/torn down), so anything testing RPC *and something else* is already a functional/integration test 18:05 < wumpus> the internal functions used by RPC methods can of course, independently, be tested in unit tests, especially if they're pure utility functions 18:05 < wumpus> but really calling RPC methods from unit tests is somewhat frowned upon, unless it's unit testing of the RPC system 18:53 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-nueclfyykucknpyf] has quit [Quit: Connection closed for inactivity] 18:58 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-idfegfmwnvineexl] has joined #bitcoin-core-pr-reviews 19:16 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 19:21 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 244 seconds] 19:49 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Ping timeout: 260 seconds] 19:50 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 21:03 -!- insolace [~insolace@157-131-95-76.fiber.dynamic.sonic.net] has joined #bitcoin-core-pr-reviews 21:13 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-idfegfmwnvineexl] has quit [Quit: Connection closed for inactivity] 21:17 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 21:21 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 244 seconds] 21:38 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-beynyrpyhaamyxfm] has joined #bitcoin-core-pr-reviews 21:39 -!- insolace [~insolace@157-131-95-76.fiber.dynamic.sonic.net] has quit [Remote host closed the connection] 23:05 -!- insolace [~insolace@157-131-95-76.fiber.dynamic.sonic.net] has joined #bitcoin-core-pr-reviews 23:08 -!- insolace [~insolace@157-131-95-76.fiber.dynamic.sonic.net] has quit [Remote host closed the connection] 23:17 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 23:22 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 268 seconds]