ReserveKeyFromKeyPool's previous behaviour is to set nIndex to -1 if the keypool is empty, OR throw an exception for technical failures. Instead, we now return nIndex (always >= 0) and throw a keypool_empty exception if the keypool is empty.
This is to ensure calling code must handle the empty case, and so the compiler can provide use-without-assignment warnings when appropriate.
ReserveKeyFromKeyPool's previous behaviour is to set nIndex to -1 if the keypool is empty, OR throw an exception for technical failures.
Instead, we now return nIndex (always >= 0) and throw a keypool_empty exception if the keypool is empty.
This is to ensure calling code must handle the empty case, and so the compiler can provide use-without-assignment warnings when appropriate.
utACK
Needs rebase
2963 | @@ -2963,13 +2964,13 @@ void CWallet::ReturnKey(int64_t nIndex) 2964 | 2965 | bool CWallet::GetKeyFromPool(CPubKey& result) 2966 | { 2967 | - int64_t nIndex = 0; 2968 | + int64_t nIndex;
Would be nice if this were declared after the lock, closer to where it's used.
utACK ba98de0b129bf510a52e3a65bece11ff492ec8d7. Reasonable change, needs rebase.
Gaa, can we not add more C++ exceptions? They are generally very strongly discouraged across the board by sandard C++ guidelines, for various reasons. As an alternative which accomplishes most of the same goal, maybe still make ReserveKeyFromKeyPool return the nIndex and add the stanard "not using returned values" checks that most compilers support?
Needs rebase and maybe discussion about @TheBlueMatt's points about the C++ exceptions (or close).
Needs rebase. This seems like a reasonable use of exceptions, so I'd stand by my previous ACK, but no strong opinion one way or the other.
Concept ACK in the sense that using -1 as exceptional value may be improvable. I'm not a fan of using exceptions though; what about boost::optional?
I'll take this.
