feat: bury v20 fork - fire up test chains by first block - 4/n #6225

knst · 2024-08-23T10:01:13Z

Issue being fixed or feature implemented

V20 is activated on mainnet: time to bury it!

#6186

What was done?

Hard-fork v20 is buried and it requires to implement multiple fixes, simplifications, refactoring:

some tests for EHF moved from functional tests to unit tests
fixed crash in Credit Pool if DIP3 is not activated yet
added a requirement for v20 activation for CMNHFManager::GetSignalsStage
removed useless code from functional test feature_llmq_rotation
renamed variables "v20" to "mn_rr" in feature_llmq_evo.py so far as actually used fork is mn_rr

How Has This Been Tested?

Some unit and functional tests to succeed.

Done reindex (just in case):

src/qt/dash-qt -reindex  -assumevalid=0
src/qt/dash-qt -reindex  -assumevalid=0 -testnet

Breaking Changes

N/A

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

PastaPastaPasta · 2024-08-25T02:20:48Z

src/evo/specialtxman.cpp

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


dcab562: a devnet or something like that could have a mnhf tx get mined before v20 is active though right?

dcab562 it would not affect mainnet, testnet, indeed.

UPD: dropped

PastaPastaPasta · 2024-08-25T02:22:02Z

src/evo/specialtxman.cpp

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


4afc853: I mean, this is kinda a breaking change no? Wouldn't affect mainnet but...

4afc853 it would not affect mainnet, testnet, indeed.

UPD: dropped

PastaPastaPasta · 2024-08-25T02:23:29Z

src/evo/mnhftx.cpp

+    if (!Params().IsValidMNActivation(mnhfTx.signal.versionBit, pindexPrev->GetMedianTimePast())) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-non-ehf");
+    }


6c69808: could probably be it's own distinct PR?

could not, because it's impossible (or very hard) to implement this functionality in functional tests within v20 buried.

PastaPastaPasta · 2024-08-25T02:24:05Z

src/evo/mnhftx.cpp

-            if (Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) {
-                signals.insert({versionBit, mined_height});
-            }
-
+            signals.insert({versionBit, mined_height});


3cd7a42: how's this duplicated?

see lines 222...225

This condition already checked for each signal in new_signals above and if it fails for any signal this function will return std::nullopt as a result

PastaPastaPasta · 2024-08-25T02:24:47Z

src/chainparams.cpp

