wallet: 'CommitTransaction', remove extra wtx lookup and add exception for db write error #25239
Conversation
…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.
|
ACK 57fb37c |
|
|
||
| // 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"); |
There was a problem hiding this comment.
Might be simpler and easier to maintain if this is raised directly at the source?
@@ -1001,8 +1001,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
// Write to disk
if (fInsertedNew || fUpdated)
- if (!batch.WriteTx(wtx))
- return nullptr;
+ if (!batch.WriteTx(wtx)) throw std::runtime_error(std::string(__func__) + ": Wallet db error, transaction commit failed");
@@ -2116,11 +2115,6 @@ 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");
- }
-Edit: Would it be helpful to indicate with which tx the db write failed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, SGTM and +1 on adding doxygen documentation to AddToWallet() as you propose in https://github.com/bitcoin/bitcoin/pull/25272/files#diff-9ce137cd784ea308778842120aa2af6d2bb8369485b71f25e72b2a32cf0a5b21R508.
…okup and add exception for db write error 57fb37c 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 57fb37c jonatack: ACK 57fb37c ryanofsky: Code review ACK 57fb37c. 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
…ring chain sync 9e04cfa test: add coverage for wallet inconsistent state during sync (furszy) 77de5c6 wallet: guard and alert about a wallet invalid state during chain sync (furszy) Pull request description: Follow-up work to my comment in #25239. Guarding and alerting the user about a wallet invalid state during chain synchronization. #### Explanation if the `AddToWallet` tx write fails, the method returns a wtx `nullptr` without removing the recently added transaction from the wallet's map. Which makes that `AddToWalletIfInvolvingMe` return false (even when the tx is on the wallet's map already), --> which makes `SyncTransaction` skip the `MarkInputsDirty` call --> which leads 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. Plus, as we only store the arriving transaction inside `AddToWalletIfInvolvingMe` when we synchronize/scan block/s from the chain and nowhere else, it makes sense to treat the transaction db write error as a runtime error to notify the user about the problem. Otherwise, the user will lose all the not stored transactions after a wallet shutdown (without be able to recover them automatically on the next startup because the chain sync would be above the block where the txs arrived). Note: On purpose, the first commit adds test coverage for it. Showing how the wallet can end up in an invalid state. The second commit corrects it with the proposed solution. ACKs for top commit: achow101: re-ACK 9e04cfa jonatack: ACK 9e04cfa Tree-SHA512: 81f765eca40547d7764833d8ccfae686b67c7728c84271bc00dc51272de643dafc270014079dcc9727b47577ba67b340aeb5f981588b54e69a06abea6958aa96
5 participants
Two points for
CWallet::CommitTransaction:The extra wtx lookup:
As we are calling to
AddToWalletfirst, 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.The db write error:
AddToWalletcan only return a nullptr if the db write fails, which insideCommitTransactiontranslates to an exception throw cause. We expect everywhere thatCommitTransactionalways succeed.Extra note:
This finding generated another working path for me :)
It starts with the following question: why are we returning a nullptr from
AddToWalletif 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 🚜 --