feat: improvements for v19, v20 and dip3 - fire up test chains by first block 5/n #6269

knst · 2024-09-13T08:49:04Z

Issue being fixed or feature implemented

This PR is 5th in the achieving ultimate goal to activate old forks from block 1.
It helps to run unit and functional tests faster; it helps for platform's dev-environment to start faster.

What was done?

v20 on RegTest is activated from same block as v19 (height 1200 changed to 900)
relaxed condition for special transactions Asset Lock (can be mined any block so far as v20 is activated long time ago).
unify code for regtest, mainnet, testnet for Asset Unlock validation
removed 2 checkpoints: TestChainDIP3Setup and TestChainV19Setup from unit tests which make further changes for forks easier
enforced flag fast_dip3_enforcement=True from functional tests which is always true

How Has This Been Tested?

Run unit and functional tests

tsan job runs 500 seconds faster of real time and 2000seconds faster for "accumulated time"
https://gitlab.com/dashpay/dash/-/jobs/7817453421 - this PR
https://gitlab.com/dashpay/dash/-/jobs/7805625816 - some old PR for reference

No breakdown per tests here, because they affect each other and runs in parallel.

Breaking Changes

Regtest has v20 activated on same block as v19 if otherwise is not specified with -testactivationheight=v20@1200

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

UdjinM6 · 2024-09-13T18:35:42Z

src/evo/specialtxman.cpp

@@ -53,18 +53,11 @@ static bool CheckSpecialTxInner(CDeterministicMNManager& dmnman, const Chainstat
            }
            return CheckMNHFTx(chainman, qman, tx, pindexPrev, state);
        case TRANSACTION_ASSET_LOCK:
-            if (!DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_V20)) {


why keeping similar checks for "unlock" and "mnhf" if we drop this one?

@UdjinM6 don't we normally keep checks like this? for stuff like new devnets, or in case someone is sending us a bad fork I guess?

what will happen if someone will send us a bad fork for devnet? If fork will be accepted - that's fine, whatever it has asset-unlock/mnehf/etc transaction or not. we have chainlocks also.

That's important to keep conditions like that if they change behavior incompatible way. For example DIP0001 has not only introduced a bigger blocks size, but also introduced a limit for size of transaction and some old blocks do not satisfy that requirement. So, for that type of consensus condition we should keep it, otherwise mainnet/testnet can't re-sync properly for old blocks. But there's completely fine.

Can you show in our code base any condition like that which we keep intentionally over-versions?

also, we have all soft-forks on all devnets activated from blocks 300 (soon from block 2), so, it should not be a problem also
https://github.com/dashpay/dash/pull/6187/files#diff-ff53e63501a5e89fd650b378c9708274df8ad5d38fcffa6c64be417c4d438b6dL568-L573

with headers-first sync and assumevalid in place these checks do not matter that much anymore and we can drop them safely imo

UdjinM6

utACK 4950be0

UdjinM6

utACK 07c510f

github-actions · 2024-09-17T02:28:12Z

This pull request has conflicts, please rebase.

knst · 2024-09-17T08:12:30Z

@knst knst force-pushed the forks-speedup-p4 branch from 07c510f to c422e37

rebased due to conflict with #6271

UdjinM6

rebase looks clean

re-utACK c422e37

PastaPastaPasta · 2024-09-19T15:02:37Z

src/evo/specialtxman.cpp

-            if (Params().NetworkIDString() != CBaseChainParams::REGTEST && !DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_MN_RR)) {
-                return state.Invalid(TxValidationResult::TX_CONSENSUS, "assetunlocks-before-mn_rr");
-            }


same as before comment, re: devnets bad chain etc?

PastaPastaPasta · 2024-09-19T15:03:57Z

src/evo/specialtxman.cpp

-            if (!DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_V20)) {
-                return state.Invalid(TxValidationResult::TX_CONSENSUS, "mnhf-before-v20");
-            }


github-actions · 2024-09-24T13:55:05Z

This pull request has conflicts, please rebase.

UdjinM6

utACK cad8aff

knst · 2024-09-25T13:34:00Z

@knst knst force-pushed the forks-speedup-p4 branch from cad8aff to cd037ad

Force pushed due to CI failure after rebase. There's extra fix now

UdjinM6

utACK cd037ad

github-actions · 2024-09-25T23:30:09Z

This pull request has conflicts, please rebase.

