wallet: Catch ios_base::failure specifically #15710

pull Bushstar wants to merge 2 commits into bitcoin:master from Bushstar:walletdb-readthrow changing 3 files +31 −1

Bushstar commented at 10:56 AM on March 31, 2019: contributor

In #2950 a hash of the pubkey and private was added to speed up key import, this was made backwards compatible by reading the hash in a try block with an ellipses catch all in case the hash was not present.

CDataStream::read() specifically throws std::ios_base::failure, backwards compatibility expects only that error to be thrown, if something else gets thrown we should not be catching it. The change in this commit is to catch that exception only. If any other exception is thrown other than std::ios_base::failure it will be caught by the wider try block and an error written to the log and/or console.

CDataStream::read() throwing std::ios_base::failure. https://github.com/bitcoin/bitcoin/blob/2c364fde423e74b4e03ebcff4582a9db7a6c4e4b/src/streams.h#L191

Wider catch statements that pick up all others exceptions other than ios_base::failure. https://github.com/bitcoin/bitcoin/blob/2c364fde423e74b4e03ebcff4582a9db7a6c4e4b/src/wallet/walletdb.cpp#L425

https://github.com/bitcoin/bitcoin/blob/2c364fde423e74b4e03ebcff4582a9db7a6c4e4b/src/wallet/walletdb.cpp#L430
fanquake added the label Wallet on Mar 31, 2019
Bushstar force-pushed on Mar 31, 2019
MarcoFalke renamed this:
~~Catch ios_base::failure specifically~~
wallet: Catch ios_base::failure specifically
on Mar 31, 2019
promag commented at 1:12 PM on April 2, 2019: member

Concept ACK, begin specific is better IMO.

it will be caught by the wider try block and an error written to the log and/or console.

It would be nice to permalink that too.
practicalswift commented at 1:49 PM on April 2, 2019: contributor
Concept ACK, agree with @promag @Bushstar, if you feel like it you might want to manually investigate the correctness of these:
```
$ git grep 'catch *(\.\.\.)' -- "*.cpp" "*.h"
$ git grep 'catch *(.*std::exception.*)' -- "*.cpp" "*.h"
```
One gotcha to be aware of with the catch (const std::exception& e) cases is that not all exceptions in the project are derived from the std::exception hierarchy.
MarcoFalke commented at 2:18 PM on April 2, 2019: member

This needs a test case that exercises the logic
Bushstar commented at 7:04 AM on April 3, 2019: contributor

Updated the PR to include permalinks to the relevant places. @MarcoFalke I'll add a test to check that the expected exception is thrown, I did have a concern that the exception type might be changed in future so this makes sense. If the type thrown from CDataStream::read() changes a test can flag it.
Bushstar force-pushed on Apr 3, 2019
Bushstar commented at 3:29 PM on April 3, 2019: contributor

Added a test but not sure if the location is correct, test/streams_tests.cpp seemed sensible however this test is related to ReadKeyValue() in wallet/walletdb.cpp so I created walletdb_tests.cpp, however this does not include or use anything from walletdb.h/.cpp. Please advise of best practice for this circumstance and I'll update, squash and take note.
laanwj commented at 6:08 PM on May 16, 2019: member

Concept ACK, catch (...) really shouldn't be used, and doubly so not to ignore errors.
DrahtBot closed this on Mar 9, 2020
DrahtBot commented at 8:34 PM on March 9, 2020: member

The last travis run for this pull request was 341 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot reopened this on Mar 9, 2020
Catch ios_base::failure specifically 32def8d1c2
Tests: Unit test related to WalletDB ReadKeyValue 7486e2771e

in src/wallet/test/walletdb_tests.cpp:5 in cb33603467 outdated

   0 | @@ -0,0 +1,28 @@
   1 | +// Copyright (c) 2012-2019 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include <test/test_bitcoin.h>

MarcoFalke commented at 3:12 AM on March 10, 2020:

This no longer compiles. Maybe this file was moved to test/util/setup_common

Bushstar force-pushed on Mar 10, 2020
Bushstar commented at 12:17 PM on March 10, 2020: contributor

Please give the test a prod.

The job exceeded the maximum time limit for jobs, and has been terminated.
laanwj commented at 11:32 AM on November 19, 2020: member

Code review ACK 7486e2771e7b5d6fa84df6e954be76350c84e220 Thanks for adding a test that checks the behavior.
laanwj merged this on Nov 19, 2020
laanwj closed this on Nov 19, 2020
sidhujag referenced this in commit e0ef59e967 on Nov 19, 2020
PastaPastaPasta referenced this in commit 6fc94f3d08 on Jun 27, 2021
PastaPastaPasta referenced this in commit d75ef3021a on Jun 28, 2021
PastaPastaPasta referenced this in commit bef5666a7e on Jun 29, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)