feat: start DIP0024 from block 1 - fire up test chains by first block - 7/n #6325

knst · 2024-10-10T09:11:04Z

Issue being fixed or feature implemented

This PR is 7th 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?

Prior work: #6187, #6189, #6214, #6225, #6269, #6275

This PR:

simplify DIP0024 activation and activate it from block 1 at RegTest
removes command lines arguments: -bip147height, -dip8params
fixed intermittent errors in feature_index_prune.py and feature_llmq_simplepose.py
fix assertion crash on Regtest if using strange combination of -testactivationheight and -dip3params

How Has This Been Tested?

Run unit, functional tests.

Breaking Changes

[regtest only] -dip8params, -bip147height are removed.

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

knst · 2024-10-17T16:08:59Z

converted to draft because feature_llmq_simplepose.py is failing often on CI

github-actions · 2024-10-21T16:19:54Z

This pull request has conflicts, please rebase.

github-actions · 2024-11-11T17:36:07Z

This pull request has conflicts, please rebase.

github-actions · 2024-11-11T17:36:28Z

This pull request has conflicts, please rebase.

knst · 2024-11-22T16:41:39Z

converted to draft because feature_llmq_simplepose.py is failing often on CI

CI seems succeed now, please help to review it @UdjinM6 @PastaPastaPasta

I believe there's still some underlying issue remain in our llmq-code or in our test_framework, because every time something relevant changed, the functional test feature_llmq_simplepose.py becomes unstable:

github-actions · 2024-12-04T15:56:22Z

This pull request has conflicts, please rebase.

It can happen because now order of activation of hardforks v20, mn_rr, dip3 can be changed by using testactivationheight on Regtest and no more guarantee about this assertions

…itPool

bip147height is superseeded by -testactivationheight=bip147@height dip8params is superseeded by -testactivationheight=dip0008@height

For current implementation it is not important when exactly fork happened yet important to know when first rotating quorum formed

…uorums

knst · 2024-12-06T16:29:23Z

I believe there's still some underlying issue remain in our llmq-code or in our test_framework, because every time something relevant changed, the functional test feature_llmq_simplepose.py becomes unstable:

ok, I fixed intermittent error in feature_llmq_simplepose.py. See 5ec071a

What a twist!!!

UdjinM6 · 2024-12-08T06:24:53Z

Looks good overall but 45bf353 and 5092207 change nothing for me, tried many times and 1-2 out of 8 jobs are failing with or without these commits. And it feels more like we are hiding the underlying issue rather than fixing it, would prefer them dropped. Also, backport commits should be in their own PR imo.

…ion.py test

knst · 2024-12-09T08:46:38Z

dropped backports, dropped 45bf353 and 5092207 which seems not necessary anymore after 5ec071a

@UdjinM6 please re-review

UdjinM6

utACK f1905ca

PastaPastaPasta · 2024-12-09T21:33:02Z

src/evo/creditpool.cpp

