[kernel 3c/n] Decouple validation cache initialization from ArgsManager #25527

pull dongcarl wants to merge 6 commits into bitcoin:master from dongcarl:2022-06-libbitcoinkernel-argsman-caches changing 15 files +152 −44
  1. dongcarl commented at 7:23 pm on July 1, 2022: contributor

    This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18

    This PR is NOT dependent on any other PRs.

    a.k.a. “Stop calling gArgs.GetIntArg("-maxsigcachesize") from validation code”

    This PR introduces the ValidationCacheSizes struct and its corresponding ApplyArgsManOptions function, removing the need to call gArgs from Init{Signature,ScriptExecution}Cache(). This serves to further decouple ArgsManager from libbitcoinkernel code.

    More context can be gleaned from the commit messages.

  2. dongcarl added this to the "WIP PRs" column in a project
  3. dongcarl moved this from the "WIP PRs" to the "Ready for Review PRs" column in a project
  4. dongcarl commented at 7:43 pm on July 1, 2022: contributor

    So it seems like the static asserts I added revealed that on MAX_MAX_SIG_CACHE_SIZE * (1 << 20) overflows size_t on 32-bit systems… This is an existing bug.

    https://cirrus-ci.com/task/5077719523262464?logs=ci#L2274

  5. DrahtBot added the label Refactoring on Jul 1, 2022
  6. dongcarl force-pushed on Jul 1, 2022
  7. dongcarl commented at 11:38 pm on July 1, 2022: contributor

    Push 3168fbf6ae9493df25727a11925a2f6677f30af2 -> 7f443a019ef90ee1a2a324f0b6a9abc98ef92ce5

    • Abolish the arbitrary MAX_MAX_SIG_CACHE_SIZE limit (16GiB) and instead return a friendly error when unable to allocate memory for validation caches
    • Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr DEFAULT_MAX_SIG_CACHE_SIZE_BYTES to utilize the compile-time integer arithmetic overflow checking available to constexprs.
  8. DrahtBot commented at 11:38 pm on July 1, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25704 (refactor: Remove all validation option globals by MarcoFalke)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  9. dongcarl force-pushed on Jul 2, 2022
  10. dongcarl commented at 0:19 am on July 2, 2022: contributor

    Push 7f443a019ef90ee1a2a324f0b6a9abc98ef92ce5 -> 1bcae96af276b8f4ea4ea15f9f3a5c8e2180c44c

    • Add overflow check when converting from total size to number of elements in setup_bytes
  11. DrahtBot added the label Needs rebase on Jul 7, 2022
  12. dongcarl force-pushed on Jul 7, 2022
  13. dongcarl commented at 3:38 pm on July 7, 2022: contributor

    Pushed 1bcae96af276b8f4ea4ea15f9f3a5c8e2180c44c -> 8e407571e99b0d4aad7f225b8197f35c6b6cea45

    • Rebased over master
  14. DrahtBot removed the label Needs rebase on Jul 7, 2022
  15. in src/bitcoin-chainstate.cpp:64 in 7aab67b9e3 outdated 
    60@@ -61,8 +61,8 @@ int main(int argc, char* argv[])
61     // Necessary for CheckInputScripts (eventually called by ProcessNewBlock),
62     // which will try the script cache first and fall back to actually
63     // performing the check with the signature cache.
64-    InitSignatureCache();
65-    InitScriptExecutionCache();
66+    assert(InitSignatureCache());
    ryanofsky commented at 5:54 pm on July 8, 2022:

    In commit “tests: Reduce calls to InitS*Cache()” (39ca267f885fd58f9467c782a72d97dc7f797225)

    Here and below should s/assert/Assert/. It is bad to execute non-debug code in a c++ assert statement, because depending on compiler and preprocessor settings, the assert argument may not be evaluated, and this is supposed to be an example program that can be built with external compilers and build systems. Also code is easier to read and skim if assert statements communicate what assumptions code is making, not what code is doing

    In the future it would be to good add actual error handling here, and let code work when NDEBUG is defined in general.

    dongcarl commented at 6:59 pm on July 11, 2022:
    Fixed in 0940f47e5e
  16. in src/script/sigcache.cpp:99 in 7aab67b9e3 outdated 
     99     // setup_bytes creates the minimum possible cache (2 elements).