@@ -192,7 +192,8 @@ class CMainParams : public CChainParams {
        consensus.DIP0024Height = 1737792; // 0000000000000001342be9c0b75ad40c276beaad91616423c4d9cb101b3db438
        consensus.DIP0024QuorumsHeight = 1738698; // 000000000000001aa25181e4c466e593992c98f9eb21c69ee757b8bb0af50244
        consensus.V19Height = 1899072; // 0000000000000015e32e73052d663626327004c81c5c22cb8b42c361015c0eae
-        consensus.MinBIP9WarningHeight = 1899072 + 2016; // V19 activation height + miner confirmation window
+        consensus.V20Height = 1987776; // 000000000000001bf41cff06b76780050682ca29e61a91c391893d4745579777


what's the minimum set of commits needed in this PR to merge in 47def13?

two commits can be excluded from this PR as just useful follow-ups:

c15b3d9 (feat: activate v20 on the same block as v19)

4ade970 (feat: speedup functional test feature_governance.py by generating less blocks)

and probably 3cd7a42

All others commits are required for 47def13

PastaPastaPasta · 2024-08-25T02:26:33Z

test/functional/feature_block.py

        self.extra_args = [[
-            '-dip3params=2000:2000',
+            '-dip3params=2000:2000', # Must set '-dip3params=2000:2000' to create pre-dip3 blocks only


why not use testactivationheight?

it's out of scope this PR, I will do that in PR where I set DIP3 activation from block 1

PastaPastaPasta

clang-diff format is not happy

…locks during re-index Relevant crash dump: Assertion failed: detected inconsistent lock order for 'cs_main' in node/blockstorage.cpp:778 (in thread 'main'), details in debug log. Posix Signal: Aborted 0#: (0x62349D233974) stl_vector.h:115 - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_copy_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data const&) 1#: (0x62349D233974) stl_vector.h:127 - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_swap_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data&) 2#: (0x62349D233974) stl_vector.h:1959 - std::vector<unsigned long, std::allocator<unsigned long> >::_M_move_assign(std::vector<unsigned long, std::allocator<unsigned long> >&&, std::integral_constant<bool, true>) 3#: (0x62349D233974) stl_vector.h:768 - std::vector<unsigned long, std::allocator<unsigned long> >::operator=(std::vector<unsigned long, std::allocator<unsigned long> >&&) 4#: (0x62349D233974) stacktraces.cpp:777 - HandlePosixSignal 5#: (0x71E435442990) libc_sigaction.c - ??? 6#: (0x71E435499A1B) pthread_kill.c:44 - __pthread_kill_implementation 7#: (0x71E435499A1B) pthread_kill.c:78 - __pthread_kill_internal 8#: (0x71E435499A1B) pthread_kill.c:89 - __GI___pthread_kill 9#: (0x71E4354428E6) raise.c:27 - __GI_raise 10#: (0x71E4354268B7) abort.c:81 - __GI_abort 11#: (0x62349D239956) basic_string.h:390 - std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_check_length(unsigned long, unsigned long, char const*) const 12#: (0x62349D239956) basic_string.h:1461 - std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::append(char const*) 13#: (0x62349D239956) basic_string.h:1365 - std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::operator+=(char const*) 14#: (0x62349D239956) sync.cpp:114 - potential_deadlock_detected 15#: (0x62349D2403DE) sync.cpp:188 - push_lock<std::recursive_mutex> 16#: (0x62349D2403DE) sync.cpp:212 - void EnterCritical<std::recursive_mutex>(char const*, char const*, int, std::recursive_mutex*, bool) 17#: (0x62349CAA8B72) unique_lock.h:150 - std::unique_lock<std::recursive_mutex>::try_lock() 18#: (0x62349CAA8B72) sync.h:162 - UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::Enter(char const*, char const*, int) 19#: (0x62349CAA8B72) sync.h:183 - UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::UniqueLock(AnnotatedMixin<std::recursive_mutex>&, char const*, char const*, int, bool) 20#: (0x62349CE349CD) chain.h:225 - CBlockIndex::GetBlockPos() const 21#: (0x62349CE349CD) blockstorage.cpp:778 - operator() 22#: (0x62349CE349CD) blockstorage.cpp:778 - ReadBlockFromDisk(CBlock&, CBlockIndex const*, Consensus::Params const&) 23#: (0x62349D102F82) mnhftx.cpp:284 - CMNHFManager::GetForBlock(CBlockIndex const*) 24#: (0x62349D103E9B) mnhftx.cpp:58 - CMNHFManager::GetSignalsStage(CBlockIndex const*) 25#: (0x62349D07F0E9) versionbits.cpp:222 - SignalHeight 26#: (0x62349D07F90D) versionbits.cpp:37 - AbstractThresholdConditionChecker::GetStateFor(CBlockIndex const*, Consensus::Params const&, std::map<CBlockIndex const*, ThresholdState, std::less<CBlockIndex const*>, std::allocator<std::pair<CBlockIndex const* const, ThresholdState> > >&) const 27#: (0x62349D080D93) sync.h:199 - UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex> >::~UniqueLock() 28#: (0x62349D080D93) versionbits.cpp:260 - VersionBitsCache::State(CBlockIndex const*, Consensus::Params const&, Consensus::DeploymentPos) 29#: (0x62349CC06C73) deterministicmns.cpp:227 - CDeterministicMNList::GetProjectedMNPayees(gsl::not_null<CBlockIndex const* const>, int) const Aborted (core dumped)

PastaPastaPasta

generally LGMT; time for Udjin to review. I'll wanna reindex it, but I'll do that later

UdjinM6

reindexed with -assumevalid=0 on both mainnet and testnet, no issues

ACK 2381255

PastaPastaPasta

utACK 2381255

fix: crash in CreditPool: it meant to check that DIP0003 is activated

ca83b26

knst force-pushed the forks-speedup-p3 branch 3 times, most recently from f8a4893 to 318df9e Compare August 23, 2024 15:08

knst requested review from UdjinM6 and PastaPastaPasta August 24, 2024 07:31

PastaPastaPasta reviewed Aug 25, 2024

View reviewed changes

knst force-pushed the forks-speedup-p3 branch from 318df9e to 6e66cce Compare August 26, 2024 05:58

knst added 5 commits August 26, 2024 14:19

fix: remove v20 from test feature_llmq_evo as far as mn_rr used

7735631

chore: drop irrelevant bip9 code from feature_llmq_rotation.py

762a808

feat: actually test something EHF unit tests

64cedb3

feat: bury v20 deployment

1087489

knst force-pushed the forks-speedup-p3 branch from 6e66cce to 2381255 Compare August 26, 2024 07:28

knst requested a review from PastaPastaPasta August 26, 2024 11:17

PastaPastaPasta reviewed Sep 5, 2024

View reviewed changes

UdjinM6 approved these changes Sep 6, 2024

View reviewed changes

UdjinM6 added this to the 21.2 milestone Sep 6, 2024

PastaPastaPasta approved these changes Sep 10, 2024

View reviewed changes

PastaPastaPasta merged commit 6d42651 into dashpay:develop Sep 10, 2024
35 of 39 checks passed

knst mentioned this pull request Sep 13, 2024

refactor: Harden DEPLOYMENT_V20 activation #6186

Closed

knst deleted the forks-speedup-p3 branch September 16, 2024 13:37

knst mentioned this pull request Oct 10, 2024

feat: start DIP0024 from block 1, backport bitcoin#24527, bitcoin#23086 - fire up test chains by first block - 7/n #6325

Open

5 tasks

UdjinM6 removed this from the 21.2 milestone Oct 29, 2024

UdjinM6 added this to the 22 milestone Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bury v20 fork - fire up test chains by first block - 4/n #6225

feat: bury v20 fork - fire up test chains by first block - 4/n #6225

knst commented Aug 23, 2024 •

edited

Loading

PastaPastaPasta Aug 25, 2024

knst Aug 26, 2024 •

edited

Loading

PastaPastaPasta Aug 25, 2024

knst Aug 26, 2024 •

edited

Loading

PastaPastaPasta Aug 25, 2024

knst Aug 25, 2024 •

edited

Loading

PastaPastaPasta Aug 25, 2024

knst Aug 25, 2024 •

edited

Loading

PastaPastaPasta Aug 25, 2024

knst Aug 25, 2024 •

edited

Loading

PastaPastaPasta Aug 25, 2024

knst Aug 26, 2024

PastaPastaPasta left a comment

PastaPastaPasta left a comment

UdjinM6 left a comment

PastaPastaPasta left a comment

feat: bury v20 fork - fire up test chains by first block - 4/n #6225

feat: bury v20 fork - fire up test chains by first block - 4/n #6225

Conversation

knst commented Aug 23, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

PastaPastaPasta Aug 25, 2024

Choose a reason for hiding this comment

knst Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta Aug 25, 2024

Choose a reason for hiding this comment

knst Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta Aug 25, 2024

Choose a reason for hiding this comment

knst Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta Aug 25, 2024

Choose a reason for hiding this comment

knst Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta Aug 25, 2024

Choose a reason for hiding this comment

knst Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

PastaPastaPasta Aug 25, 2024

Choose a reason for hiding this comment

knst Aug 26, 2024

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta 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 Aug 23, 2024 •

edited

Loading

knst Aug 26, 2024 •

edited

Loading

knst Aug 26, 2024 •

edited

Loading

knst Aug 25, 2024 •

edited

Loading

knst Aug 25, 2024 •

edited

Loading

knst Aug 25, 2024 •

edited

Loading