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

Yet another PrivateSend refactoring/optimization - core part #1134

Merged
merged 4 commits into from Nov 16, 2016
Merged

Yet another PrivateSend refactoring/optimization - core part #1134

merged 4 commits into from Nov 16, 2016

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Nov 7, 2016

That's the core part of #1120. Progress of fixing/refactoring/cleanup can be checked there (see checkboxes and/or merged references). The rest of changes is pretty much about changing the way mixing states are handled i.e. core feature so I think it makes sense to push them all at once. I squashed the history into one commit to exclude previous fixes and all "fix", "another fix", "cleanup" changes :) I also tried to break changes in kind of sub-categories below, hope it helps a bit.

Refactoring:

  • IsDenomCompatibleWithSession, split in 3 - CreateNewSession, AddUserToExistingSession and IsAcceptableDenomAndCollateral
  • CheckTimeout:
    • should not rely on CDarkSendEntry nTimeAdded to clear entries, instead should rely on time of the last successful step i.e. nLastTimeChanged
    • nLastTimeChanged should only be set when mixing moved forward in some way

State related:

  • local pool should be in POOL_STATE_IDLE initially
  • local pool should switch to POOL_STATE_QUEUE when connected to mn
  • local pool should set session id only when in POOL_STATE_QUEUE
  • SetState should set state local only, no relaying
  • mixing wallets should rely on local logic and expected set of state switches rather then updates from masternodes
  • deprecate STATUS_SET_STATE, POOL_STATE_UNKNOWN, POOL_STATE_TRANSMISSION, POOL_STATE_FINALIZE_TRANSACTION

Session related:

  • deprecate fSessionFoundMasternode, use nSessionID instead
  • deprecate nSessionUsers, use vecSessionCollaterals.size() instead

Other:

  • deprecate IsNull()
  • move few things to private
  • remove deprecated
  • bump min ps peer proto

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 8, 2016

rebased

Refactoring:
- IsDenomCompatibleWithSession, split in 3 - CreateNewSession, AddUserToExistingSession and IsAcceptableDenomAndCollateral
- CheckTimeout:
    - should not rely on CDarkSendEntry nTimeAdded to clear entries, instead should rely on time of the last successful step i.e. nLastTimeChanged
    - nLastTimeChanged should only be set when mixing moved forward in some way

State related:
- local pool should be in POOL_STATE_IDLE initially
- local pool should switch to POOL_STATE_QUEUE when connected to mn
- local pool should set session id only when in POOL_STATE_QUEUE
- SetState should set state local only, no relaying
- mixing wallets should rely on local logic and expected set of state switches rather then updates from masternodes
- deprecate STATUS_SET_STATE, POOL_STATE_UNKNOWN, POOL_STATE_TRANSMISSION, POOL_STATE_FINALIZE_TRANSACTION

Session related:
- deprecate fSessionFoundMasternode, use nSessionID instead
- deprecate nSessionUsers, use vecSessionCollaterals.size() instead

Other:
- deprecate IsNull()
- move few things to private
- remove deprecated
- bump min ps peer proto
@UdjinM6
Copy link
Author

UdjinM6 commented Nov 9, 2016

So, just a quick high-level overview of how states/messages should work now:

  • client side: state POOL_STATE_IDLE ---(send DSACCEPT)---> state POOL_STATE_QUEUE ---(DSQUEUE with fReady == true received)---> send inputs (DSVIN), state POOL_STATE_ACCEPTING_ENTRIES ---(DSFINALTX recieved)---> sign your inputs in final tx and send it back (DSSIGNFINALTX), state POOL_STATE_SIGNING ---(DSCOMPLETE received)---> state POOL_STATE_IDLE
  • mn side: state POOL_STATE_IDLE ---(DSACCEPT received)---> state POOL_STATE_QUEUE, relay DSQUEUE ---(enough DSACCEPT received)---> state POOL_STATE_ACCEPTING_ENTRIES, create and relay new DSQUEUE with fReady == true ---(enough DSVIN received)---> state POOL_STATE_SIGNING, relay tx to sign ---(enough DSSIGNFINALTX received)---> relay DSTX and DSCOMPLETE, state POOL_STATE_IDLE

Reviewers wanted :)

@crowning- ? ;)

@crowning-
Copy link

That's quite some work, so it would be nice if someone else would review it as well.

Since I can't do a complete review I'll update whenever I find some time.

SetNull();
SetState(POOL_STATE_ERROR);
strLastMessage = GetMessageByID(nMessageID);
return true;
Copy link

@crowning- crowning- Nov 9, 2016

Choose a reason for hiding this comment

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

Is nLastTimeChanged reinitialized (aka is CreateNewSession called) when the client tries again?
If not he would immediately run into a timeout when he tries again, so a nLastTimeChanged = GetTimeMillis(); would be needed here.

Copy link
Author

Choose a reason for hiding this comment

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

nLastTimeChanged is changed at every successful step, not only as a part of CreateNewSession() - in both SetNull() and DoAuto() if we talk about (re)starting a session. Also it times out only when nState != POOL_STATE_IDLE. Technically, time is set first DoAuto() before switching from idle to queue, but it's two lines next to each other in the same function and I doubt it can cause issues. I think it won't hurt changing the order of instructions though for better readability/logic of handling them. Also nLastTimeChanged name itself doesn't say what it really does now, I'll fix that too.

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 11, 2016

2 more fixes:

  • rename UpdatePoolStateOnClient to CheckPoolStateUpdate
  • reset pSubmittedToMasternode in SetNull()

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 16, 2016

Ok, no more comments for the last week so I guess it's ok to merge :)
Note: this will break mixing on testnet until proto bump, that's expected, not an issue

@UdjinM6 UdjinM6 merged commit 5128085 into dashpay:v0.12.1.x Nov 16, 2016
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

2 participants