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.
utACK 1c01ea6
bugfix needed for 0.15
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?
Signed overflow is undefined behaviour, so the compiler is allowed to optimize this assert out (it can assume overflow never happens).
heh, i almost commented that it would be clearer to check against max
Heh, but in practice it doesnt, plus its just an assert, so whatever :).
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.
Would be better (non-UB, but also more clear to read) to check against std::numeric_limits<int64_t>::max() before the increase.
Done
utACK 1c01ea6f780ba1fff12f03cc14b24f70541e3cc8
Needs rebase.
Rebased.
LGTM, what a foresight Satoshi had to use 64 bit numbers for keypool indexes in walletdb.
re-utACK 44d0996
@TheBlueMatt plz2rebase
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.
Rebased.
re-re-utACK 1fc8c3d (one more and I think git will do it for me)
post merge ACK
Milestone
0.15.0