Skip to content

Conversation

@ogabrielides
Copy link

@ogabrielides ogabrielides commented Mar 16, 2023

Issue being fixed or feature implemented

In #5229, we disabled rotation when running a regtest with 3 nodes.
Instantsend were non-deterministic islocks.
This PR aims to make them deterministic for a 3-nodes regtest network (Platform dev environment).

What was done?

Added the possibility to overwrite llmqinstantsend and llmqinstantsenddip0024.
By starting a regtest with:
-llmqinstantsend=llmq_test
-llmqinstantsenddip0024=llmq_test_instantsend

Instantsend locks will be deterministic without running quorum rotation.

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@ogabrielides ogabrielides added this to the 20 milestone Mar 16, 2023
std::string strLLMQType = gArgs.GetArg("-llmqinstantsend", std::string(llmq_params_opt->name));

Consensus::LLMQType llmqType = Consensus::LLMQType::LLMQ_NONE;
for (const auto& params : consensus.llmqs) {
Copy link
Collaborator

@knst knst Mar 16, 2023

Choose a reason for hiding this comment

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

nit: it seems as this loop already implemented several times:

         if (auto optLLMQParams = ranges::find_if_opt(Params().GetConsensus().llmqs, 
                                                     [&strLLMQType](const auto& params){return params.name == strLLMQType;})) {

in src/llmq/utils.cpp and src/chainparams.cpp:

    Consensus::LLMQType llmqType = Consensus::LLMQType::LLMQ_NONE;
    for (const auto& params : consensus.llmqs) {
        if (params.name == strLLMQType) {
            llmqType = params.type;
        }
    }

May be it's time to refactor to a new method:

    const Consensus::LLMQParams& GetLLMQ(const std::string& llmqTypeName) const;

?

@ogabrielides
Copy link
Author

ogabrielides commented Mar 17, 2023

@knst All your suggestions are valid.
Similar refactoring can be made in UpdateDevnetLLMQInstantSendFromArgs, UpdateDevnetLLMQInstantSendDIP0024FromArgs and since those are irrelevant to this PR..
Please feel free to open a new PR refactoring all the cases (once this gets merged) :)

@ogabrielides
Copy link
Author

@PastaPastaPasta @UdjinM6 Moved this to v19 as requested by @markin-io.

UdjinM6
UdjinM6 previously approved these changes Mar 20, 2023
Copy link

@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
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 for squash merge (built locally)

@PastaPastaPasta PastaPastaPasta merged commit 444bc61 into dashpay:develop Mar 20, 2023
@ogabrielides ogabrielides deleted the regtest_isdlocks branch March 20, 2023 15:59
PastaPastaPasta pushed a commit that referenced this pull request Oct 30, 2023
…enddip0024` (#5654)

## Issue being fixed or feature implemented
This reverts #5636 and introduces 2 similar cmd-line/config params which
are made specifically for regtest. Turned out Platform guys actually
still need smth like that for local testing #5259.

## What was done?
pls see individual commits

## How Has This Been Tested?
run tests but we don't really have(/need?) tests for this.

## Breaking Changes
n/a

## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_

cc @shumkov
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.

4 participants