Handle unusual maxsigcachesize gracefully #9777

1. 
   jnewbery commented at 2:55 PM on February 16, 2017: member

   belt-and-braces approach:

   • first commit fixes the bug where maxsigcachesize being zero causes segfault
   • second commit handles maxsigcachesize being negative by setting it to zero if a negative number is provided
   • third commit handles maxsigcachesize being too large by setting it to MAX_MAX_SIG_CACHE_SIZE if an unreasonably large number is provided.

   I've set MAX_MAX_SIG_CACHE_SIZE absurdly high (16GB) for now, so setting maxsigcachesize to anything unreasonably large will still cause a bad_alloc for most users. Input welcomed on what a more reasonable MAX_MAX_SIG_CACHE_SIZE should be, or whether we should leave it out entirely.

   Replaces https://github.com/bitcoin/bitcoin/pull/9770

2. Allow maxsigcachesize to be zero

   If the -maxsigcachesize parameter is set to zero, setup a minimum sized
   sigcache (2 elements) rather than segfaulting.

   d72fe44fc1
3. Handle maxsigcachesize being negative 7796d2ea25
4. handle maxsigcachesize being too large e6374a03b9
5. laanwj added this to the milestone 0.14.0 on Feb 16, 2017
6. laanwj added the label Bug on Feb 16, 2017
7. 
   laanwj commented at 4:06 PM on February 16, 2017: member

   I've set MAX_MAX_SIG_CACHE_SIZE absurdly high (16GB) for now, so setting maxsigcachesize to anything unreasonably large will still cause a bad_alloc for most users. Input welcomed on what a more reasonable MAX_MAX_SIG_CACHE_SIZE should be, or whether we should leave it out entirely.

   I'd say that is reasonable. We don't want bad_alloc for negative amounts (which you fixed), but if the user specifies a higher value than available memory then it's what you expect. Even better error handling would catch the bad_alloc and print a friendly error message, but that's overdoing it in this particular case.

8. 
   JeremyRubin commented at 6:26 PM on February 16, 2017: contributor

   utACK e6374a0.

   I would say it'd also be fine to restrict to ~512MB (covers at least a full day of maximum sigop blocks) if you want to be more aggressive about it, but 16GB is fine too.

9. 
   sipa commented at 7:00 PM on February 16, 2017: member

   utACK

10. 
    morcos commented at 8:34 PM on February 16, 2017: member

    utACK e6374a0

11. 
    jtimon commented at 8:53 PM on February 16, 2017: contributor

    utACK e6374a0, perhaps some squashing can be done, at least the first 2 commits. Not a big deal though.

12. in src/script/sigcache.cpp:None in e6374a03b9

      94 | @@ -95,7 +95,7 @@ void InitSignatureCache()
      95 |  {
      96 |      // nMaxCacheSize is unsigned. If -maxsigcachesize is set to zero,
      97 |      // setup_bytes creates the minimum possible cache (2 elements).
      98 | -    size_t nMaxCacheSize = std::max((int64_t)0, GetArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE)) * ((size_t) 1 << 20);
      99 | +    size_t nMaxCacheSize = std::min(std::max((int64_t)0, GetArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE)), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
    

    ---

    ---

    

    jtimon commented at 8:55 PM on February 16, 2017:

    Maybe ((size_t) 1 << 20) could be made a constant/macro, not necessarily in this PR.

13. 
    paveljanik commented at 9:07 PM on February 16, 2017: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/9777/commits/e6374a03b9869362595acd5cc5e5b8994b244af1

    -maxsigcachesize=0 does not result in a crash anymore. Max limit applies (=20000 results in 16384).

    +1 for squashing.

14. gmaxwell approved
15. 
    gmaxwell commented at 3:17 AM on February 17, 2017: contributor

    utACK 16384 is implausibly large but fine. Reducing memory option footguns is something for a future time when we've solved the nearly infinite number of more pressing issues, and is better than doing nothing there.

    I believe the most common case of a large value though is misunderstanding the units (e.g. giving it a limit in bytes).

16. 
    jnewbery commented at 3:28 AM on February 17, 2017: member

    I believe the most common case of a large value though is misunderstanding the units (e.g. giving it a limit in bytes).

    Or using a pre-2016 bitcoin.conf when maxsigcachesize was measured in cache entries instead of MiB :(

17. 
    gmaxwell commented at 4:04 AM on February 17, 2017: contributor

    ouch. uh. that is an argument for throwing an error or renaming the argument rather than just claiming to an insane value.

18. 
    laanwj commented at 8:00 AM on February 17, 2017: member

    ouch. uh. that is an argument for throwing an error or renaming the argument rather than just claiming to an insane value.

    Yes it also would have been perfectly valid to make InitSignatureCache() return an error status, and handle the error upstream by printing a message (hopefully a generalized "value out of range" one instead of adding a specific translation message) instead of clamping it here.

    However now that we have this, I don't see a point in redesigning it again and going through another review. As you say there are an infinite number of more pressing issues.

19. 
    laanwj commented at 8:07 AM on February 17, 2017: member

    squashed d72fe44fc1989575ff5de98c882c0010d1de26e7..e6374a03b9869362595acd5cc5e5b8994b244af1 into 55c403b8febe02555c52bac7028cd6b1f006fad1, merged via 8dee8221770893fbf0ec6c19ad385537984ba44f

20. laanwj closed this on Feb 17, 2017

    ---

21. laanwj referenced this in commit 8dee822177 on Feb 17, 2017
22. codablock referenced this in commit faeb6cc2ec on Jan 19, 2018
23. codablock referenced this in commit 86fd57b8cd on Jan 23, 2018
24. andvgal referenced this in commit 6d17bcb0c6 on Jan 6, 2019
25. CryptoCentric referenced this in commit ed72e026c7 on Feb 27, 2019
26. DrahtBot locked this on Sep 8, 2021

Milestone
0.14.0