Skip to content

Conversation

@xdustinface
Copy link

This PR is a fix for #3597. The issue there is that we run in cases where the change output in CPrivateSendClientSession::MakeCollateralAmounts is too small to be considered as private send collateral. This leads to those change outputs returning false for CPrivateSend::IsCollateralAmount in

(wtx.tx->vout[0].nValue == nPreMaxCollateralAmount && CPrivateSend::IsCollateralAmount(wtx.tx->vout[1].nValue)) ||
(wtx.tx->vout[1].nValue == nPreMaxCollateralAmount && CPrivateSend::IsCollateralAmount(wtx.tx->vout[0].nValue));
Then the transaction will not become seen as “PrivateSend Make Collateral Inputs” transaction but instead it ends up being seen as “Payment to yourself” transaction. Those are the unexpected transaction popping up in the issue.

As fix CPrivateSendClientSession::MakeCollateralAmounts simply skips “PrivateSend Make Collateral Inputs” transactions with change outputs smaller than GetCollateralAmount(). This way we also don’t leave tiny outputs in the wallet but instead a bit bigger ones if they don’t contain enough.. i guess that makes sense?

a195832 is alternative GUI only fix which also allows change outputs smaller than GetCollateralAmount() by setting the minimum required value to GetMaxCollateralAmount() in TransactionRecord::decomposeTransaction

(wtx.tx->vout[0].nValue == nPreMaxCollateralAmount && wtx.tx->vout[1].nValue <= CPrivateSend::GetMaxCollateralAmount()) ||
(wtx.tx->vout[1].nValue == nPreMaxCollateralAmount && wtx.tx->vout[0].nValue <= CPrivateSend::GetMaxCollateralAmount());
By doing this we still end up parsing the “PrivateSend Make Collateral Input" transactions with too small changes properly and not see them as “Payment to yourself" transactions like with the issue.

I tested a195832 and it works but i couldn’t verify 0fbfd9b since its not so easy to reproduce the issue. But just from looking at the code, it should make sense and we should go with 0fbfd9b, no?

@UdjinM6
Copy link

UdjinM6 commented Jul 30, 2020

Interesting find 👍 but I think the root cause is incorrect MakeCollateralAmounts calculations which precede tx creation. Pls see #3640

@xdustinface
Copy link
Author

Interesting find 👍 but I think the root cause is incorrect MakeCollateralAmounts calculations which precede tx creation. Pls see #3640

Better find 👍 I had the fee under suspicion but seems like i lost track anywhere down the road haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants