--- Log opened Wed Aug 03 00:00:40 2022
00:28 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 268 seconds]
00:32 -!- __gotcha [~Thunderbi@94.105.116.253.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
00:34 -!- brunoerg [~brunoerg@187.183.43.40] has joined #bitcoin-core-pr-reviews
00:37 -!- __gotcha [~Thunderbi@94.105.116.253.dyn.edpnet.net] has quit [Ping timeout: 240 seconds]
00:37 -!- __gotcha [~Thunderbi@94.105.116.253.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
00:38 -!- brunoerg [~brunoerg@187.183.43.40] has quit [Ping timeout: 252 seconds]
00:40 -!- brunoerg [~brunoerg@187.183.43.40] has joined #bitcoin-core-pr-reviews
00:45 -!- brunoerg [~brunoerg@187.183.43.40] has quit [Ping timeout: 240 seconds]
00:46 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
01:33 -!- ziggie [uid521459@user/ziggie] has joined #bitcoin-core-pr-reviews
01:48 -!- __gotcha [~Thunderbi@94.105.116.253.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
01:49 -!- __gotcha [~Thunderbi@94.105.116.253.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
01:49 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 268 seconds]
01:50 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
01:55 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 240 seconds]
02:01 -!- brunoerg [~brunoerg@187.183.43.40] has joined #bitcoin-core-pr-reviews
03:06 -!- brunoerg [~brunoerg@187.183.43.40] has quit [Ping timeout: 252 seconds]
03:14 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
03:21 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
03:21 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
03:32 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has quit [Remote host closed the connection]
03:33 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
03:38 -!- jonatack [~jonatack@user/jonatack] has quit [Quit: Connection closed]
04:10 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
04:18 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 268 seconds]
04:20 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
05:32 -!- __gotcha1 [~Thunderbi@94.105.116.253.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
05:32 -!- __gotcha [~Thunderbi@94.105.116.253.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
05:32 -!- __gotcha1 is now known as __gotcha
06:12 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 245 seconds]
06:19 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
06:20 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
06:25 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 268 seconds]
06:35 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
06:35 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
06:54 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
06:54 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
07:35 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
07:45 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Remote host closed the connection]
07:50 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
07:55 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Remote host closed the connection]
07:59 -!- rush is now known as sloorush
08:16 -!- __gotcha [~Thunderbi@94.105.116.253.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
08:22 -!- Zenton [~user@user/zenton] has quit [Ping timeout: 268 seconds]
08:51 -!- Zenton [~user@user/zenton] has joined #bitcoin-core-pr-reviews
09:49 -!- Amirreza [~Amirreza7@2.177.18.94] has joined #bitcoin-core-pr-reviews
09:52 -!- dongcarlalt [~dongcarla@4.53.92.114] has joined #bitcoin-core-pr-reviews
09:53 -!- dongcarlalt is now known as dongcarlimposter
09:53 < dongcarlimposter> Ping
09:54 < fanquake> pong
09:55 < dongcarlimposter> 😊
09:55 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
09:56 -!- hernanmarino [~hernanmar@186.153.60.185] has joined #bitcoin-core-pr-reviews
09:56 -!- sanya [~sanya@109-93-38-19.dynamic.isp.telekom.rs] has joined #bitcoin-core-pr-reviews
09:56 -!- juanca4 [~juanca@pool-74-96-218-208.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
09:56 -!- juanca4 [~juanca@pool-74-96-218-208.washdc.fios.verizon.net] has left #bitcoin-core-pr-reviews []
09:57 -!- juancama [~juancama@pool-74-96-218-208.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
09:57 -!- BlueMoon [~BlueMoon@dgb3.dgbiblio.unam.mx] has joined #bitcoin-core-pr-reviews
09:58 -!- adam2k [~adam2k@ip24-254-208-245.hr.hr.cox.net] has joined #bitcoin-core-pr-reviews
09:58 < adam2k> 👋 hello all
09:58 < juancama> hello!
09:58 < glozow> evanlinjin: sorry for the late reply. I should have said an *unconfirmed* tx with no descendants has a descendant count of 1. So a `OutputGroup` containing at least 1 unconfirmed UTXO will have `m_descendants` at least 1.
09:58 < glozow> ^If it only contains confirmed UTXOs, it's 0
09:59 < BlueMoon> Hello!!
09:59 < glozow> hi! if you're looking for PR review club, you're in the right place. we'll be starting in 1 minute :) 
09:59 < Amirreza> Hi
10:00 < glozow> #startmeeting
10:00 < stickies-v> hi!
10:00 < glozow> Welcome to review club everyone! Today, we're looking at #25527: https://bitcoincore.reviews/25527
10:00 < larryruane> hi!
10:00 < michaelfolkson> hi
10:01 -!- Lov3r_Of_Bitcoin [~Lov3r_Of_@45-27-31-99.lightspeed.sntcca.sbcglobal.net] has joined #bitcoin-core-pr-reviews
10:01 < juancama> Hey everybody
10:01 < svav> Hi
10:01 < Lov3r_Of_Bitcoin> hello
10:01 < glozow> This PR has a pretty small diff and doesn't change behavior, but there are a lot of details to pay attention to. You shouldn't be ACKing a PR just because you ran the tests and they passed :) The goal today is to "learn how to review better" by zooming in and asking ourselves specific questions to convince ourselves this PR is correct
10:02 -!- dongcarlimposter [~dongcarla@4.53.92.114] has quit [Quit: Connection closed]
10:02 < hernanmarino> Hello 
10:02 -!- rewe [~rewe@191-221-129-105.user3p.brasiltelecom.net.br] has joined #bitcoin-core-pr-reviews
10:02 -!- dongcarlalt [~dongcarla@4.53.92.114] has joined #bitcoin-core-pr-reviews
10:02 < glozow> This review club is intended for beginners, so always feel free to ask questions :) don't worry about questions being too basic or off topic, the goal is to learn.
10:03 < glozow> So did you all get a chance to look at the PR or the notes? How about a y/n
10:03 < hernanmarino> y
10:03 < Lov3r_Of_Bitcoin> yes
10:03 < juancama> y
10:03 < stickies-v> y
10:03 < larryruane> 0.5y
10:03 < michaelfolkson> y
10:03 < dongcarlalt> 0.5y
10:03 < svav> y
10:03 < Amirreza> Wasn't this PR about two different topics? ArgsManager and caching sigs? I didn't grasp the connection between them.
10:03 < BlueMoon> y
10:03 < glozow> Excellent! Can someone who give us a 1-2sentence summary?
10:04 < Amirreza> y but understood little of it :(
10:04 < adam2k> y - confused on some of the bitwise shifting operations
10:04 < svav> Put the core consensus engine in its own module.
10:04 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
10:04 < larryruane> This is one small part of a long process to separate consensus-critical code from the rest of the code
10:05 < stickies-v> this PR removes the direct dependency in consensus code on the global gArgs ArgsManager that (amongst others) allows the user to define the max cache size for script and sig verification
10:05 < svav> This will make consensus functionality easier to maintain and implement.
10:06 < glozow> thanks svav and larryruane, stickies-v: here's a ⭐ for the best answer
10:06 < larryruane> stickies-v: 🎉
10:06 < Amirreza> What is the consensus functionality?
10:06 < glozow> thanks svav for giving us some background on the overall libbitcoinkernel project, you can find more details here: https://github.com/bitcoin/bitcoin/issues/24303
10:07 < glozow> Let's move on to the questions, starting with whether the PR is a good idea conceptually (beyond "it's part of kernel" so it's good)
10:07 < glozow>  In your own words, what does the `ArgsManager` do? Why or why not should it belong in src/kernel vs src/node?
10:07 -!- dongcarlalt [~dongcarla@4.53.92.114] has quit [Quit: Connection closed]
10:08 < larryruane> Amirreza: (if I could take a try at answering) It's very important for the nodes on the bitcoin network to all agree on whether a particular block or a particular transaction is VALID or not ... so we don't get a chain-split! So consensus code is any code that contributes to that determination (valid or not), hope that helps
10:08 < stickies-v> Amirreza: I wouldn't dare give a comprehensive answer, but my take is - code that's responsible for validating the consensus rules, e.g. transaction and block validation rules. The code that, if different clients have different logic, would cause the network to split
10:08 < hernanmarino> It s a data structure for handling command line arguments. If the objective is to decouple funtionality and isolate it in a core consensus modules, command line should definitely be far from it.
10:08 < BlueMoon> ArgsManager is a class where users can customise the configuration of their nodes.
10:08 < stickies-v> hah larryruane very similar answers, nice
10:09 < glozow> BlueMoon: hernanmarino: great answers! I would maybe replace the words "command line" with "configuration"
10:09 < Amirreza> larryruane, stickies-v thanks for both answers.
10:09 < hernanmarino> yes, you are right 
10:09 < svav> consensus functionality and rules are for block acceptance, i.e. nodes reaching consensus that a block is valid.
10:09 < glozow> And in your own words, what are the validation caches? Why would they belong in src/kernel vs src/node?
10:09 < stickies-v> I think even regardless of libbitcoinkernel, reducing dependency on globals is an improvement! so yes, I think it's a good idea
10:09 < larryruane> If src/kernel looks at config settings, and since nodes can easily have different config settings, we thereby risk consensus failure (disagreement)
10:09 -!- rewe [~rewe@191-221-129-105.user3p.brasiltelecom.net.br] has quit [Quit: Connection closed]
10:10 < larryruane> (it is true that nodes can run incompatible consensus code, but that requires more deliberation, not as likely to be accidental)
10:10 < juancama> should belong in src/kernel vs src/node because of cross contamination risk to answer the second part of the q
10:11 < larryruane> glozow: "validation caches ... Why belong in src/kernel?" I'm unclear on that. A "cache", in normal usage, means just a performance optimization (the more stuff you cache, the quicker you can get to it) ... so why are those caches related to consensus?
10:11 < hernanmarino> stickies-v: I agree
10:12 < sipa> @larryruane If the cache has a bug, it may impact consensus.
10:12 < Amirreza> stickies-v: and now the the consensus code is in src/kernel? or src/node?
10:12 -!- Timothe [~Timothe@hff6.n1.ips.mtn.co.ug] has joined #bitcoin-core-pr-reviews
10:12 < larryruane> Ah, i see, never would have thought of that, thanks sipa:
10:12 -!- dongcarlimposter [~dongcarli@4.53.92.114] has joined #bitcoin-core-pr-reviews
10:13 < glozow> larryruane: the code in kernel can still have parameters. `struct ValidationCacheSizes` is in kernel
10:14 < stickies-v> Amirreza: others can answer this better, but at the moment the consensus code is in quite a few places, which is kind of the point of libbitcoinkernel (and libbitcoinconsensus). validation.cpp has a lot of the consensus code
10:14 < glozow> Amirreza: src/kernel contains "kernel" code, i.e. consensus
10:14 < glozow> sorry yes thanks for the clarification stickies-v. is intended to contain* kernel code
10:15 < dongcarlimposter> src/kernel is definite a work in progress though, validation isn’t even in there
10:15 < larryruane> Amazing, so bitcoin core is designed so that, IF there's a (completely unknown!) bug, we can guard against it causing a disaster by careful design
10:16 < Amirreza> stickies-v, glozow : I'm confused with the src/consensus, what is that? It would be removed in the future?
10:16 < adam2k> Amirreza we probably have to migrate things over time instead of doing it all at once?
10:16 < glozow> src/consensus is intended to hold consensus rules, AFAIK, but definitely doesn't contain everything "consensus-critical".
10:17 < svav> https://github.com/bitcoin/bitcoin/tree/master/src/consensus
10:17 < glozow> Let's start looking at the commits.  The first commit removes the call to `InitScriptExecutionCache()` in checkinputs_test. How can you verify that this call was unnecessary? 
10:17 < Amirreza> Ah, so there is difference between consensus and consensus-critical? (Sorry I think I'm getting a lot of time of the meeting)
10:17 < glozow> link to commit: https://github.com/bitcoin-core-review-club/bitcoin/commit/1124ead57af262f57ac83205467b4366124408c1
10:18 < juancama> future commit makes it fail again
10:19 < stickies-v> Amirreza: no I don't think so, consensus-critical is commonly referred to something that's critical because it's consensus. Anything that can affect consensus is critical
10:19 < stickies-v> *commonly used to refer to
10:19 < hernanmarino> glozow: Because they were already initialized. Dersig100Setup  indirectly calls code from BasicTestingSetup which Initializes the cache in an assert
10:21 < glozow> Amirreza: not sure about semantics, but we're basically referring to "consensus" as consensus rules themselves, e.g. signature verification. and we're referring to signature caching as "consensus-critical" functionality because, if we have an invalid signature cached, our node is no longer enforcing consensus rules.
10:21 < adam2k> hernanmarino +1
10:22 < stickies-v> yeah I didn't really know how to test this, beyond checking that it's indeed done in BasicTestingSetup (https://github.com/bitcoin/bitcoin/blob/e4e201dfd9a9dbd8e22cac688dbbde16234cd937/src/test/util/setup_common.cpp#L139-L140)
10:22 < Amirreza> stickies-v, glozow Thanks
10:22 < glozow> hernanmarino: stickies-v: great!
10:22 < larryruane> glozow: ".. unnecessary?" -- okay, this is kind of cheating, but I would run the test in the debugger, set a BP on that function, and verify that it gets called before the call that's being removed
10:22 < glozow> A good larryruane-style way of checking this is to run this in gdb and set a breakpoint at `InitScriptExecutionCache()`
10:23 < glozow> ooh jinx! xD
10:23 < glozow> that is definitely not cheating. I'd say it's the best way to check this.
10:24 < glozow> The second commit returns approximate total size from `InitSignatureCache()` and `InitScriptExecutionCache`. It also adds the `[[nodiscard]]` attribute to both functions. What does `[[nodiscard]]` do, and why or why not is it appropriate to add here?
10:24 < glozow> commit link: https://github.com/bitcoin-core-review-club/bitcoin/commit/f66cd5b3aa25f27638b05fbffde7470dd844951b
10:24 < glozow> Hint: Link to reference here: https://en.cppreference.com/w/cpp/language/attributes/nodiscard
10:24 < larryruane> OHH OHH OHH! It means callers of the function can't silently ignore the return value (but I think they can cast to void)
10:24 -!- pablomartin [~pablomart@106.15.202.62.static.wline.lns.sme.cust.swisscom.ch] has joined #bitcoin-core-pr-reviews
10:24 < Amirreza> glozow: it warns at compile time if the return value will be discarderd 
10:24 < stickies-v> the compiler will raise a warning if a nodiscard type/return value is ignored/not used. it is appropriate to add here because previously `InitS*Cache()` returned `void`, but now it returns a `bool` that we *need* to check to ensure the cache was actually initialized.
10:26 < glozow> Great! and why does it make sense here? What happens if the function returns false and we just keep going?
10:26 -!- pablomartin11 [~pablomart@106.15.202.62.static.wline.lns.sme.cust.swisscom.ch] has joined #bitcoin-core-pr-reviews
10:26 < glozow> Alternatively: why doesn't it make sense here?
10:26 < pablomartin11> hello, sorry Im late, I had some connection issues
10:26 < adam2k> does that mean the InitScriptExecutionCache failed and we should not continue?
10:27 < glozow> adam2k: indeed
10:27 -!- nassersaazi [~nassersaa@hff2.n1.ips.mtn.co.ug] has joined #bitcoin-core-pr-reviews
10:28 < adam2k> That seems like a critical error because it's an init function and something is probably seriously wrong that cannot be recovered from.
10:28 -!- nassersaazi [~nassersaa@hff2.n1.ips.mtn.co.ug] has quit [Client Quit]
10:28 -!- dongcarlimposter [~dongcarli@4.53.92.114] has quit [Quit: Connection closed]
10:29 < glozow> adam2k: so maybe we should make sure code calling `InitScriptExecutionCache()` always checks the return value, eh?
10:29 -!- pablomartin [~pablomart@106.15.202.62.static.wline.lns.sme.cust.swisscom.ch] has quit [Ping timeout: 245 seconds]
10:29 -!- dongcarlimposter [~dongcarli@4.53.92.114] has joined #bitcoin-core-pr-reviews
10:29 -!- Timothe [~Timothe@hff6.n1.ips.mtn.co.ug] has quit [Quit: Connection closed]
10:29 -!- pablomartin11 is now known as pablomartin
10:30 < larryruane> "why doesn't it make sense here?" -- I can't think of a reason
10:30 < adam2k> ah..is that what is happening here src/test/util/setup_common.cpp on lines 145 and 146 with the Assert statements?
10:30 < adam2k> Sorry, 146 & 147
10:31 < hernanmarino> glozow: i believe that's the reason for this 
10:31 < glozow> larryruane: in that commit, do the functions ever return false?
10:32 < hernanmarino> adam2k : the assert check for true or, the reason for this change is to detect other lines of codes not checking for it i believe
10:32 < larryruane> Oh i wonder if an alternative to [[nodiscard]] in this case might be to internally assert if something goes wrong?
10:32 < larryruane> (if it's true that failures there can't be recovered from)
10:33 < hernanmarino> larryruane: it s also a good way for the developer to detect other calls to this function (and change them if any )
10:33 < glozow> adam2k: yes, remember this is checked at compile time. It's to help make sure the developer doesn't forget to add a check that the initialization succeeded. Though not foolproof of course
10:34 < adam2k> glozow hernanmarino thanks!
10:34 < glozow> The third commit removes `MAX_MAX_SIG_CACHE_SIZE`. What is it? Why is it ok to remove it?
10:35 -!- dongcarlimposter [~dongcarli@4.53.92.114] has quit [Quit: Connection closed]
10:35 -!- dongcarlimposter [~dongcarli@4.53.92.114] has joined #bitcoin-core-pr-reviews
10:35 -!- sanya [~sanya@109-93-38-19.dynamic.isp.telekom.rs] has quit [Quit: Connection closed]
10:35 < stickies-v> hmm glozow from how you phrase it, maybe the [[nodiscard]] is only really necessary after the uint32 overflow commit, because then InitScrtipExecutionCache can actually return false too?
10:35 < larryruane> @glozow "in that commit, do the functions ever return false?" - Oh I see, no they don't, in that commit (but do in a later commit)
10:36 < glozow> Hint: The commit message links to the commit at which this value was added, and why it doesn't apply anymore  https://github.com/bitcoin-core-review-club/bitcoin/commit/42add4bfe80009c51ab92456b4d72cab5ef33126
10:37 < glozow> larryruane: yes bingo! :) i was hoping someone would ask "it's impossible for this to not succeed, so what's the value?"
10:37 < larryruane> we're kinda slow (haha)
10:38 < adam2k> glozow It looks like the change was made from `entries` to `MiB`
10:38 < glozow> stickies-v: yes well observed :D
10:38 -!- dongcarlimposter [~dongcarli@4.53.92.114] has quit [Client Quit]
10:38 -!- pablomartin [~pablomart@106.15.202.62.static.wline.lns.sme.cust.swisscom.ch] has quit [Quit: Connection closed]
10:38 < glozow> adam2k: yep that's the crux of it!
10:39 < glozow> And why is it okay to remove it now?
10:39 -!- pablomartin [~pablomart@106.15.202.62.static.wline.lns.sme.cust.swisscom.ch] has joined #bitcoin-core-pr-reviews
10:39 -!- Amirreza [~Amirreza7@2.177.18.94] has quit [Quit: Leaving]
10:40 -!- dongcarlimposter [~dongcarli@143.244.47.100] has joined #bitcoin-core-pr-reviews
10:40 < stickies-v> I don't see a reason to set a max size, except for the overflow bug (which is only fixed in the commit after)
10:41 < adam2k> 🤔 `src/validation.cpp` is calculating the nMaxCacheSize now?
10:42 < adam2k> There's also a comment in the PR that says `-maxsigcachesize is a DEBUG_ONLY option`
10:42 < adam2k> correction, in the commit
10:43 < glozow> well it made sense when `-maxsigcachesize` was changed from number of entries to number of mebibytes. At the time, it would have made sense in case somebody's test had it set to 100,000 entries, and this change meant the config = 100GiB sigcache.
10:44 < hernanmarino> okey, makes sense
10:44 < glozow> adam2k: yeah, this is a debug-only option, so such a mistake wouldn't be too bad. also it's been 7 years so it's unlikely that somebody hasn't updated their settings yet
10:44 < stickies-v> ah right, I suppose another way would have been to introduce a new parameter `maxsigcachesizemib` to make that more explicit
10:45 < glozow> stickies-v: yeah maybe, but it's debug-only 🤷
10:45 < stickies-v> (and not re my previous comment: this `MAX_MAX_SIG_CACHE_SIZE` did not actually prevent the overflow, but a lower value for it could have, I was just talking about a max value in general)
10:45 < larryruane> so if cache is now based on memory, does that mean that different hardware platforms or different compilers could result in nodes caching different numbers of entries? didn't we want to avoid that?
10:46 < larryruane> (i mean, for nodes having the same cache settings)
10:46 < glozow> larryruane: it's not a goal for all nodes' caches to be identical
10:46 < adam2k> larryruane wouldn't the MiB still be calculated the same way for any system?
10:46 < stickies-v> larryruane: but I think it's unreasonable to ask users how many elements they want to cache, because they'd have no idea how much memory is required for that?
10:46 < hernanmarino> it s only a cache after all 
10:47 < glozow> it's just that the caching is consensus-critical, and thus belongs in the consensus-critical section
10:47 < glozow> Ok well that was lucky for us that Carl wrote the explanation for `MAX_MAX_SIG_CACHE_SIZE` and linked to the code in the commit message! If he hadn't done this, what tools could you have used for "code archeology" to understand the background of why some code exists?
10:47 < larryruane> `git log -p filename`
10:47 -!- pablomartin_ [~pablomart@45.149.175.249] has joined #bitcoin-core-pr-reviews
10:48 < larryruane> (i do that ALL the time)
10:48 < michaelfolkson> <sipa> @larryruane If the cache has a bug, it may impact consensus.
10:48 -!- pablomartin [~pablomart@106.15.202.62.static.wline.lns.sme.cust.swisscom.ch] has quit [Ping timeout: 245 seconds]
10:48 < adam2k> git blame is good too
10:48 < michaelfolkson> I'm trying to think what kind of bug would impact consensus
10:48 -!- pablomartin_ is now known as pablomartin
10:48 < sipa> michaelfolkson: Anything where the cache reports the wrong thing.
10:48 < larryruane> the nice thing about `git log -p` compared with `git blame` is that the former shows removed lines, shows diffs
10:49 < stickies-v> and you can also search https://github.com/bitcoin/bitcoin/pulls with `commit:<commit_sha>` to show the PR that contained the commit, and read the discussion there
10:49 < michaelfolkson> sipa: But the cache is just storing something temporarily right? It isn't changing what is being verified or the verification result
10:49 -!- nassersaazi [~nassersaa@hff2.n1.ips.mtn.co.ug] has joined #bitcoin-core-pr-reviews
10:49 < larryruane> plus you see the full commit message ... I often search the output of `git log -p` (pipe it into `less` for example)
10:50 < dongcarlimposter> I used `git blame` for a long time, but GitHub's blame is really good now, and very convenient, there's a "View blame prior to this change" button that removes the need to blame over and over again
10:50 < sipa> michaelfolkson: If it works correctly, yes. If it doesn't, who knows what it does?
10:50 < larryruane> dongcarlimposter: oh cool, i didn't know about that TIL!
10:51 < larryruane> (but i still think `git log -p` is awesome)
10:51 < michaelfolkson> sipa: Hmm ok. Lacking some imagination on what a cache bug might do perhaps :)
10:51 < adam2k> I just have GitLens in VSCode and it'll show the git blame inline, but I'll check out the Github Version.  That sounds useful!
10:51 < glozow> dongcarlimposter: yeah, it's way better now! i remember trying to use blame on validation.cpp and it just sat there loading for 5 minutes
10:51 < stickies-v> michaelfolkson: if the cache incorrectly says that a certain sig/script is in there, then we're just going to assume it's valid and not reevaluate it again
10:52 < dongcarlimposter> lmao someone at GitHub did some work on caches probably XP
10:52 < sipa> michaelfolkson: We're not trying to reason about what a bug would look like, or even what kind of bugs are likely. It's just a fact that if the cache doesn't function correctly, it may affect consensus.
10:52 -!- pablomartin_ [~pablomart@45.149.175.249] has joined #bitcoin-core-pr-reviews
10:52 < glozow> larryruane: ye I use git blame too! thanks for sharing :)
10:52 < larryruane> the problem with `git blame` is it only shows the most recent change to each line that's current in the file (I think that's why people end up doing repeated git blames)
10:52 < glozow> (also sometimes i literally just search stuff using the github search bar, also quite good 😅)
10:53 < larryruane> yes i've started doing that recently too, it's pretty good
10:53 < glozow> The fourth commit references a uint "overflow". Describe what overflow could happen and under what conditions would it get hit?
10:53 < glozow> link to commit: https://github.com/bitcoin-core-review-club/bitcoin/commit/3c5555de81ab7f51c655dadffc5e939c4515f65d
10:54 < stickies-v> `setup_bytes()` could pass a value to `setup()` that exceeds the bounds of a uint32
10:54 -!- nassersaazi [~nassersaa@hff2.n1.ips.mtn.co.ug] has quit [Quit: Connection closed]
10:54 -!- pablomartin [~pablomart@45.149.175.249] has quit [Ping timeout: 268 seconds]
10:54 < stickies-v> this would happen if `-maxsigcachesize` is larger than slightly over 8000 (calculated the value earlier but didn't write it down hah)
10:54 -!- pablomartin_ is now known as pablomartin
10:55 < hernanmarino> When setup_bytes from the cuckooCache  is big enough, the implicit conversion to uint32_t in the call to setup will overflow.
10:56 < glozow> stickies-v: hernanmarino: thanks!
10:56 < glozow> Before we run out of time. The last commit changes the type of signature_cache_bytes and script_execution_cache_bytes from int64_t to size_t. What is the difference between int64_t, uint64_t, and size_t, and why should a size_t hold these values?
10:56 < stickies-v> ((4294967295 / ((1 << 20) / 2)) =~ 8192)
10:56 < glozow> commit: https://github.com/bitcoin-core-review-club/bitcoin/commit/d29f2fd9b49ac00a3721af7260dbbf59e9e8387c
10:56 < michaelfolkson> sipa: stickies-v example of cache misreporting what is in the cache is a possible example of a cache bug? I'd have thought something stored in the cache wouldn't change while in the cache
10:57 < sipa> michaelfolkson: again: if the cache works correctly, then no, it will report exactly what was entered into the cache.
10:57 < michaelfolkson> Ok thanks
10:57 < sipa> We're talking about the scenario where the cache has a bug. There is no bound on what can go wrong in that case.
10:58 < larryruane> `size_t` is either 4 bytes or 8 bytes (see assumptions.h)
10:59 < larryruane> on any system where `size_t` is 32 bits, the memory size of any object is guaranteed to be within 2 ^ 32 (4gb) -- I think!
10:59 < sipa> larryruane: indeed
10:59 < larryruane> so i think size_t is appropriate for the memory size of something
11:00 < glozow> here's a stack overflow post if people are interested https://stackoverflow.com/questions/1951519/when-to-use-stdsize-t
11:00 < glozow> yep. size_t is meant to hold sizes
11:00 < glozow> okay that's all we have time for today
11:00 < glozow> remember to review the other commits too :P
11:01 < glozow> #endmeeting
11:01 < adam2k> Thanks glozow!
11:01 < Lov3r_Of_Bitcoin> thanks
11:01 < BlueMoon> Thank you very much, I was attentive.
11:01 < larryruane> thanks @glozow! 
11:01 < michaelfolkson> Thanks glozow!
11:01 -!- Lov3r_Of_Bitcoin [~Lov3r_Of_@45-27-31-99.lightspeed.sntcca.sbcglobal.net] has quit [Quit: Connection closed]
11:02 -!- BlueMoon [~BlueMoon@dgb3.dgbiblio.unam.mx] has quit [Quit: Connection closed]
11:02 < hernanmarino> Thanks ! If anyone wnats to stay to discuss the Quiz, I'm in :)
11:02 < pablomartin> thanks @glozow
11:02 < svav> Thanks glozow and all!
11:02 < dongcarlimposter> Thanks glozow!
11:03 < stickies-v> thank you glozow for hosting, very on point questions! and thank you dongcarlimposter for being here on behalf of dongcarl
11:03 < larryruane> hernanmarino: definitely not (A)
11:03 < juancama> thank you for hosting
11:03 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
11:03 < hernanmarino> i agree
11:03 < dongcarlimposter> stickies-v: XP
11:03 < hernanmarino> definitely not (B)
11:04 -!- dongcarlimposter [~dongcarli@143.244.47.100] has quit [Quit: Connection closed]
11:04 -!- juancama [~juancama@pool-74-96-218-208.washdc.fios.verizon.net] has left #bitcoin-core-pr-reviews []
11:04 < hernanmarino> any other insights ? I only have some intuitions, I am not sure 
11:05 < larryruane> if there's only 1 right answer, i'm thinking E but not sure
11:05 < sipa> what is the question?
11:05 < hernanmarino> E is my best choice, I think
11:05 < larryruane> A mainnet node receives a new block at height 710,000 containing a few transactions. Which of the following transactions could hit its script cache, assuming nothing has been evicted? (Note: taproot activated at height 709,632)
11:05 < hernanmarino> (A) the coinbase transaction
11:06 < hernanmarino> (B) a transaction the node has never seen before
11:06 < larryruane> sipa: oh that doesn't show the answers.. go to https://bitcoincore.reviews/25527
11:06 < hernanmarino> (C) a transaction with the same txid but different witness as a transaction in its mempool
11:06 < larryruane> (the bottom of that page)
11:06 < hernanmarino> (D) a transaction that was in its mempool but replaced by another
11:06 < hernanmarino> (E) a transaction with no taproot inputs or outputs accepted to the mempool at height 709,000
11:06 -!- pablomartin_ [~pablomart@185.169.233.25] has joined #bitcoin-core-pr-reviews
11:06 < hernanmarino> (F) All of the above
11:06 < hernanmarino> (G) None of the above
11:06 < stickies-v> I was actually thinking F
11:07 < larryruane> (C) is a contender also
11:07 < hernanmarino> We should go through the code to be sure, and i didnt do that
11:07 < larryruane> stickies-v: well no, a coinbase tx wouldn't be in your cache, right?
11:07 < stickies-v> well we're talking script cache, right
11:07 < hernanmarino> Im between D and E 
11:07 < stickies-v> could very well be that the coinbase tx sends to a scriptpubkey that already exists, i think?
11:07 < sipa> The coinbase tx has no executed scripts.
11:07 -!- pablomartin [~pablomart@45.149.175.249] has quit [Ping timeout: 245 seconds]
11:08 < sipa> stickies-v: The script cache is for script execution, which happens at spending time.
11:08 < sipa> The coinbase doesn't spend anything.
11:08 < hernanmarino> but depends on the logic . I cannot think of a reason for E to be erased from the cache ....
11:08 < stickies-v> ohh right
11:08 < hernanmarino> but perhaps I m missing something 
11:08 < larryruane> hernanmarino: yes, i was thinking E also
11:08 < michaelfolkson> sipa: Sorry to belabor point, I am reading what you've said. I'm just trying to think how one can analyze these things when one isn't sure if something could affect consensus or not
11:09 < michaelfolkson> Instinctively I want to think of a possible bug that could affect consensus
11:09 < glozow> It's not E, the script cache includes the script verification flags. after 709632, you'd look for an entry with SCRIPT_VERIFY_TAPROOT and wouldn't find one
11:09 < sipa> michaelfolkson: The only criterion is: the consensus code depends on the cache. Thus if the cache reports the wrong thing, conensus will be affected.
11:09 < hernanmarino> (D) might be true or False, intuitvely 
11:09 < hernanmarino> glozow: thanks 
11:09 < michaelfolkson> Otherwise I'm not sure how to assess
11:09 < sipa> Consensus code e.g. doesn't depend on the wallet, so this is not true for the wallet.
11:09 < sipa> michaelfolkson: You look at the code.
11:10 < michaelfolkson> With the LevelDB bug, was it obvious from the code?
11:10 < sipa> It was obvious from the code that consensus depends on LevelDB, yes.
11:10 < pablomartin_> hernanmarino: (D) a transaction that was in its mempool but replaced by another...
11:10 < sipa> (or BDB)
11:10 < sipa> That doesn't mean the bug itself is obvious.
11:11 < larryruane> gosh i'm starting to think the answer is G!
11:11 < hernanmarino> pablomartin_: that depends on the code , it might get erased or not ...
11:11 < hernanmarino> larryruane: it might be the case , if (D) is not the answer 
11:12 < glozow> here's the code for computing script cache entry: https://github.com/bitcoin/bitcoin/blob/4a4289e2c98cfbc51b05716f21065838afed80f6/src/validation.cpp#L1712
11:12 < hernanmarino> we should really read the code :)  but didn't get to do it today ...
11:12 < larryruane> I don't think D, because if tx-a is replaced by tx-b (RBF), then why would we keep tx-a's script in the cache?
11:12 < pablomartin_> hernanmarino: we've discarded C & E?
11:13 -!- pablomartin_ is now known as pablomartin
11:14 < glozow> larryruane: can you point to the code where the replaced tx is removed from the script cache?
11:15 < larryruane> not quickly 😅
11:15 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
11:15 < larryruane> (i was more just guessing that it would be removed!)
11:16 -!- hernanmarino_ [~hernanmar@186.153.60.185] has joined #bitcoin-core-pr-reviews
11:16 -!- hernanmarino [~hernanmar@186.153.60.185] has quit [Ping timeout: 240 seconds]
11:17 < hernanmarino_> sorry, got disconnected ...
11:18 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
11:19 < pablomartin> larryuane: glozow: perhaps there's something on lines 575-579...
11:25 < larryruane> pablomartin: which file?
11:26 < pablomartin> the one glozow sent above... src/validation.cpp
11:28 < pablomartin> I'm running out of battery... pls forgive me if I suddenly leave...
11:32 < larryruane> np ... I'm not sure, investigating ... will post here later if I figure it out!
11:32 < hernanmarino_> same for me , still reading the code, but have to attend another minute now ...
11:34 < pablomartin> pls ignore the lines I pointed you, it's not there
11:34 < hernanmarino_> another meeting*
11:35 < hernanmarino_> my only doubt is if transactions replaced by RFB get erased from the cache ...i coludn't find that in the code so far  
11:36 < hernanmarino_> that would really tell us if (D) is correct, or otherwise (G)
11:37 < hernanmarino_> But I have to leave now , will get back later if anyone is still here . Goodbye everybody !
11:38 -!- pablomartin [~pablomart@185.169.233.25] has quit [Ping timeout: 252 seconds]
11:47 < michaelfolkson> Any calls/interactions between the wallet/GUI and libbitcoinkernel would also be considered consensus right? In theory wallet code could be written maliciously to change some consensus state in libbitcoinkernel. 
11:48 < sipa> Well if you consider malicious code, you need far better separation.
11:48 < sipa> Like running in another process, with well-defined, secure APIs, which don't trust callers
11:48 < sipa> That's generally not the model we use for assessing what is consensus critical.
11:49 < michaelfolkson> The cache example was malicious (or at least incompetent)?
11:49 < sipa> No, just buggy.
11:49 < sipa> Like: it could be returning the wrong thing.
11:49 < sipa> But we don't consider the possibility that it may start looking through the process' memory and go make deliberate changes to it.
11:50 < sipa> If that were the case, we can assume it'd be caught by review.
11:50 < sipa> Otherwise literally all of Bitcoin Core's C++ code would need to be considered consensus critical.
11:51 < sipa> It's definitely the case that all of it could, in theory, if permitted to run arbitrary malicious code, affect consensus outcome. But it's also true that some things are orders of magnitude more risky in that regard than others.
11:52 < sipa> So generally the idea is to look at code/dataflow dependencies: does consensus either directly or indirectly change behavior based on values returned from the code under consideration.
11:53 < sipa> While consensus does send signals which trigger callbacks in the wallet code (e.g. to inform the wallet about new transactions that arrived), those don't return anything that validation code then uses. So we say the wallet is not consensus critical.
11:55 < sipa> If the wallet were actually malicious, it could just go modify values in memory related to consensus data structures... but that would need extremely suspicious code.
11:56 < michaelfolkson> I guess I was putting the cache example in the very low risk bucket (needs suspicious code). But consensus does need caching and it doesn't need a wallet. So it is more of a consensus dependency argument rather than a risk argument
11:56 < sipa> It would not need suspicious code, not to the extent I meant it above.
11:58 < sipa> Like, just literally inserting a single "!" somewhere in the cache code in its lookup or insertion code, would mean consensus is immediately affected. That's very easy to overlook.
11:58 < sipa> The kind of "suspicious" code I'm referring to is something like the wallet directly accessing consensus data structures.
11:59 < sipa> Say, if the wallet contained a call to CBlockIndex::RaiseValidity, e.g., that would be extremely worrisome - the wallet shouldn't be doing something like that.
12:00 < sipa> I think if you had some familiarity with the source code, this distinction would be very obvious.
12:01 -!- adam2k [~adam2k@ip24-254-208-245.hr.hr.cox.net] has quit [Ping timeout: 245 seconds]
12:10 < michaelfolkson> "some familiarity" is relative. Knowing which calls would be worrisome/suspicious and which wouldn't from a consensus perspective seems like one might need more than "some familiarity". At least pre libbitcoinkernel anyway
12:54 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
13:09 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 268 seconds]
14:01 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Remote host closed the connection]
14:01 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
14:06 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 268 seconds]
14:35 -!- pablomartin [~pablomart@185.169.233.26] has joined #bitcoin-core-pr-reviews
14:35 -!- brunoerg [~brunoerg@187.183.43.40] has joined #bitcoin-core-pr-reviews
14:40 -!- brunoerg [~brunoerg@187.183.43.40] has quit [Ping timeout: 268 seconds]
15:09 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
15:21 -!- pablomartin [~pablomart@185.169.233.26] has quit [Ping timeout: 268 seconds]
16:12 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 268 seconds]
16:27 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
17:30 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 244 seconds]
17:59 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
18:24 -!- yashraj [yashraj@gateway/vpn/protonvpn/yashraj] has joined #bitcoin-core-pr-reviews
18:47 -!- yashraj [yashraj@gateway/vpn/protonvpn/yashraj] has quit [Ping timeout: 252 seconds]
19:03 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 268 seconds]
19:07 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has joined #bitcoin-core-pr-reviews
19:11 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:ad8f:dc61:e61d:d077] has quit [Ping timeout: 268 seconds]
19:15 -!- brunoerg [~brunoerg@187.183.43.40] has joined #bitcoin-core-pr-reviews
20:17 -!- brunoerg [~brunoerg@187.183.43.40] has quit [Ping timeout: 268 seconds]
20:30 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:8c6:9573:9b29:d2f3] has joined #bitcoin-core-pr-reviews
21:12 -!- hernanmarino_ [~hernanmar@186.153.60.185] has quit [Remote host closed the connection]
21:32 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:8c6:9573:9b29:d2f3] has quit [Ping timeout: 268 seconds]
21:33 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:24d4:e3c:35e8:fc2] has joined #bitcoin-core-pr-reviews
21:38 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:24d4:e3c:35e8:fc2] has quit [Ping timeout: 244 seconds]
21:39 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:24d4:e3c:35e8:fc2] has joined #bitcoin-core-pr-reviews
21:44 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:24d4:e3c:35e8:fc2] has quit [Ping timeout: 268 seconds]
21:50 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:24d4:e3c:35e8:fc2] has joined #bitcoin-core-pr-reviews
22:35 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
22:37 -!- gleb106892 [~gleb@178.150.137.228] has quit [Remote host closed the connection]
22:37 -!- gleb106892 [~gleb@178.150.137.228] has joined #bitcoin-core-pr-reviews
22:39 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 240 seconds]
22:48 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
22:52 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 240 seconds]
22:55 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
22:56 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:24d4:e3c:35e8:fc2] has quit [Ping timeout: 268 seconds]
22:58 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:24d4:e3c:35e8:fc2] has joined #bitcoin-core-pr-reviews
23:59 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 252 seconds]
--- Log closed Thu Aug 04 00:00:41 2022