Decouple validation cache initialization from `ArgsManager` (refactoring)

Host: glozow  -  PR author: dongcarl

The PR branch HEAD was d29f2fd at the time of this review club meeting.


  • The libbitcoinkernel project is an effort to decouple Bitcoin Core’s consensus engine from other non-consensus modules (such as the various indices) in the codebase. Previously, we have covered libbitcoinkernel-related PRs #24410 and #20158.

  • ArgsManager is a Bitcoin Core-specific data structure responsible for handling configuration options. It returns help strings for configuration options, parses user input, and stores the configured values. While this functionality is helpful for users that want to customize their nodes, not every node participating in the network needs it.

  • Script (specifically, signature) verification is the most computationally expensive part of validating a transaction. The validation caches, g_signatureCache and g_scriptExecutionCache, speed up block validation performance by caching successful signatures and scripts, respectively, validated when transactions were submitted to the node’s mempool.

    • The debug-only -maxsigcachesize option limits the aggregate memory usage of both caches.

    • The caches are cuckoo caches, introduced in PR #8895. Also see Cuckoo hash tables if you are interested.

    • You do not need to fully understand cuckoo cache and validation cache usage to review this PR, but feel free to take the scenic route if it piques your interest. Also, looking at the implementation of CheckInputScripts() may help clarify what is going on in txvalidationcache_tests.

  • PR #25527 is one of a series of PRs that decouples ArgsManager from kernel-related code (see also #25290 and #25487). Specifically, it removes uses of ArgsManager when initializing the signature and script caches. It also removes a limit on signature cache sizes (MAX_MAX_SIG_CACHE_SIZE), patches a uint32 overflow, and other cleanups.


  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. In your own words, what does the ArgsManager do? Why or why not should it belong in src/kernel vs src/node?

  3. In your own words, what are the validation caches? Why would they belong in src/kernel vs src/node?

  4. The first commit removes the call to InitScriptExecutionCache() in txvalidationcache_tests.cpp/checkinputs_test. How can you verify that this call was unnecessary?

    (Hint a) What do setting cacheSigStore and cacheFullScriptStore do in CheckInputScripts()?

    (Hint b) What needs to be called/initialized for the test to use the script cache?

    (Hint c) The checkinputs_test test case uses Dersig100Setup. How can you check if this has a script cache setup?

  5. 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?

  6. The third commit removes MAX_MAX_SIG_CACHE_SIZE. What is it? Why is it ok to remove it?

  7. What tools do you use for “code archeology” to understand the background of why a value exists?

  8. Describe the uint32 overflow in the fourth commit. Under what conditions would it get hit?

  9. The fifth commit introduces src/node/validation_cache_args and adds it to the include-what-you-use CI check. What does this check do? Were you able to run it yourself?

  10. This sixth 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?

  11. What is the maximum value for ValidationCacheSizes::signature_cache_bytes after this diff? Is this a behavior change? Why or why not is the code correct?

  12. Quiz: 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)

(A) the coinbase transaction
(B) a transaction the node has never seen before
(C) a transaction with the same txid but different witness as a transaction in its mempool
(D) a transaction that was in its mempool but replaced by another
(E) a transaction with no taproot inputs or outputs accepted to the mempool at height 709,000
(F) All of the above
(G) None of the above

