Skip to content

Conversation

@UdjinM6
Copy link
Contributor

@UdjinM6 UdjinM6 commented Feb 26, 2021

The main idea is to let all active ChainLock quorums create new CLSIG messages at every block and to require the majority of these quorums to vote for the same hash to actually lock the chain. Unlike in current system, having a malicious quorum will no longer be enough to disrupt ChainLocks via withholding attack or by producing "fake" CLSIGs (CLSIGs for some future height with some random hashes). This also deprecates the "signing attempts" part cause it feels redundant in this setup.

PoC for Dash Core: dashpay/dash#4016

@UdjinM6 UdjinM6 marked this pull request as ready for review March 7, 2021 08:36
Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

Good idea 👍 I didn't think that much about the details/impact yet but overall i think it makes sense, see comments below for some thoughts after a first read + table-of-contents section also needs some adjustments :) e.g. this dead link -> signing-attempts

Also, did you think about just making a separate DIP out of this? I honestly don't have a strong opinion on wether it should be separate or not but i think it could make sense to make a new and refer to this one. Thoughts?

UdjinM6 and others added 2 commits March 10, 2021 02:36
@UdjinM6 UdjinM6 changed the title DIP0008: Replace signing attempts with a majority of quorums singing the same height DIP0008: Replace signing attempts with a majority of quorums signing the same height Mar 10, 2021
@UdjinM6
Copy link
Contributor Author

UdjinM6 commented Mar 10, 2021

@xdustinface

Also, did you think about just making a separate DIP out of this? I honestly don't have a strong opinion on wether it should be separate or not but i think it could make sense to make a new and refer to this one. Thoughts?

I thought about it initially but imo it's ok to have this as an update because the core idea is the same - use (potentially) multiple LLMQs to secure the chain. The difference is basically that in signing attempts it was (up to) 2 quorums - one was signing the attempt and (potentially) another one was finalizing CLSIG while in MQ CLs every LLMQ just signs its own CLSIG for whatever block it considers the right one and then anyone can aggregate these CLSIGs to agree afterwards. So it's more like an evolution of the initial proposal rather than a radically new idea imo.

@QuantumExplorer
Copy link
Member

QuantumExplorer commented Mar 10, 2021

I'm not sure I understand the reasoning behind this change. Could you elaborate more @UdjinM6? Taking control of a 400 member quorum requires close to 60% of the network or 50% and a lot of hash power. If you are afraid this is not enough we could use a higher threshold quorum, and/or have more members.

@UdjinM6
Copy link
Contributor Author

UdjinM6 commented Mar 10, 2021

The idea is to make ChainLocks as robust as possible basically. We can avoid some issues by tweaking the way we handle ChainLocks internally by applying stricter rules to CLSIG messages (see dashpay/dash#3978) but it can potentially make ChainLocks more fragile in edge cases e.g. when network is under high load and blocks aren't propagated fast enough. The solution proposed here solves that.

Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

For me the step stuff looks a bit confusing tbh, thoughts about c064801? See xdustinface/dash@36628cf for an implementation of it.

Might be only me but it think this makes it easier to understand and its
imo just cleaner.
@UdjinM6
Copy link
Contributor Author

UdjinM6 commented Mar 12, 2021

@xdustinface Makes sense, picked both. Thanks! 👍

Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

Some more (and my last) points:

  • Let's maybe just move the signer stuff into the table like in 3f02341?

  • Maybe we should mention somewhere that the value of SPORK_19_CHAINLOCKS_ENABLED is used to decide if multi LLMQ signing is enabled or not.

  • It looks like there is a mismatch between dashpay/dash#4016 and the changes here. For me when i read it here it sounds like:

    • If single LLMQ signing is enabled, i.e. the spork is active and != 1, the CLSIG has structure A (signers fields are not present)
    • If multi LLMQ signing is enabled, i.e. the spork value is 1, the CLSIG has structure B (signer fields present).

    But it's actually always structure B with protocol version 70220 but with signers size 0, no? So we could maybe merge those both tables, put them somewhere (maybe a separate message section) below the signing section and somehow mark the new fields introduced by the new version. Thoughts?

xdustinface
xdustinface previously approved these changes Mar 15, 2021
Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

Looks all good to me now 👍

@thephez
Copy link
Collaborator

thephez commented Mar 15, 2021

Is the "Calculations" section still accurate with this change? Also, I was thinking it might be nice to include a link to the original version of the document (whatever the most recent change was) so people can easily refer back to it. I think that could be added to the "Prior Work" section.

@UdjinM6 UdjinM6 dismissed stale reviews from xdustinface via e9cf478 March 15, 2021 16:26
Co-authored-by: thephez <thephez@users.noreply.github.com>
@UdjinM6
Copy link
Contributor Author

UdjinM6 commented Mar 15, 2021

Is the "Calculations" section still accurate with this change?

Should be accurate for a single LLMQ mode but yeah, for a multi-quorum mode you'd need to disrupt 2 quorums (or control 3 quorums to produce invalid clsig) at the same time, so I guess you should take all these probabilities roughly to the power of 2 (or 3 respectively) or so? Let's ping @DarrenTa :)

Also, I was thinking it might be nice to include a link to the original version of the document (whatever the most recent change was) so people can easily refer back to it. I think that could be added to the "Prior Work" section.

Hmm... current version in master is https://github.com/dashpay/dips/blob/81e81b52799114191729364ed36dc0e372062fbe/dip-0008.md but I'm not sure if it makes a lot of sense to link back to itself, especially in a git repo..

@DarrenTa
Copy link
Contributor

Thank you so much for the ping. I can provide the following:

  1. I can review this change and form an opinion, backed by data, if this should be live or not.
  2. I do agree with @QuantumExplorer above. I would want a comparable security level out of the new process.
  3. I almost wonder if a straight up multisig would be the way to go? The threshold setup does require overhead.
  4. In response to @thephez perhaps DIP0008 should not be edited at all. It would be best to make a new DIP and then change the status of 0008 (to something like superseded).

I have cited DIP0008 in my academic work, it would make my citations cleaner if DIP0008 was not changed significantly. I was lazy and did not put date accessed.

I've also cited (withdrawn) 0005 and think the citations provide for a robust discussion, even if not live. No reason 0008 has to be edited to agree with the live implementation.

I'll circle back here in a week and report back with a proper analysis.

@UdjinM6
Copy link
Contributor Author

UdjinM6 commented Mar 16, 2021

Hmm.. ok, you guys convinced me to at least create an alternative PR to have smth to compare to :) Pls see #89.

@UdjinM6
Copy link
Contributor Author

UdjinM6 commented Mar 17, 2021

closing in fav of #89

@UdjinM6 UdjinM6 closed this Mar 17, 2021
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.

5 participants