No longer ever reuse keypool indexes #10795

1. 
   TheBlueMatt commented at 4:18 pm on July 11, 2017: member

   This fixes an issue where you could reserve a keypool entry, then top up the keypool, writing out a new key at the given index, then return they key from the pool. This isnt likely to cause issues, but given there is no reason to ever re-use keypool indexes (they’re 64 bits…), best to avoid it alltogether.

   Builds on #10235, should probably get a 15 tag.

2. laanwj added this to the milestone 0.15.0 on Jul 11, 2017
3. laanwj added the label Wallet on Jul 11, 2017
4. TheBlueMatt force-pushed on Jul 11, 2017
5. TheBlueMatt force-pushed on Jul 11, 2017
6. fanquake deleted a comment on Jul 11, 2017
7. TheBlueMatt force-pushed on Jul 12, 2017
8. 
   morcos commented at 8:19 pm on July 12, 2017: member

   utACK 1c01ea6

   bugfix needed for 0.15

9. in src/wallet/wallet.cpp:3180 in 1c01ea6f78 outdated

   3145-            }
   3146-            if (!setExternalKeyPool.empty()) {
   3147-                nEnd = std::max(nEnd, *(setExternalKeyPool.rbegin()) + 1);
   3148-            }
   3149+            int64_t index = ++m_max_keypool_index;
   3150+            assert(index >= 0); // How in the hell did you use so many keys?
   

   ---

   ---

   

   sipa commented at 0:35 am on July 13, 2017:

   Signed overflow is undefined behaviour, so the compiler is allowed to optimize this assert out (it can assume overflow never happens).

   ---

   

   morcos commented at 5:29 pm on July 13, 2017:

   heh, i almost commented that it would be clearer to check against max

   ---

   

   TheBlueMatt commented at 3:14 pm on July 14, 2017:

   Heh, but in practice it doesnt, plus its just an assert, so whatever :).

   ---

   

   gmaxwell commented at 6:27 am on July 17, 2017:

   Code like this makes me go “wtf were the authors of this smoking”: asserts like this are usually optimized out, so the code looks like it’s saying that the author thought the signed overflow was kosher and that they thought it could happen here.

   ---

   

   laanwj commented at 7:42 am on July 17, 2017:

   Would be better (non-UB, but also more clear to read) to check against std::numeric_limits<int64_t>::max() before the increase.

   ---

   

   TheBlueMatt commented at 3:01 pm on July 17, 2017:

   Done
10. 
    sipa commented at 0:37 am on July 13, 2017: member

    utACK 1c01ea6f780ba1fff12f03cc14b24f70541e3cc8
11. 
    sipa commented at 0:39 am on July 16, 2017: member

    Needs rebase.
12. TheBlueMatt force-pushed on Jul 16, 2017
13. 
    TheBlueMatt commented at 10:51 pm on July 16, 2017: member

    Rebased.
14. 
    laanwj commented at 7:45 am on July 17, 2017: member

    LGTM, what a foresight Satoshi had to use 64 bit numbers for keypool indexes in walletdb.
15. 
    morcos commented at 10:20 am on July 17, 2017: member

    re-utACK 44d0996
16. TheBlueMatt force-pushed on Jul 17, 2017
17. 
    gmaxwell commented at 3:31 pm on July 17, 2017: contributor

    @TheBlueMatt plz2rebase
18. No longer ever reuse keypool indexes

    This fixes an issue where you could reserve a keypool entry, then
    top up the keypool, writing out a new key at the given index, then
    return they key from the pool. This isnt likely to cause issues,
    but given there is no reason to ever re-use keypool indexes
    (they're 64 bits...), best to avoid it alltogether.

    1fc8c3de0c
19. 
    TheBlueMatt commented at 4:12 pm on July 17, 2017: member

    Rebased.
20. TheBlueMatt force-pushed on Jul 17, 2017
21. 
    morcos commented at 5:51 pm on July 17, 2017: member

    re-re-utACK 1fc8c3d (one more and I think git will do it for me)
22. laanwj merged this on Jul 18, 2017
23. laanwj closed this on Jul 18, 2017

    ---

24. laanwj referenced this in commit 7b6e8bc442 on Jul 18, 2017
25. 
    jonasschnelli commented at 8:25 am on July 18, 2017: contributor

    post merge ACK
26. PastaPastaPasta referenced this in commit 4bab9cb2d0 on Sep 16, 2019
27. PastaPastaPasta referenced this in commit b8c1d66fb9 on Sep 18, 2019
28. barrystyle referenced this in commit 9e562ac2a1 on Jan 22, 2020
29. MarcoFalke locked this on Sep 8, 2021

Contributors
TheBlueMatt morcos sipa gmaxwell laanwj jonasschnelli

Labels
Wallet

Milestone
0.15.0