fix: devnet stuck on block 2 with crash

[knst]

Issue being fixed or feature implemented

Failure on devnet:

+extra logs haven't got to screenshot:

2024-10-31T13:19:21Z CreateNewBlock(): total size 1000 txs: 0 fees: 0 sigops 100
2024-10-31T13:19:21Z Posix Signal: Aborted

Locally similar error is caught with stacktrace:

    Assertion failure:
      assertion: (tx.nType == T::SPECIALTX_TYPE)
      file: ./evo/specialtx.h, line: 33
      function: std::optional<_Tp> GetTxPayload(const TxType&, bool) [with T = CCbTx; TxType = CTransaction]
       0#: (0x5CE686FB3AB0) stacktraces.cpp:655 - __wrap___assert_fail
       1#: (0x5CE686BB9120) specialtx.h:33      - std::optional<CCbTx> GetTxPayload<CCbTx, CTransaction>(CTransaction const&, bool)
       2#: (0x5CE686BB9120) cbtx.cpp:463        - GetCoinbaseTx(CBlockIndex const*)
       3#: (0x5CE686BB916D) cbtx.cpp:470        - GetNonNullCoinbaseChainlock(CBlockIndex const*)
       4#: (0x5CE686BB92AA) optional:469        - std::_Optional_base_impl<std::pair<CBLSSignature, unsigned int>, std::_Optional_base<std::pair<CBLSSignature, unsigned int>, false, false> >::_M_is_engaged() const
       5#: (0x5CE686BB92AA) optional:986        - std::optional<std::pair<CBLSSignature, unsigned int> >::has_value() const
       6#: (0x5CE686BB92AA) cbtx.cpp:401        - CalcCbTxBestChainlock(llmq::CChainLocksHandler const&, CBlockIndex const*, unsigned int&, CBLSSignature&)
       7#: (0x5CE686D36318) miner.cpp:233       - BlockAssembler::CreateNewBlock(CScript const&)

What was done?

Added check if DIP0003 is indeed activated before retrieving Chainlock of previous block.

How Has This Been Tested?

No crash observed locally

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

[ogabrielides]

utACK 6044e06c676c0f10f32e8f47dba629ea1fcc7e29

[UdjinM6]

Should be a part of GetCoinbaseTx (because that's where we get CCbTx from), should use DeploymentActiveAt (because pindex is the block we are trying to read, not the one after it), should be if (_not_ active) return nullopt. Should also drop all unrelated refactoring.

tldr: the patch should look like that

diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp
index a5f8d6b1cd..48baccd9d9 100644
--- a/src/evo/cbtx.cpp
+++ b/src/evo/cbtx.cpp
@@ -454,6 +454,11 @@ std::optional<CCbTx> GetCoinbaseTx(const CBlockIndex* pindex)
         return std::nullopt;
     }
 
+    // There's no CbTx before DIP0003 activation
+    if (!DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0003)) {
+        return std::nullopt;
+    }
+
     CBlock block;
     if (!ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
         return std::nullopt;

[knst]

Good catch for DeploymentActiveAt, I don't know why it worked locally o_O

I'd prefer to drop GetCoinbaseTx completely because it's a confusing that do a lot of checks inside and used in only one module without any benefits, see commits

[UdjinM6]

I'd prefer to drop GetCoinbaseTx completely because it's a confusing that do a lot of checks inside and used in only one module without any benefits, see commits

I agree it's confusing but let's keep this PR focused

[UdjinM6]

pls drop 24027f8e0e220d09dce2b1a4b5bfb73b7b1af02c

[knst]

pls drop 24027f8

can you be consistent? #6336 (comment) previous review you blocked my PR because you didn't like my negative condition, and here: #6336 (comment)

[DashCoreAutoGuix]

Guix Automation has began to build this PR tagged as v22.0.0-devpr6375.dde1edf3. A new comment will be made when the image is pushed.

[UdjinM6]

pls drop 24027f8

can you be consistent? #6336 (comment) previous review you blocked my PR because you didn't like my negative condition, and here: #6336 (comment)

These are 2 completely different cases.

if (A) {
 do_smth_related_to_A
} else {
 do_smth_else
}

vs

if (not A) {
  bail_out
}

[UdjinM6]

btw If you still want to keep 24027f8e0e220d09dce2b1a4b5bfb73b7b1af02c, you should fix its title - it doesn't match the code rn

[knst]

pls drop 24027f8

I created new PR #6376
I will give later quick view to src/llmq, src/evo code, maybe there's some more functions can be hidden from global namespace also.

[DashCoreAutoGuix]

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.0.0-devpr6375.dde1edf3. The image should be on dockerhub soon.

[UdjinM6]

light ACK 50ea5e1 (reindexed on mainnet and testnet with -assumevalid set to pre-v20 block hashes, no issues)

[PastaPastaPasta]

utACK 50ea5e1