Meeting Log

  117:00 <glozow> #startmeeting
  217:00 <stickies-v> hi!
  317:00 <glozow> Welcome to review club everyone! Today, we're looking at #25527:
  417:00 <larryruane> hi!
  517:00 <michaelfolkson> hi
  617:01 <juancama> Hey everybody
  717:01 <svav> Hi
  817:01 <Lov3r_Of_Bitcoin> hello
  917: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
 1017:02 <hernanmarino> Hello
 1117: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.
 1217:03 <glozow> So did you all get a chance to look at the PR or the notes? How about a y/n
 1317:03 <hernanmarino> y
 1417:03 <Lov3r_Of_Bitcoin> yes
 1517:03 <juancama> y
 1617:03 <stickies-v> y
 1717:03 <larryruane> 0.5y
 1817:03 <dongcarlalt> 0.5y
 1917:03 <michaelfolkson> y
 2017:03 <svav> y
 2117:03 <Amirreza> Wasn't this PR about two different topics? ArgsManager and caching sigs? I didn't grasp the connection between them.
 2217:03 <BlueMoon> y
 2317:03 <glozow> Excellent! Can someone who give us a 1-2sentence summary?
 2417:04 <Amirreza> y but understood little of it :(
 2517:04 <adam2k> y - confused on some of the bitwise shifting operations
 2617:04 <svav> Put the core consensus engine in its own module.
 2717:04 <larryruane> This is one small part of a long process to separate consensus-critical code from the rest of the code
 2817: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
 2917:05 <svav> This will make consensus functionality easier to maintain and implement.
 3017:06 <glozow> thanks svav and larryruane, stickies-v: here's a ⭐ for the best answer
 3117:06 <larryruane> stickies-v: 🎉
 3217:06 <Amirreza> What is the consensus functionality?
 3317:06 <glozow> thanks svav for giving us some background on the overall libbitcoinkernel project, you can find more details here:
 3417: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)
 3517:07 <glozow> In your own words, what does the `ArgsManager` do? Why or why not should it belong in src/kernel vs src/node?
 3617: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
 3717: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
 3817: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.
 3917:08 <BlueMoon> ArgsManager is a class where users can customise the configuration of their nodes.
 4017:08 <stickies-v> hah larryruane very similar answers, nice
 4117:09 <glozow> BlueMoon: hernanmarino: great answers! I would maybe replace the words "command line" with "configuration"
 4217:09 <Amirreza> larryruane, stickies-v thanks for both answers.
 4317:09 <hernanmarino> yes, you are right
 4417:09 <svav> consensus functionality and rules are for block acceptance, i.e. nodes reaching consensus that a block is valid.
 4517:09 <glozow> And in your own words, what are the validation caches? Why would they belong in src/kernel vs src/node?
 4617: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
 4717:09 <larryruane> If src/kernel looks at config settings, and since nodes can easily have different config settings, we thereby risk consensus failure (disagreement)
 4817:10 <larryruane> (it is true that nodes can run incompatible consensus code, but that requires more deliberation, not as likely to be accidental)
 4917:10 <juancama> should belong in src/kernel vs src/node because of cross contamination risk to answer the second part of the q
 5017: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?
 5117:11 <hernanmarino> stickies-v: I agree
 5217:12 <sipa> @larryruane If the cache has a bug, it may impact consensus.
 5317:12 <Amirreza> stickies-v: and now the the consensus code is in src/kernel? or src/node?
 5417:12 <larryruane> Ah, i see, never would have thought of that, thanks sipa:
 5517:13 <glozow> larryruane: the code in kernel can still have parameters. `struct ValidationCacheSizes` is in kernel
 5617: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
 5717:14 <glozow> Amirreza: src/kernel contains "kernel" code, i.e. consensus
 5817:14 <glozow> sorry yes thanks for the clarification stickies-v. is intended to contain* kernel code
 5917:15 <dongcarlimposter> src/kernel is definite a work in progress though, validation isn’t even in there
 6017: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
 6117:16 <Amirreza> stickies-v, glozow : I'm confused with the src/consensus, what is that? It would be removed in the future?
 6217:16 <adam2k> Amirreza we probably have to migrate things over time instead of doing it all at once?
 6317:16 <glozow> src/consensus is intended to hold consensus rules, AFAIK, but definitely doesn't contain everything "consensus-critical".
 6417:17 <svav>
 6517: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?
 6617: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)
 6717:17 <glozow> link to commit:
 6817:18 <juancama> future commit makes it fail again
 6917: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
 7017:19 <stickies-v> *commonly used to refer to
 7117:19 <hernanmarino> glozow: Because they were already initialized. Dersig100Setup indirectly calls code from BasicTestingSetup which Initializes the cache in an assert
 7217: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.
 7317:21 <adam2k> hernanmarino +1
 7417:22 <stickies-v> yeah I didn't really know how to test this, beyond checking that it's indeed done in BasicTestingSetup (
 7517:22 <Amirreza> stickies-v, glozow Thanks
 7617:22 <glozow> hernanmarino: stickies-v: great!
 7717: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
 7817:22 <glozow> A good larryruane-style way of checking this is to run this in gdb and set a breakpoint at `InitScriptExecutionCache()`
 7917:23 <glozow> ooh jinx! xD
 8017:23 <glozow> that is definitely not cheating. I'd say it's the best way to check this.
 8117: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?
 8217:24 <glozow> commit link:
 8317:24 <glozow> Hint: Link to reference here:
 8417: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)
 8517:24 <Amirreza> glozow: it warns at compile time if the return value will be discarderd
 8617: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.
 8717:26 <glozow> Great! and why does it make sense here? What happens if the function returns false and we just keep going?
 8817:26 <glozow> Alternatively: why doesn't it make sense here?
 8917:26 <pablomartin11> hello, sorry Im late, I had some connection issues
 9017:26 <adam2k> does that mean the InitScriptExecutionCache failed and we should not continue?
 9117:27 <glozow> adam2k: indeed
 9217: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.
 9317:29 <glozow> adam2k: so maybe we should make sure code calling `InitScriptExecutionCache()` always checks the return value, eh?
 9417:30 <larryruane> "why doesn't it make sense here?" -- I can't think of a reason
 9517:30 <adam2k> that what is happening here src/test/util/setup_common.cpp on lines 145 and 146 with the Assert statements?
 9617:30 <adam2k> Sorry, 146 & 147
 9717:31 <hernanmarino> glozow: i believe that's the reason for this
 9817:31 <glozow> larryruane: in that commit, do the functions ever return false?
 9917: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
