--- Day changed Wed Oct 28 2020 02:35 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 02:35 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 02:57 -!- jesseposner [~jesse@98.37.146.62] has quit [Ping timeout: 264 seconds] 03:08 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 03:11 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 03:11 -!- vasild_ is now known as vasild 03:31 -!- jesseposner [~jesse@98.37.146.62] has joined #bitcoin-core-pr-reviews 03:50 -!- jesseposner [~jesse@98.37.146.62] has quit [Ping timeout: 260 seconds] 03:56 <@jnewbery> Hi folks. We'll get started in about 6 hours. 03:58 <@jnewbery> It's a big PR this week. Don't worry if you haven't managed to review all 82 commits and eight hundred and something lines of code change :) 03:59 <@jnewbery> I'd recommend taking a look at this commit first: https://github.com/bitcoin/bitcoin/pull/20158/commits/5cb0350d5c1e14c3cd6153bd9166b80c53b12f83 . This is the eventual goal, to remove g_chainman, and make sure that our ChainstateManager is constructed/initialized/destructed inside init. 03:59 < [filebot]> commit: 04:00 <@jnewbery> Once you understand that goal, the other commits become quite mechanical. You can start at the beginning and work through them commit by commit. 04:00 <@jnewbery> We'll discuss more in the meeting. See you all there 04:20 -!- gleb [~gleb@178.150.137.228] has quit [Ping timeout: 240 seconds] 04:51 -!- belcher_ [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 04:54 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 265 seconds] 05:15 -!- jonatack [~jon@213.152.161.239] has quit [Ping timeout: 260 seconds] 05:17 -!- Rahul34Towne [~Rahul34To@static.57.1.216.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews 05:29 -!- Rahul34Towne [~Rahul34To@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 240 seconds] 05:46 -!- jesseposner [~jesse@98.37.146.62] has joined #bitcoin-core-pr-reviews 05:49 < wumpus> hah so it's dongcarl's PR's turn 05:52 -!- jesseposner [~jesse@98.37.146.62] has quit [Ping timeout: 240 seconds] 06:04 -!- wumpus [~ircclient@pdpc/supporter/professional/wumpus] has quit [Quit: brb] 06:05 -!- wumpus [~ircclient@pdpc/supporter/professional/wumpus] has joined #bitcoin-core-pr-reviews 06:22 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection] 06:22 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 06:24 -!- jonatack [~jon@109.232.227.138] has joined #bitcoin-core-pr-reviews 06:33 -!- jonatack [~jon@109.232.227.138] has quit [Ping timeout: 260 seconds] 06:38 -!- varioust [~varioust@rrcs-76-79-47-154.west.biz.rr.com] has joined #bitcoin-core-pr-reviews 06:48 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has joined #bitcoin-core-pr-reviews 06:58 -!- varioust [~varioust@rrcs-76-79-47-154.west.biz.rr.com] has quit [Ping timeout: 265 seconds] 07:01 -!- varioust [~varioust@rrcs-76-79-47-154.west.biz.rr.com] has joined #bitcoin-core-pr-reviews 07:06 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has quit [Remote host closed the connection] 07:19 -!- jonatack [~jon@213.152.162.154] has joined #bitcoin-core-pr-reviews 07:40 <@jnewbery> wumpus: that's right. We'll spend 5 hours going through each commit in minute detail :D 07:41 -!- jonatack [~jon@213.152.162.154] has quit [Quit: jonatack] 07:46 -!- jonatack [~jon@213.152.161.133] has joined #bitcoin-core-pr-reviews 07:48 -!- jesseposner [~jesse@98.37.146.62] has joined #bitcoin-core-pr-reviews 07:53 -!- jesseposner [~jesse@98.37.146.62] has quit [Ping timeout: 265 seconds] 07:55 -!- belcher_ is now known as belcher 07:56 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has joined #bitcoin-core-pr-reviews 08:01 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has quit [Remote host closed the connection] 08:03 -!- varioust [~varioust@rrcs-76-79-47-154.west.biz.rr.com] has quit [Ping timeout: 256 seconds] 08:07 < dongcarl> Is this the first draft PR on pr reviews? :O 08:08 < dongcarl> In any case, I'm honoured and excited to review with y'all! 08:08 < jonatack> time zone wise, we start in 3 hours? 08:08 < dongcarl> Last I checked it's 2 hours... Will recheck 08:08 < jonatack> dongcarl: thanks for hosting ✨ 08:09 < dongcarl> 1 hr 50 mins according to WolframAlpha! 08:09 < jonatack> dongcarl: oh no https://twitter.com/BitcoinCorePRs/status/1321468458329059328 i'll rectify 08:10 < dongcarl> Haha nw :-) 08:11 < jonatack> dongcarl: ty, just changed to winter hours over the weekend here so i'm confused :D 08:12 < dongcarl> Ah I see! 08:16 < emzy> I hate daylight savings. 08:19 -!- varioust [~varioust@rrcs-76-79-47-154.west.biz.rr.com] has joined #bitcoin-core-pr-reviews 08:25 < michaelfolkson2> A hour less of prep :/ 08:27 < jonatack> emzy: same, hopefully next year it goes away if the various eurozone countries can agree on which one (summer vs winter) to adopt 08:28 < emzy> jonatack: I'm waiting for this over 10 years. But I still hope it will happen. 08:29 * emzy realizes he needs to (re)lern c++ 08:30 < jonatack> michaelfolkson2: tbh i come back to these notes/questions/logs when actually reviewing the PR. with release branch-off in a few days i'm prioritisng things that need to make the deadline. 08:30 < michaelfolkson2> What you looking at? 08:31 < jonatack> michaelfolkson2: #20187 atm 08:31 < jonatack> and basically stuff in https://github.com/bitcoin/bitcoin/milestone/45 08:32 -!- michaelfolkson2 is now known as michaelfolkson 08:34 < jonatack> (review beg for #20115 and #20220) 08:35 -!- dhruvm [~noreply@c-73-158-59-66.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 08:44 < michaelfolkson> The TODO follow ups to #20220 will be done post release branch-off? 08:46 < michaelfolkson> Can't be having tests that fail incorrectly in a release I'm assuming... 08:52 < michaelfolkson> #20115 looks fine (have't tested yet) 08:53 < jonatack> michaelfolkson: thanks for having a look! the send RPC is experimental, so i guess mildly inconsistent behavior and edge case bugs are ok. i wanted to fix them, but that PR is pretty long already and time is short to get it reviewed before Nov 1 08:54 < jonatack> e.g. ./src/bitcoin/cli help send -> "EXPERIMENTAL warning: this call may be changed in future releases." 08:58 < michaelfolkson> Cool, I will add a review after the meeting. I need to look at this ChainstateManager stuff if I'm going to have any idea of what's going on ;) 08:59 < jonatack> if 20220 is merged, then i can move forward to fix the bugs, but i told myself to not spend even more time on the topic unless there is enough reviewer interest to justify doing so ;) 08:59 < jonatack> yes! great 09:21 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 09:21 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Client Quit] 09:21 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 09:39 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection] 09:40 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 09:50 -!- jesseposner [~jesse@98.37.146.62] has joined #bitcoin-core-pr-reviews 09:51 -!- rav3n [92732bd1@146.115.43.209] has joined #bitcoin-core-pr-reviews 09:53 <@jnewbery> hi folks. I have a very exciting announcement 09:53 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has joined #bitcoin-core-pr-reviews 09:53 <@jnewbery> review club starts in 7 minutes! 09:54 -!- dappdever [17e28383@23.226.131.131] has joined #bitcoin-core-pr-reviews 09:54 -!- jesseposner [~jesse@98.37.146.62] has quit [Ping timeout: 240 seconds] 09:58 -!- elle [~ellemouto@155.93.252.70] has joined #bitcoin-core-pr-reviews 09:58 < michaelfolkson> That is just toooooo exciting 10:00 < dongcarl> #startmeeting 10:00 < jamesob> hi 10:00 < b10c> hi 10:00 < dongcarl> hello! 10:00 < michaelfolkson> hi 10:00 < emzy> hi 10:00 <@jnewbery> Hi folks! Welcome to Bitcoin Core PR Review Club. Feel free to say hi to let everyone know you're here 10:00 < rav3n> hi 10:00 < elle> hi! 10:00 < dappdever> hello 10:00 <@jnewbery> (or don't. Lurkers are also welcome!) 10:00 <@jnewbery> Anyone here for the first time? 10:01 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews 10:01 < dappdever> I am! 10:01 < troygiorshev> hi! 10:01 < dongcarl> Welcome! 10:01 <@jnewbery> dappdever: welcome! 10:01 < Klopp> Hi allI am new (as a lurker) 10:01 -!- washroom [~washroom@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 10:01 < dongcarl> Alright, let's get started. I think this is the first Draft PR? 10:01 < dongcarl> Did everyone get a chance to review the PR? How about a quick y/n from everyone 10:02 < dappdever> y 10:02 < troygiorshev> y partial :D 10:02 < elle> y 10:02 < Klopp> n 10:02 < rav3n> n 10:02 < emzy> y 10:02 < b10c> n 10:02 <@jnewbery> 5/82 y 10:02 < michaelfolkson> At a high level 10:02 < dongcarl> Hehe it is quite a chonky one 10:02 < dongcarl> Let's delve into the questions! 10:02 < jonatack> hi 10:02 < MarcoFalke> 3/82 and concept 10:03 < dongcarl> At a high level: Concept ACK, approach ACK, ACK, or NACK? 10:03 < washroom> hi 10:03 < jonatack> will look at it after release branch-off 10:03 < jamesob> n 10:03 < jamesob> Concept ACK 10:04 < emzy> Concept ACK 10:04 < elle> Concept ACK 10:04 <@jnewbery> Big big concept ACK 10:04 < MarcoFalke> Concept ACK 10:04 < dappdever> concept ack 10:04 < troygiorshev> Concept ACK 10:04 < dongcarl> Alright, let's get into the details 10:05 < nehan> hi 10:05 < michaelfolkson> Concept ACK (assuming I've understood it). Is "consensus engine" another term for "libconsensus"? 10:05 < dongcarl> Oh good question! 10:05 < dongcarl> The way I've thought about it is that "consensus engine" is an encapsulation of how our consensus code works _currently_ 10:06 < dongcarl> Next Q: How did the PRs linked in the historical context notes change the organization of validation.cpp? What kind of state/logic now belongs in each consensus-related object (ChainstateManager, CChainState, BlockManager)? 10:08 < dongcarl> For those needing a link: https://bitcoincore.reviews/20158 10:08 < michaelfolkson> BlockManager is chainstate agnostic block metadata 10:08 < elle> CChainState now owns mempool, blockfeeestimator and other things that represent our local view of the best chain. 10:08 < dappdever> It seems to have compartmentalized logic into objects, de-coupling a bit 10:08 < troygiorshev> CChainState is what will eventually become the libconsensus. It hold everything related to the curent tip of the chain. 10:09 < jamesob> troygiorshev: CChainstate and not ChainstateManager? 10:09 < MarcoFalke> jamesob: assumeutxo is not yet merged ;) 10:09 < jamesob> heh 10:10 < dongcarl> dappdever: You're right on that one! You can take a look at validation pre-10279 to see its state 10:10 < dongcarl> I call validation pre-10279 the "primordial soup of validation globals" 10:10 < troygiorshev> jamesob: yeah I think! Everything to do with, say, connecting blocks is part of CChainState? Might be CChainstate through ChainstateManager 10:10 < michaelfolkson> They both have chainstate in them though right? I want to understand the MarcoFalke joke 10:11 <@jnewbery> ChainstateManager can hold multiple chains. It was introduced as part of the assumeutxo project 10:11 < jamesob> at the moment, there's only one chainstate running around. But eventually there may be multiple. In any case, at the moment ChainstateManager is responsible for constructing chainstates, which involves e.g. injecting a BlockManager instance 10:12 <@jnewbery> currently there's only one chain - the chain that we consider to be the best. With assumeutxo, there would be multiple chains 10:12 < washroom> would ChainstateManager allow the daemon to run mainnet and signet simultaneously? 10:12 <@jnewbery> washroom: no 10:12 < dongcarl> elle michaelfolkson troygiorshev: True as well. A very rough approximation I have in my head: BlockManager = the blockchain/metadata, CChainState = everything related to current tip (UTXO, etc) 10:12 < michaelfolkson> Multiple chains ie the one we are assuming is ok from tip and the chain we are validating in the background? 10:12 <@jnewbery> All the chains would be running the same consensus parameters 10:12 < michaelfolkson> Surely you don't need more than two.... 10:12 < jamesob> michaelfolkson: right 10:13 < nehan> multiple chains mean multiple mempools too? 10:13 < dongcarl> jamesob is quite the expert here, as he write two of the 4 PRs linked! 10:13 < jamesob> michaelfolkson: in concept, you could have n chainstates and validate n separate regions of the chain historically, simultaneously (maybe with something like utreexo) but that's way off 10:13 < washroom> jnewbery michaelfolkson ah I see now, a method to assess all available chain tips 10:14 <@jnewbery> hi nehan! No, only one mempool. We only have a mempool for transactions that we think can be added to the best current chain 10:14 < troygiorshev> dongcarl: and an eventual libconsensus api would most closely follow which class? CChainSate, no? 10:15 < washroom> mempool would just pertain to chain-tip extension rather than assessing or constructing branching consensus-equivalent chains -- right? 10:15 < jamesob> troygiorshev: if I had to guess, probably none - it would be some functional layer that accesses chainstate objects through the chainmanager... but defer to dongcarl there 10:15 < dongcarl> troygiorshev: I don't like to speculate about eventual APIs, because we're sooooo far from that right now. BUT I think a good intermediary goal is to encapsulate ChainstateManager and its dependencies first! 10:16 <@jnewbery> dongcarl: I agree. If there's obvious incremental improvements, we should do them, regardless of what the end state could be 10:16 < troygiorshev> jamesob dongcarl: ok! 10:16 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews 10:17 <@jnewbery> even if we never get to libconsensus, simply cleaning up/encapsulating/modularizing are very beneficial 10:17 < washroom> +1 10:17 < dongcarl> jnewbery: +1 10:17 < dongcarl> Alright, feel free to keep discussing but here's the next topic: Where and how are the aforementioned consensus-related objects initialized? 10:17 < michaelfolkson> washroom: The start of the statement is right. But the assumeutxo project is validating from genesis in the background while assuming the tip is ok 10:18 < michaelfolkson> (Or at least the vanilla version is. And it isn't merged yet) 10:18 < dongcarl> bonus if you can identify some differences between constructing (as in C++ constructors) and initializing! 10:21 < dongcarl> Carl's tip for codebase navigation: try out sourcetrail! It's like grep, but you can click around and it understands C++/Python/etc 10:21 < troygiorshev> +1 10:22 < jamesob> hint: init.cpp 10:22 <@jnewbery> Construction means something very specific in C++. It's the operations that get called when a new object gets instantiated. Those operations include the initialization list, the constructor functions of any base classes and the constructor function of the class itself 10:22 < troygiorshev> ChainstateManager is a global g_chainman in validation.cpp, which is then pointed to by the NodeContext node in AppInitMain in init.cpp 10:23 <@jnewbery> I think initialization is more vague and means 'make sure the object is ready to use' 10:24 < troygiorshev> g_chainman is constructed, er, at some point before main() is run? 10:24 <@jnewbery> troygiorshev: yes, exactly 10:24 < dongcarl> troygiorshev: Yes! 10:24 * troygiorshev wipes sweat 10:24 -!- ninfireblade [5b75cc0b@11.204.117.91.dynamic.reverse-mundo-r.com] has joined #bitcoin-core-pr-reviews 10:24 <@jnewbery> we don't have control over when global objects are constructed, and it could even be before main() 10:25 < michaelfolkson> I thought initialization did not infer the initialized object was in a usable state. Just that it was initialized 10:25 < sipa> jnewbery: it has to be before main 10:25 < dongcarl> michaelfolkson: This is correct -> "construction did not infer the constructed object was in a usable state. Just that it was constructed" 10:26 < troygiorshev> sipa: as long as it's a global in the global namespace, right? 10:26 < sipa> troygiorshev: right 10:26 <@jnewbery> often, initialization happens during construction, but if initialization requires some context, then obviously we can't initialize the object during construction if it's a global 10:27 < sipa> jnewbery: what do you mean by initialization in this context? 10:28 < dongcarl> Exactly what jnewbery said! If a variable is a global, and it needs some context to get initialized to steady state, then we need a two-step initialization. See: InitializeChainstate 10:29 < sipa> oh, initialization as in an application-level concept, ok 10:29 < dongcarl> Ah I should clarify, what I mean by initialization is "making the object ready to use/at stead-state" 10:29 < dongcarl> Yup! 10:29 <@jnewbery> sipa: I mean things like what dongcarl said. Make the object ready to use. 10:30 < dongcarl> So ChainstateManager is constructed as a global and initialized in init.cpp... Anywhere else it's initialized? 10:30 < jamesob> (fwiw, here's the C++ definition of initialization: https://en.cppreference.com/w/cpp/language/initialization) 10:30 <@jnewbery> sipa: does this: https://stackoverflow.com/a/1271692/933705 only apply to objects with internal linkage? ie if they're in the global namespace then the have to be constructed before main? 10:31 < sipa> jnewbery: it may also be outdated; C++11 added guarantees around initialization order 10:32 <@jnewbery> sipa: ah. thanks! 10:32 <@jnewbery> jamesob: I think that's just for simple types. It doesn't say anything about classes 10:32 < sipa> ah, they may be right - that globals are only guaranteed to be initialized before calling any function in the same compilation unit 10:32 < dappdever> @dongcarl it seems that init.cpp is the only caller of InitiatilizeChainState 10:33 < dongcarl> Hint for my small q: take a look at how our test framework starts up 10:34 < dappdever> copied from the global chainman? 10:34 < troygiorshev> c++ stackoverflow being outdated about a compilation question really embodies spooky season 10:34 < sipa> jnewbery: c++11 added a guarantee that local static variables are only initialized once, even when the function is called simultaneously from multiple threads for the first time 10:35 < nehan> testing setup calls it 10:35 < dongcarl> dappdever: Not copied, per se, but in both init and TestingSetup, we point node.chainman to the g_chainman! 10:35 < dongcarl> nehan: Bingo! 10:36 < dappdever> ah, yes, got it! 10:36 < dongcarl> The reason why this is important from a higher level is to realize that we've got two different initialization codepaths 10:36 < dongcarl> And because of that, it is possible to have differences between how our test framework initializes, and how our software actually works 10:37 < nehan> can you talk more about that? why does the test setup do this outside of initializing a node? 10:37 < nehan> oh -- these are the C++ tests. nevermind. 10:37 < dongcarl> nehan: Hehe yeah, it _is_ initializing a node! 10:38 < dongcarl> Okay, let's get to the next one! 10:38 < nehan> followup question: why does the code path for the test setup "count" as a different code path that one should care about? 10:38 < MarcoFalke> Ideally the unit tests would re-use most of the init.cpp logic 10:38 < dongcarl> nehan: Because I don't think the test framework ever calls AppInitMain 10:38 < nehan> MarcoFalke: would that require factoring it out of main()? 10:39 < nehan> dongcarl: right, that. why not? 10:39 <@jnewbery> nehan: take a look at setup_common to see how the unit tests set up their environment 10:39 < michaelfolkson> MarcoFalke: Unless you're testing the init.cpp logic. But that would be functional testing...? 10:40 < troygiorshev> Is this a consious choice (along the lines of keeping Arrange Act Assert separate) or is it force on us? 10:40 < MarcoFalke> Well, if the unit tests need to spin up a node, they should do it as similar as AppInitMain as possible 10:40 < dongcarl> I'm not sure there's a reason why it's done the way it is right now. However, what I think would be a nice goal to have: an initialization codepath that's used by all binaries with the right toggles. 10:41 <@jnewbery> Take a look at the logic inside init.cpp. A lot of the code there is actually initializing the individual sub-components. We need to do the same setup inside our unit tests, otherwise we're not actually testing the code paths that run in production 10:41 < dappdever> do the new chainmanager tests use the same setup code? It seems like they no longer use the global instance (?) 10:41 < troygiorshev> dongcarl jnewbery: ok thanks 10:41 < dongcarl> As always jnewbery, puts it nicely :-) 10:42 < dongcarl> Let's move on to the next one so we have time! 10:42 <@jnewbery> I think it'd be nice if as much of the initialization of the subcomponents was done within those subcomponents, perhaps inside the constructor function. That way the unit tests could just instantiate that subcomponent and know that it's testing what will actually run on the live system 10:42 < dongcarl> What are the advantages and disadvantages of using global variables and functions? 10:42 < MarcoFalke> jnewbery: Indeed. And removing globals helps there 10:43 < dongcarl> dappdever: Take a look here! :-) https://github.com/bitcoin/bitcoin/pull/20158/commits/5cb0350d5c1e14c3cd6153bd9166b80c53b12f83 10:43 < [filebot]> commit: 10:43 <@jnewbery> And then of course we'd be able to unit test units of our code, without worrying about global state interfering 10:43 < MarcoFalke> advantage of globals would be less verbosity in code and if there can only be one instance of an object, passing it around explicitly doesn't help code clarity 10:43 < elle> dongcarl: advantages: easy access from all over. disadvantages: easy access from all over. Plus: hard to mock for tests. Also: not always known if the object is yet initialised (in its steady state) 10:43 < troygiorshev> advantage: can access from anywhere. disadvantage: can access from anywhere... 10:44 <@jnewbery> troygiorshev: :) 10:44 < dongcarl> elle: Hehe exactly! 10:44 -!- daniel__ [~daniel@88-117-109-129.adsl.highway.telekom.at] has joined #bitcoin-core-pr-reviews 10:44 < troygiorshev> elle: well said! 10:44 < dappdever> if globals can be modified from anywhere, it seems like a security risk 10:44 < michaelfolkson> Lots of disadvantages mostly covered. Thread safety too 10:44 < nehan> it's easy to get lazy and not have good encapsulation and abstraction 10:45 < michaelfolkson> Getting rid of globals is also a pain 10:45 < MarcoFalke> michaelfolkson: I'd say thread safety is another orthogonal issue 10:45 < dongcarl> nehan: ...and end up needing a 80 commit PR to clean it up haha 10:46 < sipa> dongcarl: squash it all 10:46 < MarcoFalke> Our NodeContext is not thread safe. It just happens to be modified in at most one thread at a time 10:46 < sipa> (not a serious suggestion) 10:46 < dongcarl> I like what I'm hearing from everyone 10:47 < dappdever> do globals perform better because there is less initiatlization? 10:47 < nehan> is there a reason setup_common.cpp doesn't call AppInitMain()? what makes it hard to do so, if anything? 10:47 < michaelfolkson> I guess I'm saying use of globals can impact thread safety but avoiding them doesn't guarantee it MarcoFalke 10:47 < dongcarl> nehan: Hmmm... Don't think there is. MarcoFalke? 10:48 < MarcoFalke> nehan: Good question. I think AppInitMain does too much (like calling weird compat code I don't understand) 10:48 < dongcarl> dappdever: Since initialization is mostly a one-and-done affair, the performance impact is not something we'd be too concerned about. I also doubt there's any performance gains to be made from being global. 10:48 < nehan> seems like that would be best, if possible, to test the initialization flow (as you pointed out) 10:49 < MarcoFalke> AppInitMain also parses from the argsmanager, but unit tests historically didn't use "command line" args for initialization 10:49 < dongcarl> nehan: Agreed, this is definitely in my list of followup projects :-) 10:50 < dongcarl> Alright the last one is a little tricky, so let's leave some time for it! 10:50 < nehan> dongcarl: ok! I haven't looked closely yet at the differences between TestingSetup() and AppInitMain() but will do so 10:50 < dongcarl> Why are the first few fix commits in the branch necessary? Why does the previous code work prior to de-globalizing g_chainman and why doesn’t it work now without the fix? 10:50 <@jnewbery> nehan: At that point, you're arguably not unit testing - it'd be closer to integration testing 10:50 < dongcarl> I'm talking about https://github.com/bitcoin/bitcoin/pull/20158/commits/22d44a3caf47701a7d16761751563e446e5f2bdf 10:50 < [filebot]> commit: 10:50 <@jnewbery> with unit testing you want to be able to isolate each individual component 10:50 < dongcarl> https://github.com/bitcoin/bitcoin/pull/20158/commits/e4be2ad63c4acbe22266174cac77caf7ce5d9215 10:50 < [filebot]> commit: 10:50 < dongcarl> and https://github.com/bitcoin/bitcoin/pull/20158/commits/1ca9317560335cf0f2cb8d69d2c12edc7b6d1526 10:50 < [filebot]> commit: 10:51 < nehan> jnewbery: one could think of it as unit testing the initialization function. i'm not sure it's in any way bad to test the same code path used in actual use, if possible. 10:51 < MarcoFalke> good point jnewbery. AppInitMain should also be split up to accomodate the testing setups that don't need all the node components. 10:51 -!- [filebot] was kicked from #bitcoin-core-pr-reviews by jnewbery [[filebot]] 10:51 < nehan> MarcoFalke: yeah, maybe it can be split up so the unit tests can use pieces 10:52 < dongcarl> COALESCE THE INIT CODEPATHS 2020 10:52 < MarcoFalke> Most tests don't even need a chain. (low level net tests for example) 10:52 < MarcoFalke> dongcarl: good luck getting that done in 2020 ;) 10:53 <@jnewbery> nehan: yes, a unit test that tests initialization is fine. But you shouldn't need to initialize the whole system if you just want to test one or a few subcomponent 10:53 < nehan> dongcarl: i will make hats 10:53 < dongcarl> Hehe 10:53 < nehan> jnewbery: agreed! 10:53 < sipa> dongcarl: last-minute presidential campaign? 10:53 < dongcarl> For those looking at the first few commits: please ignore "test: Add new ChainTestingSetup and use it" 10:53 < jamesob> sipa: I'm writing him in regardless 10:54 < dongcarl> legoooo 10:55 < dappdever> @dongcarl are the tests using the same instance of m_node across tests, whereas before each tests was creating the chain from a new node instance? 10:57 < dongcarl> dappdever: Close! It does have something to do with m_node. It's mostly because after this PR, the code will now reach for the chainman inside the local context instead of g_chainman. So it's important that the local node context contains the chainman we want to work with instead of not having one! 10:57 < dongcarl> Alright we're nearing the end now! 10:58 < dongcarl> I want to thank everyone who came out and participated, jnewbery for organizing! 10:58 <@jnewbery> Anyone have any last minute questions? 10:58 < dongcarl> Any last questions? 10:58 < dongcarl> Heh 10:58 < troygiorshev> when undraft and merge 10:58 < washroom> wait there are voters who didn't write in dongcarl ? 10:58 < dongcarl> Surprised me too! 10:59 < washroom> : P 10:59 < dongcarl> troygiorshev: As soon as I'm 100% that it's bug-free! 10:59 <@jnewbery> dongcarl: how do you plan to get this merged? are you going to slice off parts of it into smaller PRs? 10:59 < MarcoFalke> dongcarl: How are you going to split up the pull? 11:00 < dongcarl> Aha! I think the FIX commits will definitely be split off first. Those are very easy. 11:00 < troygiorshev> not all of us are ryanofsky and can review it all in one go :) 11:00 < michaelfolkson> 82 commits. Split into groups of 5 haha 11:00 < michaelfolkson> (Joke) 11:00 < dongcarl> I can split out the rest of the commits into 7 bundles for review and discussions. 11:01 < dongcarl> However, I would hope that we can get it merged in quick succession 11:01 < dongcarl> As a lot of the benefits come from the latter bundles of commits 11:01 < dongcarl> Oh time's up! 11:01 <@jnewbery> dongcarl: great. Feel free to announce them in this channel so people know where to look 11:01 < dongcarl> jnewbery: Will do! 11:01 < dongcarl> #endmeeting 11:01 < washroom> ty dongcarl 11:01 < troygiorshev> thanks dongcarl! 11:01 < emzy> Thank you dongcarl, jnewbery and all. 11:01 < nehan> thanks! 11:01 <@jnewbery> Thanks Carl. That was great! 11:01 < dappdever> thank you!~ 11:01 < dongcarl> Thank y'all! 11:02 < michaelfolkson> Thanks dongcarl! 11:03 -!- elle [~ellemouto@155.93.252.70] has quit [Quit: leaving] 11:04 < jonatack> Thank you, Carl! 🏆 11:04 < sipa> dongcarl: are you a natural-born US citizen, who has been a resident for at least 14 years, and at least 35 years of age? 11:05 <@jnewbery> sipa: define natural-born 11:06 < emzy> Is this about drinking alcohol? ;) 11:06 < jamesob> thanks dongcarl, nice one 11:06 < jamesob> looking forward to reviewing 11:06 < dongcarl> sipa: In many parallel universes, I am! 11:06 < sipa> jnewbery: this is a difficult question: https://en.wikipedia.org/wiki/Natural-born-citizen_clause 11:06 < dongcarl> jamesob: ty, lmk if you run into something! 11:06 < jamesob> dongcarl: you know I will bb 11:07 < sipa> if the constitution were written in C++, this would be an undefined reference 11:07 * troygiorshev remembers that ted cruz (a canadian) was in the presidential primaries in 2016 11:08 < emzy> Some version control would also help. ;) 11:09 < michaelfolkson> Next week's PR review club will be day after dongcarl becomes President. We hope you will be free to attend and won't be too busy 11:09 -!- asae [c2e69e2d@194.230.158.45] has joined #bitcoin-core-pr-reviews 11:10 < dongcarl> Hehe the next review club PR makes this current PR shorter so... Gotta be there! 11:10 < sipa> emzy: arguably written legislation actually is; as no clauses can be removed, they just get replaced/modified by future ones... so you could say that the law text is actually a commit log, and courts try to determine what the actual final state is ;) 11:10 < dongcarl> Meanwhile in france... https://gitlab.inria.fr/verifisc/mlang 11:11 < emzy> sipa: I prefer git. :) 11:12 -!- asae [c2e69e2d@194.230.158.45] has quit [Remote host closed the connection] 11:24 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has left #bitcoin-core-pr-reviews [] 11:46 -!- washroom [~washroom@195.181.160.175.adsl.inet-telecom.org] has left #bitcoin-core-pr-reviews [] 11:51 -!- jesseposner [~jesse@98.37.146.62] has joined #bitcoin-core-pr-reviews 11:55 -!- tryphe [~tryphe@unaffiliated/tryphe] has quit [Read error: Connection reset by peer] 11:55 -!- midnight_ is now known as midnight 11:56 -!- tryphe [~tryphe@unaffiliated/tryphe] has joined #bitcoin-core-pr-reviews 12:26 -!- dappdever [17e28383@23.226.131.131] has quit [Remote host closed the connection] 12:38 -!- prayank [849ae2c9@132.154.226.201] has joined #bitcoin-core-pr-reviews 12:42 < prayank> Hello Everyone. I have two questions. 12:42 < prayank> 1. How to use IRC on Android with Tor 12:46 < sipa> 1. My setup is: juicessh on my phone, ssh into my VPS (non-tor), where I run irssi in screen, which connects via tor to freenode and other networks 12:46 < sipa> happy to share configuration for any of those 12:50 -!- prayank [849ae2c9@132.154.226.201] has quit [Ping timeout: 245 seconds] 12:51 < sipa> i guess not... 12:51 -!- prayank [310ff766@49.15.247.102] has joined #bitcoin-core-pr-reviews 12:52 < sipa> prayank: 1. My setup is: juicessh on my phone, ssh into my VPS (non-tor), where I run irssi in screen, which connects via tor to freenode and other networks 12:53 < prayank> sipa: will be helpful if you could share steps that I need to follow for irssi 12:53 < prayank> Will try on Ubuntu 12:54 < sipa> prayank: in general, or to connect to freenode over tor? 12:54 < prayank> to connect to freenode over tor 12:54 < sipa> you first need to be registered with freenode 12:55 < sipa> they don't allow anonymous logins over tor 12:55 < sipa> https://freenode.net/kb/answer/registration 12:55 < prayank> hmm.. maybe this is what I was missing. Let me check the link 13:00 -!- MasterdonX [~masterdon@titan.pathogen.is] has quit [Ping timeout: 256 seconds] 13:03 -!- MasterdonX [~masterdon@titan.pathogen.is] has joined #bitcoin-core-pr-reviews 13:07 -!- willcl_ark_ [~quassel@cpc123762-trow7-2-0-cust7.18-1.cable.virginm.net] has quit [Quit: Quit] 13:08 -!- MasterdonX [~masterdon@titan.pathogen.is] has quit [Ping timeout: 264 seconds] 13:08 -!- willcl_ark [~quassel@cpc123762-trow7-2-0-cust7.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews 13:13 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!] 13:21 -!- MasterdonX [~masterdon@titan.pathogen.is] has joined #bitcoin-core-pr-reviews 13:25 -!- MasterdonX [~masterdon@titan.pathogen.is] has quit [Read error: Connection reset by peer] 13:26 -!- MasterdonX [~masterdon@89.187.168.52] has joined #bitcoin-core-pr-reviews 13:45 < prayank> sipa: Registered my nickname successfully and tried to connect from "konversation" application using Tor. Let me share the error that I get. 13:45 < prayank> [13:43] [Nick] Nickname already in use. Trying prayank_. 13:46 < sipa> prayank: you're not talking to me from konversation, right? 13:47 < prayank> No. This is from https://webchat.freenode.net/ in browser 13:48 -!- masterdonx2 [~masterdon@titan.pathogen.is] has joined #bitcoin-core-pr-reviews 13:48 < sipa> that explains why you can't get prayank from konversation then 13:48 < sipa> some other connection with nickname prayank (namely you, here) has it 13:49 < prayank> Okay so we can't have multiple login sessions. 13:49 -!- MasterdonX [~masterdon@89.187.168.52] has quit [Ping timeout: 260 seconds] 13:49 < sipa> no, IRC has no concept of identities 13:49 < sipa> every connection has one unique nickname 13:50 < sipa> freenode's registration mechanism allows tying an identity to a nickname, but that's a layer on top - the base irc protocol has no such concept 13:51 < prayank> Cool 13:51 < prayank> But SASL error 13:51 < prayank> I created the self signed cert as mentioned here: https://freenode.net/kb/answer/certfp 14:03 -!- prayank [310ff766@49.15.247.102] has quit [Remote host closed the connection] 14:04 -!- ninfireblade [5b75cc0b@11.204.117.91.dynamic.reverse-mundo-r.com] has quit [Remote host closed the connection] 14:18 -!- Zenton [~user@unaffiliated/vicenteh] has quit [Read error: Connection reset by peer] 14:18 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 14:21 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Remote host closed the connection] 14:42 -!- varioust [~varioust@rrcs-76-79-47-154.west.biz.rr.com] has quit [Ping timeout: 240 seconds] 14:46 -!- kristapsk_ is now known as kristapsk 14:59 -!- Netsplit *.net <-> *.split quits: @jnewbery, tynes, marcinja_, Henry151 14:59 -!- jnewbery_ [~john@164.90.178.190] has joined #bitcoin-core-pr-reviews 14:59 -!- Netsplit over, joins: tynes 14:59 -!- marcinja [~marcinja@2604:a880:400:d1::89a:e001] has joined #bitcoin-core-pr-reviews 15:00 -!- Netsplit over, joins: Henry151 15:04 -!- daniel__ [~daniel@88-117-109-129.adsl.highway.telekom.at] has quit [Ping timeout: 264 seconds] 15:11 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 15:13 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 15:22 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has joined #bitcoin-core-pr-reviews 16:20 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has quit [Ping timeout: 256 seconds] 17:22 -!- varioust [~varioust@72-46-56-102.lnk.ne.static.allophone.net] has joined #bitcoin-core-pr-reviews 17:43 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Quit: pinheadmz] 17:52 -!- seven_ [~seven@cpe-90-157-197-248.static.amis.net] has quit [Ping timeout: 260 seconds] 18:17 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews 18:25 -!- Jay47 [94651092@148.101.16.146] has joined #bitcoin-core-pr-reviews 18:26 -!- Jay47 [94651092@148.101.16.146] has quit [Remote host closed the connection] 18:36 -!- varioust [~varioust@72-46-56-102.lnk.ne.static.allophone.net] has quit [Ping timeout: 264 seconds] 18:46 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Quit: pinheadmz] 18:48 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews 19:00 -!- Jackielove4u [uid43977@gateway/web/irccloud.com/x-mxlomfeqruovckob] has quit [Ping timeout: 272 seconds] 19:00 -!- valwal_ [sid334773@gateway/web/irccloud.com/x-pxseioohccmgrxvz] has quit [Ping timeout: 260 seconds] 19:00 -!- Jackielove4u [uid43977@gateway/web/irccloud.com/x-xsmtlxhhfuereiur] has joined #bitcoin-core-pr-reviews 19:03 -!- valwal_ [sid334773@gateway/web/irccloud.com/x-ovtfecsoawfrexqq] has joined #bitcoin-core-pr-reviews 19:40 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has joined #bitcoin-core-pr-reviews 19:51 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has quit [Ping timeout: 240 seconds] 19:53 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has joined #bitcoin-core-pr-reviews 20:16 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has quit [Ping timeout: 260 seconds] 20:19 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has joined #bitcoin-core-pr-reviews 20:37 -!- varioust [~varioust@cpe-108-167-11-88.neb.res.rr.com] has quit [Ping timeout: 265 seconds] 21:56 -!- S3RK [~S3RK@116.118.83.69] has quit [Ping timeout: 240 seconds] 22:07 -!- S3RK [~S3RK@116.118.83.69] has joined #bitcoin-core-pr-reviews 22:14 -!- S3RK [~S3RK@116.118.83.69] has quit [Ping timeout: 260 seconds] 22:37 -!- S3RK [~S3RK@116.118.83.69] has joined #bitcoin-core-pr-reviews 23:36 -!- ares_ [~ares@gateway/tor-sasl/virtu] has quit [Ping timeout: 240 seconds] 23:38 -!- ares_ [~ares@gateway/tor-sasl/virtu] has joined #bitcoin-core-pr-reviews