100-    size_t nMaxCacheSize = std::min(std::max((int64_t)0, gArgs.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
101-    size_t nElems = signatureCache.setup_bytes(nMaxCacheSize);
102-    LogPrintf("Using %zu MiB out of %zu/2 requested for signature cache, able to store %zu elements\n",
103-            (nElems*sizeof(uint256)) >>20, (nMaxCacheSize*2)>>20, nElems);
104+    size_t nMaxCacheSize = std::max((int64_t)0, gArgs.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2) * ((size_t) 1 << 20);
    ryanofsky commented at 6:04 pm on July 8, 2022:

    In commit “cuckoocache: Return approximate memory size, failure” (6d0d7d9759c90c0533528893ce084f04be9387ee)

    I think it is worth adding a release note in this commit documenting that -maxsigcachesize internal cap is removed, so existing configurations with larger values may now be broken or demand more memory.

    dongcarl commented at 6:59 pm on July 11, 2022:
    Fixed in 0940f47e5e, let me know if the release note message is missing anything or feel free to resolve this!
  17. in src/cuckoocache.h:369 in be4a51448d outdated 
    374      */
375     std::optional<std::pair<uint32_t, size_t>> setup_bytes(size_t bytes)
376     {
377-        auto num_elems = setup(bytes/sizeof(Element));
378+        size_t requested_num_elems = bytes / sizeof(Element);
379+        if (std::numeric_limits<uint32_t>::max() < requested_num_elems) {
    ryanofsky commented at 6:13 pm on July 8, 2022:

    In commit “cuckoocache: Check for uint32 overflow in setup_bytes” (9f9911595f90b920fca71e53d2807f5b6feca137)

    Adding this check seems good but what was motivation? Is it just a problem with the previous code that you noticed and decided to fix? Or does it help with something that comes later? Would be good to say in commit message how this change relates to the PR

    dongcarl commented at 7:00 pm on July 11, 2022:
    Added some more context in eb8b6a9c5e, feel free to reopen if not clear!
    ryanofsky commented at 7:34 pm on July 12, 2022:

    Added some more context in eb8b6a9, feel free to reopen if not clear!

    CuckooCahce is misspelled but thanks for the update this is clear!

  18. in src/init.cpp:115 in bedef3249c outdated 
    109+using node::ApplyArgsManOptions;
110 using node::CacheSizes;
111 using node::CalculateCacheSizes;
112-using node::ChainstateLoadVerifyError;
113 using node::ChainstateLoadingError;
114+using node::ChainstateLoadVerifyError;
    ryanofsky commented at 6:27 pm on July 8, 2022:

    In commit “validationcaches: Add and use ValidationCacheSizes” (bedef3249cfa013a48347b19fb58712a8d0fdd28)

    I guess all these changes come from your editor doing case-insensitive sorts, which maybe is a preference. When I added these statements here and other places in #23497 I did purposefully use case-sensitive sorts, since I think case sensitive sorts make more sense in a case sensitive language, and look less glitchy visually, and do a better job grouping related identifiers together. You may perhaps want to keep this sorting, if it is not too much of a pain.

    dongcarl commented at 7:00 pm on July 11, 2022:
    I agree! Pesky editor settings… Fixed in caaa5be4ec
  19. in src/node/validation_cache_args.cpp:26 in 8e407571e9 outdated 
    20@@ -18,9 +21,10 @@ namespace node {
21 void ApplyArgsManOptions(const ArgsManager& argsman, ValidationCacheSizes& cache_sizes)
22 {
23     if (auto max_size = argsman.GetIntArg("-maxsigcachesize")) {
24+        size_t clamped_size_each = std::max<int64_t>(*max_size, 0) / 2 * (1 << 20);
    ryanofsky commented at 6:37 pm on July 8, 2022:

    In commit “validationcaches: Use size_t for sizes” (8e407571e99b0d4aad7f225b8197f35c6b6cea45)

    Deleted comment about if size “is set to zero, setup_bytes creates the minimum possible cache (2 elements)”, may be useful to keep to explain why 0 is the floor

    dongcarl commented at 7:03 pm on July 11, 2022:
    Restored and tweaked the comment in 7dfe0c29ec
  20. ryanofsky approved
  21. ryanofsky commented at 6:45 pm on July 8, 2022: contributor
    Code review ACK 8e407571e99b0d4aad7f225b8197f35c6b6cea45. Makes sense and cleans up some things nicely
  22. dongcarl force-pushed on Jul 11, 2022
  23. dongcarl commented at 6:58 pm on July 11, 2022: contributor

    Push 8e407571e99b0d4aad7f225b8197f35c6b6cea45 -> 7dfe0c29ec5a11fe60030413660155edcadc3268

    • Addressed Ryan’s review comments (Thanks Ryan!)
  24. ryanofsky approved
  25. ryanofsky commented at 7:38 pm on July 12, 2022: contributor
    Code review ACK 7dfe0c29ec5a11fe60030413660155edcadc3268, just suggested changes since last review
  26. dongcarl force-pushed on Jul 12, 2022
  27. dongcarl commented at 9:20 pm on July 12, 2022: contributor

    Push 7dfe0c29ec5a11fe60030413660155edcadc3268 -> 66767ab8395f940793c00371e2a14174ad46d6c8

  28. in src/test/util/setup_common.cpp:140 in 0940f47e5e outdated 
    135@@ -136,8 +136,8 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
136     m_node.kernel = std::make_unique<kernel::Context>();
137     SetupEnvironment();
138     SetupNetworking();
139-    InitSignatureCache();
140-    InitScriptExecutionCache();
141+    assert(InitSignatureCache());
142+    assert(InitScriptExecutionCache());
    ryanofsky commented at 2:56 pm on July 14, 2022:

    In commit “cuckoocache: Return approximate memory size, failure” (0940f47e5e93d184d2688ad67ada3dec62659581)

    Would also change these assert -> Assert for clarity since these actually need to execute code and are not just checking preconditions, and since compiler settings can disable assert() statements.

    dongcarl commented at 3:55 pm on July 14, 2022:
    Fixed in 51c08032b8
  29. in src/script/sigcache.cpp:95 in 77c649f9ec outdated 
    91@@ -92,11 +92,11 @@ static CSignatureCache signatureCache;
92 
93 // To be called once in AppInitMain/BasicTestingSetup to initialize the
94 // signatureCache.
95-bool InitSignatureCache()
96+bool InitSignatureCache(int64_t max_size)
    ryanofsky commented at 3:03 pm on July 14, 2022:

    In commit “validationcaches: Add and use ValidationCacheSizes” (77c649f9ecd684be69412a5699a55367fcd4571d)

    Throughout this commit, adding _bytes suffix to new argument names would be nice to avoid confusion with the other units used elsewhere (mib, number of elements).

    dongcarl commented at 3:55 pm on July 14, 2022:
    Fixed in 1c89fb7f24
  30. ryanofsky approved
  31. ryanofsky commented at 3:09 pm on July 14, 2022: contributor

    Code review ACK 66767ab8395f940793c00371e2a14174ad46d6c8. Just cuckoocache spelling fix since last review.

    I am the lonely reviewer here so would be good to have more review. This PR adds the first ever code to catch bad_alloc, and no one knows when the bad_alloc will strike. This is exciting stuff!

  32. dongcarl force-pushed on Jul 14, 2022
  33. dongcarl commented at 3:52 pm on July 14, 2022: contributor

    Push 66767ab839 -> 365a04e9ac

    Thanks Ryan!

  34. dongcarl force-pushed on Jul 14, 2022
  35. dongcarl force-pushed on Jul 14, 2022
  36. dongcarl force-pushed on Jul 14, 2022
  37. dongcarl commented at 3:59 pm on July 14, 2022: contributor

    Push 365a04e9ac6373c6619aaa943b02f5deead5a7c4 -> 9580544010

    • Rebased over master for Desig(...) retirement
  38. ryanofsky approved
  39. ryanofsky commented at 12:34 pm on July 15, 2022: contributor
    Code review ACK 95805440101820cc246bb4379d8505d81074eb99. Since last review, just some changes assert -> Assert, max_size -> max_size_bytes and switching to designated initializers
  40. DrahtBot added the label Needs rebase on Jul 18, 2022
  41. dongcarl force-pushed on Jul 18, 2022
  42. DrahtBot removed the label Needs rebase on Jul 18, 2022
  43. DrahtBot added the label Needs rebase on Jul 19, 2022
  44. dongcarl force-pushed on Jul 20, 2022
  45. DrahtBot removed the label Needs rebase on Jul 20, 2022
  46. ryanofsky approved
  47. ryanofsky commented at 2:49 pm on July 20, 2022: contributor

    Code review ACK 915c8ff123c770f0cce82358256ab02b93018767. Just rebased since last review.

    Would encourage more review of this PR. If it is not clear what this is doing I’d summarize it as “Stop calling gArgs.GetIntArg("-maxsigcachesize”) from validation code"

  48. in src/cuckoocache.h:350 in 915c8ff123 outdated 
    350+        try {
351+            table.reserve(size);
352+            table.resize(size);
353+        } catch (std::bad_alloc const&) {
354+            return std::nullopt;
355+        }
    aureleoules commented at 8:23 am on July 21, 2022:
    This is never reached because there is already a global handler to handle bad allocs, so I think you can remove the std::optional. https://github.com/bitcoin/bitcoin/blob/915c8ff123c770f0cce82358256ab02b93018767/src/init.cpp#L753-L764
    dongcarl commented at 6:38 pm on July 21, 2022:
    Oooof that sucks… There’s no way to have a new handler that can access the std::bad_alloc object?
    aureleoules commented at 11:11 am on July 22, 2022:
    AFAIK there doesn’t seem to be a way :/
    dongcarl commented at 3:45 pm on July 26, 2022:
    Removed as of f66cd5b3aa, I think the existence of a new_handler makes this not too big of a problem now, and as was noted -maxsigcachesize is DEBUG_ONLY
    dongcarl commented at 3:29 pm on August 2, 2022:
    Closing, feel free to open if still a problem.
  49. in src/script/sigcache.cpp:106 in 557a9ce55c outdated 
    106+    auto setup_results = signatureCache.setup_bytes(nMaxCacheSize);
107+    if (!setup_results) return false;
108+
109+    const auto [num_elems, approx_size_bytes] = *setup_results;
110+    LogPrintf("Using %zu MiB out of %zu requested for signature cache, able to store %zu elements\n",
111+              approx_size_bytes, nMaxCacheSize, num_elems);
    mzumsande commented at 7:46 pm on July 24, 2022:
    This changes the logged value from MiB to bytes (on purpose?), but the log still says “MiB”. Also, didn’t you like the previous way of adding “/2” to the actual requested amount, which made it clear that the requested memory was split evenly between the two caches?
    dongcarl commented at 3:44 pm on July 26, 2022:

    The bytes thing should be fixed now in f66cd5b3aa

    Since we now pass in the actual amount in e4d5bab88e (not amount * 2), I think we shouldn’t say “/2”. Since libbitcoinkernel users might specify different sizes, or we might decide to split it differently in the future.

    dongcarl commented at 3:29 pm on August 2, 2022:
    Closing, feel free to open if still a problem!
  50. in src/validation.cpp:1672 in 557a9ce55c outdated 
    1671+
1672+    auto setup_results = g_scriptExecutionCache.setup_bytes(nMaxCacheSize);
1673+    if (!setup_results) return false;
1674+
1675+    const auto [num_elems, approx_size_bytes] = *setup_results;
1676+    LogPrintf("Using %zu MiB out of %zu requested for script execution cache, able to store %zu elements\n",
    mzumsande commented at 7:47 pm on July 24, 2022:
    Same as above for the SignatureCache.
    dongcarl commented at 3:44 pm on July 26, 2022:
    See: #25527 (review)
  51. in doc/release-notes-25527.md:4 in 557a9ce55c outdated 
    0@@ -0,0 +1,8 @@
1+Updated settings
2+----------------
3+
4+- Specifying a `-maxsigcachesize` MiB value which is too large will now result
    mzumsande commented at 0:35 am on July 25, 2022:
    -maxsigcachesize is DEBUG_ONLY, should we cover these in release notes too?
    dongcarl commented at 3:39 pm on July 26, 2022:
    Very good point! Fixed in f66cd5b3aa
  52. mzumsande commented at 0:35 am on July 25, 2022: contributor
    Concept ACK
  53. dongcarl force-pushed on Jul 25, 2022
  54. dongcarl commented at 3:39 pm on July 26, 2022: contributor

    Push 915c8ff123c770f0cce82358256ab02b93018767 -> d29f2fd9b49ac00a3721af7260dbbf59e9e8387c

    • Realize that -maxsigcachesize is DEBUG_ONLY, remove release notes and adjust estimated importance of back-compatibility
    • Restore CuckooCache::cache::setup to return a non-optional again, since we can’t actually catch a std::bad_alloc (see: #25527 (review))
    • Fix log line for memory reporting, fixing:
    • Split out limit abolition to its own commit
  55. in src/script/sigcache.cpp:101 in f66cd5b3aa outdated 
     98     // nMaxCacheSize is unsigned. If -maxsigcachesize is set to zero,
 99     // setup_bytes creates the minimum possible cache (2 elements).
100     size_t nMaxCacheSize = std::min(std::max((int64_t)0, gArgs.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
101-    size_t nElems = signatureCache.setup_bytes(nMaxCacheSize);
102+
103+    auto setup_results = signatureCache.setup_bytes(nMaxCacheSize);
    ryanofsky commented at 7:06 pm on July 29, 2022:

    In commit “cuckoocache: Return approximate memory size” (f66cd5b3aa25f27638b05fbffde7470dd844951b)

    ~It’s not optional anymore so no need for intermediate setup_results variable which is only referenced once~

    EDIT: A future commit does makes it optional again, so I guess this is needed. Keeping this comment to avoid other reviewers wondering about this.

  56. in src/init.cpp:1175 in f66cd5b3aa outdated 
    1170@@ -1171,8 +1171,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1171                   args.GetArg("-datadir", ""), fs::PathToString(fs::current_path()));
1172     }
1173 
1174-    InitSignatureCache();
1175-    InitScriptExecutionCache();
1176+    if (!InitSignatureCache() || !InitScriptExecutionCache()) {
1177+        return InitError(strprintf(_("Unable to allocate memory for -maxsigcachesize: '%s' MiB"), args.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE)));
    ryanofsky commented at 7:12 pm on July 29, 2022:

    In commit “cuckoocache: Return approximate memory size” (f66cd5b3aa25f27638b05fbffde7470dd844951b)

    ~This condition seems impossible to hit and maybe it doesn’t make sense for InitSignatureCache and InitScriptExecutionCache to change return types from void to nodiscard bool if they always return true~

    EDIT: A future commit does makes it fail again, so I guess this is needed. Keeping this comment to avoid other reviewers being confused by unused code.

  57. in src/init.cpp:557 in e4d5bab88e outdated 
    553@@ -550,7 +554,7 @@ void SetupServerArgs(ArgsManager& argsman)
554     argsman.AddArg("-addrmantest", "Allows to test address relay on localhost", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
555     argsman.AddArg("-capturemessages", "Capture all P2P messages to disk", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
556     argsman.AddArg("-mocktime=<n>", "Replace actual time with " + UNIX_EPOCH_TIME + " (default: 0)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
557-    argsman.AddArg("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
558+    argsman.AddArg("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_MAX_SIG_CACHE_SIZE_BYTES >> 20), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    ryanofsky commented at 10:00 pm on July 29, 2022:

    In commit “validationcaches: Add and use ValidationCacheSizes” (e4d5bab88e9acad53cb8d636d836d2cdf9bb49b8)

    Throughout this commit could replace _size_bytes with _bytes. A lot of these names are pretty long and a bytes value is obviously a size.

    dongcarl commented at 3:27 pm on August 2, 2022:
    Fixed as of eeb224e4d3
  58. in src/node/validation_cache_args.cpp:21 in e4d5bab88e outdated 
    16+namespace node {
17+void ApplyArgsManOptions(const ArgsManager& argsman, ValidationCacheSizes& cache_sizes)
18+{
19+    if (auto max_size = argsman.GetIntArg("-maxsigcachesize")) {
20+        cache_sizes = {
21+            .signature_cache_bytes = *max_size / 2 * (1 << 20),
    ryanofsky commented at 10:02 pm on July 29, 2022:

    In commit “validationcaches: Add and use ValidationCacheSizes” (e4d5bab88e9acad53cb8d636d836d2cdf9bb49b8)

    I guess this would be a change in behavior, but maybe tweak order of operations so if a 1 MiB cache is requested this uses .5 MiB for each instead of 0

    dongcarl commented at 3:28 pm on August 2, 2022:
    Fixed as of eeb224e4d3, great catch!
  59. ryanofsky approved
  60. ryanofsky commented at 10:19 pm on July 29, 2022: contributor
    Code review ACK d29f2fd9b49ac00a3721af7260dbbf59e9e8387c
  61. dongcarl force-pushed on Aug 2, 2022
  62. dongcarl commented at 4:40 pm on August 2, 2022: contributor

    Push d29f2fd9b49ac00a3721af7260dbbf59e9e8387c -> c7138d317e8db5874c01cbd44a64942200214d1a

  63. ryanofsky commented at 7:06 pm on August 2, 2022: contributor
    Code review ACK c7138d317e8db5874c01cbd44a64942200214d1a, now rounding -maxsigcachesize down to nearest 1MiB instead of 2MiB (slight change in behavior) and renaming _size_bytes -> _bytes as suggested
  64. DrahtBot added the label Needs rebase on Aug 3, 2022
  65. in src/validation.cpp:1670 in f66cd5b3aa outdated 
    1663@@ -1664,9 +1664,13 @@ void InitScriptExecutionCache() {
1664     // nMaxCacheSize is unsigned. If -maxsigcachesize is set to zero,
1665     // setup_bytes creates the minimum possible cache (2 elements).
1666     size_t nMaxCacheSize = std::min(std::max((int64_t)0, gArgs.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
1667-    size_t nElems = g_scriptExecutionCache.setup_bytes(nMaxCacheSize);
1668+
1669+    auto setup_results = g_scriptExecutionCache.setup_bytes(nMaxCacheSize);
1670+
1671+    const auto [num_elems, approx_size_bytes] = setup_results;
    glozow commented at 9:26 am on August 3, 2022:
    Thanks @ryanofsky for leaving https://github.com/bitcoin/bitcoin/pull/25527/commits/f66cd5b3aa25f27638b05fbffde7470dd844951b#r933549579, it helped me here as well!
  66. in src/script/sigcache.h:19 in eeb224e4d3 outdated 
    15 
16 // DoS prevention: limit cache size to 32MB (over 1000000 entries on 64-bit
17 // systems). Due to how we count cache size, actual memory usage is slightly
18 // more (~32.25 MB)
19-static const unsigned int DEFAULT_MAX_SIG_CACHE_SIZE = 32;
20+static constexpr size_t DEFAULT_MAX_SIG_CACHE_BYTES{32 << 20};
    glozow commented at 9:39 am on August 3, 2022:

    in eeb224e4d308cd2d3c00743715699abf6dfd3f03

    Should the comment above be edited from MB to MiB?

    dongcarl commented at 4:04 pm on August 3, 2022:
    Fixed in 41c5201a90, thanks!
  67. glozow commented at 11:37 am on August 3, 2022: member

    code review ACK c7138d317e8db5874c01cbd44a64942200214d1a

    happy to reACK after rebase

  68. tests: Reduce calls to InitS*Cache() 
    In src/test/fuzz/script_sigcache.cpp, we should really be setting up a
full working BasicTestingSetup. The initialize_ function is only run
once anyway.

In src/test/txvalidationcache_tests.cpp, the Dersig100Setup inherits
from BasicTestingSetup, which should have already set up a global script
execution cache without the need to explicitly call
InitScriptExecutionCache.
    0dbce4b103
  69. cuckoocache: Return approximate memory size 
    Returning the approximate total size eliminates the need for
InitS*Cache() to do nElems*sizeof(uint256). The cuckoocache has a better
idea of this information.
    08dbc6ef72
  70. validationcaches: Abolish arbitrary limit 
    1. -maxsigcachesize is a DEBUG_ONLY option

2. Almost 7 years has passed since its semantics change in
   830e3f3d027ba5c8121eed0f6a9ce99961352572 from "number of entries" to
   "number of mebibytes"

3. A std::new_handler was added to the codebase after the original PR
   which introduced this limit, which will terminate immediately instead
   of causing trouble by being caught somewhere unexpected.
    b370164b31
  71. cuckoocache: Check for uint32 overflow in setup_bytes 
    This fixes an potential overflow which existed prior to this patchset.

If CuckooCache::cache<Element, Hash>::setup_bytes is called with a
`size_t bytes` which, when divided by sizeof(Element), does not fit into
an uint32_t, the implicit conversion to uint32_t in the call to setup
will result in an overflow.

At least on x86_64, this overflow is possible:

static_assert(std::numeric_limits<size_t>::max() / 32 <= std::numeric_limits<uint32_t>::max());
static_assert(std::numeric_limits<size_t>::max() / 4 <= std::numeric_limits<uint32_t>::max());

This commit detects such cases and signals to callers that the `size_t
bytes` input is too large.
    82d3058539
  72. validationcaches: Add and use ValidationCacheSizes 
    Also:

- Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr
  DEFAULT_MAX_SIG_CACHE_BYTES to utilize the compile-time integer
  arithmetic overflow checking available to constexpr.
- Fix comment (MiB instead of MB) for DEFAULT_MAX_SIG_CACHE_BYTES.
- Pass in max_size_bytes parameter to InitS*Cache(), modify log line to
  no longer allude to maxsigcachesize being split evenly between the two
  validation caches.
- Fix possible integer truncation and add a comment.

[META] I've kept the integer types as int64_t in order to not introduce
       unintended behaviour changes, in the next commit we will make
       them size_t.
    41c5201a90
  73. validationcaches: Use size_t for sizes 
    ...also move the 0-clamping logic to ApplyArgsManOptions, where it
   belongs.
    0f3a2532c3
  74. dongcarl force-pushed on Aug 3, 2022
  75. dongcarl commented at 4:04 pm on August 3, 2022: contributor

    Push c7138d317e -> 0f3a2532c3

  76. DrahtBot removed the label Needs rebase on Aug 3, 2022
  77. hernanmarino approved
  78. hernanmarino commented at 8:21 pm on August 3, 2022: contributor
    Code review ACK .
  79. glozow commented at 8:04 am on August 4, 2022: member
    re ACK 0f3a2532c3
  80. theStack approved
  81. theStack commented at 2:13 pm on August 4, 2022: contributor

    Code-review ACK 0f3a2532c38074dd9789d1c4c667db6ca46ff0ab

    For a better understanding, I can (as always) highly recommend reading this PR’s review club notes (and chat log) to other potential reviewers.

  82. ryanofsky approved
  83. ryanofsky commented at 3:39 pm on August 4, 2022: contributor
    Code review ACK 0f3a2532c38074dd9789d1c4c667db6ca46ff0ab. Rebase and comment tweak since last
  84. glozow merged this on Aug 4, 2022
  85. glozow closed this on Aug 4, 2022
  86. glozow moved this from the "Ready for Review PRs" to the "Done or Closed or Rethinking" column in a project
  87. sidhujag referenced this in commit d288bf1e76 on Aug 4, 2022
  88. ryanofsky commented at 12:59 pm on August 6, 2022: contributor

    I just realized I asked to rename size -> size_bytes, and then later asked rename size_bytes to bytes :facepalm:

    At least it didn’t go in a circle, though

  89. ryanofsky referenced this in commit ba91ffa1ce on Aug 17, 2022
  90. ryanofsky referenced this in commit d98d2aa19f on Aug 17, 2022
  91. ryanofsky referenced this in commit 841c3a1205 on Aug 17, 2022
  92. ryanofsky referenced this in commit b86aabc520 on Aug 17, 2022
  93. ryanofsky referenced this in commit 13d466e247 on Aug 17, 2022
  94. ryanofsky referenced this in commit 370189fe96 on Aug 18, 2022
  95. ryanofsky referenced this in commit a6b3dbeca4 on Aug 22, 2022
  96. ryanofsky referenced this in commit 1c77e2e0db on Aug 22, 2022
  97. ryanofsky referenced this in commit a76f628585 on Aug 26, 2022
  98. ryanofsky referenced this in commit 4e65b3b222 on Aug 26, 2022
  99. ryanofsky referenced this in commit cf0b02fb27 on Oct 14, 2022
  100. achow101 referenced this in commit 9321df4487 on Feb 17, 2023
  101. fanquake referenced this in commit b175bdb9b2 on Mar 14, 2023
  102. fanquake referenced this in commit e695d8536e on Mar 16, 2023
  103. fanquake referenced this in commit 369d4c03b7 on Apr 3, 2023
  104. fanquake referenced this in commit c2f2abd0a4 on May 11, 2023
  105. ryanofsky referenced this in commit 153a6882f4 on Jun 9, 2023
  106. bitcoin locked this on Aug 16, 2023


dongcarl DrahtBot ryanofsky aureleoules mzumsande glozow hernanmarino theStack

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me