Changes to handle invalid coin(s) in privacy-wallet-service. #3002
Conversation
snehal-das
changed the title
|
Changes to the privacy-wallet-service is complete and tested to work as expected. Changing status from draft to ready for merge. |
| @@ -275,14 +275,16 @@ ::grpc::Status PrivacyWalletServiceImpl::handleUserClaimCoinsRequest(::grpc::Ser | |||
| response->set_err(err_msg); | |||
| return grpc::Status(grpc::StatusCode::ABORTED, err_msg); | |||
| } | |||
| } catch (const libutt::api::operations::InvalidCoinsInTransfer& e) { | |||
| std::cout << e.what() << std::endl; | |||
| response->mutable_claim_coins_response()->set_warning(e.what()); | |||
There was a problem hiding this comment.
In this case, shouldn't we return false in the succ?
Also, I would suggest returning the invalid nullifiers and the reply.
Finally, we need to expose a way for the user to remove these invalid coins, otherwise it won't be able to proceed (the invalid coins will be chose over and over again)
There was a problem hiding this comment.
In this case, shouldn't we return false in the succ?
Technically, the recipent was able to claim coins received. Reporting back false after claiming all valid coins seems incorrect to me. The more correct approach seems to be to return true with a warning.
Also, I would suggest returning the invalid nullifiers and the reply.
I didn't want the dapp to handle the invalid coins, but this is something that can be implemented. I'll discuss with you in more detail.
| @@ -447,9 +447,13 @@ void User::updateTransferTx(const Transaction& tx, const TxOutputSigs& sigs) { | |||
|
|
|||
| // Claim coins | |||
| auto claimedCoins = pImpl_->client_->claimCoins(uttTx, pImpl_->params_, sigs); | |||
|
|
|||
| bool invalidCoinsInTransfer(false); | |||
There was a problem hiding this comment.
use the regular convention here
either invalidCoinsInTransfer = false or invalidCoinsInTransfer{false}
There was a problem hiding this comment.
I'll update in the next commit.
| for (auto& coin : claimedCoins) { | ||
| if (!pImpl_->client_->validate(coin)) throw std::runtime_error("Invalid normal coin in transfer!"); | ||
| if (!pImpl_->client_->validate(coin)) { | ||
| logdbg_user << "Invalid coin found; coin details: " << dbgPrintCoins({coin}) << endl; |
There was a problem hiding this comment.
why not throw the exception here?
(and then you don't need invalidCoinsInTransfer)
There was a problem hiding this comment.
The idea is to prevent prematurely exiting from the function. The recipient should be able to claim coins received after the invalid coin, so I am letting it process all coins before raising the exception.
|
I am not able to edit the reviewers list, the only option I see is to mention the person here. @bsenthilr, @yontyon Please take a look and add your thoughts. Some items pending are:
|
Problem Overview
Current privacy library does not handle invalid coins gracefully. The logic of claiming coins is modified to handle this scenario.
Testing Done
Testcase: Create a scenario where an invalid coin is transferred to a user. Observe how the invalid coin is handled at the recipient's end.
Observation: The invalid coin is no longer reported to the DApp, the user does not notice any issue and future transactions are synced successfully.