fix: do not check chainlock state in IsTxSafeForMining (#5444)

## Issue being fixed or feature implemented
Disabled or non-enforced Chainlocks does not mean you can safely mine
non-locked txes, you could end up mining a block that is going to be
rejected by everyone else if a conflicting tx (missing on your node)
would be IS-locked. I can't find any reason why we have this besides "if
Chainlocks are disabled then smth is wrong so let them all be mined" but
we have spork_2 and spork_3 to control IS behaviour and we check them in
`IsTxSafeForMining` already, that would be a much more straightforward
way to deal with a potential issue.

Noticed this while reviewing #5150 and also while testing v19.2 during
recent testnet v19 re-fork.

## What was done?
Drop this check, adjust tests

## How Has This Been Tested?
Run tests locally

## Breaking Changes
Not quote breaking changes but a change in behaviour: with CLs disabled
it will now take 10 minutes for non-locked txes to be mined, same as
when CLs are enabled.

## 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 _(for repository
code-owners and collaborators only)_

src/llmq/chainlocks.cpp

@@ -449,17 +449,7 @@ CChainLocksHandler::BlockTxs::mapped_type CChainLocksHandler::GetBlockTxs(const
 
 bool CChainLocksHandler::IsTxSafeForMining(const CInstantSendManager& isman, const uint256& txid) const
 {
-    if (!isman.RejectConflictingBlocks()) {
-        return true;
-    }
-    if (!isEnabled || !isEnforced) {
-        return true;
-    }
-
-    if (!isman.IsInstantSendEnabled()) {
-        return true;
-    }
-    if (isman.IsLocked(txid)) {
+    if (!isman.RejectConflictingBlocks() || !isman.IsInstantSendEnabled() || isman.IsLocked(txid)) {
         return true;
     }
 

test/functional/feature_dip4_coinbasemerkleroots.py

@@ -44,6 +44,12 @@ class LLMQCoinbaseCommitmentsTest(DashTestFramework):
         self.set_dash_test_params(4, 3, fast_dip3_enforcement=True)
 
     def run_test(self):
+        # No IS or Chainlocks in this test
+        self.bump_mocktime(1)
+        self.nodes[0].sporkupdate("SPORK_2_INSTANTSEND_ENABLED", 4070908800)
+        self.nodes[0].sporkupdate("SPORK_19_CHAINLOCKS_ENABLED", 4070908800)
+        self.wait_for_sporks_same()
+
         self.test_node = self.nodes[0].add_p2p_connection(TestP2PConn())
 
         self.confirm_mns()
@@ -85,9 +91,7 @@ class LLMQCoinbaseCommitmentsTest(DashTestFramework):
 
         self.nodes[0].generate(1)
         oldhash = self.nodes[0].getbestblockhash()
-        # Have to disable ChainLocks here because they won't let you to invalidate already locked blocks
-        self.nodes[0].sporkupdate("SPORK_19_CHAINLOCKS_ENABLED", 4070908800)
-        self.wait_for_sporks_same()
+
         # Test DIP8 activation once with a pre-existing quorum and once without (we don't know in which order it will activate on mainnet)
         self.test_dip8_quorum_merkle_root_activation(True)
         for n in self.nodes:
@@ -95,11 +99,6 @@ class LLMQCoinbaseCommitmentsTest(DashTestFramework):
         self.sync_all()
         first_quorum = self.test_dip8_quorum_merkle_root_activation(False, True)
 
-        # Re-enable ChainLocks again
-        self.nodes[0].sporkupdate("SPORK_19_CHAINLOCKS_ENABLED", 0)
-        self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0)
-        self.wait_for_sporks_same()
-
         # Verify that the first quorum appears in MNLISTDIFF
         expectedDeleted = []
         expectedNew = [QuorumId(100, int(first_quorum, 16)), QuorumId(104, int(first_quorum, 16)), QuorumId(106, int(first_quorum, 16))]

test/functional/p2p_instantsend.py

@@ -35,7 +35,9 @@ class InstantSendTest(DashTestFramework):
 
         # feed the sender with some balance
         sender_addr = sender.getnewaddress()
-        self.nodes[0].sendtoaddress(sender_addr, 1)
+        is_id = self.nodes[0].sendtoaddress(sender_addr, 1)
+        for node in self.nodes:
+            self.wait_for_instantlock(is_id, node)
         self.bump_mocktime(1)
         self.nodes[0].generate(2)
         self.sync_all()
@@ -54,11 +56,15 @@ class InstantSendTest(DashTestFramework):
         for node in connected_nodes:
             self.wait_for_instantlock(is_id, node)
         # send doublespend transaction to isolated node
-        isolated.sendrawtransaction(dblspnd_tx['hex'])
+        dblspnd_txid = isolated.sendrawtransaction(dblspnd_tx['hex'])
         # generate block on isolated node with doublespend transaction
+        self.bump_mocktime(599)
+        wrong_early_block = isolated.generate(1)[0]
+        assert not "confirmation" in isolated.getrawtransaction(dblspnd_txid, 1)
+        isolated.invalidateblock(wrong_early_block)
         self.bump_mocktime(1)
-        isolated.generate(1)
-        wrong_block = isolated.getbestblockhash()
+        wrong_block = isolated.generate(1)[0]
+        assert_equal(isolated.getrawtransaction(dblspnd_txid, 1)["confirmations"], 1)
         # connect isolated block to network
         self.reconnect_isolated_node(self.isolated_idx, 0)
         # check doublespend block is rejected by other nodes
@@ -90,7 +96,9 @@ class InstantSendTest(DashTestFramework):
 
         # feed the sender with some balance
         sender_addr = sender.getnewaddress()
-        self.nodes[0].sendtoaddress(sender_addr, 1)
+        is_id = self.nodes[0].sendtoaddress(sender_addr, 1)
+        for node in self.nodes:
+            self.wait_for_instantlock(is_id, node)
         self.bump_mocktime(1)
         self.nodes[0].generate(2)
         self.sync_all()