-
Couldn't load subscription status.
- Fork 1.2k
Require all participants to submit equal number of inputs #2075
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,8 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm | |
|
|
||
| LogPrint("privatesend", "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CPrivateSend::GetDenominationsToString(dsa.nDenom), dsa.txCollateral.ToString()); | ||
|
|
||
| if(dsa.nInputCount < 0 || dsa.nInputCount > PRIVATESEND_ENTRY_MAX_SIZE) return; | ||
|
|
||
| masternode_info_t mnInfo; | ||
| if(!mnodeman.GetMasternodeInfo(activeMasternode.outpoint, mnInfo)) { | ||
| PushStatus(pfrom, STATUS_REJECTED, ERR_MN_LIST, connman); | ||
|
|
@@ -98,6 +100,7 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm | |
| LogPrint("privatesend", "DSQUEUE -- %s new\n", dsq.ToString()); | ||
|
|
||
| if(dsq.IsExpired()) return; | ||
| if(dsq.nInputCount < 0 || dsq.nInputCount > PRIVATESEND_ENTRY_MAX_SIZE) return; | ||
|
|
||
| masternode_info_t mnInfo; | ||
| if(!mnodeman.GetMasternodeInfo(dsq.masternodeOutpoint, mnInfo)) return; | ||
|
|
@@ -165,6 +168,18 @@ void CPrivateSendServer::ProcessMessage(CNode* pfrom, const std::string& strComm | |
| return; | ||
| } | ||
|
|
||
| if(nSessionInputCount != 0 && entry.vecTxDSIn.size() != nSessionInputCount) { | ||
| LogPrintf("DSVIN -- ERROR: incorrect number of inputs! %d/%d\n", entry.vecTxDSIn.size(), nSessionInputCount); | ||
| PushStatus(pfrom, STATUS_REJECTED, ERR_INVALID_INPUT_COUNT, connman); | ||
| return; | ||
| } | ||
|
|
||
| if(nSessionInputCount != 0 && entry.vecTxOut.size() != nSessionInputCount) { | ||
| LogPrintf("DSVIN -- ERROR: incorrect number of outputs! %d/%d\n", entry.vecTxOut.size(), nSessionInputCount); | ||
| PushStatus(pfrom, STATUS_REJECTED, ERR_INVALID_INPUT_COUNT, connman); | ||
| return; | ||
| } | ||
|
|
||
| //do we have the same denominations as the current session? | ||
| if(!IsOutputsCompatibleWithSessionDenom(entry.vecTxOut)) { | ||
| LogPrintf("DSVIN -- not compatible with existing transactions!\n"); | ||
|
|
@@ -492,7 +507,7 @@ void CPrivateSendServer::CheckForCompleteQueue(CConnman& connman) | |
| if(nState == POOL_STATE_QUEUE && IsSessionReady()) { | ||
| SetState(POOL_STATE_ACCEPTING_ENTRIES); | ||
|
|
||
| CDarksendQueue dsq(nSessionDenom, activeMasternode.outpoint, GetAdjustedTime(), true); | ||
| CDarksendQueue dsq(nSessionDenom, nSessionInputCount, activeMasternode.outpoint, GetAdjustedTime(), true); | ||
| LogPrint("privatesend", "CPrivateSendServer::CheckForCompleteQueue -- queue is ready, signing and relaying (%s)\n", dsq.ToString()); | ||
| dsq.Sign(); | ||
| dsq.Relay(connman); | ||
|
|
@@ -670,6 +685,12 @@ bool CPrivateSendServer::IsAcceptableDSA(const CDarksendAccept& dsa, PoolMessage | |
| return false; | ||
| } | ||
|
|
||
| if(dsa.nInputCount < 0 || dsa.nInputCount > PRIVATESEND_ENTRY_MAX_SIZE) { | ||
| LogPrint("privatesend", "CPrivateSendServer::%s -- requested count is not valid!\n", __func__); | ||
| nMessageIDRet = ERR_INVALID_INPUT_COUNT; | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -692,13 +713,16 @@ bool CPrivateSendServer::CreateNewSession(const CDarksendAccept& dsa, PoolMessag | |
| nMessageIDRet = MSG_NOERR; | ||
| nSessionID = GetRandInt(999999)+1; | ||
| nSessionDenom = dsa.nDenom; | ||
| // nInputCount is not covered by legacy signature, require SPORK_6_NEW_SIGS to activate to use new algo | ||
| // (to make sure nInputCount wasn't modified by some intermediary node) | ||
| nSessionInputCount = sporkManager.IsSporkActive(SPORK_6_NEW_SIGS) ? dsa.nInputCount : 0; | ||
|
|
||
| SetState(POOL_STATE_QUEUE); | ||
| nTimeLastSuccessfulStep = GetTime(); | ||
|
|
||
| if(!fUnitTest) { | ||
| //broadcast that I'm accepting entries, only if it's the first entry through | ||
| CDarksendQueue dsq(dsa.nDenom, activeMasternode.outpoint, GetAdjustedTime(), false); | ||
| CDarksendQueue dsq(dsa.nDenom, dsa.nInputCount, activeMasternode.outpoint, GetAdjustedTime(), false); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Due to the way it's activated this shouldn't really matter but I agree that using |
||
| LogPrint("privatesend", "CPrivateSendServer::CreateNewSession -- signing and relaying new queue: %s\n", dsq.ToString()); | ||
| dsq.Sign(); | ||
| dsq.Relay(connman); | ||
|
|
@@ -734,14 +758,21 @@ bool CPrivateSendServer::AddUserToExistingSession(const CDarksendAccept& dsa, Po | |
| return false; | ||
| } | ||
|
|
||
| if(dsa.nInputCount != nSessionInputCount) { | ||
| LogPrintf("CPrivateSendServer::AddUserToExistingSession -- incompatible count %d != nSessionInputCount %d\n", | ||
| dsa.nInputCount, nSessionInputCount); | ||
| nMessageIDRet = ERR_INVALID_INPUT_COUNT; | ||
| return false; | ||
| } | ||
|
|
||
| // count new user as accepted to an existing session | ||
|
|
||
| nMessageIDRet = MSG_NOERR; | ||
| nTimeLastSuccessfulStep = GetTime(); | ||
| vecSessionCollaterals.push_back(MakeTransactionRef(dsa.txCollateral)); | ||
|
|
||
| LogPrintf("CPrivateSendServer::AddUserToExistingSession -- new user accepted, nSessionID: %d nSessionDenom: %d (%s) vecSessionCollaterals.size(): %d\n", | ||
| nSessionID, nSessionDenom, CPrivateSend::GetDenominationsToString(nSessionDenom), vecSessionCollaterals.size()); | ||
| LogPrintf("CPrivateSendServer::AddUserToExistingSession -- new user accepted, nSessionID: %d nSessionDenom: %d (%s) nSessionInputCount: %d vecSessionCollaterals.size(): %d\n", | ||
| nSessionID, nSessionDenom, CPrivateSend::GetDenominationsToString(nSessionDenom), nSessionInputCount, vecSessionCollaterals.size()); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in
JoinExistingQueueyou do selection for whole range of PS rounds. And you assume this amount of inputs to setnInputCount(or to check inJoinExistingQueue)But the more value of
nInputCount, the more likely failure of this code, since inPrepareDenominateselection is limited to only one PS round.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good point but the code that follows the one you pointed to covers such cases, so shouldn't be an issue imo.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but now mixing will be complete mess:
Edit: however I'm not sure is it good or bad for privacy ;-)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see what you mean. But the former one is just the optimal strategy to get some PS balance quicker, while the later one is still a good one because it either advances low PS rounds inputs (which will help to get some more PS balance later) or mixes inputs which already have enough rounds and this shouldn't hurt either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or am I missing some another/new vulnerability here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the best possible privacy, the inputs likely should be selected randomly. Any other setup has the potential for data leakage and trace-ability, maybe there is a nice way to make it look random while also maintaining speed of getting a balance. However, the random setup would also have a problem for single input private-sending, unless there was very high liquidity of single input only mixing. Maybe we need to say you can't only mix one input?
However, having it be that "in some cases inputs will be selected with highest number of rounds" is likely a very bad idea as there will likely be grouping (when you look at Redeemed in and previous output) happening allowing for the breaking of a mix.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with PaulieD. If the inputs were selected randomly then I think you couldn't really deduce this:
. But if the most recent inputs are always used then I think that InhumanPerfection is correct.
I don't really understand the original issue. If someone could come up with an example scenario where the privacy is reduced by participants using a different number of inputs, then I might be able to understand it. The way I see it currently is that since a blockchain observer shouldn't be able to know which inputs belong to which mixing participant, then the observer also shouldn't be able to tell how many inputs belong to each participant.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulied Do you think prioritizing inputs with the highest number of rounds might have also caused #2092 or made it significantly more common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that there is a definite problem that this pr should fix. @UdjinM6 might be able to provide more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Antti-Kaikkonen yeah, if the selections were random you certainly wouldn't see that