consenus: Implement LLMQ_100_67 quorums by UdjinM6 · Pull Request #3844 · dashpay/dash

UdjinM6 · 2020-12-01T21:07:29Z

Implements #3729

Re-uses DEPLOYMENT_V17 as an activation mechanism.

And to avoid a potentialy endless "while" loop

xdustinface · 2020-12-04T20:03:44Z

See suggestions-3844 for some renaming ideas. Feel free to pick only what you like 😄 It's nothing which would hold me up giving an utACK.

My thoughts about the llmq_type_new to llmq_type_v17: If we add another one its not the new one anymore haha.

UdjinM6 · 2020-12-04T20:30:00Z

Picked some of your suggestions. How about de92158 instead of the rest of them?

xdustinface

How about de92158 instead of the rest of them?

Okay yeah agree, the term "new" might make sense there if we add another LLMQ at some point :D

There is one thing though where im not totally sure about yet: Will things work well if we have all members of 100 member LLMQs connected (Spork21 is currently active for this new LLMQ Type)?

@codablock You might have tested that? Can you share your thoughts here also?

PastaPastaPasta · 2020-12-05T21:13:00Z

+        .keepOldConnections = 25,
+        .recoveryMembers = 50,


We need a bit more thought about these values and figuring out what a good value is

Per https://github.com/dashpay/dash/blob/master/src/consensus/params.h#L119-L121, keepOldConnections has to be 25 (or more).

…uce GetLLMQParams Signed-off-by: pasta <pasta@dashboost.org>

PastaPastaPasta · 2020-12-08T18:50:07Z

Just talked with @QuantumExplorer, due to modifications he is making in tenderdash, there is no longer a need for a high minimum size of 90 (no longer using a dynamic threshold), so I propose we drop that to 80.

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

PastaPastaPasta · 2020-12-09T20:07:01Z

What is the behavior of this quorum in regards to Concentrated Recovery?

UdjinM6 · 2020-12-09T20:13:29Z

There are no changes in CLLMQUtils::IsAllMembersConnectedEnabled atm so it's going to participate just like 50_60 quorum does. Should I exclude it? (cant find anything about it in the original issue)

PastaPastaPasta · 2020-12-09T20:19:08Z

It wasn't mentioned in the original issue... I'm not sure if it's a good idea or not. I guess it would depend on if concentrated recovery / MNs could handle another massive hunk of connections...

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

PastaPastaPasta

LGTM, utACK, at this point waiting on tests

xdustinface

utACK

Current `develop` tests fail. This was basically introduced by dashpay#3844 but it didn't come up before dashpay#3853 because the `v17` fork wasn't activated in `feature_llmq_dkgerrors.py`. After dashpay#3853 `dip0008` activation takes [200 blocks](dashpay@b95cf01#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd88a15d272512266c76R539) which was normally activated after [10 blocks](dashpay@b95cf01#diff-b92fa0fafafa27172736ebc88f9f9b658b1160caca512a318eefb7d93d22bf3cL18) in `feature_llmq_dkgerrors.py`. Now with the 200 blocks `v17` gets activated during test which then leads to MN1, MN2 banning MN0 because it lies in DKG of `LLMQ_TEST` and `LLMQ_TEST_V17`. There are other ways to solve it, like enabling `dip0008` earlier or enable `v17` later but IMO its anyway better to restrict `ShouldSimulateError` to only trigger for `LLMQ_TEST`.

…3871) * llmq: Restrict `ShouldSimulateError` to trigger for LLMQ_TEST only Current `develop` tests fail. This was basically introduced by #3844 but it didn't come up before #3853 because the `v17` fork wasn't activated in `feature_llmq_dkgerrors.py`. After #3853 `dip0008` activation takes [200 blocks](b95cf01#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd88a15d272512266c76R539) which was normally activated after [10 blocks](b95cf01#diff-b92fa0fafafa27172736ebc88f9f9b658b1160caca512a318eefb7d93d22bf3cL18) in `feature_llmq_dkgerrors.py`. Now with the 200 blocks `v17` gets activated during test which then leads to MN1, MN2 banning MN0 because it lies in DKG of `LLMQ_TEST` and `LLMQ_TEST_V17`. There are other ways to solve it, like enabling `dip0008` earlier or enable `v17` later but IMO its anyway better to restrict `ShouldSimulateError` to only trigger for `LLMQ_TEST`. * Revert "llmq: Restrict `ShouldSimulateError` to trigger for LLMQ_TEST only" This reverts commit ec42d86. * llmq: Restrict `ShouldSimulateError` to trigger for LLMQ_TEST only (alternative) Move ShouldSimulateError into CDKGSession Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

* Add LLMQ_100_67 quorums * Re-use DEPLOYMENT_V17 bit to activate LLMQ_100_67 quorums * Add LLMQ_TEST_NEW quorum and test its activation * Tweak mine_quorum to work correctly with multiple quorum types And to avoid a potentialy endless "while" loop * llmq: Rename IsQuorumTypeEnabledAtBlock -> IsQuorumTypeEnabled * chainparams|test: Rename llmq_test_new -> llmq_test_v17 * chainparams|consensus|llmq: Rename LLMQ_TEST_NEW -> LLMQ_TEST_V17 * Tweak few strings and the name of the test * llmq: Make GetEnabledQuorumTypes return a vector of LLMQTypes, introduce GetLLMQParams Signed-off-by: pasta <pasta@dashboost.org> * Tweak minSize Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> * Exclude LLMQ_100_67 from Concentrated Recovery * Update test/functional/feature_new_quorum_type_activation.py Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: xdustinface <xdustinfacex@gmail.com> Co-authored-by: pasta <pasta@dashboost.org> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

…ashpay#3871) * llmq: Restrict `ShouldSimulateError` to trigger for LLMQ_TEST only Current `develop` tests fail. This was basically introduced by dashpay#3844 but it didn't come up before dashpay#3853 because the `v17` fork wasn't activated in `feature_llmq_dkgerrors.py`. After dashpay#3853 `dip0008` activation takes [200 blocks](dashpay@b95cf01#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd88a15d272512266c76R539) which was normally activated after [10 blocks](dashpay@b95cf01#diff-b92fa0fafafa27172736ebc88f9f9b658b1160caca512a318eefb7d93d22bf3cL18) in `feature_llmq_dkgerrors.py`. Now with the 200 blocks `v17` gets activated during test which then leads to MN1, MN2 banning MN0 because it lies in DKG of `LLMQ_TEST` and `LLMQ_TEST_V17`. There are other ways to solve it, like enabling `dip0008` earlier or enable `v17` later but IMO its anyway better to restrict `ShouldSimulateError` to only trigger for `LLMQ_TEST`. * Revert "llmq: Restrict `ShouldSimulateError` to trigger for LLMQ_TEST only" This reverts commit ec42d86. * llmq: Restrict `ShouldSimulateError` to trigger for LLMQ_TEST only (alternative) Move ShouldSimulateError into CDKGSession Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6 added this to the 17 milestone Dec 1, 2020

UdjinM6 force-pushed the platformquorum branch 6 times, most recently from 10b9db5 to 9ec9dbe Compare December 2, 2020 20:42

UdjinM6 added 4 commits December 3, 2020 13:38

Add LLMQ_100_67 quorums

5429d82

Re-use DEPLOYMENT_V17 bit to activate LLMQ_100_67 quorums

25e1fba

Add LLMQ_TEST_NEW quorum and test its activation

44a0faf

Tweak mine_quorum to work correctly with multiple quorum types

0e0305b

And to avoid a potentialy endless "while" loop

UdjinM6 force-pushed the platformquorum branch from 9ec9dbe to 0e0305b Compare December 3, 2020 10:41

UdjinM6 marked this pull request as ready for review December 3, 2020 10:58

xdustinface changed the title ~~consenus: Implement LLMQ_100_70 quorums~~ consenus: Implement LLMQ_100_67 quorums Dec 3, 2020

xdustinface and others added 4 commits December 4, 2020 23:15

llmq: Rename IsQuorumTypeEnabledAtBlock -> IsQuorumTypeEnabled

2d05a85

chainparams|test: Rename llmq_test_new -> llmq_test_v17

d00c9aa

chainparams|consensus|llmq: Rename LLMQ_TEST_NEW -> LLMQ_TEST_V17

b092b79

Tweak few strings and the name of the test

de92158

xdustinface reviewed Dec 5, 2020

View reviewed changes

PastaPastaPasta requested changes Dec 5, 2020

View reviewed changes

llmq: Make GetEnabledQuorumTypes return a vector of LLMQTypes, introd…

1ae70b1

…uce GetLLMQParams Signed-off-by: pasta <pasta@dashboost.org>

PastaPastaPasta reviewed Dec 9, 2020

View reviewed changes

Comment thread src/chainparams.cpp Outdated

Tweak minSize

8f82215

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

PastaPastaPasta reviewed Dec 9, 2020

View reviewed changes

Comment thread src/llmq/quorums_utils.cpp

Exclude LLMQ_100_67 from Concentrated Recovery

ff997db

PastaPastaPasta requested changes Dec 9, 2020

View reviewed changes

Comment thread test/functional/feature_new_quorum_type_activation.py Outdated

Update test/functional/feature_new_quorum_type_activation.py

6b71409

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

PastaPastaPasta approved these changes Dec 9, 2020

View reviewed changes

xdustinface approved these changes Dec 9, 2020

View reviewed changes

xdustinface merged commit cf64339 into dashpay:develop Dec 9, 2020

xdustinface mentioned this pull request Dec 12, 2020

llmq: Restrict ShouldSimulateError to trigger for LLMQ_TEST only #3871

Merged

UdjinM6 deleted the platformquorum branch July 1, 2021 21:47

Conversation

UdjinM6 commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xdustinface commented Dec 4, 2020

Uh oh!

UdjinM6 commented Dec 4, 2020

Uh oh!

xdustinface left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Dec 5, 2020

Choose a reason for hiding this comment

Uh oh!

thephez Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta commented Dec 8, 2020

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta commented Dec 9, 2020

Uh oh!

UdjinM6 commented Dec 9, 2020

Uh oh!

PastaPastaPasta commented Dec 9, 2020

Uh oh!

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

xdustinface left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

UdjinM6 commented Dec 1, 2020 •

edited

Loading