chore: Post v19 cleanup

[UdjinM6]

Issue being fixed or feature implemented

Now that v19 is buried we can enforce basic bls scheme usage in governance and coinjoin and drop some extra code we used for backwards compatibility.

What was done?

pls see individual commits

How Has This Been Tested?

run tests, sync and mix on 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 (for repository code-owners and collaborators only)

[PastaPastaPasta]

What about something like below as well? Running both governance tests is happy, but I'm mildly concerned there's a potential deadlock somewhere

Subject: [PATCH] refactor: minimize cs_main locking for governance
---
Index: src/governance/object.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/governance/object.cpp b/src/governance/object.cpp
--- a/src/governance/object.cpp	(revision f557f0bc3ca1a5f5e72fda44ee94098ff137dd57)
+++ b/src/governance/object.cpp	(date 1697725020762)
@@ -449,7 +449,6 @@
 
 void CGovernanceObject::UpdateLocalValidity()
 {
-    AssertLockHeld(cs_main);
     // THIS DOES NOT CHECK COLLATERAL, THIS IS CHECKED UPON ORIGINAL ARRIVAL
     fCachedLocalValidity = IsValidLocally(strLocalValidityError, false);
 }
@@ -464,8 +463,6 @@
 
 bool CGovernanceObject::IsValidLocally(std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const
 {
-    AssertLockHeld(cs_main);
-
     fMissingConfirmations = false;
 
     if (fUnparsable) {
@@ -537,14 +534,13 @@
 
 bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingConfirmations) const
 {
-    AssertLockHeld(cs_main);
-
     strError = "";
     fMissingConfirmations = false;
     uint256 nExpectedHash = GetHash();
 
     // RETRIEVE TRANSACTION IN QUESTION
     uint256 nBlockHash;
+    // Locks cs_main internally
     CTransactionRef txCollateral = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, nCollateralHash, Params().GetConsensus(), nBlockHash);
     if (!txCollateral) {
         strError = strprintf("Can't find collateral tx %s", nCollateralHash.ToString());
@@ -596,9 +592,9 @@
 
     // GET CONFIRMATIONS FOR TRANSACTION
 
-    AssertLockHeld(cs_main);
     int nConfirmationsIn = 0;
     if (nBlockHash != uint256()) {
+        LOCK(cs_main);
         const CBlockIndex* pindex = g_chainman.m_blockman.LookupBlockIndex(nBlockHash);
         if (pindex && ::ChainActive().Contains(pindex)) {
             nConfirmationsIn += ::ChainActive().Height() - pindex->nHeight + 1;
Index: src/governance/object.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/governance/object.h b/src/governance/object.h
--- a/src/governance/object.h	(revision f557f0bc3ca1a5f5e72fda44ee94098ff137dd57)
+++ b/src/governance/object.h	(date 1697725020758)
@@ -247,12 +247,12 @@
 
     // CORE OBJECT FUNCTIONS
 
-    bool IsValidLocally(std::string& strError, bool fCheckCollateral) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool IsValidLocally(std::string& strError, bool fCheckCollateral) const;
 
-    bool IsValidLocally(std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool IsValidLocally(std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const;
 
     /// Check the collateral transaction for the budget proposal/finalized budget
-    bool IsCollateralValid(std::string& strError, bool& fMissingConfirmations) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool IsCollateralValid(std::string& strError, bool& fMissingConfirmations) const;
 
     void UpdateLocalValidity();
 

[UdjinM6]

@PastaPastaPasta but this patch doesn't drop any lock, it only drops thread safety checks...

[PastaPastaPasta]

@PastaPastaPasta but this patch doesn't drop any lock, it only drops thread safety checks...

oh.. lol 🙈 give me a sec

[ogabrielides]

ACK, synced Testnet

[PastaPastaPasta]

Take a look at: 5776b23

This passes tests at least; thoughts?

[PastaPastaPasta]

utACK for squash merge; my patch is kinda unrelated