refactor: make MNActivationHeight in Params() indeed constant #5658

knst · 2023-10-30T18:11:02Z

Issue being fixed or feature implemented

Addressed issues and comments from PR comment and PR comment

Params() should be const; global variable CMNHFManager is a better out-come.

What was done?

The helpers and direct calls of UpdateMNParams for each block to update non-constant member in Params() is not needed anymore. Instead CMNHFManager takes cares about status of Signals for each block, update them dynamically and save in evo db.

How Has This Been Tested?

Run unit/functional tests.

Breaking Changes

Changed rpc getblockchaininfo.
the field ehf changed meaning: it's now only a flag -1/0; but it is introduced a new field ehf_height now that a height.

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
I have assigned this pull request to a milestone

src/consensus/params.h

src/rpc/blockchain.cpp

test/functional/test_framework/test_framework.py

src/evo/mnhftx.cpp

UdjinM6 · 2023-10-31T15:11:18Z

(also, pls rebase to fix linter/tests)

UdjinM6

few more suggestions

src/chainparams.cpp

UdjinM6 · 2023-11-01T10:14:38Z

also, I'd like to see that in v20 :)

src/chainparams.cpp

UdjinM6

LGTM, utACK

knst · 2023-11-01T15:22:14Z

also, I'd like to see that in v20 :)

previous implementation is tested on devnet-ouzo, this one - only on reg-test. @UdjinM6
If this one will be included in v20, we need to get it in next beta/RC and activate EHF (MN_RR) on testnet with this changes merged.
@PastaPastaPasta

PastaPastaPasta · 2023-11-06T15:20:16Z

src/evo/mnhftx.cpp

+        // TODO: can it actually happen? Maybe if try to use old version with re-index and update after MN_RR activated
+        assert(false);


why'd we add this?

if it will happen (this line will be run), it means that instead legit list of signals the node will have an empty list (that is not expected).

probably we can throw some exception out then? not sure what is good enough solution; but current implementation behaviour is not expecting (because it can cause that some hard-fork will be deactivated or something similar)

Well; we shouldn't be asserting in paths that are affected by p2p; which this appears to be (as I think this is called on block connect)

@UdjinM6 can you evaluate these asserts?

We should never reach this point on a potential invalid chain (mined past v20 activation block by some older version) because it's going to fail much earlier due to other changes activated via v20 I think. So this assert simply ensures that we read signals from evodb for some block only if we wrote them earlier. And if not, there is GetFromCache somewhere that is called too early which would be a logical error and we shouldn't continue. In fact, I would probably make it even more explicit 7d0cb4a. We could probably also avoid writing useless data to evodb for pre-v20 blocks while we are at it 2ca4aee.

PastaPastaPasta · 2023-11-06T15:22:25Z

src/versionbits.cpp

+AbstractEHFManager* AbstractEHFManager::globalInstance{nullptr};
+
+AbstractEHFManager* AbstractEHFManager::getInstance()
+{
+    return globalInstance;
+}


I don't think I really like this new "AbstractEHFManager" thing; why not just have a global like we do now in other parts of codebase, or refactor it to not need a global and use member variables.

it's to avoid circular dependency versionbits -> mnhfmanager -> versionbits.

This abstract interface is meant to break this circular dependency over a small simple interface AbstractEHFManager.

If there is a logical dependency; then I think it's best for there to just be a circular dependency.

there's nothing like "logical dependency". It just means that there's not enough interfaces between components; components should know public interfaces but not private details of implementatino.

## Issue being fixed or feature implemented There's too much spamming log items related to new v20 features: credit pool, asset locks, EHF manager, EHF Signaling for MN_RR. Some logs are still spamming after this PR but related code is not changed here #5658 ## What was done? - Removed some log items, tidy-up other. - logs that supposed to appear for each block are moved to new categories EHF and CREDITPOOL ## How Has This Been Tested? Run unit/functional tests, reviewed log output ## 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

The helpers and direct calls of UpdateMNParams for each block to update non-constant member in Params() is not needed anymore. Instead CMNHFManager takes cares about status of Signals for each block, update them dynamically and save in evo db.

…o/mnhftx" It appeared in previous commit, but gone now

- updated help for RPC - int64 replaced to bool in chain param nMNActivationHeight -> useEHF - simplified helper in CMNHFManager

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

knst · 2023-11-07T19:23:37Z

@knst knst force-pushed the refactor-mnehf-manager branch from 4cf7447 to 6b7f514

rebased on top of current develop due to conflicts with #5669 and #5668

UdjinM6

utACK

src/versionbits.h

src/versionbits.cpp

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

It should be initialized before the very first call

UdjinM6

utACK

src/rpc/blockchain.cpp

Co-authored-by: thephez <thephez@users.noreply.github.com>

UdjinM6

utACK

PastaPastaPasta

utACK for squash merge

knst added the RPC Some notable changes to RPC params/behaviour/descriptions label Oct 30, 2023

