Add 5th denom, drop deprecated logic and bump min PS version #2318

UdjinM6 · 2018-09-22T21:57:19Z

add 5th denom (0.001)
always allow OP_RETURN in PS collaterals (i.e. drop deprecated logic)
collateral amount is always 1/10 of the smallest denom (i.e. no need to update it manually if we are going to add yet another denom later)
bump MIN_PRIVATESEND_PEER_PROTO_VERSION to ~~70211~~70212 to avoid conflicts with 70210 (esp. during migration)
drop backward compatibility in dsa/dsq serialization (we won't process messages from nodes below MIN_PRIVATESEND_PEER_PROTO_VERSION anyway)

Note: After merging this develop won't be able to mix on current mainnet until migration, you'd have to revert it locally to keep mixing on mainnet.

PastaPastaPasta · 2018-09-23T00:48:50Z

How will the rollout occur? Will some users not be able to mix if they have the new version and the majority of the network doesn't? Based on what you said the upgrade doesn't sound like it would be smooth

PastaPastaPasta · 2018-09-23T00:50:22Z

Also, mostly fixes #2198 but doesn't remove the 10 denom. I'd still like to see that change done. I don't think the 10 has enough usage for it to retain its privacy

UdjinM6 · 2018-09-23T07:20:49Z

How will the rollout occur? Will some users not be able to mix if they have the new version and the majority of the network doesn't? Based on what you said the upgrade doesn't sound like it would be smooth

Yes, there will be a couple of days when it won't make sense to even try to mix on a newest version. If you reeeeally need mixing you probably should just wait and keep using the old version until majority of the network is upgraded. Probably smth to be highlighted in release notes.

Also, mostly fixes #2198 but doesn't remove the 10 denom. I'd still like to see that change done. I don't think the 10 has enough usage for it to retain its privacy.

I'm not sure about that one, imo 10s still have non-negligible mixing volume https://dashradar.com/charts/mixing-transactions-per-day-by-denomination

PastaPastaPasta · 2018-09-23T16:02:36Z

Yesterday there were only 24 mixes using 10 denoms. I would consider that very very low mixing volume

UdjinM6 · 2018-09-23T17:59:36Z

Low number of mixing sessions - yes, but low volume - I wouldn't say so.

Full stats for yesterday are like this:
24x10 vs 80x1 vs 185x0.1 vs 312x0.01

For 1 input per session that would be:
240 vs 80 vs 18.5 vs 3.12
(per participant)

Yes, avg number of inputs might be higher for smaller denoms. Let's say it's 5 inputs for all other denoms but 10s:
240 vs 400 vs 92.5 vs 15.6
(per participant)

Which still gives the 2nd place by volume for 10s and it would be the 1st place if avg for 10s would be 2 inputs per session (per participant) instead.

PastaPastaPasta · 2018-09-24T01:37:19Z

I don't see how that is relevant to its effective privacy. Low volume was likely bad word choice on my side as the number of mixing sessions is what really matters

UdjinM6 · 2018-09-26T14:48:43Z

The problem with removing 10s is basically like this: if you are mixing large amounts and want to spend > 10 DASH via PS later you need 10s because otherwise you are stuck with so many 1s in 1 spending tx that it becomes not-so-private for cluster analysis algos. Having not so many sessions is not that bad imo.

PastaPastaPasta · 2018-09-26T16:53:03Z

Agreed, lack of sessions concerns me but I agree with you on the cluster analysis remark

gladcow · 2018-09-28T12:08:20Z

utACK

nmarley · 2018-10-09T15:47:34Z

src/privatesend.h

Would prefer to not assume knowledge of vecStandardDenominations here and instead use some kind of GetSmallestDenomination, but I guess we can do some clean up later.

(Only because this exposes internals of how we manage PS denoms, and we are directly accessing the internal vector instead of using accessors, which provide an abstraction and allow calling code to not worry about implementation.)

Note: I'm not suggesting any changes here, just commenting my thoughts for future clean up.

nmarley · 2018-10-09T15:49:57Z

src/privatesend.h

@UdjinM6 Will this change the wire serialization of DSQ (and cause incompatibility between versions)? Or does that not matter since Spork 6 was never activated?

Yes, serialization will change, but we are bumping MIN_PRIVATESEND_PEER_PROTO_VERSION too here (and we don't process PS messages from/to nodes below it), so it should be fine.

All 12.4 nodes will reject DSA and DSQ from any 12.3 node, and all 12.3 nodes will not be able to decode DSA and DSQ from any 12.4 node.

I see 2 issues here during the network upgrade:

there may be problems with propagation of DSQ messages through the network,

12.3 nodes can mix only with another 12.3 nodes only through 12.3 MNs, the same for 12.4 nodes/MNs

The Second issue would not be so big if the First issue didn't make mixing unstable for quite long time. Thoughts?

Small correction: 12.4 won't relay any PS message to 12.3 nodes. But yes, migration issues you highlighted are correct - mixing usability will be degraded for a week or so while network is upgrading, especially if you are not using the version which is used by the majority (MNs/mixing participants) atm.

nmarley

Just one inline question about dsq serialization, otherwise looks good.

nmarley

utACK, 👍

UdjinM6 · 2018-10-15T16:18:20Z

Rebased to fix merge conflict (due to dropping nInputCount). Also addressed the comment regarding using GetSmallestDenomination while at it. Another thing I have to note (after reviewing old PRs) is that we'll probably need smth like #1372 now that the smallest denom is lower.

codablock

utACK

nmarley

utACK

UdjinM6 · 2018-11-04T12:00:19Z

Bumped MIN_PRIVATESEND_PEER_PROTO_VERSION to the most recent proto version (70212).

PastaPastaPasta

utACK

- add 5th denom (0.001) - always allow OP_RETURN in PS collaterals - collateral amount is always 1/10 of the smallest denom - bump MIN_PRIVATESEND_PEER_PROTO_VERSION to 70211

UdjinM6 · 2018-11-05T09:35:23Z

Rebased to fix merge conflicts after #2373 . Ready for the final review :)

nmarley

utACK

PastaPastaPasta

utACK

codablock

re-utACK

UdjinM6 · 2018-11-07T07:40:56Z

NOTE: revert this ( 2624547 ) is you want to keep mixing on current testnet/mainnet

Related to dashpay/dash#2075, dashpay/dash#2259, and dashpay/dash#2318

* P2P - Remove dsa and dsq input count Related to dashpay/dash#2075, dashpay/dash#2259, and dashpay/dash#2318 * P2P - Update dsq and dstx to include BLS signature Related to dashpay/dash#2352 * P2P - Update govobj and govobjvote to include BLS signature Related to dashpay/dash#2352

UdjinM6 added this to the 12.4 milestone Sep 22, 2018

UdjinM6 mentioned this pull request Oct 3, 2018

Fix recently introduced PS bugs #2330

Merged

UdjinM6 changed the title ~~Add 5th denom, drop deprecated logic and bump min PS version~~ [DO NOT MERGE YET] Add 5th denom, drop deprecated logic and bump min PS version Oct 5, 2018

nmarley reviewed Oct 9, 2018

View reviewed changes

nmarley previously approved these changes Oct 9, 2018

View reviewed changes

UdjinM6 dismissed nmarley’s stale review via 3629cdc October 15, 2018 16:11

UdjinM6 force-pushed the bumpps branch from 8d7b3bc to 3629cdc Compare October 15, 2018 16:11

UdjinM6 changed the title ~~[DO NOT MERGE YET] Add 5th denom, drop deprecated logic and bump min PS version~~ Add 5th denom, drop deprecated logic and bump min PS version Oct 29, 2018

codablock previously approved these changes Oct 30, 2018

View reviewed changes

nmarley previously approved these changes Nov 2, 2018

View reviewed changes

UdjinM6 dismissed stale reviews from nmarley and codablock via eace306 November 4, 2018 11:58

PastaPastaPasta previously approved these changes Nov 4, 2018

View reviewed changes

UdjinM6 added 4 commits November 5, 2018 12:33

Bump PS

f7a29fa

- add 5th denom (0.001) - always allow OP_RETURN in PS collaterals - collateral amount is always 1/10 of the smallest denom - bump MIN_PRIVATESEND_PEER_PROTO_VERSION to 70211

drop backward compatibility in dsa/dsq serialization

60347bb

Use GetSmallestDenomination() in GetCollateralAmount()

5514ea4

bump MIN_PRIVATESEND_PEER_PROTO_VERSION to 70212

b1dce3f

UdjinM6 dismissed PastaPastaPasta’s stale review via b1dce3f November 5, 2018 09:34

UdjinM6 force-pushed the bumpps branch from eace306 to b1dce3f Compare November 5, 2018 09:34

UdjinM6 requested a review from nmarley November 5, 2018 09:47

nmarley approved these changes Nov 5, 2018

View reviewed changes

PastaPastaPasta approved these changes Nov 5, 2018

View reviewed changes

codablock approved these changes Nov 5, 2018

View reviewed changes

UdjinM6 merged commit 2624547 into dashpay:develop Nov 7, 2018

PastaPastaPasta mentioned this pull request Nov 12, 2018

Lower minimum denomination for PrivateSend and remove the 10 Dash denomination #2198

Closed

thephez added a commit to dash-docs/dash-docs that referenced this pull request Dec 26, 2018

P2P - Remove dsa and dsq input count

7f829d4

Related to dashpay/dash#2075, dashpay/dash#2259, and dashpay/dash#2318

UdjinM6 deleted the bumpps branch November 26, 2020 13:25

Add 5th denom, drop deprecated logic and bump min PS version #2318

Add 5th denom, drop deprecated logic and bump min PS version #2318

Uh oh!

Conversation

UdjinM6 commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PastaPastaPasta commented Sep 23, 2018

Uh oh!

PastaPastaPasta commented Sep 23, 2018

Uh oh!

UdjinM6 commented Sep 23, 2018

Uh oh!

PastaPastaPasta commented Sep 23, 2018

Uh oh!

UdjinM6 commented Sep 23, 2018

Uh oh!

PastaPastaPasta commented Sep 24, 2018

Uh oh!

UdjinM6 commented Sep 26, 2018

Uh oh!

PastaPastaPasta commented Sep 26, 2018

Uh oh!

gladcow commented Sep 28, 2018

Uh oh!

nmarley Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

nmarley Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

nmarley Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InhumanPerfection Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmarley left a comment

Choose a reason for hiding this comment

Uh oh!

nmarley left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Oct 15, 2018

Uh oh!

codablock left a comment

Choose a reason for hiding this comment

Uh oh!

nmarley left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Nov 4, 2018

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Nov 5, 2018

Uh oh!

nmarley left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

codablock left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

UdjinM6 commented Sep 22, 2018 •

edited

Loading

UdjinM6 Oct 9, 2018 •

edited

Loading

UdjinM6 Oct 16, 2018 •

edited

Loading