--- Log opened Wed Sep 15 00:00:09 2021
00:12 -!- stonefox [~stonefox@user/stonefox] has joined #bitcoin-core-pr-reviews
03:24 -!- Henrik [~textual@84.212.107.177] has joined #bitcoin-core-pr-reviews
04:39 -!- Henrik [~textual@84.212.107.177] has quit [Quit: My MacBook Air has gone to sleep. ZZZzzz…]
04:49 -!- Henrik [~textual@84.212.107.177] has joined #bitcoin-core-pr-reviews
04:50 -!- Henrik [~textual@84.212.107.177] has quit [Client Quit]
06:13 -!- yonson [~yonson@2600:8801:d900:7bb::d7c] has quit [Remote host closed the connection]
06:13 -!- yonson [~yonson@2600:8801:d900:7bb::d7c] has joined #bitcoin-core-pr-reviews
06:15 -!- ryanofsky [~russ@jumpy.yanofsky.org] has joined #bitcoin-core-pr-reviews
08:07 -!- Henrik [~textual@84.212.107.177] has joined #bitcoin-core-pr-reviews
08:10 -!- common [~common@096-033-221-075.res.spectrum.com] has quit [Quit: Leaving]
08:40 -!- Henrik [~textual@84.212.107.177] has quit [Quit: My MacBook Air has gone to sleep. ZZZzzz…]
09:49 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
10:00 < glozow> #startmeeting
10:00 < glozow> Hello friends! Welcome to PR Review Club! Feel free to say hi to let us know you're here
10:00 < sandipndev> hi!
10:01 < glozow> We're looking at PR#22677 today: "Cut the validation <-> txmempool circular dependency"
10:01 < emzy> hi
10:01 < glozow> PR: https://github.com/bitcoin/bitcoin/pull/22677 
10:01 < glozow> Notes: https://bitcoincore.reviews/22677
10:01 -!- pglazman [~pglazman@c-73-71-224-94.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews
10:01 -!- pg_ [uid518209@id-518209.hampstead.irccloud.com] has joined #bitcoin-core-pr-reviews
10:01 < schmidty> hi
10:02 < glozow> did anyone get a chance to look at the notes or review the PR?
10:02 -!- pg_ is now known as pg-
10:02 < emzy> n
10:02 -!- Sachin [~Sachin@c-73-252-206-163.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews
10:03 < sandipndev> n
10:03 -!- pg- is now known as pg2
10:03 < pg2> 0.5y
10:03 -!- Henrik [~textual@84.212.107.177] has joined #bitcoin-core-pr-reviews
10:03 < glozow> no problem. we'll start conceptual: What is a circular dependency, and why should we avoid circular dependencies?
10:03 -!- janb [~arjan@home.connected.by.freedominter.net] has joined #bitcoin-core-pr-reviews
10:04 < pg2> Two or more modules depend on each other
10:04 < larryruane> hi
10:04 < glozow> pg2: indeed!
10:04 < larryruane> testing is much easier
10:04 < glozow> and why are circular dependencies bad?
10:04 < sandipndev> we can't seperately use one module individually if another is linked with it (they might recursively call each other)
10:05 < glozow> sandipndev: right, it's much harder to isolate and test our code if it's tangled up with a bunch of other stuff
10:06 < glozow> What functionality lives in the validation module? What lives in txmempool?
10:06 < larryruane> with circular dependencies present, it's harder to get a clear mental model
10:06 < pg2> valiation.{h.cpp} update local knowledge of the current best chain and corresponding UTXO set, and process new blocks
10:06 < pg2> txmempool.{h.cpp} store the actual transaction in the pool (class `CTxMemPool`), and metadata about the transactions (class `CTxMemPoolEntry`)
10:07 < glozow> larryruane: yeah, i agree with that too!
10:08 < pg2> Removing circular dependencies is desirable, but are there any downside or risks for this kind of refactoring? (e.g. could this break any downstream systems built on top of Bitcoin Core?) 
10:09 < glozow> pg2: good answer. i might add (for the context of this PR) that txmempool is a "dumb" data structure that shouldn't know any details about consensus rules
10:09 < glozow> (that's my opnion though of course)
10:10 < sandipndev> i see there are a few expected circular dependencies, since circular deps are bad, why are they even present and expected?
10:10 -!- Sachin [~Sachin@c-73-252-206-163.hsd1.ca.comcast.net] has quit [Quit: Connection closed]
10:11 < glozow> pg2: good question, I suppose forks of bitcoin core could have some trouble incorporating our changes if our architectures diverge
10:11 < sipa> because the codebase is old, and works, so we don't want to throw it out
10:12 < glozow> for context, the circular dependencies linter was added in https://github.com/bitcoin/bitcoin/pull/13695
10:13 < glozow> listing them explicitly probably helps us see when they're removed and be warned if someone adds a new one
10:14 < glozow> having a list of expected circular dependencies allows us to use the linter without ripping everything out
10:14 < glozow> sandipndev: does that answer your question?
10:15 < sandipndev> yes, absolutely!
10:15 < glozow> cool! :)
10:15 < glozow> So, in case it wasn't clear, we currently have a circular dependency between txmempool and validation.
10:16 < glozow> There would theoretically be 2 ways to remove this: make txmempool not depend on validation anymore, or make validation not depend on txmempool anymore
10:16 < glozow> Why should/shouldn’t validation depend on txmempool? Why should/shouldn’t txmempool depend on validation?
10:16 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
10:17 < pg2> Validation should depend on txmempool, because we need to know what is in the mempool to validate new transactions
10:17 < glozow> pg2: very logical answer, I agree :D
10:17 < pg2> Txmempool shouldn't depend on validation, because the transactions in mempool are already validated (and therefore donot need to depend on anything in validation.{h.cpp}).
10:18 < glozow> i also think an argument could be made for neither depending on the other - you don't really need to know about the existence of a mempool in order to apply validation rules to a transaction
10:19 < pg2> glozow: thanks for your answers. after this PR, is txmempool "complete dumb", or "dumb enough"? Or there are places where it still knows about consensus rules (unnecessarily)?
10:20 < glozow> pg2: ooh good question
10:21 < glozow> since i've already started saying possibly controversial things... another thing the mempool probably doesn't need to be responsible for is the fee estimator
10:21 < _aj_> glozow: "neither depending on the other" -- isn't validation the *action* of accepting a block (which means the txs in the block should no longer be in the mempool since they're no longer valid on top of the new tip) ; the consensus rules themselves are in consensus/ and script/ and the like?
10:22 < _aj_> (also the action of accepting a tx into the mempool)
10:23 < glozow> _aj_: ah true, i completely agree. I guess in general I mean that consensus rules themselves (i.e. consensus/ and script/) and the mempool are not conceptually related
10:24 < glozow> but yes, the action of validating transactions would depend on both
10:26 < glozow> OK next question. One of the functions that causes txmempool to rely on validation is the `check()` function. What checks does it perform, and how does this PR split the checks into two categories?
10:27 < pg2> For every transaction input in mempool, `check()` checks if the input refers to any other mempool transactions, otherwise it checks if the previous transaction output is unspent.
10:28 < pg2> I don't find anywhere in `check()` checks the sender has enough balance to cover the sum of output amounts. Is there such a check somewhere? 
10:29 < glozow> pg2: see `CheckTxInputs` in src/consensus/tx_verify
10:29 < pg2> glozow: thanks
10:29 < glozow> https://github.com/bitcoin/bitcoin/blob/2161a058552ac938f2079b311a2d12f5d1772d01/src/consensus/tx_verify.cpp#L201-L206
10:30 < glozow> pg2: yes, that's one thing that `check()` does. what else?
10:30 -!- Henrik [~textual@84.212.107.177] has quit [Quit: My MacBook Air has gone to sleep. ZZZzzz…]
10:30 < glozow> (oops - the link I sent has the wrong line numbers. it should be a few lines above, the `bad-txns-in-belowout` check)
10:31 -!- pglazman [~pglazman@c-73-71-224-94.hsd1.ca.comcast.net] has quit [Quit: Textual IRC Client: www.textualapp.com]
10:33 < pg2> glozow: that's the only part where I finished reading the code, that's why I can only give a partial answer
10:33 < glozow> no worries. just seeing if anyone is willing to answer the rest :)
10:34 < glozow> so I've (arbitrarily) split the `check()` assertions into 2 categories: contextual checks and internal consistency checks. Contextual is what pg2 already mentioned - we go through and make sure all the transactions' inputs refer to something available in our mempool or UTXO set.
10:36 < glozow> Internal consistency checks are things like making sure the ancestor/descendant counts of each entry add up correctly, checking that our total fee accounting and dynamic memory usage are accurate, etc.
10:36 < glozow> It may or may not make sense to split `check()` up this way; up to you as a reviewer
10:37 < glozow> Moving on to next question: Another function that causes txmempool to rely on validation is the `removeForReorg()` function, which calls `CheckSequenceLocks()` and `CheckFinalTx()`. Why do we need to re-check sequence locks in a reorg?
10:38 < larryruane> Yes, I think this conceptual distinction between contextual and non-contextual checks appears also with respect to blocks
10:39 < larryruane> glozow: because time is "backing up" so a tx that used to be valid is no longer valid (yet)
10:40 < glozow> larryruane: right! but... how is it possible for a coinbase spend in the mempool to become premature in a reorg? Don't we only reorg when we see a longer chain?!
10:41 < larryruane> a reorg could result in a shorter chain, because it may have more _work_
10:41 < glozow> larryruane: aha!!!!
10:43 < _aj_> the first block in a multi-block reorg will be lower than the last block before the reorg even if difficulty doesn't change? [A B C D] -> [A E F G H], E has lower height than D?
10:43 -!- Sachin [~Sachin@136-24-115-54.cab.webpass.net] has joined #bitcoin-core-pr-reviews
10:44 -!- Sachin [~Sachin@136-24-115-54.cab.webpass.net] has quit [Client Quit]
10:44 < _aj_> (or are we only readding txs after processing all the reorg blocks somewhere?)
10:45 < sipa> _aj_: i believe so
10:45 -!- Sachin [~Sachin@136-24-115-54.cab.webpass.net] has joined #bitcoin-core-pr-reviews
10:46 < glozow> _aj_: righto. i think in a multi-block reorg we fill up a disconnectpool and then call `removeForReorg` after
10:46 < glozow> not 100% sure tho?
10:47 < glozow> will check
10:51 < glozow> next question is about lock annotations. what are we saying to the compiler when we annotate a callable with `EXCLUSIVE_LOCKS_REQUIRED`?
10:55 < _aj_> glozow: (hmmmm, i think i had a bug way back whenever when i was investigating something related to this then!)
10:56 < glozow> _aj_: (i am... about 16% confident in that statement)
10:58 < glozow> hok so the answer to my previous question about lock annotations, the answer is here: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#requires-requires-shared
10:59 < glozow> the rest of the questions are left as an exercise to the reader
10:59 < glozow> thanks for coming!
10:59 < glozow> #endmeeting
11:00 < pg2> thanks glozow!
11:01 < _aj_> glozow: (i just looked, and i'm about 50% confident in your statement, leading to a 90% reduction in confidence in what i previously thought i'd found)
11:01 < larryruane> thank you glozow!
11:01 < sandipndev> thanks glozow: this was my first day and i learnt a lot, you've been an amazing host! :D
11:01 < emzy> Thank you glozow
11:02 < glozow> sandipndev: ahh welcome as first-timer! i'm glad! and thanks for the lovely compliment ^_^
11:30 -!- Henrik [~textual@84.212.107.177] has joined #bitcoin-core-pr-reviews
11:41 -!- common [~common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
12:01 -!- Henrik [~textual@84.212.107.177] has quit [Quit: My MacBook Air has gone to sleep. ZZZzzz…]
12:03 -!- luke-jr [~luke-jr@user/luke-jr] has quit [Quit: ZNC - http://znc.sourceforge.net]
12:05 -!- luke-jr [~luke-jr@user/luke-jr] has joined #bitcoin-core-pr-reviews
12:08 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
13:41 -!- Sachin [~Sachin@136-24-115-54.cab.webpass.net] has quit [Quit: Connection closed]
14:18 -!- ghost43_ [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
14:21 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has quit [Ping timeout: 276 seconds]
16:26 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
17:21 -!- belcher [~belcher@user/belcher] has quit [Ping timeout: 268 seconds]
17:33 -!- belcher [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
19:42 -!- yonson [~yonson@2600:8801:d900:7bb::d7c] has quit [Remote host closed the connection]
19:42 -!- yonson [~yonson@2600:8801:d900:7bb::d7c] has joined #bitcoin-core-pr-reviews
21:06 -!- harding [quassel@newmail.dtrt.org] has quit [Quit: http://quassel-irc.org - Chat comfortably. Anywhere.]
21:18 -!- harding [~quassel@newmail.dtrt.org] has joined #bitcoin-core-pr-reviews
22:19 -!- Cory [~Cory@user/pasha] has quit [Ping timeout: 268 seconds]
23:05 -!- Cory [~Cory@user/pasha] has joined #bitcoin-core-pr-reviews
--- Log closed Thu Sep 16 00:00:09 2021