Skip to content
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

Split PS into Manager and Session and allow running multiple mixing sessions in parallel (on client side) #2203

Merged
merged 10 commits into from Sep 4, 2018

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Aug 5, 2018

This should considerably speed things up (when there are enough participants ofc). Should also mitigate mixing speed degradation which could happen if we introduce additional mixing denoms (i.e. fix #2198) and keep the rest of it as is.

@UdjinM6 UdjinM6 added this to the 12.4 milestone Aug 5, 2018
@@ -40,13 +44,18 @@ void CPrivateSendClient::ProcessMessage(CNode* pfrom, const std::string& strComm
CDarksendQueue dsq;
vRecv >> dsq;

{
Copy link
Member

Choose a reason for hiding this comment

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

Why are these brackets here? If there's a reason for them to be there shouldn't it's contents be indented?

Copy link
Author

Choose a reason for hiding this comment

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

That's for cs_vecqueue lock. Same logic for changes as in #2200 basically but since the block here is pretty small and there are a lot of changes anyway I guess I should probably add some whitespaces here. Will fix.

UnlockCoins();
keyHolderStorage.ReturnAll();
SetNull();
}

void CPrivateSendClient::SetNull()
void CPrivateSendClientManager::ResetPool()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we keep CPrivateSendClientSession methods together and CPrivateSendClientManager methods together and not intermingle?

Copy link
Author

Choose a reason for hiding this comment

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

The idea for the split is to change the behaviour, so imo it's best to keep the logic that was split close to the original one in code to make it easier to review. I would create a separate move-only PR after this one is merged (there are quite a few other dash-specific modules which could also be refactored this way btw).

for (auto& session : vecSessions) {
strSessionDenoms += (session.nSessionDenom ? CPrivateSend::GetDenominationsToString(session.nSessionDenom) : "N/A") + "; ";
}
return strSessionDenoms.empty() ? "N/A" : strSessionDenoms;
Copy link
Member

Choose a reason for hiding this comment

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

Is `"N/A" consistent to use? I don't recall seeing that elsewhere and it seems a little weird.
Also, in current there is no difference in return between having 1 session which has no denoms and having no sessions, which I would think sound be a problem

Copy link
Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean tbh. Any suggestions how to modify this?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if "N/A" is a good message to use. It seems very non-descriptive but since I don't really know what the overall use case is I can't say if it should be changed or if it's fine and dandy

Copy link
Author

Choose a reason for hiding this comment

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

}

void CPrivateSendClientManager::CheckTimeout()
Copy link
Member

Choose a reason for hiding this comment

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

add comment to remain consistent?

CheckQueue();

for (auto& session : vecSessions) {
if (!session.CheckTimeout()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does false mean timedout and true means not timedout? I get that the normal thing is true means success false means failed, but this just isn't intuitive imo and confuses a reader

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, will rewrite CPrivateSendClientSession::CheckTimeout() a bit.

static const int MAX_PRIVATESEND_ROUNDS = 16;
static const int MAX_PRIVATESEND_AMOUNT = MAX_MONEY / COIN;
static const int MAX_PRIVATESEND_LIQUIDITY = 100;
static const int DEFAULT_PRIVATESEND_SESSIONS = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reasoning for this and MAX_PRIVATESEND_SESSIONS or do you just love 4 and 10 😉 ?

Copy link
Author

Choose a reason for hiding this comment

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

Don't want to keep too many connections open to avoid high bandwidth usage while still having some positive effect of using multiple sessions even with default settings. 4 and 10 seems like reasonable numbers for normal and extreme usage respectively IMO :)

Copy link
Member

Choose a reason for hiding this comment

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

Alright, we probably want to put some documentation out about how much bandwidth it'll use, etc on different values and if that might affect privacy in any way. Although that might be more @strophy 's domain?

Copy link
Author

Choose a reason for hiding this comment

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

Well, by default node keeps MAX_OUTBOUND_CONNECTIONS (8) regular connections open, so each new connections adds some initial sync bandwidth (should be negligible) and relay everything one which depends on current network activity. Anyway, good that you asked - I realized that we should also bump MAX_OUTBOUND_MASTERNODE_CONNECTIONS to make sure mixing sessions fit into the limit, see 213a080 🙂 👍

if (privateSendClient.GetMixingMasternodeInfo(mnInfo)) {
obj.push_back(Pair("outpoint", mnInfo.outpoint.ToStringShort()));
obj.push_back(Pair("addr", mnInfo.addr.ToString()));
// TODO:
Copy link
Member

Choose a reason for hiding this comment

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

remove todo

Copy link
Author

Choose a reason for hiding this comment

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

It's for state and entries - not sure if I want them to be removed or implemented in pprivateSendBaseManager, so I just added TODO here as a reminder.

@PastaPastaPasta
Copy link
Member

utACK

@Antti-Kaikkonen
Copy link

I'm a little bit concerned that by making the mixing speed faster you might end up mixing with fewer people thus reducing your privacy. Any thoughts?

@UdjinM6
Copy link
Author

UdjinM6 commented Aug 14, 2018

@Antti-Kaikkonen yes, that's a good point but it also could be that you will mix with more people because 1) when mixing is faster more people could be willing to mix and 2) you could mix with those you would miss if you were using one session only. So I guess it's hard to say what the final outcome will be but imo it's a balanced one.

@thephez
Copy link
Collaborator

thephez commented Aug 14, 2018

@Antti-Kaikkonen Also, while the default number of sessions is 4, you can override that and set it to anywhere between 1 and 10 so "legacy mode" is still available if you set it to 1.

gladcow
gladcow previously approved these changes Aug 17, 2018
Copy link

@gladcow gladcow left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 changed the title Split PS into Manager and Session and allow running multiple mixing sessions in parallel (on client side) [WIP] Split PS into Manager and Session and allow running multiple mixing sessions in parallel (on client side) Aug 22, 2018
@UdjinM6 UdjinM6 force-pushed the splitps branch 3 times, most recently from 4d0f297 to 4f50895 Compare August 22, 2018 23:44
@UdjinM6 UdjinM6 changed the title [WIP] Split PS into Manager and Session and allow running multiple mixing sessions in parallel (on client side) Split PS into Manager and Session and allow running multiple mixing sessions in parallel (on client side) Aug 25, 2018
@UdjinM6
Copy link
Author

UdjinM6 commented Aug 25, 2018

No potential deadlocks or conflicting transactions (finally) for a couple of days of non-stop mixing and a bunch of restarts here. Pls (re-)review/test :)

Copy link

@gladcow gladcow left a comment

Choose a reason for hiding this comment

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

ACK, slightly tested

Copy link

@nmarley nmarley left a comment

Choose a reason for hiding this comment

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

utACK, nothing stands out to me, but I'm not too familiar w/PS code currently

Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit b164bcc into dashpay:develop Sep 4, 2018
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request May 16, 2019
…essions in parallel (client side) (dashpay#2203)

* Split PS into Manager and Session

* Adjust log messages accordingly

* add -privatesendsessions cmd-line option

* address review comments

* bump MAX_OUTBOUND_MASTERNODE_CONNECTIONS to 30

+10 for parallel mixing

* protect vecSessions

* constructors

* Rewrite CMasternodeMan::ProcessMasternodeConnections() to use CPrivateSendClientManager::GetMixingMasternodesInfo().

This should solve potential deadlock cs_vecqueue vs cs_vNodes.

* Drop no longer used IsMixingMasternode()

* lock cs_wallet when mixing uses balance related functions
@UdjinM6 UdjinM6 deleted the splitps branch November 26, 2020 13:29
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.

None yet

7 participants