[Wallet][Trivial] Fix exception message to reference actual thrower. #8376

pull pstratem wants to merge 1 commits into bitcoin:master from pstratem:2016-07-19-cwallet-sethmasterkey changing 1 files +3 −3

pstratem commented at 12:38 AM on July 20, 2016: contributor

No description provided.
pstratem force-pushed on Jul 20, 2016
pstratem force-pushed on Jul 20, 2016
instagibbs commented at 1:13 AM on July 20, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/8376/commits/22c74d2f7fe6b147c3515ce054e300d765b9edb3
CodeShark commented at 1:17 AM on July 20, 2016: contributor

ACK
pstratem force-pushed on Jul 20, 2016
pstratem renamed this:
~~Fix exception message to reference actual thrower.~~
[Wallet][Trivial] Fix exception message to reference actual thrower.
on Jul 20, 2016
paveljanik commented at 5:54 AM on July 20, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/pull/8376/commits/b2d87e9906d4235b51231ae9e8e73f5e73efd66a
laanwj added the label Wallet on Jul 20, 2016
laanwj commented at 7:51 AM on July 20, 2016: member

Trivial ACK (though we don't have a convention of adding the raising function to exceptions at all so removing the function names would have been OK too)
in src/wallet/wallet.cpp:None in b2d87e9906 outdated
```
3297 | @@ -3298,7 +3298,7 @@ bool CWallet::InitLoadWallet()
3298 |              CKey key;
3299 |              key.MakeNewKey(true);
3300 |              if (!walletInstance->SetHDMasterKey(key))
3301 | -                throw std::runtime_error("CWallet::GenerateNewKey(): Storing master key failed");
3302 | +                throw std::runtime_error("CWallet::InitLoadWallet(): Storing master key failed");
```
MarcoFalke commented at 8:00 AM on July 20, 2016:

This is still dead code. Please revive or delete.

pstratem commented at 8:02 AM on July 20, 2016:

There is no way for SetHDMasterKey to fail currently.

However if that changes in the future it would be best that the failure was handled cleanly.

MarcoFalke commented at 8:06 AM on July 20, 2016:

It is not clear to me when to use return false over throw.

Wouldn't it make sense to modify SetHDMasterKey() to return false?

pstratem commented at 8:14 AM on July 20, 2016:

Maybe but it's something that can be dealt with later as part of a broader refactor which I am working on. #8152

jonasschnelli commented at 8:39 AM on July 20, 2016:

It's kinda dead code now, but we should keep this check for further changes in SetHDMasterKey().

MarcoFalke commented at 8:42 AM on July 20, 2016:

Fine with me. I think it would help if someone added a short sentence to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md about returning a bool vs throwing an exception.
MarcoFalke commented at 8:06 AM on July 20, 2016: member

utACK b2d87e9
MarcoFalke added the label Needs backport on Jul 20, 2016
MarcoFalke added the label Docs and Output on Jul 20, 2016
jonasschnelli commented at 8:39 AM on July 20, 2016: contributor

Thanks Patrick for the cleanup. utACK https://github.com/bitcoin/bitcoin/pull/8376/commits/22c74d2f7fe6b147c3515ce054e300d765b9edb3
MarcoFalke commented at 8:43 AM on July 20, 2016: member

Looks good. I have tagged with backport, as it is only changing strings and we will have a rc2 anyway.
MarcoFalke commented at 10:12 AM on July 20, 2016: member

@pstratem Needs rebase.
Fix exception messages in CWallet to reference actual thrower. 4522ea05c1
pstratem force-pushed on Jul 20, 2016
pstratem commented at 9:58 PM on July 20, 2016: contributor

@MarcoFalke rebased
MarcoFalke commented at 8:25 AM on July 21, 2016: member

utACK 4522ea05c1d7aaa2502ffb9415dd3e79db393266
MarcoFalke commented at 8:26 AM on July 21, 2016: member

though we don't have a convention of adding the raising function to exceptions at all

I think the reason is to aid debugging?
laanwj commented at 3:37 PM on July 21, 2016: member

I think the reason is to aid debugging?

In the current code, for debug log messages the __func__ is often added, as those messages are formatted anyway. However exceptions are usually just plain strings.

One could use strprintf(). Hardcoding the function name is ill-advised because people always forget to change it when they move the code.

I don't think it's extremely useful - usually a certain warning/error only happens in one place in the program, and it's easy enough to find where.

in src/wallet/wallet.cpp:None in 4522ea05c1

1177 | @@ -1178,7 +1178,7 @@ bool CWallet::SetHDMasterKey(const CKey& key)
1178 |      // key metadata is not required
1179 |      CPubKey pubkey = key.GetPubKey();
1180 |      if (!AddKeyPubKey(key, pubkey))
1181 | -        throw std::runtime_error("CWallet::GenerateNewKey(): AddKey failed");
1182 | +        throw std::runtime_error("CWallet::SetHDMasterKey(): AddKey failed");

laanwj commented at 3:38 PM on July 21, 2016:

Nit: if you do want to keep the function name here, please use strprintf("%s: AddKey failed", __func__) instead of hardcoding it.

MarcoFalke commented at 2:24 PM on July 31, 2016:

Makes sense, so it can be moved or renamed easier.

MarcoFalke commented at 2:16 PM on July 31, 2016: member

Needs rebase after #8389 (review)
MarcoFalke removed the label Needs backport on Aug 3, 2016
MarcoFalke commented at 4:57 PM on August 18, 2016: member

@pstratem Needs rebase, if still relevant
laanwj closed this on Aug 22, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8548 [wallet] Use __func__ to get function name for output printing