highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-21 13:10:08 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#25239: wallet: 'CommitTransaction', remove extra wtx lookup and add exception for db write error

Browse Source 
57fb37c275 wallet: CommitTransaction, remove extra wtx lookup and add exception for a possible db write error. (furszy)

Pull request description:

  Two points for `CWallet::CommitTransaction`:

  1) The extra wtx lookup:
      As we are calling to `AddToWallet` first, which returns the recently added/updated wtx pointer, there is no need to look up the wtx again few lines later. We can just use it.

  2) The db write error:
      `AddToWallet` can only return a nullptr if the db write fails, which inside `CommitTransaction` translates to an exception throw cause. We expect everywhere that `CommitTransaction` always succeed.

  ------------------------------------------------

  Extra note:
  This finding generated another working path for me :)
   It starts with the following question: why are we returning a nullptr from `AddToWallet` if the db write failed without removing the recently added transaction from the wallet's map?..
   Can led to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.
  -- I'm writing it here to gather some feedback first and not forget it, will create a follow-up PR in the coming days 🚜 --

ACKs for top commit:
  achow101:
    ACK 57fb37c275
  jonatack:
    ACK 57fb37c
  ryanofsky:
    Code review ACK 57fb37c275. Seems like a clear improvement. Better to fail earlier with a better error message if the failure is going to happen anyway

Tree-SHA512: 80e59c01852cfbbc70a5de1a1c2c59b5e572f9eaa08c2175112cb515256e63fa04c7942f92a513b620d6b06e66392029ebe8902287c456efdbee58a7a5ae42da
This commit is contained in:
Andrew Chow
2022-06-07 14:57:17 -04:00
parent e282764e04 57fb37c275
commit 79cabe3a5b
1 changed files with 7 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
src/wallet/wallet.cpp
View File

@@ -2106,7 +2106,7 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
// Add tx to wallet, because if it has change it's also ours,
// otherwise just for transaction history.
AddToWallet(tx, TxStateInactive{}, [&](CWalletTx& wtx, bool new_tx) {
CWalletTx* wtx = AddToWallet(tx, TxStateInactive{}, [&](CWalletTx& wtx, bool new_tx) {
CHECK_NONFATAL(wtx.mapValue.empty());
CHECK_NONFATAL(wtx.vOrderForm.empty());
wtx.mapValue = std::move(mapValue);
@@ -2116,6 +2116,11 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
return true;
});
// wtx can only be null if the db write failed.
if (!wtx) {
throw std::runtime_error(std::string(__func__) + ": Wallet db error, transaction commit failed");
}
// Notify that old coins are spent
for (const CTxIn& txin : tx->vin) {
CWalletTx &coin = mapWallet.at(txin.prevout.hash);
@@ -2123,17 +2128,13 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
NotifyTransactionChanged(coin.GetHash(), CT_UPDATED);
}
// Get the inserted-CWalletTx from mapWallet so that the
// wtx cached mempool state is updated correctly
CWalletTx& wtx = mapWallet.at(tx->GetHash());
if (!fBroadcastTransactions) {
// Don't submit tx to the mempool
return;
}
std::string err_string;
if (!SubmitTxMemoryPoolAndRelay(wtx, err_string, true)) {
if (!SubmitTxMemoryPoolAndRelay(*wtx, err_string, true)) {
WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string);
// TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure.
}