V20 is activated long time on mainnet and testnet, so, it doesn't matter anymore

…works It simplify implementation and unify RegTest, Mainnet and Testnet No asset-unlock transaction has actually be mined yet, but v20 and mn_rr are activated long time ago. So, this changes are not breaking changes

… from unit tests It takes time to update each checkpoint if any forks changes in unit tests: new height, new bit, and extra params. Reduced scope of changes for future updates

It also change dip3params default from 30:50 to 2:2 De facto, that always equals True, so, there's no meaning to have it

It is not a breaking changes, because this fork is already happened in past and no EHF transaction is in blockchain at that moment which requires versioning

This fork already happened and no versioning is required

knst · 2024-09-26T07:18:10Z

@knst knst force-pushed the forks-speedup-p4 branch from 0da73ac to 643f9bc
@knst knst force-pushed the forks-speedup-p4 branch from 643f9bc to 6030611

last 2 force-pushes due to rebase mistakes

knst · 2024-09-26T07:20:55Z

utACK https://github.com/dashpay/dash/commit/4950be0df22b23346004c191787424f899e6819f
utACK https://github.com/dashpay/dash/commit/07c510fa08d7ec8335b9bf3090276a5d860dc05b
>  This pull request has conflicts, please rebase.
re-utACK https://github.com/dashpay/dash/commit/c422e37181cb1a3353df9184b39272bfe2429dd5
> This pull request has conflicts, please rebase.
utACK https://github.com/dashpay/dash/commit/cad8aff1fd963bf9cfe2fc9e20289d95f3465f59
> Force pushed due to [CI failure](https://gitlab.com/dashpay/dash/-/jobs/7906337253) after rebase. There's extra fix now
utACK https://github.com/dashpay/dash/commit/cd037ad15bed4431d6bb0088343b444f3f836e1f
> This pull request has conflicts, please rebase.
> @knst knst force-pushed the forks-speedup-p4 branch from 0da73ac to 643f9bc
> @knst knst force-pushed the forks-speedup-p4 branch from 643f9bc to 6030611
...

what a journey! @PastaPastaPasta please, help with this PR finally

UdjinM6

utACK 6030611

PastaPastaPasta

utACK 6030611

knst added this to the 21.2 milestone Sep 13, 2024

knst requested review from UdjinM6 and PastaPastaPasta September 13, 2024 14:57

knst changed the title ~~feat: improvements for v19, v20 and dip3 - fire up test chains by first block 5/n~~ feat: improvements for v19, v20 and dip3 - fire up test chains by first block 5/n Sep 13, 2024

UdjinM6 reviewed Sep 13, 2024

View reviewed changes

knst requested a review from UdjinM6 September 14, 2024 05:35

UdjinM6 previously approved these changes Sep 14, 2024

View reviewed changes

knst dismissed UdjinM6’s stale review via 07c510f September 14, 2024 13:20

UdjinM6 previously approved these changes Sep 14, 2024

View reviewed changes

github-actions bot added the needs rebase label Sep 17, 2024

knst dismissed UdjinM6’s stale review via c422e37 September 17, 2024 08:11

knst force-pushed the forks-speedup-p4 branch from 07c510f to c422e37 Compare September 17, 2024 08:11

github-actions bot removed the needs rebase label Sep 17, 2024

knst mentioned this pull request Sep 17, 2024

feat: bury mn_rr fork - fire up test chains by first block - 6/n #6275

Open

5 tasks

UdjinM6 previously approved these changes Sep 17, 2024

View reviewed changes

knst force-pushed the forks-speedup-p4 branch from c422e37 to 8411fc9 Compare September 19, 2024 06:51

PastaPastaPasta reviewed Sep 19, 2024

View reviewed changes

knst requested a review from PastaPastaPasta September 19, 2024 15:20

This was referenced Sep 19, 2024

fix: new fork 'withdrawals' and fixes for asset unlock signature verification #6279

Open

fix: do not use extra_args for enforced DashTestFramework arguments #6283

Open

github-actions bot added the needs rebase label Sep 24, 2024

knst dismissed UdjinM6’s stale review via cad8aff September 24, 2024 14:03

knst force-pushed the forks-speedup-p4 branch from 8411fc9 to cad8aff Compare September 24, 2024 14:03

github-actions bot removed the needs rebase label Sep 24, 2024

UdjinM6 previously approved these changes Sep 24, 2024

View reviewed changes

knst dismissed UdjinM6’s stale review via cd037ad September 25, 2024 13:33

knst force-pushed the forks-speedup-p4 branch from cad8aff to cd037ad Compare September 25, 2024 13:33

knst requested a review from UdjinM6 September 25, 2024 13:34

UdjinM6 previously approved these changes Sep 25, 2024

View reviewed changes

github-actions bot added the needs rebase label Sep 25, 2024

knst dismissed UdjinM6’s stale review via 0da73ac September 26, 2024 06:01

knst force-pushed the forks-speedup-p4 branch from cd037ad to 0da73ac Compare September 26, 2024 06:01

github-actions bot removed the needs rebase label Sep 26, 2024

knst force-pushed the forks-speedup-p4 branch from 0da73ac to 643f9bc Compare September 26, 2024 06:02

knst added 9 commits September 26, 2024 14:16

feat: let asset locks be mined before v20

e0d97cf

V20 is activated long time on mainnet and testnet, so, it doesn't matter anymore

perf: activate v20 on the same block as v19 for Reg Test

4b4001b

refactor: moves evo-deterministicmns_tests specific code from header

3fffb0c

feat: removed 2 checkpoints: TestChainDIP3Setup and TestChainV19Setup…

0add6bc

… from unit tests It takes time to update each checkpoint if any forks changes in unit tests: new height, new bit, and extra params. Reduced scope of changes for future updates

refactor: drop fast_dip3_enforcement=True from functional tests.

8639298

It also change dip3params default from 30:50 to 2:2 De facto, that always equals True, so, there's no meaning to have it

feat: drop v20 requirement for special EHF transaction

b3e9e5c

It is not a breaking changes, because this fork is already happened in past and no EHF transaction is in blockchain at that moment which requires versioning

feat: drop requirement of v20 for Asset Unlock transactions

f01338f

This fork already happened and no versioning is required

style: apply clang format for new changes

6030611

knst force-pushed the forks-speedup-p4 branch from 643f9bc to 6030611 Compare September 26, 2024 07:17

UdjinM6 approved these changes Sep 26, 2024

View reviewed changes

PastaPastaPasta approved these changes Sep 27, 2024

View reviewed changes

PastaPastaPasta merged commit 9b21aef into dashpay:develop Sep 27, 2024
27 checks passed

knst deleted the forks-speedup-p4 branch September 27, 2024 04:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improvements for v19, v20 and dip3 - fire up test chains by first block 5/n #6269

feat: improvements for v19, v20 and dip3 - fire up test chains by first block 5/n #6269

knst commented Sep 13, 2024 •

edited

Loading

UdjinM6 Sep 13, 2024

PastaPastaPasta Sep 19, 2024

knst Sep 19, 2024

knst Sep 19, 2024

UdjinM6 Sep 23, 2024 •

edited

Loading

UdjinM6 left a comment

UdjinM6 left a comment

github-actions bot commented Sep 17, 2024

knst commented Sep 17, 2024

UdjinM6 left a comment

PastaPastaPasta Sep 19, 2024

PastaPastaPasta Sep 19, 2024

github-actions bot commented Sep 24, 2024

UdjinM6 left a comment

knst commented Sep 25, 2024

UdjinM6 left a comment

github-actions bot commented Sep 25, 2024

knst commented Sep 26, 2024

knst commented Sep 26, 2024

UdjinM6 left a comment

PastaPastaPasta left a comment

feat: improvements for v19, v20 and dip3 - fire up test chains by first block 5/n #6269

feat: improvements for v19, v20 and dip3 - fire up test chains by first block 5/n #6269

Conversation

knst commented Sep 13, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

UdjinM6 Sep 13, 2024

Choose a reason for hiding this comment

PastaPastaPasta Sep 19, 2024

Choose a reason for hiding this comment

knst Sep 19, 2024

Choose a reason for hiding this comment

knst Sep 19, 2024

Choose a reason for hiding this comment

UdjinM6 Sep 23, 2024 • edited Loading

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

github-actions bot commented Sep 17, 2024

knst commented Sep 17, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta Sep 19, 2024

Choose a reason for hiding this comment

PastaPastaPasta Sep 19, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

knst commented Sep 25, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2024

knst commented Sep 26, 2024

knst commented Sep 26, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst commented Sep 13, 2024 •

edited

Loading

UdjinM6 Sep 23, 2024 •

edited

Loading