10017:32 <larryruane> Oh i wonder if an alternative to [[nodiscard]] in this case might be to internally assert if something goes wrong?
10117:32 <larryruane> (if it's true that failures there can't be recovered from)
10217:33 <hernanmarino> larryruane: it s also a good way for the developer to detect other calls to this function (and change them if any )
10317: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
10417:34 <adam2k> glozow hernanmarino thanks!
10517:34 <glozow> The third commit removes `MAX_MAX_SIG_CACHE_SIZE`. What is it? Why is it ok to remove it?
10617: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?
10717: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)
10817:36 <glozow> Hint: The commit message links to the commit at which this value was added, and why it doesn't apply anymore
10917:37 <glozow> larryruane: yes bingo! :) i was hoping someone would ask "it's impossible for this to not succeed, so what's the value?"
11017:37 <larryruane> we're kinda slow (haha)
11117:38 <adam2k> glozow It looks like the change was made from `entries` to `MiB`
11217:38 <glozow> stickies-v: yes well observed :D
11317:38 <glozow> adam2k: yep that's the crux of it!
11417:39 <glozow> And why is it okay to remove it now?
11517: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)
11617:41 <adam2k> 🤔 `src/validation.cpp` is calculating the nMaxCacheSize now?
11717:42 <adam2k> There's also a comment in the PR that says `-maxsigcachesize is a DEBUG_ONLY option`
11817:42 <adam2k> correction, in the commit
11917: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.
12017:44 <hernanmarino> okey, makes sense
12117: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
12217:44 <stickies-v> ah right, I suppose another way would have been to introduce a new parameter `maxsigcachesizemib` to make that more explicit
12317:45 <glozow> stickies-v: yeah maybe, but it's debug-only 🤷
12417: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)
12517: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?
12617:46 <larryruane> (i mean, for nodes having the same cache settings)
12717:46 <glozow> larryruane: it's not a goal for all nodes' caches to be identical
12817:46 <adam2k> larryruane wouldn't the MiB still be calculated the same way for any system?
12917: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?
13017:46 <hernanmarino> it s only a cache after all
13117:47 <glozow> it's just that the caching is consensus-critical, and thus belongs in the consensus-critical section
13217: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?
13317:47 <larryruane> `git log -p filename`
13417:48 <larryruane> (i do that ALL the time)
13517:48 <michaelfolkson> <sipa> @larryruane If the cache has a bug, it may impact consensus.
13617:48 <adam2k> git blame is good too
13717:48 <michaelfolkson> I'm trying to think what kind of bug would impact consensus
13817:48 <sipa> michaelfolkson: Anything where the cache reports the wrong thing.
13917:48 <larryruane> the nice thing about `git log -p` compared with `git blame` is that the former shows removed lines, shows diffs
14017:49 <stickies-v> and you can also search with `commit:<commit_sha>` to show the PR that contained the commit, and read the discussion there
14117:49 <michaelfolkson> sipa: But the cache is just storing something temporarily right? It isn't changing what is being verified or the verification result
14217:49 <larryruane> plus you see the full commit message ... I often search the output of `git log -p` (pipe it into `less` for example)
14317: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
14417:50 <sipa> michaelfolkson: If it works correctly, yes. If it doesn't, who knows what it does?
14517:50 <larryruane> dongcarlimposter: oh cool, i didn't know about that TIL!
14617:51 <larryruane> (but i still think `git log -p` is awesome)
14717:51 <michaelfolkson> sipa: Hmm ok. Lacking some imagination on what a cache bug might do perhaps :)
14817: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!
14917: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
15017: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
15117:52 <dongcarlimposter> lmao someone at GitHub did some work on caches probably XP
15217: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.
15317:52 <glozow> larryruane: ye I use git blame too! thanks for sharing :)
15417: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)
15517:52 <glozow> (also sometimes i literally just search stuff using the github search bar, also quite good 😅)
15617:53 <larryruane> yes i've started doing that recently too, it's pretty good
15717:53 <glozow> The fourth commit references a uint "overflow". Describe what overflow could happen and under what conditions would it get hit?
15817:53 <glozow> link to commit:
15917:54 <stickies-v> `setup_bytes()` could pass a value to `setup()` that exceeds the bounds of a uint32
16017: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)
16117:55 <hernanmarino> When setup_bytes from the cuckooCache is big enough, the implicit conversion to uint32_t in the call to setup will overflow.
16217:56 <glozow> stickies-v: hernanmarino: thanks!
16317: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?
16417:56 <stickies-v> ((4294967295 / ((1 << 20) / 2)) =~ 8192)
16517:56 <glozow> commit:
16617: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
16717:57 <sipa> michaelfolkson: again: if the cache works correctly, then no, it will report exactly what was entered into the cache.
16817:57 <michaelfolkson> Ok thanks
16917: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.
17017:58 <larryruane> `size_t` is either 4 bytes or 8 bytes (see assumptions.h)
17117: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!
17217:59 <sipa> larryruane: indeed
17317:59 <larryruane> so i think size_t is appropriate for the memory size of something
17418:00 <glozow> here's a stack overflow post if people are interested
17518:00 <glozow> yep. size_t is meant to hold sizes
17618:00 <glozow> okay that's all we have time for today
17718:00 <glozow> remember to review the other commits too :P
17818:01 <glozow> #endmeeting
17918:01 <adam2k> Thanks glozow!
18018:01 <Lov3r_Of_Bitcoin> thanks
18118:01 <BlueMoon> Thank you very much, I was attentive.
18218:01 <larryruane> thanks @glozow!
18318:01 <michaelfolkson> Thanks glozow!
18418:02 <hernanmarino> Thanks ! If anyone wnats to stay to discuss the Quiz, I'm in :)
18518:02 <pablomartin> thanks @glozow
18618:02 <svav> Thanks glozow and all!
18718:02 <dongcarlimposter> Thanks glozow!
18818:03 <stickies-v> thank you glozow for hosting, very on point questions! and thank you dongcarlimposter for being here on behalf of dongcarl
18918:03 <larryruane> hernanmarino: definitely not (A)
19018:03 <juancama> thank you for hosting
19118:03 <hernanmarino> i agree
19218:03 <dongcarlimposter> stickies-v: XP
19318:03 <hernanmarino> definitely not (B)
19418:04 ← juancama left (
19518:04 <hernanmarino> any other insights ? I only have some intuitions, I am not sure
19618:05 <larryruane> if there's only 1 right answer, i'm thinking E but not sure
19718:05 <sipa> what is the question?
19818:05 <hernanmarino> E is my best choice, I think
19918: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)
20018:05 <hernanmarino> (A) the coinbase transaction
20118:06 <hernanmarino> (B) a transaction the node has never seen before
20218:06 <larryruane> sipa: oh that doesn't show the answers.. go to
20318:06 <hernanmarino> (C) a transaction with the same txid but different witness as a transaction in its mempool
20418:06 <larryruane> (the bottom of that page)
20518:06 <hernanmarino> (D) a transaction that was in its mempool but replaced by another
20618:06 <hernanmarino> (E) a transaction with no taproot inputs or outputs accepted to the mempool at height 709,000
20718:06 <hernanmarino> (F) All of the above
20818:06 <hernanmarino> (G) None of the above
20918:06 <stickies-v> I was actually thinking F
21018:07 <larryruane> (C) is a contender also
21118:07 <hernanmarino> We should go through the code to be sure, and i didnt do that
21218:07 <larryruane> stickies-v: well no, a coinbase tx wouldn't be in your cache, right?
21318:07 <stickies-v> well we're talking script cache, right
21418:07 <hernanmarino> Im between D and E
21518:07 <stickies-v> could very well be that the coinbase tx sends to a scriptpubkey that already exists, i think?
21618:07 <sipa> The coinbase tx has no executed scripts.
21718:08 <sipa> stickies-v: The script cache is for script execution, which happens at spending time.
21818:08 <sipa> The coinbase doesn't spend anything.
21918:08 <hernanmarino> but depends on the logic . I cannot think of a reason for E to be erased from the cache ....
22018:08 <stickies-v> ohh right
22118:08 <hernanmarino> but perhaps I m missing something
22218:08 <larryruane> hernanmarino: yes, i was thinking E also
22318: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
22418:09 <michaelfolkson> Instinctively I want to think of a possible bug that could affect consensus
22518: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
22618: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.
22718:09 <hernanmarino> (D) might be true or False, intuitvely
22818:09 <hernanmarino> glozow: thanks
22918:09 <michaelfolkson> Otherwise I'm not sure how to assess
23018:09 <sipa> Consensus code e.g. doesn't depend on the wallet, so this is not true for the wallet.
23118:09 <sipa> michaelfolkson: You look at the code.
23218:10 <michaelfolkson> With the LevelDB bug, was it obvious from the code?
23318:10 <sipa> It was obvious from the code that consensus depends on LevelDB, yes.
23418:10 <pablomartin_> hernanmarino: (D) a transaction that was in its mempool but replaced by another...
23518:10 <sipa> (or BDB)
23618:10 <sipa> That doesn't mean the bug itself is obvious.
23718:11 <larryruane> gosh i'm starting to think the answer is G!
23818:11 <hernanmarino> pablomartin_: that depends on the code , it might get erased or not ...
23918:11 <hernanmarino> larryruane: it might be the case , if (D) is not the answer
24018:12 <glozow> here's the code for computing script cache entry:
24118:12 <hernanmarino> we should really read the code :) but didn't get to do it today ...
24218: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?
24318:12 <pablomartin_> hernanmarino: we've discarded C & E?
24418:14 <glozow> larryruane: can you point to the code where the replaced tx is removed from the script cache?
24518:15 <larryruane> not quickly 😅
24618:15 <larryruane> (i was more just guessing that it would be removed!)
24718:17 <hernanmarino_> sorry, got disconnected ...
24818:19 <pablomartin> larryuane: glozow: perhaps there's something on lines 575-579...
24918:25 <larryruane> pablomartin: which file?
25018:26 <pablomartin> the one glozow sent above... src/validation.cpp
25118:28 <pablomartin> I'm running out of battery... pls forgive me if I suddenly leave...
25218:32 <larryruane> np ... I'm not sure, investigating ... will post here later if I figure it out!
25318:32 <hernanmarino_> same for me , still reading the code, but have to attend another minute now ...
25418:34 <pablomartin> pls ignore the lines I pointed you, it's not there
25518:34 <hernanmarino_> another meeting*
25618: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
25718:36 <hernanmarino_> that would really tell us if (D) is correct, or otherwise (G)
25818:37 <hernanmarino_> But I have to leave now , will get back later if anyone is still here . Goodbye everybody !
25918: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.
26018:48 <sipa> Well if you consider malicious code, you need far better separation.
26118:48 <sipa> Like running in another process, with well-defined, secure APIs, which don't trust callers
26218:48 <sipa> That's generally not the model we use for assessing what is consensus critical.
26318:49 <michaelfolkson> The cache example was malicious (or at least incompetent)?
26418:49 <sipa> No, just buggy.
26518:49 <sipa> Like: it could be returning the wrong thing.
26618: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.
26718:50 <sipa> If that were the case, we can assume it'd be caught by review.
26818:50 <sipa> Otherwise literally all of Bitcoin Core's C++ code would need to be considered consensus critical.
26918: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.
27018: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.
27118: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.
27218: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.
27318: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
27418:56 <sipa> It would not need suspicious code, not to the extent I meant it above.
27518: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.
27618:58 <sipa> The kind of "suspicious" code I'm referring to is something like the wallet directly accessing consensus data structures.
27718: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.
27819:00 <sipa> I think if you had some familiarity with the source code, this distinction would be very obvious.