@@ -114,17 +114,20 @@ void CCreditPoolManager::AddToCache(const uint256& block_hash, int height, const

 static std::optional<CBlock> GetBlockForCreditPool(const CBlockIndex* const block_index, const Consensus::Params& consensusParams)
 {
+    // There's no CbTx before DIP0003 activation
+    if (!DeploymentActiveAt(*block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0003)) {


block_index can be null here?

it seems unrelated because it was used in ReadBlockFromDisk before without extra checks:

bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams) { const FlatFilePos block_pos{WITH_LOCK(cs_main, return pindex->GetBlockPos())};

Anyway, I added extra annotations just in case, see a new commit

PastaPastaPasta · 2024-12-09T21:35:49Z

src/llmq/options.cpp

-    const bool fDIP0024IsActive{optDIP0024IsActive.value_or(DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_DIP0024))};
-    const bool fHaveDIP0024Quorums{optHaveDIP0024Quorums.value_or(pindexPrev->nHeight >= consensusParams.DIP0024QuorumsHeight)};
+    const bool fDIP0024IsActive{
+        optIsDIP0024Active.value_or(DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_DIP0024))};


nit: we should rework this, currently this will always call DeploymentActiveAfter as it's a param. Should put in a lambda or something to avoid calling DeploymentActiveAfter if optIsDIP0024Active is set

we discussed it, this optIsDIP0024Active is used only for Unit test and it means that on production (mainnet, testnet, any devnet) it would be a case of OR in value_or for 100% calls. Not close to 100%, but indeed each one. So, this optimization won't give any extra benefits.

Also just a side note (not really important in this particular case) the function DeploymentActiveAfter for buried deployment (which is DIP0024) is simplified to one single if condition with height comparision, so, there's not much calculation is done anyway

UdjinM6

utACK 906c2d7

pls update PR title

PastaPastaPasta

utACK 906c2d7

knst added this to the 22 milestone Oct 10, 2024

knst changed the title ~~feat: bury mn_rr fork - fire up test chains by first block - 7/n~~ feat: bury mn_rr fork - fire up test chains by first block - 7/n Oct 10, 2024

knst force-pushed the forks-speedup-p6 branch from 6a16b30 to a320a6c Compare October 11, 2024 06:04

knst changed the title ~~feat: bury mn_rr fork - fire up test chains by first block - 7/n~~ feat: bury DIP0024 fork - fire up test chains by first block - 7/n Oct 11, 2024

knst changed the title ~~feat: bury DIP0024 fork - fire up test chains by first block - 7/n~~ feat: start DIP0024 from block 1, backport bitcoin#24527, bitcoin#23086 - fire up test chains by first block - 7/n Oct 11, 2024

knst requested review from UdjinM6 and PastaPastaPasta October 11, 2024 15:46

knst force-pushed the forks-speedup-p6 branch from a320a6c to 8073334 Compare October 17, 2024 08:35

knst marked this pull request as draft October 17, 2024 16:08

github-actions bot added the needs rebase label Oct 21, 2024

knst force-pushed the forks-speedup-p6 branch from 8073334 to 7c39ab2 Compare November 11, 2024 07:44

github-actions bot removed the needs rebase label Nov 11, 2024

github-actions bot added the needs rebase label Nov 11, 2024

PastaPastaPasta modified the milestones: 22, 22.1 Nov 14, 2024

knst force-pushed the forks-speedup-p6 branch from e0bcb0a to 80ef2b2 Compare November 21, 2024 09:50

github-actions bot removed the needs rebase label Nov 21, 2024

knst force-pushed the forks-speedup-p6 branch 6 times, most recently from 61bf984 to cd2f39a Compare November 22, 2024 14:59

knst marked this pull request as ready for review November 22, 2024 16:24

github-actions bot added the needs rebase label Dec 4, 2024

knst added 8 commits December 5, 2024 17:10

fix: assertion in Credit Pool validation during connecting blocks

26e9813

It can happen because now order of activation of hardforks v20, mn_rr, dip3 can be changed by using testactivationheight on Regtest and no more guarantee about this assertions

fix: add check that DIP0003 activated before retrieving CbTx for Cred…

4dafec8

…itPool

docs: updated comment for DIP0003 activation on RegTest

d8ce0a7

refactor: remove command line argument -bip147height, -dip8params

de821b9

bip147height is superseeded by -testactivationheight=bip147@height dip8params is superseeded by -testactivationheight=dip0008@height

fix: intermittent error in feature_index_prune due to DKG influence

343c74b

feat: re-bury DIP0024 with new height when quorums actually appeared

632c4c4

For current implementation it is not important when exactly fork happened yet important to know when first rotating quorum formed

feat: put DIP0024 activation to block 1 on RegTest

2bafadf

fix: intermittent error for feature_llmq_simplepose due to rotating q…

efd4701

…uorums

knst force-pushed the forks-speedup-p6 branch from cd2f39a to 5092207 Compare December 5, 2024 17:25

github-actions bot removed the needs rebase label Dec 5, 2024

knst added 2 commits December 9, 2024 13:44

fix: bump time for all nodes during mine_quorum in feature_llmq_rotat…

cf84dff

…ion.py test

fix: intermittent missing of PoSe ban in feature_llmq_simplepose.py

f1905ca

knst force-pushed the forks-speedup-p6 branch from 5ec071a to f1905ca Compare December 9, 2024 06:52

knst mentioned this pull request Dec 9, 2024

feat: mnauth always use basic scheme #6467

Merged

5 tasks

UdjinM6 previously approved these changes Dec 9, 2024

View reviewed changes

PastaPastaPasta reviewed Dec 9, 2024

View reviewed changes

knst dismissed UdjinM6’s stale review via bb82af5 December 10, 2024 08:44

knst requested a review from PastaPastaPasta December 10, 2024 08:46

refactor: add annotation gsl::not_null for ConstructCreditPool

906c2d7

knst force-pushed the forks-speedup-p6 branch from bb82af5 to 906c2d7 Compare December 10, 2024 08:52

UdjinM6 approved these changes Dec 10, 2024

View reviewed changes

knst changed the title ~~feat: start DIP0024 from block 1, backport bitcoin#24527, bitcoin#23086 - fire up test chains by first block - 7/n~~ feat: start DIP0024 from block 1 - fire up test chains by first block - 7/n Dec 10, 2024

PastaPastaPasta approved these changes Dec 10, 2024

View reviewed changes

PastaPastaPasta merged commit 977048f into dashpay:develop Dec 10, 2024
22 checks passed

knst deleted the forks-speedup-p6 branch December 10, 2024 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: start DIP0024 from block 1 - fire up test chains by first block - 7/n #6325

feat: start DIP0024 from block 1 - fire up test chains by first block - 7/n #6325

knst commented Oct 10, 2024 •

edited

Loading

knst commented Oct 17, 2024

github-actions bot commented Oct 21, 2024

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

knst commented Nov 22, 2024

github-actions bot commented Dec 4, 2024

knst commented Dec 6, 2024

UdjinM6 commented Dec 8, 2024

knst commented Dec 9, 2024

UdjinM6 left a comment

PastaPastaPasta Dec 9, 2024

knst Dec 10, 2024

PastaPastaPasta Dec 9, 2024

knst Dec 10, 2024 •

edited

Loading

UdjinM6 left a comment

PastaPastaPasta left a comment

feat: start DIP0024 from block 1 - fire up test chains by first block - 7/n #6325

feat: start DIP0024 from block 1 - fire up test chains by first block - 7/n #6325

Conversation

knst commented Oct 10, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

knst commented Oct 17, 2024

github-actions bot commented Oct 21, 2024

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

knst commented Nov 22, 2024

github-actions bot commented Dec 4, 2024

knst commented Dec 6, 2024

UdjinM6 commented Dec 8, 2024

knst commented Dec 9, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta Dec 9, 2024

Choose a reason for hiding this comment

knst Dec 10, 2024

Choose a reason for hiding this comment

PastaPastaPasta Dec 9, 2024

Choose a reason for hiding this comment

knst Dec 10, 2024 • edited Loading

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 Oct 10, 2024 •

edited

Loading

knst Dec 10, 2024 •

edited

Loading