PrivateSend: dont waste keys from keypool on failure in CreateDenominated#1473
Conversation
| 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 |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
ok! let's leave it as 11 then!
| nOutputs++; | ||
| nValueLeft -= nDenomValue; | ||
| LogPrintf("CreateDenominated1: nOutputsTotal: %d, nOutputs: %d, nValueLeft: %f\n", nOutputsTotal, nOutputs, (float)nValueLeft/COIN); | ||
| LogPrintf("CreateDenominated1: nOutputsTotal: %d, nOutputs: %d, nValueLeft: %f\n", nOutputsTotal + nOutputs, nOutputs, (float)nValueLeft/COIN); |
There was a problem hiding this comment.
why? the log output seems incorrect now...
There was a problem hiding this comment.
the idea here was to show the actual number of total outputs created.
now logs look like this:
2017-05-19 16:50:04 CreateDenominated1: nOutputsTotal: 0, nOutputs: 1, nValueLeft: 999.984985
2017-05-19 16:50:04 CreateDenominated1: nOutputsTotal: 0, nOutputs: 2, nValueLeft: 999.974976
2017-05-19 16:50:04 CreateDenominated1: nOutputsTotal: 0, nOutputs: 3, nValueLeft: 999.965027
2017-05-19 16:50:04 CreateDenominated1: nOutputsTotal: 0, nOutputs: 4, nValueLeft: 999.955017
2017-05-19 16:50:04 CreateDenominated1: nOutputsTotal: 0, nOutputs: 5, nValueLeft: 999.945007
2017-05-19 16:50:04 CreateDenominated1: nOutputsTotal: 0, nOutputs: 6, nValueLeft: 999.934998
2017-05-19 16:50:04 CreateDenominated1: nOutputsTotal: 0, nOutputs: 7, nValueLeft: 999.924988
2017-05-19 16:50:05 CreateDenominated1: nOutputsTotal: 0, nOutputs: 8, nValueLeft: 999.914978
2017-05-19 16:50:05 CreateDenominated1: nOutputsTotal: 0, nOutputs: 9, nValueLeft: 999.905029
2017-05-19 16:50:05 CreateDenominated1: nOutputsTotal: 0, nOutputs: 10, nValueLeft: 999.895020
2017-05-19 16:50:05 CreateDenominated1: nOutputsTotal: 0, nOutputs: 11, nValueLeft: 999.885010
2017-05-19 16:50:05 CreateDenominated1: nOutputsTotal: 11, nOutputs: 1, nValueLeft: 999.784973
you are right that with my change log is incorrect - variable nOutputsTotal has diffrent value than printed.
So maybe something like in may next commit?
| LogPrintf("CPrivateSendClient::CreateDenominated -- %d keys keeped\n", reservekeyDenomVec.size() + 1); | ||
|
|
||
| if(!pwalletMain->CommitTransaction(wtx, reservekeyChange)) { | ||
| LogPrintf("CPrivateSendClient::CreateDenominated -- CommitTransaction failed!\n"); |
There was a problem hiding this comment.
What if failure occurs here (ie. in CommitTransaction) ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
slightly tested (mixing as usual, not trying to force any conflicts) - works as expected
ACK
3 participants
Changes:
connected with track#19.