Wallet should not override signing errors #19568

fjahr · 2020-07-22T13:55:04Z

While reviewing #17204 I noticed that the errors in input_errors from ::SignTransaction where being overridden by CWallet::SignTransaction. For example, a Script related error led to incomplete signature data which led to CWallet::SignTransaction reporting that keys were missing, which was a less precise error than the original one.

Additionally, the error "Input not found or already spent" is duplicated in sign.cpp, so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed.

On testing: even though the errors in CWallet are covered, all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in test/functional/rpc_signrawtransaction.py seem to aim for a more generic approach.

achow101 · 2020-07-22T16:50:18Z

ACK cf9245b97a8793510eb03944c35309b6cf4f4c2d

laanwj · 2020-07-22T20:33:00Z

This makes sense, we don't want earlier error messages to get lost and replaced with less specific errors.
code review ACK cf9245b97a8793510eb03944c35309b6cf4f4c2d

It is slightly worrying that this does not affect the tests, though. It doesn't necessarily have to be in this PR, but can we test signing failure somehow?

fjahr · 2020-07-22T20:38:26Z

It is slightly worrying that this does not affect the tests, though.

Yepp, I am considering changing that in a follow-up. If I am too busy I will open an issue instead.

promag

Code review ACK cf9245b97a8793510eb03944c35309b6cf4f4c2d.

Agree that a test would be nice for this change.

promag · 2020-07-23T21:24:13Z

src/wallet/wallet.cpp

Looks like this block is unnecessary? The error Input not found or already spent is already set in ::SignTransaction.

And not sure why not set Unable to sign input, missing keys in ::SignTransaction.

DrahtBot · 2020-07-24T03:37:23Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

promag · 2020-07-24T20:51:48Z

Please see #19578 before merging this.

fjahr · 2020-07-24T20:54:55Z

Please see #19578 before merging this.

I had noticed that as well and was looking for feedback from reviewers in my description if I should do this instead. See:

Additionally, the error "Input not found or already spent" is duplicated in sign.cpp, so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here.

promag · 2020-07-24T21:08:41Z

@fjahr indeed, I've skipped that (I shouldn't). Not sure if these duplicate tests are leftovers from the multiple SPK refactor. @achow101?

achow101 · 2020-07-24T21:30:01Z

CWallet::SignTransaction andsign.cpp's SignTransaction should be safe to modify. There aren't any ongoing changes to those

fjahr · 2020-07-24T21:53:02Z

CWallet::SignTransaction andsign.cpp's SignTransaction should be safe to modify. There aren't any ongoing changes to those

I take that as the error block can be removed. It seems the SPK SignTransaction was first called after the error block so the errors returned early and then that was switched around so now it does not make sense to keep them. I will update my code here if that's ok @promag .

fanquake · 2020-07-28T07:27:49Z

@fjahr @promag Can you coordinate to pull the other change from #19578 into here if required.

fjahr · 2020-07-28T15:47:52Z

@fjahr @promag Can you coordinate to pull the other change from #19578 into here if required.

Assuming you mean the change in script/sign.cpp: I think the error, as it is set in #19578, would always either be overridden by the error returned from VerifyScript or nullified if VerifyScript succeeds. That could be fixed by emplacing these errors instead, like so: fjahr@93199c4. But I think these errors can be more helpful to users in certain cases like a failed multisig. So I would opt to leave it out.

achow101 · 2020-08-13T16:06:51Z

ACK e7448d6

meshcollider

Code review ACK e7448d6

@promag mind re-reviewing before merge?

e7448d6 wallet: Don't override signing errors (Fabian Jahr) Pull request description: While reviewing bitcoin#17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one. Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](https://github.com/bitcoin/bitcoin/blob/c7b4968552c704f1e2e9a046911e1207f5af5fe0/src/script/sign.cpp#L481), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed. On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach. ACKs for top commit: achow101: ACK e7448d6 meshcollider: Code review ACK e7448d6 Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23

Summary: > While reviewing #17204 I noticed that the errors in input_errors from ::SignTransaction where being overridden by CWallet::SignTransaction. For example, a Script related error led to incomplete signature data which led to CWallet::SignTransaction reporting that keys were missing, which was a less precise error than the original one. Note: in our codebase the errors are overwritten one more time, as a result of a change in D8100. The comment mentions "get proper error messages". None of these error messages seem to be tested at the moment, because removing this block does not cause any test to fail. This is a backport of [[bitcoin/bitcoin#19568 | core#19568]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10085

fanquake added the Wallet label Jul 22, 2020

promag reviewed Jul 23, 2020

View reviewed changes

promag mentioned this pull request Jul 23, 2020

wallet: Avoid duplicate checks on signing failure #19578

Closed

fjahr force-pushed the err_override branch from cf9245b to 2ef360b Compare July 24, 2020 21:58

wallet: Don't override signing errors

e7448d6

fjahr force-pushed the err_override branch from 2ef360b to e7448d6 Compare July 24, 2020 22:00

meshcollider reviewed Aug 13, 2020

View reviewed changes

fanquake merged commit c0b1706 into bitcoin:master Aug 14, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Wallet should not override signing errors #19568

Wallet should not override signing errors #19568

Uh oh!

Conversation

fjahr commented Jul 22, 2020

Uh oh!

achow101 commented Jul 22, 2020

Uh oh!

laanwj commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

promag Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Jul 24, 2020

Uh oh!

fjahr commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jul 24, 2020

Uh oh!

achow101 commented Jul 24, 2020

Uh oh!

fjahr commented Jul 24, 2020

Uh oh!

fanquake commented Jul 28, 2020

Uh oh!

fjahr commented Jul 28, 2020

Uh oh!

achow101 commented Aug 13, 2020

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj commented Jul 22, 2020 •

edited

Loading

fjahr commented Jul 22, 2020 •

edited

Loading

DrahtBot commented Jul 24, 2020 •

edited

Loading

fjahr commented Jul 24, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading