Skip to content

llmq: Restrict ShouldSimulateError to trigger for LLMQ_TEST only#3871

Merged
UdjinM6 merged 3 commits into
dashpay:developfrom
xdustinface:pr-llmq-fix-simulate-error
Dec 14, 2020
Merged

llmq: Restrict ShouldSimulateError to trigger for LLMQ_TEST only#3871
UdjinM6 merged 3 commits into
dashpay:developfrom
xdustinface:pr-llmq-fix-simulate-error

Conversation

@xdustinface
Copy link
Copy Markdown

@xdustinface xdustinface commented Dec 12, 2020

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 and UdjinM6:fixCLlogic_tweaktests didn't had #3844 in its history.

After #3853 dip0008 activation takes 200 blocks which was normally activated after 10 blocks 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.

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`.
@xdustinface xdustinface added this to the 17 milestone Dec 12, 2020
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK, I think this is fine to do...

@UdjinM6
Copy link
Copy Markdown

UdjinM6 commented Dec 13, 2020

Good catch! 👍 I agree that fixing ShouldSimulateError makes sense but how about doing it in a slightly different way i.e. smth like UdjinM6@6dfe11d instead? (ShouldSimulateError isn't used anywhere outside of CDKGSession anyway)

Copy link
Copy Markdown

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK. Looks fine

@UdjinM6 UdjinM6 merged commit d0b298d into dashpay:develop Dec 14, 2020
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Apr 1, 2022
…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>
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.

3 participants