wallet: ‘CommitTransaction’, remove extra wtx lookup and add exception for db write error #25239

1. 
   furszy commented at 6:03 pm on May 29, 2022: member

   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 🚜 –

2. wallet: CommitTransaction, remove extra wtx lookup and add exception for a possible db write error.

   1) `Wallet::AddToWallet` is already returning the pointer to the inserted `CWalletTx`, so there is no need to look it up in the map again.
   
   2) `Wallet::AddToWallet` can only return a nullptr if the db `writeTx` call failed. Which should be treated as an error.

   57fb37c275
3. fanquake added the label Wallet on May 29, 2022
4. 
   achow101 commented at 3:30 pm on June 2, 2022: member

   ACK 57fb37c27599fc865f20b42a27bb9c227f384de3
5. in src/wallet/wallet.cpp:2121 in 57fb37c275

   2115@@ -2116,24 +2116,25 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
   2116         return true;
   2117     });
   2118 
   2119+    // wtx can only be null if the db write failed.
   2120+    if (!wtx) {
   2121+        throw std::runtime_error(std::string(__func__) + ": Wallet db error, transaction commit failed");
   

   ---

   ---

   

   jonatack commented at 9:55 am on June 6, 2022:

   Might be simpler and easier to maintain if this is raised directly at the source?

    0@@ -1001,8 +1001,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
    1     // Write to disk
    2     if (fInsertedNew || fUpdated)
    3-        if (!batch.WriteTx(wtx))
    4-            return nullptr;
    5+        if (!batch.WriteTx(wtx)) throw std::runtime_error(std::string(__func__) + ": Wallet db error, transaction commit failed");
    6 
    7@@ -2116,11 +2115,6 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
    8         return true;
    9     });
   10 
   11-    // wtx can only be null if the db write failed.
   12-    if (!wtx) {
   13-        throw std::runtime_error(std::string(__func__) + ": Wallet db error, transaction commit failed");
   14-    }
   15-
   

   Edit: Would it be helpful to indicate with which tx the db write failed?

   ---

   

   furszy commented at 1:08 pm on June 6, 2022:

   Might be simpler and easier to maintain if this is raised directly at the source?

   At least for now (with the current code architecture), as AddToWallet is called from CommitTransaction, importPrunedFunds and AddToWalletIfInvolvingMe (and this last one is used in flows like chain sync and chain rescan), would keep it as is so we are able to throw different error descriptions on the method’s caller side. Users and debugging wise, isn’t the same to throw an error for a failure in the transaction commit flow than a failure storing a transaction when the node is syncing or rescanning the chain. They should be presented differently to the user and logs.

   Nevertheless, I agree that it’s a good point to take into account moving forward 👍🏼. We could improve the error handling returning a db failure status instead of the obscure nullptr.. (which originated other problematic -> #25272).

   Edit: Would it be helpful to indicate with which tx the db write failed?

   I don’t think that is needed on this specific process. CommitTransaction is triggered directly from a user action and the user does not know which is the id of the tx that the wallet is creating for him/her (he/she just pressed a button in the GUI or called to RPC commands like sendtoaddress, send, sendall, etc). In other words, as a user, if I press the send button on the GUI (or call sendtoaddress), and the process fails, the tx id on the error message isn’t helpful for me. It would actually be confusing.

   ---

   

   jonatack commented at 3:09 pm on June 6, 2022:

   Thanks, SGTM and +1 on adding doxygen documentation to AddToWallet() as you propose in https://github.com/bitcoin/bitcoin/pull/25272/files#diff-9ce137cd784ea308778842120aa2af6d2bb8369485b71f25e72b2a32cf0a5b21R508.
6. 
   jonatack commented at 10:03 am on June 6, 2022: contributor

   ACK 57fb37c

   One question regarding the exception.

7. ryanofsky approved
8. 
   ryanofsky commented at 4:14 pm on June 6, 2022: contributor

   Code review ACK 57fb37c27599fc865f20b42a27bb9c227f384de3. Seems like a clear improvement. Better to fail earlier with a better error message if the failure is going to happen anyway
9. maflcko assigned achow101 on Jun 7, 2022
10. achow101 merged this on Jun 7, 2022
11. achow101 closed this on Jun 7, 2022

    ---

12. sidhujag referenced this in commit fd0c3ce69c on Jun 8, 2022
13. achow101 referenced this in commit de3c46c938 on Aug 2, 2022
14. furszy deleted the branch on May 27, 2023
15. bitcoin locked this on May 26, 2024

Contributors
furszy achow101 jonatack ryanofsky

Labels
Wallet