knst added this to the 21 milestone Oct 30, 2023

knst requested review from UdjinM6 and PastaPastaPasta October 30, 2023 18:11

UdjinM6 requested changes Oct 31, 2023

View reviewed changes

src/consensus/params.h Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

src/evo/mnhftx.cpp Outdated Show resolved Hide resolved

knst force-pushed the refactor-mnehf-manager branch from 2171b01 to 0f20116 Compare October 31, 2023 15:49

UdjinM6 requested changes Nov 1, 2023

View reviewed changes

UdjinM6 reviewed Nov 1, 2023

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

UdjinM6 previously approved these changes Nov 1, 2023

View reviewed changes

This was referenced Nov 3, 2023

feat: reduce spamming logs with messages from v20 features #5669

Merged

test: v20 earlier activation for regtest #5668

Merged

knst modified the milestones: 21, 20.1 Nov 5, 2023

PastaPastaPasta modified the milestones: 20.1, 20 Nov 6, 2023

PastaPastaPasta reviewed Nov 6, 2023

View reviewed changes

knst requested a review from PastaPastaPasta November 6, 2023 16:27

knst and others added 9 commits November 8, 2023 02:12

refactor: rid of circular dependency "evo/mnhftx -> versionbits -> ev…

720afda

…o/mnhftx" It appeared in previous commit, but gone now

fix: add missing header <limits>

f92d2f3

fixes accordingly Code Review

a5ea0a9

- updated help for RPC - int64 replaced to bool in chain param nMNActivationHeight -> useEHF - simplified helper in CMNHFManager

fix: avoid implicit conversion from int64_t to bool

b4bbeb1

Update src/chainparams.cpp

f9b8b43

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

avoid useless db writes for DB_SIGNALS prior to v20

ddfb3c7

DB_SIGNALS _must_ be available since v20 activation

610cd14

follow-up dashpay#5668 - correct flags for params

6b7f514

knst dismissed UdjinM6’s stale review via 6b7f514 November 7, 2023 19:22

knst force-pushed the refactor-mnehf-manager branch from 4cf7447 to 6b7f514 Compare November 7, 2023 19:22

knst requested a review from UdjinM6 November 7, 2023 19:27

UdjinM6 previously approved these changes Nov 8, 2023

View reviewed changes

PastaPastaPasta reviewed Nov 8, 2023

View reviewed changes

src/versionbits.h Outdated Show resolved Hide resolved

src/versionbits.cpp Outdated Show resolved Hide resolved

knst dismissed UdjinM6’s stale review via 53965d6 November 8, 2023 16:33

knst and others added 3 commits November 8, 2023 23:33

Apply suggestions from code review for AbstractEHFManager

53965d6

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

fix: there's no guarantee that getIntance won't return non-nullptr

f23d136

It should be initialized before the very first call

reland gsl

d2d7a53

knst requested a review from PastaPastaPasta November 8, 2023 17:26

fixup: removed duplicated assert

0b67217

UdjinM6 previously approved these changes Nov 8, 2023

View reviewed changes

thephez reviewed Nov 8, 2023

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

Update src/rpc/blockchain.cpp

82ecc88

Co-authored-by: thephez <thephez@users.noreply.github.com>

knst dismissed UdjinM6’s stale review via 82ecc88 November 9, 2023 10:04

UdjinM6 approved these changes Nov 9, 2023

View reviewed changes

PastaPastaPasta approved these changes Nov 10, 2023

View reviewed changes

PastaPastaPasta merged commit 216a5f7 into dashpay:develop Nov 10, 2023
8 checks passed

knst deleted the refactor-mnehf-manager branch November 10, 2023 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: make MNActivationHeight in Params() indeed constant #5658

refactor: make MNActivationHeight in Params() indeed constant #5658

knst commented Oct 30, 2023

UdjinM6 commented Oct 31, 2023

UdjinM6 left a comment

UdjinM6 commented Nov 1, 2023

UdjinM6 left a comment

knst commented Nov 1, 2023 •

edited

Loading

PastaPastaPasta Nov 6, 2023

knst Nov 6, 2023

PastaPastaPasta Nov 7, 2023

PastaPastaPasta Nov 7, 2023

UdjinM6 Nov 7, 2023

PastaPastaPasta Nov 6, 2023

knst Nov 6, 2023

PastaPastaPasta Nov 7, 2023

knst Nov 7, 2023

knst commented Nov 7, 2023

UdjinM6 left a comment

UdjinM6 left a comment

UdjinM6 left a comment

PastaPastaPasta left a comment

		// TODO: can it actually happen? Maybe if try to use old version with re-index and update after MN_RR activated
		assert(false);

refactor: make MNActivationHeight in Params() indeed constant #5658

refactor: make MNActivationHeight in Params() indeed constant #5658

Conversation

knst commented Oct 30, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

UdjinM6 commented Oct 31, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 commented Nov 1, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

knst commented Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knst commented Nov 7, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst commented Nov 1, 2023 •

edited

Loading