Allow maxsigcachesize to be zero #9770

1. 
   jnewbery commented at 7:17 PM on February 15, 2017: member

   Fixes #9759

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

2. jnewbery closed this on Feb 15, 2017

   ---

3. Allow maxsigcachesize to be zero

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

   d72fe44fc1
4. jnewbery reopened this on Feb 15, 2017

   ---

5. jnewbery force-pushed on Feb 15, 2017
6. fanquake added this to the milestone 0.14.0 on Feb 15, 2017
7. 
   theuni commented at 8:34 PM on February 15, 2017: member

   Hmm, I'd expect the sigcache to be bypassed entirely in this case. I suppose this is fine for 0.14, but I think it'd probably be useful to have a SignatureCheckerNoCache to benchmark cache overhead.

8. 
   JeremyRubin commented at 3:04 AM on February 16, 2017: contributor

   @theuni I'm ambivalent about that approach but I agree that it's likely a better approach (see my comment making the same point (sans solution) in the issue #9759). Ambivalence because more reliance on virtual methods, whereas it might be nice to get rid of the virtual-ness at some point. Running with cache size 0 under this patch means doing some likely unfruitful work every check though so clearly that's the better of 2 evils.

9. 
   sipa commented at 3:06 AM on February 16, 2017: member

   Slightly suboptimal performance with sigcachesize 0 (which is afaik not recommended anywhere) is perfectly acceptable for 0.14, I think - it's not even a regression. We can reconsider for 0.15.

10. 
    laanwj commented at 9:28 AM on February 16, 2017: member

    I think this is a good pragmatic solution. What matters here is that it doesn't crash. A user shouldn't set the cache to 0 if the goal is performance. utACK https://github.com/bitcoin/bitcoin/pull/9770/commits/d72fe44fc1989575ff5de98c882c0010d1de26e7 . see below, need decision on what to do with negative values

11. 
    laanwj commented at 9:29 AM on February 16, 2017: member

    BTW: what happens with a negative argument? Will it wrap around and still crash? :) edit: bad_alloc, apparently:

    2017-02-16 09:35:27 Using at most 125 automatic connections (1024 file descriptors available)
    2017-02-16 09:35:27 
    
    ************************
    EXCEPTION: St9bad_alloc       
    std::bad_alloc       
    bitcoin in AppInit()       
    
    
    
    ************************
    EXCEPTION: St9bad_alloc       
    std::bad_alloc       
    bitcoin in AppInit()       
    

12. 
    JeremyRubin commented at 9:47 AM on February 16, 2017: contributor

    In #9759 I suggested setup_bytes(max(0, nMaxCacheSize)) for that reason, but I suppose that's not sufficient either: nMaxCacheSize should be made a signed int of some sort rather than size_t.

13. 
    laanwj commented at 9:48 AM on February 16, 2017: member

    Yeah, ssize_t would do the job. Or apply the std::max() directly to the GetArg result.

14. laanwj merged this on Feb 16, 2017
15. laanwj closed this on Feb 16, 2017

    ---

16. laanwj referenced this in commit df2f34dcad on Feb 16, 2017
17. 
    laanwj commented at 11:19 AM on February 16, 2017: member

    Didn't intend to merge this one yet, sorry. Anyhow if we want to cope with the negative amounts, need to do that in a new PR.

    Edit: needed to revert anyway - something went wrong with the tooling, the merge was not signed either.

18. 
    fanquake commented at 12:32 PM on February 16, 2017: member

    @jnewbery Can you open a new PR with these changes.

19. jnewbery deleted the branch on Feb 16, 2017
20. 
    jnewbery commented at 2:19 PM on February 16, 2017: member

    @fanquake yes I'll do that today.

21. MarcoFalke locked this on Sep 8, 2021

Milestone
0.14.0