Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
No longer ever reuse keypool indexes #10795
Conversation
laanwj
added this to the
0.15.0
milestone
Jul 11, 2017
laanwj
added the
Wallet
label
Jul 11, 2017
TheBlueMatt
referenced
this pull request
Jul 11, 2017
Merged
Track keypool entries as internal vs external in memory #10235
fanquake
deleted a comment
from MIGUELWAXX
Jul 11, 2017
|
utACK 1c01ea6 bugfix needed for 0.15 |
| - nEnd = std::max(nEnd, *(setExternalKeyPool.rbegin()) + 1); | ||
| - } | ||
| + int64_t index = ++m_max_keypool_index; | ||
| + assert(index >= 0); // How in the hell did you use so many keys? |
sipa
Jul 13, 2017
Owner
Signed overflow is undefined behaviour, so the compiler is allowed to optimize this assert out (it can assume overflow never happens).
morcos
Jul 13, 2017
Contributor
heh, i almost commented that it would be clearer to check against max
TheBlueMatt
Jul 14, 2017
Contributor
Heh, but in practice it doesnt, plus its just an assert, so whatever :).
gmaxwell
Jul 17, 2017
Member
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
Jul 17, 2017
•
Owner
Would be better (non-UB, but also more clear to read) to check against std::numeric_limits<int64_t>::max() before the increase.
|
Needs rebase. |
|
Rebased. |
|
LGTM, what a foresight Satoshi had to use 64 bit numbers for keypool indexes in walletdb. |
|
re-utACK 44d0996 |
|
@TheBlueMatt plz2rebase |
|
Rebased. |
|
re-re-utACK 1fc8c3d |
laanwj
merged commit 1fc8c3d
into
bitcoin:master
Jul 18, 2017
1 check passed
laanwj
added a commit
that referenced
this pull request
Jul 18, 2017
|
|
laanwj |
This commit was signed with a verified signature.
laanwj
Wladimir J. van der Laan
|
7b6e8bc
|
|
post merge ACK |
TheBlueMatt commentedJul 11, 2017
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.