Skip to content

PrivateSend: dont waste keys from keypool on failure in CreateDenominated #1473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
UdjinM6 merged 4 commits into from
May 28, 2017
Merged

PrivateSend: dont waste keys from keypool on failure in CreateDenominated #1473

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ffa41a2
dont waste keys from keypool on failure in CreateDenominated
May 21, 2017
4b2ebd4
bug fix - log actual number of total outputs, comment error
May 21, 2017
a74cd50
log number of total outputs as separate value
May 22, 2017
f09c0e8
add lock so no one can spend outputs used for denominations
May 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/privatesend-client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "txmempool.h"
#include "util.h"
#include "utilmoneystr.h"
#include <memory>

CPrivateSendClient privateSendClient;

Expand Down Expand Up @@ -1198,6 +1199,8 @@ bool CPrivateSendClient::MakeCollateralAmounts(const CompactTallyItem& tallyItem
// Create denominations by looping through inputs grouped by addresses
bool CPrivateSendClient::CreateDenominated()
{
LOCK2(cs_main, pwalletMain->cs_wallet);

std::vector<CompactTallyItem> vecTally;
if(!pwalletMain->SelectCoinsGrouppedByAddresses(vecTally)) {
LogPrint("privatesend", "CPrivateSendClient::CreateDenominated -- SelectCoinsGrouppedByAddresses can't find any inputs!\n");
Expand Down Expand Up @@ -1241,7 +1244,7 @@ bool CPrivateSendClient::CreateDenominated(const CompactTallyItem& tallyItem, bo
// ****** Add denoms ************ /

// make our denom addresses
CReserveKey reservekeyDenom(pwalletMain);
std::vector<std::shared_ptr<CReserveKey>> reservekeyDenomVec;

// try few times - skipping smallest denoms first if there are too much already, if failed - use them
int nOutputsTotal = 0;
Expand All @@ -1268,22 +1271,23 @@ bool CPrivateSendClient::CreateDenominated(const CompactTallyItem& tallyItem, bo

int nOutputs = 0;

// add each output up to 10 times until it can't be added again
// add each output up to 11 times until it can't be added again
Copy link
Copy Markdown

@UdjinM6 UdjinM6 May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! I'm pretty sure nOutputs were meant to be limited by 10 originally so you can fix either part, but 11 should work just as well as 10 would :)

Copy link
Copy Markdown
Author

@krychlicki krychlicki May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok! let's leave it as 11 then!

while(nValueLeft - nDenomValue >= 0 && nOutputs <= 10) {
CScript scriptDenom;
CPubKey vchPubKey;
//use a unique change address
assert(reservekeyDenom.GetReservedKey(vchPubKey)); // should never fail, as we just unlocked
std::shared_ptr<CReserveKey> reservekeyDenom = std::make_shared<CReserveKey>(pwalletMain);
reservekeyDenomVec.push_back(reservekeyDenom);

assert(reservekeyDenom->GetReservedKey(vchPubKey)); // should never fail, as we just unlocked
scriptDenom = GetScriptForDestination(vchPubKey.GetID());
// TODO: do not keep reservekeyDenom here
reservekeyDenom.KeepKey();

vecSend.push_back((CRecipient){ scriptDenom, nDenomValue, false });

//increment outputs and subtract denomination amount
nOutputs++;
nValueLeft -= nDenomValue;
LogPrintf("CreateDenominated1: nOutputsTotal: %d, nOutputs: %d, nValueLeft: %f\n", nOutputsTotal, nOutputs, (float)nValueLeft/COIN);
LogPrintf("CreateDenominated1: totalOutputs: %d, nOutputsTotal: %d, nOutputs: %d, nValueLeft: %f\n", nOutputsTotal + nOutputs, nOutputsTotal, nOutputs, (float)nValueLeft/COIN);
}

nOutputsTotal += nOutputs;
Expand Down Expand Up @@ -1316,13 +1320,17 @@ bool CPrivateSendClient::CreateDenominated(const CompactTallyItem& tallyItem, bo
nFeeRet, nChangePosRet, strFail, &coinControl, true, ONLY_NONDENOMINATED_NOT1000IFMN);
if(!fSuccess) {
LogPrintf("CPrivateSendClient::CreateDenominated -- Error: %s\n", strFail);
// TODO: return reservekeyDenom here
for(auto key : reservekeyDenomVec)
key->ReturnKey();
reservekeyCollateral.ReturnKey();
LogPrintf("CPrivateSendClient::CreateDenominated -- %d keys returned\n", reservekeyDenomVec.size() + 1);
return false;
}

// TODO: keep reservekeyDenom here
for(auto key : reservekeyDenomVec)
key->KeepKey();
reservekeyCollateral.KeepKey();
LogPrintf("CPrivateSendClient::CreateDenominated -- %d keys keeped\n", reservekeyDenomVec.size() + 1);

if(!pwalletMain->CommitTransaction(wtx, reservekeyChange)) {
LogPrintf("CPrivateSendClient::CreateDenominated -- CommitTransaction failed!\n");
Copy link
Copy Markdown

@tgflynn tgflynn May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if failure occurs here (ie. in CommitTransaction) ?

Copy link
Copy Markdown
Author

@krychlicki krychlicki May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommitTransaction can only be unsuccessful when transaction fails validation on adding to memory pull.
As we constructed this transaction by ourselves it can only fail when outputs that we want to anonymize are already spent.
If this happens our wallet is already in invalid state (must be repaired?) so losing couple of keys will have probably no meaning.

Besides that, in other places in the source code keys are released (keeped) after CreateTransaction not CommitTransaction, so to be consistent I think is better to keep it as it is right now.

Copy link
Copy Markdown

@UdjinM6 UdjinM6 May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing that was failing here was always CreateTransaction in practice. CommitTransaction can only fail if it's not accepted to mempool - must not fail. The transaction has already been signed and recorded.. However, to make sure we still actually have inputs we are trying to spend, I guess we have to LOCK2(cs_main, pwalletMain->cs_wallet); at the start of CPrivateSendClient::CreateDenominated(). Other places with Create/Commit have the same issue I guess, but they should be fixed in separate PR.

Expand Down