Replace unreachable error handling with assertions in feebumper #12361

 258 | @@ -265,11 +259,8 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
 259 |      wtxBumped.fTimeReceivedIsTxTime = true;
 260 |      wtxBumped.fFromMe = true;
 261 |      CValidationState state;
 262 | -    if (!wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
 263 | -        // NOTE: CommitTransaction never returns false, so this should never happen.
 264 | -        errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
 265 | -        return Result::WALLET_ERROR;
 266 | -    }
 267 | +    // NOTE: CommitTransaction never returns false.

MarcoFalke commented at 11:09 PM on February 6, 2018:

According to a comment, this is about to change:

// TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure.

promag commented at 9:33 AM on February 8, 2018:

ATM the comment is correct. The assertion below helps detecting if that changes.

MarcoFalke commented at 8:19 PM on February 8, 2018:

Even if we changed it to return false, it will do so very rarely. Likely never in tests. So whenever this changes we introduce a non-deterministic crash when calling an rpc and no one notices until it is out in the wild.

 263 | -        // NOTE: CommitTransaction never returns false, so this should never happen.
 264 | -        errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
 265 | -        return Result::WALLET_ERROR;
 266 | -    }
 267 | +    // NOTE: CommitTransaction never returns false.
 268 | +    assert(wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state));

MarcoFalke commented at 11:09 PM on February 6, 2018:

Could make sense to move it out of the assertion, such that the code continues to run if assertions are not compiled.

promag commented at 9:38 AM on February 8, 2018:

Agree, done.

 258 | @@ -265,11 +259,9 @@ Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransacti
 259 |      wtxBumped.fTimeReceivedIsTxTime = true;
 260 |      wtxBumped.fFromMe = true;
 261 |      CValidationState state;
 262 | -    if (!wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {
 263 | -        // NOTE: CommitTransaction never returns false, so this should never happen.
 264 | -        errors.push_back(strprintf("The transaction was rejected: %s", state.GetRejectReason()));
 265 | -        return Result::WALLET_ERROR;
 266 | -    }
 267 | +    bool result = wallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state));

sdaftuar commented at 7:34 PM on February 8, 2018:

Rather than asserting that the result is true, I think it would make more sense to change the signature of CommitTransaction so that it doesn't return anything.

promag commented at 12:42 AM on February 9, 2018:

See #12361 (review).

  39 | -        // This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)
  40 | -        // implies that we can sign for every input.
  41 | -        return -1;
  42 | -    }
  43 | +    // IsAllFromMe(ISMINE_SPENDABLE) implies that we can sign for every input.
  44 | +    assert(wallet->DummySignTx(txNew, vCoins));

sdaftuar commented at 7:45 PM on February 8, 2018:

While I think the comment here is correct, I don't think that adding an assert is very helpful to detecting if this were to change -- having the software crash on some user in the wild if somehow we violate this assumption in some edge case seems worse than recovering and producing an error message.

promag commented at 12:50 AM on February 9, 2018:

Side note, I should split in two lines as per @MarcoFalke suggestion.

promag commented at 12:53 AM on February 9, 2018:

Maybe we should remove the comment.