Return false on corrupt tx rather than asserting #23142
Conversation
Co-authored-by: Russell Yanofsky <[email protected]> Co-authored-by: Andrew Chow <[email protected]>
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
Concept/light code review ACK. A test might be good.
| // callback fills with transaction metadata. | ||
| auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) { | ||
| assert(new_tx); | ||
| if(!new_tx) { |
There was a problem hiding this comment.
| if(!new_tx) { | |
| if (!new_tx) { |
| // There's some corruption here since the tx we just tried to load was already in the wallet. | ||
| // We don't consider this type of corruption critical, and can fix it by removing tx data and | ||
| // rescanning. |
There was a problem hiding this comment.
Probably I am missing context here, so feel free to ignore my question.
Why would we consider "the tx we just tried to load was already in the wallet." as a corrupted transaction? If I am missing context here, would it be helpful to clarify this in the above comment?
There was a problem hiding this comment.
Why would we consider "the tx we just tried to load was already in the wallet." as a corrupted transaction? If I am missing context here, would it be helpful to clarify this in the above comment?
It might not be bad to update the comment above or the error message below or the tx_corrupt variable name if you have suggestions, but this all seems ok to me. If there are multiple transactions with the same hash, the wallet is corrupt, and the transactions are maybe corrupt. In this case it is probably better to not load the wallet than to load it and make new writes, and to return a loading error instead of asserting false.
There was a problem hiding this comment.
Hmm. The context I was missing then would be: !new suggests that the wallet already loaded the transaction with the given hash.
So maybe the comment above could read something like "There's some corruption here since the wallet already knows about a transaction with the same Txid"? That would clarify why this case is an error a little clearly.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 0ab4c3b. There may be room for more improvements later like better error messages or easier recovery options, but changing from an assert to an error seems like a clear improvement, and this seems to avoid all the pitfalls of the last PR that tried this.
| // There's some corruption here since the tx we just tried to load was already in the wallet. | ||
| // We don't consider this type of corruption critical, and can fix it by removing tx data and | ||
| // rescanning. |
There was a problem hiding this comment.
Why would we consider "the tx we just tried to load was already in the wallet." as a corrupted transaction? If I am missing context here, would it be helpful to clarify this in the above comment?
It might not be bad to update the comment above or the error message below or the tx_corrupt variable name if you have suggestions, but this all seems ok to me. If there are multiple transactions with the same hash, the wallet is corrupt, and the transactions are maybe corrupt. In this case it is probably better to not load the wallet than to load it and make new writes, and to return a loading error instead of asserting false.
|
ACK 0ab4c3b |
|
Very much agree with this change, assert is not an error-handling mechanism. Code review ACK 0ab4c3b |
|
Would be good to have a test for this, by the way. Oh I see jonatack brought this up too:
Do we need a "needs test" label 😄 |
0ab4c3b Return false on corrupt tx rather than asserting (Samuel Dobson) Pull request description: Takes up bitcoin#19793 Rather than asserting, we log an error and return CORRUPT so that the user is informed. This type of error isn't critical so it isn't worth `assert`ing. ACKs for top commit: achow101: ACK 0ab4c3b laanwj: Code review ACK 0ab4c3b ryanofsky: Code review ACK 0ab4c3b. There may be room for more improvements later like better error messages or easier recovery options, but changing from an assert to an error seems like a clear improvement, and this seems to avoid all the pitfalls of the last PR that tried this. Tree-SHA512: 4a1a412e7c473d176c4e09123b85f390a6b0ea195e78d28ebd50b13814b7852f8225a172511a2efb6affb555b11bd4e667c19eb8c78b060c5444b62f0fae5f7a
7 participants
Takes up #19793
Rather than asserting, we log an error and return CORRUPT so that the user is informed. This type of error isn't critical so it isn't worth
asserting.