--- Day changed Wed May 12 2021 01:05 -!- gleb [~gleb@178.150.137.228] has joined #bitcoin-core-pr-reviews 02:51 -!- belcher_ is now known as belcher 03:20 -!- Thelma89MacGyver [~Thelma89M@static.57.1.216.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews 03:29 -!- Thelma89MacGyver [~Thelma89M@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 240 seconds] 03:44 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Ping timeout: 265 seconds] 03:49 -!- queip [~queip@unaffiliated/rezurus] has quit [Remote host closed the connection] 03:51 -!- queip [~queip@unaffiliated/rezurus] has joined #bitcoin-core-pr-reviews 04:08 -!- jadi [~jadi@213.207.207.253] has quit [Remote host closed the connection] 04:08 -!- jadi [~jadi@213.207.207.253] has joined #bitcoin-core-pr-reviews 04:13 -!- jadi [~jadi@213.207.207.253] has quit [Ping timeout: 260 seconds] 04:23 -!- jadi [~jadi@213.207.207.253] has joined #bitcoin-core-pr-reviews 05:17 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection] 05:17 -!- _andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 05:24 < michaelfolkson> Part 2 of dongcarl's Chaincode Labs podcast appearance is worth a read/listen in advance of today's PR review club 05:24 < michaelfolkson> Transcript: https://btctranscripts.com/chaincode-labs/chaincode-podcast/2020-11-30-carl-dong-reproducible-builds/ 05:25 < michaelfolkson> Podcast: https://www.youtube.com/watch?v=Y5Gfli3x6rI 05:26 < michaelfolkson> Oops that is wrong link 05:26 < michaelfolkson> Podcast Part 2 is here: https://podcast.chaincode.com/2020/12/15/carl-dong-2.html 05:27 -!- rebroad_ [~rebroad@83.240.144.127] has joined #bitcoin-core-pr-reviews 05:27 -!- rebroad [~rebroad@83.240.144.127] has quit [Disconnected by services] 05:34 < michaelfolkson> (Part 1 is good too but not related to this week's PR) 06:44 -!- seven_ [~seven@90.157.197.248] has quit [Read error: Connection reset by peer] 06:44 -!- seven_ [~seven@cpe-90-157-197-248.static.amis.net] has joined #bitcoin-core-pr-reviews 07:05 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 07:13 -!- belcher [~belcher@unaffiliated/belcher] has quit [Quit: Leaving] 08:36 < jarolrod> ⎦˚◡˚⎣ 08:57 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 09:18 -!- jadi [~jadi@213.207.207.253] has quit [Remote host closed the connection] 09:24 -!- larryruane__ [uid473749@gateway/web/irccloud.com/x-rhclrfnluqltlosi] has joined #bitcoin-core-pr-reviews 09:28 < larryruane__> Reviewing 21767, I want to test the rest changes, how can I hit those rest endpoints? 09:40 < MarcoFalke> larryruane__: There might be a functional test 09:41 < MarcoFalke> Not all rest endpoints are covered though 09:43 < larryruane__> thanks 09:44 -!- Larsson [~l@2804:14d:72ad:842b::1000] has joined #bitcoin-core-pr-reviews 09:45 < jnewbery> Hi folks. We'll get started in about 15 minutes. Hope you're all ready to REFACTOR 09:49 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews 09:53 -!- gimballock [92a86696@146.168.102.150] has joined #bitcoin-core-pr-reviews 09:53 -!- marqusat [marqusat@gateway/vpn/protonvpn/marqusat] has joined #bitcoin-core-pr-reviews 09:54 * dongcarl is ready 09:56 < jarolrod> dongcarl: underrated bitcoin lyricist 09:56 -!- brad [uid498683@gateway/web/irccloud.com/x-ssjtxzrcvwczszdx] has quit [Quit: Connection closed for inactivity] 09:56 < dongcarl> Hehe 09:59 -!- levitate [~levitate@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 10:00 -!- dkf [58a6b2e4@88.166.178.228] has joined #bitcoin-core-pr-reviews 10:00 -!- YErzhan [36f0c5ee@54-240-197-238.amazon.com] has joined #bitcoin-core-pr-reviews 10:00 < jnewbery> #startmeeting 10:00 -!- ben13 [bd7a7966@189.122.121.102] has joined #bitcoin-core-pr-reviews 10:00 < levitate> hi 10:00 < marqusat> hi 10:00 < jnewbery> Hi folks! Welcome to Bitcoin Core Review Club. 10:00 < willcl_ark> hi 10:00 < michaelfolkson> hi 10:00 < jarolrod> hi 10:00 < larryruane__> hi 10:00 < glozow> hi 10:00 < jnewbery> This week, we'll be talking about PR 21767. Notes and questions are on the website: https://bitcoincore.reviews/21767 10:00 < nehan_> hi 10:00 < jnewbery> Feel free to say hi to let everyone know you're here. 10:00 < dongcarl> hi 10:00 < jnewbery> Is anyone here for the first time? 10:00 < dkf> hi 10:01 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews 10:01 < schmidty> hi 10:01 < amiti> hi 10:02 -!- nehan_ is now known as nehan 10:02 < jnewbery> I'll be asking questions from https://bitcoincore.reviews/21767 to guide the conversation, but feel free to jump in with questions at any time. 10:02 < YErzhan> Hi thanks! 10:03 < jnewbery> Who had a chance to review the PR? (y/n) 10:03 < jarolrod> y 10:03 < levitate> n 10:03 < michaelfolkson> y 10:03 < willcl_ark> y 10:03 < hernanmarino> hi ! 10:03 < nehan> y 10:03 < marqusat> just at high-level 10:03 < amiti> n 10:03 < glozow> n 10:03 < jnewbery> Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach? 10:04 -!- biteskola [~biteskola@170.76.76.188.dynamic.jazztel.es] has joined #bitcoin-core-pr-reviews 10:04 < hernanmarino> y, high level only at the notes. didn' t review de PR yet 10:04 < marqusat> Concept ACK 10:04 < ben13> Concept ACK. Reducing the use of global variables is good. 10:04 -!- biteskola_ [~biteskola@170.76.76.188.dynamic.jazztel.es] has joined #bitcoin-core-pr-reviews 10:04 < michaelfolkson> I'm a Concept ACK on the 7 PRs but it takes a while to understand the Approach ACKs on the individual PRs and still not fully there 10:04 -!- azorcode [c9d0f466@201.208.244.102] has joined #bitcoin-core-pr-reviews 10:04 < jarolrod> concept ack, don't like that we are exposing CChain in b04370d, but this can be addressed in a follow-up making changes to how we interact with that 10:04 < larryruane__> yes, minimal testing, concept & approach ACK 10:05 -!- biteskola_ [~biteskola@170.76.76.188.dynamic.jazztel.es] has quit [Client Quit] 10:05 < willcl_ark> Definite concept ACK. I also really appriciate the way dongcarl splits the commits so meticulously to make review logical/simple where possible 10:05 < jnewbery> jarolrod: Yes, let's talk about CChain a bit later! 10:05 < dongcarl> Thanks willcl_ark! 10:05 < michaelfolkson> I guess we are focusing on this particular PR rather than all 7 10:05 < jonatack> hi 10:05 < jnewbery> willcl_ark: totally agree. These PRs are a joy to review 10:05 < jarolrod> michaelfolkson: yep 10:05 < glozow> there was a review club on the full one iirc 10:06 < glozow> https://bitcoincore.reviews/20158 10:06 < michaelfolkson> glozow: https://bitcoincore.reviews/20158 10:06 < michaelfolkson> Yeah I guess that one was for high level all 7 10:06 -!- turpintinz [88248599@136.36.133.153] has joined #bitcoin-core-pr-reviews 10:06 < jnewbery> ok, next question: This PR is a refactor and is not supposed to change any functional behaviour. What are some ways we can verify that behaviour doesn’t change? 10:07 < ben13> Running functional testes 10:07 < willcl_ark> I was thinking you could *not* apply commits which touch the functional tests and check that all (the old) tests still pass with the new code 10:07 < YErzhan> Bitcoin CI system should catch that by running unit tests 10:07 < marqusat> by running tests (potentially expanding tests first in the case of any gaps in coverage identified) 10:07 < larryruane__> running unit tests, starting up a real node (letting it sync with the network) 10:07 -!- Larsson [~l@2804:14d:72ad:842b::1000] has quit [Ping timeout: 260 seconds] 10:08 < glozow> I sometimes add a bunch of asserts for would-be-obvious things and run functional tests 10:08 < jarolrod> through good testing, but the assert statements give some confidence to the move 10:08 < jnewbery> excellent answers everyone! 10:09 < michaelfolkson> glozow: So too obvious asserts that shouldn't be added to the code? 10:09 < jnewbery> ok, let's move on to the next question: This series of PRs is part of a larger project to “modularize our consensus engine”. What are some of the benefits of doing that? 10:09 < ben13> Code easier to maintain and reuse. 10:09 < michaelfolkson> glozow: I guess things you want to assert yourself but might be obvious to others 10:09 < jnewbery> ben13: I agree. Can you expand a bit? 10:10 < marqusat> easier to reason about the code, better configurability and testability 10:10 < willcl_ark> A single modular "consensus engine" is easier to reason about from a bug perspective than consensus codepaths which spread over multiple modules. The initalisation benefits for testing also seem pretty appetising. 10:10 < dkf> decouple it from the core engine to expose as little as needed for security and maintainability 10:10 < jarolrod> as to the greater bitcoin ecosystem, plug and play consensus modules for developers. Confidence that your bitcoin application won't fork you off the chain because you are just using the core consensus engine 10:10 < jarolrod> very cool chaincode labs podcast episode 10:10 < YErzhan> Easier to fork bitcoin into new altcoins for everyoneX-P 10:10 < ben13> "very cool chaincode labs podcast episode" where ? jarolrod 10:10 < jarolrod> it's also cleaner code for bitcoin core devs 10:11 < larryruane__> makes testing easier; global variables are bad because a test may change them, and then the next test is messed up ... so each test must remember to restore their initial states 10:11 < glozow> michaelfolkson: yes. such as checking that 2 numbers add up to what we expect, a data member is properly filled, a lock is held, etc. 10:11 < jnewbery> marqusat: yes. Can you give a bit more detail on configurability and testability? 10:11 < jarolrod> ben13: https://podcast.chaincode.com/2020/12/15/carl-dong-2.html 10:11 < ben13> Thanks jarolrod 10:11 < michaelfolkson> ben13: Or transcript https://btctranscripts.com/chaincode-labs/chaincode-podcast/2020-11-30-carl-dong-reproducible-builds/ 10:12 < ben13> Thanks michaelfolkson 10:12 < jnewbery> larryrane__: Yes, this should make testing much easier, and allow more test setups 10:12 < glozow> michaelfolkson: sometimes i just add many copies of the same assert before and after calls. would be too verbose to actually add 10:12 < michaelfolkson> glozow: Cool, makes sense 10:12 < dongcarl> assert-based code exploration is underrated for how powerful it is 10:12 < ben13> There is also some configuration in chainparams.{h,cpp} 10:13 -!- Larsson [~l@187.107.10.117] has joined #bitcoin-core-pr-reviews 10:13 < marqusat> jnewbery: launch params can be used to configure non-global data 10:13 < jnewbery> No-one has mentioned using debuggers to step through the code. Does anyone use that regularly? 10:13 < dongcarl> marqusat: yes, that one is pretty big in my opinion 10:14 < larryruane__> an important thing in unit testing is that each test should run independently -- if a test passes (or fails) as part of a group of tests, the same result should happen if run individually 10:14 < levitate> jnewbery as in: removing globals helps debugger use or no one has mentioned using a debugger to audit a PR? 10:14 < michaelfolkson> jnewbery: I have used pdb on functional tests (Python) but haven't used debuggers on C++ code enough yet 10:14 < larryruane__> jnewbery: yes i actually have been stepping through this PR's code 10:15 < jnewbery> marqusat: right, and we can construct the components with parameters, such as here for PeerManager: https://github.com/bitcoin/bitcoin/blob/6b49d88a5dda97fdbabe363fd7d3c4f1ce29082a/src/net_processing.h#L37-L39 10:15 < ben13> I use gdb in VS Code to debug Bitcoin Core. 10:15 < jnewbery> and that gives us more control over testing those objects with different configuration 10:15 < larryruane__> if using debugger, important to build with `CONFIGURE_FLAGS='CXXFLAGS=-O0'` 10:15 < michaelfolkson> Also this enables projects like jamesob AssumeUTXO https://github.com/bitcoin/bitcoin/pull/17737 10:16 < jnewbery> michaelfolkson: I think those things are distinct. Maybe AssumeUTXO is easier with a better defined interface with validation though 10:16 < dongcarl> larryruane__: `--enable-debug` also works I think 10:16 < ben13> AssumeUTXO requires 2 chainstates 10:17 < michaelfolkson> jnewbery: Maybe this particular PR is distinct. I think the 7 PRs on the whole help enable AssumeUTXO 10:17 < larryruane__> dongcarl: yes I think that enables more checking code as well, good point 10:17 < ben13> Is there any difference between `--enable-debug` and `CONFIGURE_FLAGS='CXXFLAGS=-O0'` ? 10:17 < jnewbery> I think it might be useful to do a PR review club just on using gdb/lldb. They're really powerful tools 10:17 < jarolrod> jnewbery: that would be nice 10:17 < larryruane__> i'd be willing to contribute (to leading) 10:17 < jnewbery> if anyone feels like hosting that, let me know! 10:17 < ben13> Great. I would be great. 10:18 < glozow> i nominate larryruane__ 10:18 < willcl_ark> I would certainly be interested at becoming more proficient in them 10:18 < michaelfolkson> +1 10:18 < jnewbery> larryruane__: thank you! 10:18 < biteskola> +1 10:18 < larryruane__> except I don't know VSCode so maybe have someone else for that 10:18 < jnewbery> Next question: Briefly, what are each of the following classes responsible for: 10:18 < jnewbery> - ChainstateManager 10:18 < jnewbery> - CChainState 10:19 < jnewbery> - CChain 10:19 < jnewbery> - BlockManager 10:19 < ben13> ChainstateManager -> manages 2 differents chainstats (IBD and snapshot) 10:19 < ben13> CChainState -> represents the current UTXO (cache and disk - the chainstate/* LevelDB database) 10:19 < jnewbery> (and bonus question: why do CChainState and CChain have a leading 'C', but ChainstateManager and BlockManager don't?) 10:19 < ben13> CChain -> an in-memory indexed chain of blocks. 10:19 < ben13> BlockManager -> Maintains a tree of blocks (stored in `m_block_index`) 10:19 -!- effexzi [uid474242@gateway/web/irccloud.com/x-ovrcsphdjheodcqc] has joined #bitcoin-core-pr-reviews 10:20 < YErzhan> Satoshi made a typo? 10:20 < glozow> c = class, hungarian style naming? 10:20 < larryruane__> jnewbery: because not having the C is the new convention (the old is C for Class) 10:20 < jnewbery> ben13: very good. CChainState contains more than just the UTXO set though. 10:21 < ben13> hungarian style. Isn't this used in Bitcoin Core code anymore ? 10:21 < willcl_ark> CChainState is an general API into the details of our current most work chain. 10:22 < larryruane__> ben13: no we're getting away from it as code is added / changed 10:22 < jnewbery> Right. Satoshi used hungarian notation (https://en.wikipedia.org/wiki/Hungarian_notation) where the type of the variable is reflected in the name, eg CChainState is a Class, fDiscover is a flag (bool), nBestScore is an int, etc 10:22 < jamesob_> willcl_ark: that's less true now that CChainState can correspond to a background validation chainstate 10:22 < michaelfolkson> Global ChainstateManager = g_chainman 10:22 < YErzhan> If CChainState contains UTXO, why not name it UTXOState for simplicity? 10:22 < jamesob_> YErzhan: it contains more than just the UTXO set 10:23 < jnewbery> the current style is not to use hungarian notation. We use m_ to represent member variables, g_ to represent globals, and no prefix to represent local variables/parameters. 10:23 < larryruane__> may be useful? https://doxygen.bitcoincore.org/class_chainstate_manager.html https://doxygen.bitcoincore.org/class_c_chain_state.html 10:23 < YErzhan> So, why prefix with C then? 10:23 < jamesob_> (and ryanofsky has been using camelCaseForMethods recently) 10:24 < michaelfolkson> I looked up when CChain was introduced. sipa in 2013 removing other globals :) https://github.com/bitcoin/bitcoin/pull/3077 10:24 < ben13> YErzhan C means Class 10:24 < willcl_ark> interesting thanks jamesob_ 10:24 < ben13> I think 10:24 < jnewbery> funnctions/methods use CamelCase. The interface methods in src/interface/* are the same except the first character is lower case (eg acceptToMemoryPool instead of AcceptToMemoryPool) 10:25 < YErzhan> ben13 BlockManager is also a class I think 10:26 < jnewbery> I think the various classes (ChainstateManager/CChainstate/CChain/BlockManager) that are used as an interface into validation are too low-level and expose too much of validation's implementation to the rest of the program, but that can be cleaned up once we have a well-defined interface 10:26 < jamesob_> jnewbery: dumb nit, but it's actually called PascalCase (thisIsCamelCase) 10:27 < ben13> YErzhan It is a recent implementation, It does not use Hungarian style. 10:27 < jnewbery> jamesob_: interesting. Wikipedia claims that camel case refers to both: https://en.wikipedia.org/wiki/Camel_case, but PascalCase specifically means with leading uppercase 10:27 < dongcarl> jnewbery: Agree that the interfaces can be tidied! 10:27 < ben13> ChainstateManager and BlockManager are more recent. CChainstate and CChain is legacy. I think. 10:28 < YErzhan> So, why not refactor for consistency? 10:28 < jnewbery> and dromedaryCase is with leading lowercase 10:28 < jnewbery> ben13: the naming is legacy, the classes are very much still used as part of the interface 10:28 < ben13> Yes. The naming is legacy. 10:29 < jnewbery> YErzhan: we discourage changes that are aesthetic refactors only. We'll change naming as that code is touched, otherwise it's unnecessary churn 10:29 < jamesob_> YErzhan: refactoring core code is tricky. Too much code churn at once creates review burden, potential for error, and can hide vulnerabilities, so refactoring has to be done sparingly and usually in the service of a very concrete goal 10:29 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 252 seconds] 10:29 < michaelfolkson> jnewbery: Churn as in rebasing of open PRs and docs? Anything else that is being churned? 10:30 < larryruane__> jamesob_: Yes, and another thing is, we should always be aware of how many projects have forked Core, and made their own changes! Nightmare for them to merge changes 10:30 < jnewbery> dongcarl: that wasn't a criticism of this PR, by the way. Clarifying the interface is a prerequisite to cleaning it up later 10:30 < glozow> increases depth for git blames 10:30 < dongcarl> :-) 10:30 < michaelfolkson> glozow: Haha 10:31 < dongcarl> at some point we should maintain a --ignore-revs-file 10:31 < jnewbery> michaelfolkson: churn as in changes to the codebase. Every time that you rename something in a header file, everyone needs to recompile any file that includes that header. 10:31 < dongcarl> (and put all the chainman refactor commits there) 10:31 < michaelfolkson> jnewbery: Gotcha, thanks 10:31 < jnewbery> and if you're switching branches often and rebuilding, that becomes very tiresome 10:31 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 10:32 < jnewbery> ok, any more questions about the various classes we're dealing with, or is everyone happy to move on? 10:32 < willcl_ark> is CChain only headers, or headers and blocks? Curious as it says "in-memory" 10:32 < larryruane__> I'm sorry we were talking about naming a lot, what is the substantive differences among these 4 classes? 10:33 < michaelfolkson> jnewbery: What is your best short definitions of those four? Or how you think of them? 10:34 < dongcarl> ChainstateManager manages multiple CChainState's, which each manages a single CChain 10:34 < jnewbery> the doxygen comments are good summaries: 10:34 < jnewbery> ChainstateManager: Provides an interface for creating and interacting with one or two chainstates 10:34 < jnewbery> CChainState: Stores and provides an API to update our local knowledge of the current best chain. 10:34 < jnewbery> CChain: An in-memory indexed chain of blocks. 10:34 < jnewbery> BlockManager: Maintains a tree of blocks (stored in `m_block_index`) which is consulted to determine where the most-work tip is. 10:34 < jamesob_> willcl_ark: it's only CBlockIndex instances, which provide information about how to retrieve the block data from disk 10:34 < willcl_ark> ah 10:34 < michaelfolkson> Ah I didn't see the doxygen comments, cool 10:35 < ben13> The 2 CChainStates are the IBD one and the snapshot one. 10:35 < dkf> ben13: what does IBD mean? 10:35 < ben13> Initial Block Download 10:36 < jnewbery> larryruane__: does that answer your question, or do you have specific questions? 10:36 < larryruane__> jnewbery: that was perfect, thanks 10:36 < jnewbery> dkf: IBD is the initial sync where your node downloads and validates the complete historic block chain 10:36 < YErzhan> In what scenarios we might need 2 ChainStates? 10:36 < dkf> thanks ben13, it has to do with this then re: syncing node https://bitcoin.org/en/full-node#special-cases 10:37 < jarolrod> YErzhan: you have an IBD chainstate and you could have a snapshot chainstate from passing in a assumeutxo snapshot 10:37 < ben13> 2 chainstates are required for AssumeUTXO. One cs is the default, from block sync. And the other, you download the UTXO Set from your peers. 10:38 < jarolrod> YErzhan: only one of these will be the active chainstate though 10:38 < jnewbery> dkf: did you mean to link to https://bitcoin.org/en/full-node#initial-block-downloadibd ? 10:38 < dkf> jnewberry: yes 10:38 < ben13> Yes, just one. There is a `m_active_chainstate` member, something like that 10:39 < jnewbery> I'll ask the next question, but if there's anything that's still unclear about the classes, please ask 10:39 < jnewbery> What is cs_main? Why does it feature so prominently in the changes in this PR? 10:39 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 10:39 < larryruane__> important thing to help you understand `CChain` is it contains a `std::vector` (of block index pointers) 10:39 < larryruane__> indexed by height (i believe) 10:39 < jarolrod> looks like cs_main locks access to CChainstate's, as so doesn't it make sense to refactor cs_main to be more self contained inside of the ChainstateManager interface 10:39 < ben13> It is a lock to synchronize changes in the Chain State. 10:39 < michaelfolkson> This AssumeUTXO use case could be verifying different portions of the blockchain in parallel....? 10:40 < willcl_ark> cs_main is a recursive mutex which is used to ensure that validation is carried out in an atomic way. 10:40 < marqusat> Mutex to guard access to validation specific variables (such as chainstate) or updating mempool 10:40 < michaelfolkson> 0-25% of the blockchain being verified at the same time as 25-50% of the blockchain 10:40 < ben13> Multiple threads can access the chain state at the same time. cs_main syncs this. 10:40 -!- YErzhan [36f0c5ee@54-240-197-238.amazon.com] has quit [Quit: Connection closed] 10:41 < jnewbery> ben13 willcl_ark marqusat: very good. Yes, it's a mutex that protects validation-specific data. 10:41 < jnewbery> it also protects a whole load of other stuff, mostly in net_processing 10:41 < larryruane__> jnewbery: I think it figures prominently in this PR because it protects the global `g_chainman` and reducing use of that variable means reducing use of `cs_main` (great!) 10:41 < jamesob_> michaelfolkson: in concept, yes - the n-chainstate approach would be abstracted under the chainstatemanager 10:41 < jnewbery> any idea why it's called cs_main? 10:41 -!- YErzhan [36f0c5ee@54-240-197-238.amazon.com] has joined #bitcoin-core-pr-reviews 10:42 < larryruane__> `main.cpp` ? 10:42 < jnewbery> larryruane__: bingo! 10:42 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 10:42 < ben13> Was ChainState originally in main.cpp ? 10:42 < jnewbery> the code that is now in validation.cpp and net_processing.cpp used to be in one file called main.cpp 10:42 < larryruane__> ben13: i think everything was originally there :) 10:42 < michaelfolkson> jamesob_: So you get that n high enough and you are looking at a *very* quick verification time. Any limit on that n? 10:42 < jnewbery> and cs_main was the mutex ("critical section") that protected data in main.cpp 10:43 < levitate> When the word "snapshot" is used in. CChainStates, what does that word mean in that context? is it: an externally submitted chainstate the node will consider potentially valid? 10:43 < ben13> larryruane__ it is true 10:43 < larryruane__> levitate: excellent question, wondering the same thing 10:43 < jnewbery> a long-term goal is to separate out those different bits of data that are protected by cs_main, so that eventually cs_main only protects the data inside validation 10:44 < larryruane__> if this isn't too much of a sidetrack, jnewbery can you give us a one-line summary of what "validation" includes? 10:45 < larryruane__> i know there's `validation.{h,cpp}` but conceptually? 10:45 < larryruane__> (it's ok if we want to move on) 10:45 < michaelfolkson> jamesob_: And you do say "in concept" too :) So that wouldn't currently be possible if all the assumeutxo PRs were merged? 10:46 < ben13> levitate I think so. There will be the hash of UTXO Set hardcoded for checkpoint, like `assumevalid` I think. 10:46 -!- kiltzman [~k1ltzman@195.189.99.96] has quit [Ping timeout: 252 seconds] 10:46 < jnewbery> levitate: snapshot in this instance refers to a chainstate that is based on a UTXO set that is provided to your node (and doesn't start from the genesis block). It's part of the AssumeUTXO project. There's excellent documentation here: https://github.com/bitcoin/bitcoin/issues/15605 10:47 * levitate mentally maps the word snapshot -> UTXO_proposal 10:47 < jnewbery> larryruane: conceptually validation stores and maintains our best view of the blockchain and associated UTXO set. It also includes an interface to submit unconfirmed transactions to the mempool 10:48 < larryruane__> jnewbery: thank you, that's very helpful 10:48 -!- YErzhan [36f0c5ee@54-240-197-238.amazon.com] has quit [Quit: Connection closed] 10:48 < michaelfolkson> levitate: It can be used to skip a lot of verification of the blockchain (which would be frowned upon if you are introducing trust in a snapshot). But it also could be used (in concept) to verify different parts of the blockchain at the same time 10:48 < jnewbery> Next question: In commit rest: Add GetChainman function and use it, what does the EnsureAnyChainman() function do? Why is it being removed from rest.cpp? 10:49 < willcl_ark> I think EnsureAnyChainman is determining whether we have a synced chain which we can use to answer RPC calls 10:50 < marqusat> extracts ChainstateManager reference or throws exception. it’s being replaced with GetChainman which returns nullptr instead of throwing exception. nullptr is then handled not crashing the execution 10:50 < willcl_ark> we are now delegating this 10:50 -!- kiltzman [~k1ltzman@195.189.99.96] has joined #bitcoin-core-pr-reviews 10:51 < jnewbery> right, the Ensure* functions are used in the RPC code and throw exceptions. Those exceptions are caught and returned to the client as errors. 10:52 < jnewbery> The REST code isn't wrapped in a try/except, so if that function throws then the program aborts 10:52 < jnewbery> so this PR replaces a call to Ensure* with code that won't throw if there isn't a chainman 10:53 < larryruane__> jnewbery: oh so this PR includes a bug fix? 10:53 < dongcarl> larryruane__: A bug introduced by me in the last bundle unfortunately :-( 10:53 < larryruane__> :) oh i see 10:53 < willcl_ark> hehe 10:54 < michaelfolkson> dongcarl: Can you explain the bug? 10:54 < jnewbery> if you introduce bug in a PR in a series it really motivates people to review the next one :) 10:54 < michaelfolkson> Haha deliberate strategy 10:54 < jnewbery> next question: 10:54 < jnewbery> What does the following code do? 10:54 < jnewbery> assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman)); 10:54 < jnewbery> Why is it added in this PR (and other PRs in the series)? 10:55 < larryruane__> is it temporary to ensure correct transition (to eliminating g_chainman completely)? 10:56 < larryruane__> baby steps :) 10:56 < jnewbery> larryruane__: yes! 10:56 < dongcarl> michaelfolkson: As jnewbery said: RPC code wraps all calls in a try/except, REST code does not. Ensure*, being part of RPC, expects that its throw's will get caught by a try/except. But if you use Ensure* in REST code, since it doesn't have a try/except wrap, a crash will happen. 10:56 < willcl_ark> checks that the global is using the same object as the new reference? 10:56 < willcl_ark> well, asserts! 10:57 < jnewbery> willcl_ark: exactly 10:57 < jnewbery> https://github.com/bitcoin/bitcoin/pull/21866 finally removes g_chainman, and removes all of these temporary asserts too 10:57 < larryruane__> just wondering for testing, is there a way to start a local bitcoind and hit those rest endpoints? (I'm also curious how the rest interface works) 10:57 < michaelfolkson> dongcarl: So using the RPC resulted in crashing after the PR was merged? 10:58 < larryruane__> michaelfolkson: i think only rest, not RPC 10:58 < jnewbery> I think these temporary asserts are a good idea to give us confidence in this refactor. The first PRs in the series were merged several weeks/months ago, and the asserts haven't been hit, so we can be quite confident that the objects really are the same objects! 10:58 < michaelfolkson> larryruane__: Oh sorry I misread, right 10:59 < dongcarl> The asserts helped me find things that resulted in: #20323 10:59 < michaelfolkson> larryruane__: Using REST after that PR was merged resulted in crashes 10:59 < jnewbery> There's one final question about raw pointers and smart pointers, but we'll leave that as an exercise for the reader 10:59 < jnewbery> any final questions in the last minute? 10:59 < larryruane__> could the assert compare g_chainman to chainman? 11:00 < jnewbery> dongcarl: very cool 11:00 < willcl_ark> larryruane__: without using std::addressof()? 11:00 -!- ben13 [bd7a7966@189.122.121.102] has quit [Quit: Connection closed] 11:00 < dongcarl> larryruane__: Sure! That would have worked too 11:01 -!- ben62 [bd7a7966@189.122.121.102] has joined #bitcoin-core-pr-reviews 11:01 -!- turpintinz [88248599@136.36.133.153] has quit [Quit: Connection closed] 11:01 < jnewbery> ok, that's time. Thanks dongcarl for giving us a very nice PR to review and talk about :) 11:01 < jnewbery> #endmeeting 11:01 < biteskola> very educational. thanks! : D 11:01 < marqusat> thanks everyone! 11:01 < dongcarl> std::addressof (IIRC) bypasses the possibility of an overridden operator&, and is not used in the codebase so it's easy to scripted-diff remove 11:01 < jarolrod> Thanks! 11:01 < schmidty> Thanks jnewbery (and dongcarl )! 11:01 < dongcarl> Thank you for hosting jnewbery! 11:02 < ben62> Thanks ! 11:02 < jonatack> thanks jnewbery! 11:02 < michaelfolkson> Thanks jnewbery dongcarl, very cool 11:02 < jonatack> (ty dongcarl!) 11:02 < levitate> thank you jnewbery dongcarl ' 11:02 < jnewbery> thanks everyone for coming. This was lots of fun. Next week, we have glozow hosting something wallety. 11:03 < glozow> thanks jnewbery! 11:03 < willcl_ark> thanks jnewbery! reallhy enjoyed that PR 11:03 -!- azorcode [c9d0f466@201.208.244.102] has quit [Quit: Connection closed] 11:03 -!- ben62 [bd7a7966@189.122.121.102] has quit [Client Quit] 11:03 -!- marqusat [marqusat@gateway/vpn/protonvpn/marqusat] has quit [Quit: Leaving] 11:04 -!- dkf [58a6b2e4@88.166.178.228] has quit [Quit: Connection closed] 11:04 -!- biteskola [~biteskola@170.76.76.188.dynamic.jazztel.es] has quit [Quit: Saliendo] 11:05 < jonatack> Meeting log is up at https://bitcoincore.reviews/21767#meeting-log 🍰 Yᵒᵘ Oᶰˡʸ Lᶤᵛᵉ Oᶰᶜᵉ 11:06 < levitate> ty jonatack 11:09 < michaelfolkson> jonatack: Can you make requests on what color your name is in the log? :) Shotgun not pink 11:10 < jonatack> re PascalCase / CamelCase, in our developer notes we distinguish between them by calling them UpperCamelCase, CamelCase, and...lowerCamelCase ⚡ 11:10 < jonatack> michaelfolkson: u can haz pink or HOT PINK 11:10 < jonatack> (haawwwwt pink) 11:11 < michaelfolkson> Ok I guess I'll go for the latter 11:11 < jonatack> (iirc it's random cycling through a fixed palate of color tones) 11:11 < michaelfolkson> Damn randomness 11:13 < jonatack> code is here, if you wanna propose improvements https://github.com/bitcoin-core-review-club/website/blob/master/_plugins/auto_logs_markup.rb 11:14 < jonatack> i think we spent about as much time on adding those colors as on the bitcoin core consensus separation project 11:15 < michaelfolkson> if nick = michaelfolkson color = hawwwwt pink 11:15 < michaelfolkson> (Translate to Ruby) 11:16 < jonatack> oh it's deterministic, not random, the colors index is seeded from part of the meeting content (it's been awhile since i wrote or looked at it) 11:17 < jonatack> color_index = (output[-256..-250] || '').bytes.sum 11:17 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 11:17 < jonatack> seeded using a few bytes of the content ~FF chars from the end of the meeting hehe 11:18 < michaelfolkson> I thought you were joking. The color is literally hotpink, no pink though 11:18 < jonatack> yeah, the colors had to work on both light and dark mode 11:19 < jonatack> i originally had like 3 times more colors but we ended up weeding out the harder-to-read ones 11:19 < michaelfolkson> Have to say hi after a certain number of other people to avoid hotpink 11:20 < michaelfolkson> Ok enough color tawk. Impressive work though. It is the little things 11:20 < jonatack> well, it depends on a short byte sequence 250-256 characters from the end, so pretty hard to predict 11:21 < jonatack> we do what we can, my onel 11:21 < jonatack> my one-liner about validation.cpp is it's like MC Hammer "U Can't Touch This", so far i never have 11:24 < michaelfolkson> 77 contributors have touched it 11:24 < michaelfolkson> Just not part of the select group jonatack. A typo PR is called for I think 11:25 < jonatack> our day will come 11:25 < jonatack> i ramp up slowly ;) 11:27 < jonatack> (never is fine too!) 11:27 < michaelfolkson> Pro ossification of validation.cpp 11:34 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 11:50 -!- levitate [~levitate@195.181.160.175.adsl.inet-telecom.org] has quit [Ping timeout: 240 seconds] 12:02 -!- tyler [uid499566@gateway/web/irccloud.com/x-mvjktsbiehrrpgco] has joined #bitcoin-core-pr-reviews 12:04 -!- tyler52 [c636833c@static-198-54-131-60.cust.tzulo.com] has joined #bitcoin-core-pr-reviews 12:04 -!- tyler52 [c636833c@static-198-54-131-60.cust.tzulo.com] has quit [Client Quit] 12:10 * tyler hello 12:11 -!- Larsson [~l@187.107.10.117] has quit [Quit: Konversation terminated!] 12:12 -!- barticus [c636833c@static-198-54-131-60.cust.tzulo.com] has joined #bitcoin-core-pr-reviews 12:12 -!- barticus [c636833c@static-198-54-131-60.cust.tzulo.com] has quit [Client Quit] 12:14 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!] 12:23 -!- tyler [uid499566@gateway/web/irccloud.com/x-mvjktsbiehrrpgco] has quit [] 12:29 -!- effexzi [uid474242@gateway/web/irccloud.com/x-ovrcsphdjheodcqc] has quit [Quit: Connection closed for inactivity] 12:38 -!- patriot2939 [~patriot29@unaffiliated/patriot2939] has joined #bitcoin-core-pr-reviews 12:48 -!- gimballock [92a86696@146.168.102.150] has quit [Quit: Connection closed] 12:57 -!- queip [~queip@unaffiliated/rezurus] has quit [Remote host closed the connection] 13:00 -!- queip [~queip@unaffiliated/rezurus] has joined #bitcoin-core-pr-reviews 13:08 -!- queip [~queip@unaffiliated/rezurus] has quit [Ping timeout: 260 seconds] 13:24 -!- larryruane__ [uid473749@gateway/web/irccloud.com/x-rhclrfnluqltlosi] has quit [Quit: Connection closed for inactivity] 14:43 -!- achow101 [~achow101@unaffiliated/achow101] has quit [Quit: Bye] 14:44 -!- achow101 [~achow101@unaffiliated/achow101] has joined #bitcoin-core-pr-reviews 14:55 -!- Traca [~Traca@gateway/tor-sasl/traca] has joined #bitcoin-core-pr-reviews 14:56 -!- Traca [~Traca@gateway/tor-sasl/traca] has quit [Client Quit] 15:00 -!- patriot2939 [~patriot29@unaffiliated/patriot2939] has quit [Quit: Leaving] 15:04 -!- outfawkesd_ [outfawkesd@gateway/vpn/privateinternetaccess/outfawkesd] has quit [Quit: Leaving] 16:42 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection] 16:42 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews 16:47 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 260 seconds] 16:52 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection] 16:54 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 17:07 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 17:08 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews 17:10 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 17:35 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 17:38 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 17:39 -!- rebroad_ [~rebroad@83.240.144.127] has quit [Ping timeout: 260 seconds] 18:06 -!- rebroad [~rebroad@83.240.144.127] has joined #bitcoin-core-pr-reviews 18:16 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 18:18 -!- rebroad [~rebroad@83.240.144.127] has quit [Remote host closed the connection] 18:19 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 18:24 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 18:27 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 268 seconds] 18:49 -!- Jackielove4u [uid43977@gateway/web/irccloud.com/x-hkyujqcciflbuzjz] has quit [Quit: Connection closed for inactivity] 19:17 -!- rebroad [~rebroad@83.240.144.127] has joined #bitcoin-core-pr-reviews 19:22 -!- Henry151 [~bishop@151.80.44.70] has joined #bitcoin-core-pr-reviews 19:28 -!- rebroad [~rebroad@83.240.144.127] has quit [Remote host closed the connection] 19:31 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 19:33 -!- rebroad [~rebroad@83.240.144.127] has joined #bitcoin-core-pr-reviews 19:45 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 19:55 -!- rjected [~weechat-h@natp-128-119-202-67.wireless.umass.edu] has quit [Quit: WeeChat 3.1] 19:56 -!- rjected [~weechat-h@natp-128-119-202-67.wireless.umass.edu] has joined #bitcoin-core-pr-reviews 20:00 -!- rebroad [~rebroad@83.240.144.127] has quit [Ping timeout: 252 seconds] 20:38 -!- belcher_ [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 20:41 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 268 seconds] 20:42 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 20:44 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 20:44 -!- rebroad [~rebroad@83.240.144.127] has joined #bitcoin-core-pr-reviews 21:02 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has quit [Read error: Connection reset by peer] 23:06 -!- davterra [~davterra@gateway/tor-sasl/tralfaz] has quit [Remote host closed the connection] 23:07 -!- davterra [~davterra@gateway/tor-sasl/tralfaz] has joined #bitcoin-core-pr-reviews 23:07 -!- queip [~queip@unaffiliated/rezurus] has joined #bitcoin-core-pr-reviews 23:15 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Read error: Connection reset by peer] 23:17 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 23:21 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 23:24 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 265 seconds]