Skip to content

Decouple lite mode and client-side PrivateSend - #2893

Merged
UdjinM6 merged 4 commits into
dashpay:developfrom
UdjinM6:decoupleps
Jun 27, 2019
Merged

Decouple lite mode and client-side PrivateSend#2893
UdjinM6 merged 4 commits into
dashpay:developfrom
UdjinM6:decoupleps

Conversation

@UdjinM6

@UdjinM6 UdjinM6 commented Apr 30, 2019

Copy link
Copy Markdown

This allows client-side mixing on pruned nodes running in lite mode.

Redefines fEnablePrivateSend and -enableprivatesend:

  1. off by default in lite mode and on in full mode (but can be overridden in both modes);
  2. does not start mixing anymore.

Introduces fPrivateSendRunning and -privatesendautostart (default is off). Use this to start mixing automatically (or privatesend rpc to start mixing manually).

Also, privatesend_balance is shown in rpcs regardless of lite mode or PS state now.

@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Apr 30, 2019
@UdjinM6 UdjinM6 modified the milestone: 14.0 May 1, 2019
@UdjinM6 UdjinM6 added this to the 14.1 milestone May 10, 2019
@UdjinM6
UdjinM6 force-pushed the decoupleps branch 2 times, most recently from 171617a to ec8f7fa Compare May 23, 2019 09:15
@UdjinM6

UdjinM6 commented May 23, 2019

Copy link
Copy Markdown
Author

Travis is green after manual restart https://travis-ci.org/UdjinM6/dash

Comment thread src/qt/sendcoinsdialog.cpp Outdated

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One suggestion(that's not terribly relevant to this PR but 🤷‍♂️), otherwise utACK. didn't test tho

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Testing on testnet commit ec8f7faa64 : works not in litemode, on litemode doesn't work.

Started client with ./dash-qt --testnet --litemode
In console privatesend start yields Mixing start failed: , will retry Starting with --privatesend didn't work either.

Hmm, forgot about the -enableprivatesend command, but still, I don't like that error message, not very clear that I need to enable privatesend via launch option

Alright, testing on litemode and it's working now with -enableprivatesend

@UdjinM6
UdjinM6 force-pushed the decoupleps branch 4 times, most recently from 364d900 to cbf7cae Compare May 29, 2019 18:56
@UdjinM6

UdjinM6 commented May 29, 2019

Copy link
Copy Markdown
Author

Rebased to fix conflicts after #2936

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There should be a better error message when attempting to start PS from non lite-mode client

@UdjinM6
UdjinM6 requested a review from codablock June 15, 2019 12:17

@codablock codablock left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See inline comments.

I'm also wondering if it actually makes sense to have -enableprivatesend at all if at the same time fPrivateSendRunning is actually controlling whether its running or not. What about removing all the conditionals around fEnablePrivateSend (treat it as if its always "enabled") and just handle fPrivateSendRunning? The only real effect this would have on users is that they can't disable the PS UI.

Comment thread src/privatesend/privatesend-client.cpp Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe out of scope of this PR, but I have the feeling that this check and the corresponding = false should happen at inititialization time instead of here and this location should just check for !fEnablePrivateSend || !fPrivateSendRunning

@UdjinM6 UdjinM6 Jun 17, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

cbf7caef629306246d979ecbb54da8e503b5efac kind of does the init part but I pushed more replacements for !pwalletMain just now.

Comment thread src/qt/sendcoinsdialog.cpp Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is the move of this into the !fEnablePrivateSend intended? If I'm not mistaken, this was present run in all cases before.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same with updateInstantSend actually, but I assume it's ok in that case as you're moving it into the !fLiteMode case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, there is no reason to use connect for an invisible PS checkbox (which is the case when PS is disabled, line 93).

@UdjinM6

UdjinM6 commented Jun 17, 2019

Copy link
Copy Markdown
Author

fEnablePrivateSend controls not only the UI but also some wallet code (which was initially the reason it was added back then when ps balance calculations was horribly slow).

codablock
codablock previously approved these changes Jun 24, 2019

@codablock codablock left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

utACK

@PastaPastaPasta

Copy link
Copy Markdown
Member

Was there ever any change for the error message if a user launches in lightmode but doesn't -enableprivatesend and tries to run PS commands?

@UdjinM6

UdjinM6 commented Jun 24, 2019

Copy link
Copy Markdown
Author

@PastaPastaPasta ah, I see what you mean now, wasn't clear before :)

@UdjinM6

UdjinM6 commented Jun 24, 2019

Copy link
Copy Markdown
Author

Added 7050f08, pls re-review.

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utack 7050f08

UdjinM6 added 4 commits June 26, 2019 14:45
`-enableprivatesend`:
1. off by default in lite mode but can be overridden
2. does not start mixing, use `-privatesendautostart` (default is off) or `privatesend` rpc instead
@UdjinM6

UdjinM6 commented Jun 26, 2019

Copy link
Copy Markdown
Author

Rebased to fix merge conflicts after #2992 (added a couple of gArgs.s)

@codablock codablock left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

re-utACK

@UdjinM6
UdjinM6 merged commit b75e1ce into dashpay:develop Jun 27, 2019
@UdjinM6
UdjinM6 deleted the decoupleps branch November 26, 2020 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants