refactor: Make coinsel an enum class

This makes the new transaction creation parameter `coinsel` a documented
enum class.

Also fixes an issue where it was allowed to pass invalid coinsel value
to CreateTransaction.

src/wallet/coinselection.h

@@ -14,6 +14,26 @@ static constexpr Amount MIN_CHANGE{COIN / 100};
 //! final minimum change amount after paying for fees
 static constexpr Amount MIN_FINAL_CHANGE = MIN_CHANGE / 2;
 
+//! Hint on what attributes to prioritize when performing coin selection.
+enum class CoinSelectionHint {
+    //! The default case all-around coin selection algorithm.
+    Default = 0,
+
+    //! Over all other features, prioritize speed.
+    Fast = 1,
+
+    //! Value reserved for future algorithm that prioritizes speed.
+    FastReserved = 2,
+
+    //! Largest value in this enum represents invalid.
+    Invalid = 3
+};
+
+inline bool IsValidCoinSelectionHint(int c) {
+    return c >= static_cast<int>(CoinSelectionHint::Default)
+        && c < static_cast<int>(CoinSelectionHint::Invalid);
+}
+
 class CInputCoin {
 public:
     CInputCoin(const CTransactionRef &tx, unsigned int i) {

src/wallet/rpcwallet.cpp

@@ -350,7 +350,7 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock &locked_chain,
                                  const CTxDestination &address, Amount nValue,
                                  bool fSubtractFeeFromAmount,
                                  const CCoinControl &coinControl,
-                                 mapValue_t mapValue, int coinsel) {
+                                 mapValue_t mapValue, CoinSelectionHint coinsel) {
     if (pwallet->GetBroadcastTransactions() && !g_connman) {
         throw JSONRPCError(
             RPC_CLIENT_P2P_DISABLED,
@@ -511,14 +511,13 @@ static UniValue sendtoaddress(const Config &config,
 
     CCoinControl coinControl;
 
-    int coinsel = 0;
+    CoinSelectionHint coinsel(CoinSelectionHint::Default);
     if (!request.params[5].isNull()) {
-        // RPC API is an int to allow for multiple speed settings in the future
-        coinsel = (request.params[5].get_int());
-    }
-    // coinsel==2 will be another fast algorithm, for which we maintain forwards compatibility
-    if (coinsel < 0 || coinsel > 2) {
-        throw JSONRPCError(RPC_TYPE_ERROR, "Unsupported coin selection algorithm");
+        int c = (request.params[5].get_int());
+        if (!IsValidCoinSelectionHint(c)) {
+            throw JSONRPCError(RPC_TYPE_ERROR, "Unsupported coin selection algorithm");
+        }
+        coinsel = static_cast<CoinSelectionHint>(c);
     }
 
     EnsureWalletIsUnlocked(pwallet);

src/wallet/test/wallet_tests.cpp

@@ -385,7 +385,8 @@ class ListCoinsTestingSetup : public TestChain100Setup {
 
     ~ListCoinsTestingSetup() { wallet.reset(); }
 
-    CWalletTx &AddTx(CRecipient recipient, int coinsel=0) {
+    CWalletTx &AddTx(CRecipient recipient,
+                     CoinSelectionHint coinsel = CoinSelectionHint::Default) {
         CTransactionRef tx;
         CReserveKey reservekey(wallet.get());
         Amount fee;
@@ -492,7 +493,7 @@ BOOST_FIXTURE_TEST_CASE(FastTransaction, ListCoinsTestingSetup) {
     std::string coinbaseAddress = coinbaseKey.GetPubKey().GetID().ToString();
 
     for (uint8_t i=0; i<2; i++) {
-        int coinsel = i;
+        CoinSelectionHint coinsel(static_cast<CoinSelectionHint>(i));
 
         BOOST_CHECK(wallet->GetBalance() == 50 * COIN);
 
@@ -509,6 +510,22 @@ BOOST_FIXTURE_TEST_CASE(FastTransaction, ListCoinsTestingSetup) {
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(wallet_error_on_invalid_coinselection_hint, ListCoinsTestingSetup) {
+    CTransactionRef tx;
+    CReserveKey reservekey(wallet.get());
+    Amount fee;
+    int changePos = -1;
+    std::string error;
+    CCoinControl dummy;
+    CRecipient recipient{GetScriptForRawPubKey({}), 1 * COIN, true};
+
+    BOOST_CHECK_EQUAL(CreateTransactionResult::CT_INVALID_PARAMETER,
+            wallet->CreateTransaction(
+                *m_locked_chain, {recipient}, tx, reservekey, fee,
+                changePos, error, dummy, true,
+                CoinSelectionHint::Invalid));
+}
+
 BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) {
     auto chain = interfaces::MakeChain();
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(

src/wallet/wallet.cpp

@@ -3035,7 +3035,7 @@ CreateTransactionResult CWallet::CreateTransaction(
     const std::vector<CRecipient> &vecSend, CTransactionRef &tx,
     CReserveKey &reservekey, Amount &nFeeRet, int &nChangePosInOut,
     std::string &strFailReason, const CCoinControl &coinControl, bool sign,
-    int coinsel) {
+    CoinSelectionHint coinsel) {
     Amount nValue = Amount::zero();
     int nChangePosRequest = nChangePosInOut;
     unsigned int nSubtractFeeFromAmount = 0;
@@ -3067,17 +3067,20 @@ CreateTransactionResult CWallet::CreateTransaction(
         LOCK(cs_wallet);
 
         std::vector<COutput> vAvailableCoins;
-        // coinsel == 2 is planned to be a future fast algorithm, so we will
-        // make 2 as fast as we can here to ease the upgrade transition
-        if (coinsel == 1 || coinsel == 2) {
+        // 'FastReserced' is planned to be a future fast algorithm, so we will
+        // make it as fast as we can here to ease the upgrade transition
+        if (coinsel == CoinSelectionHint::Fast || coinsel == CoinSelectionHint::FastReserved) {
             AvailableCoins(*locked_chain, vAvailableCoins, true, &coinControl,
                 SATOSHI,   // nMinimumAmount
                 MAX_MONEY, // nMaximumAmount
                 10 * nValue, // nMinimumSumAmount
                 0, 0, 9999999,
                 GetMinimumFeeRate(*this, coinControl, g_mempool));
-        } else {
+        } else if (coinsel == CoinSelectionHint::Default) {
             AvailableCoins(*locked_chain, vAvailableCoins, true, &coinControl);
+        } else {
+            strFailReason = _("Invalid coin selection hint");
+            return CreateTransactionResult::CT_INVALID_PARAMETER;
         }
 
         // Parameters for coin selection, init with dummy

src/wallet/wallet.h

@@ -1140,7 +1140,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface {
                       Amount &nFeeRet, int &nChangePosInOut,
                       std::string &strFailReason,
                       const CCoinControl &coin_control, bool sign = true,
-                      int coinsel = 0);
+                      CoinSelectionHint coinsel = CoinSelectionHint::Default);
     bool CommitTransaction(
         CTransactionRef tx, mapValue_t mapValue,
         std::vector<std::pair<std::string, std::string>> orderForm,