Merge dashpay#6369: feat: increased withdrawal limits to flat 2000 fr…

…om v22

e43ca62 feat: replace assert to error in p2p code of Asset Lock (Konstantin Akimov)
c97f5f5 feat: update some asserts related to CreditPool in consensus code to exceptions (Konstantin Akimov)
877aa08 feat: generate less blocks in feature_asset_locks.py to make it faster (Konstantin Akimov)
a51ade5 style: apply clang-format (Konstantin Akimov)
ef6190e docs: add release notes for withdrawal changes in v22 (Konstantin Akimov)
5b0a2f5 fix: string in credit pool logs 'previous' is renamed to recently unlocked (Konstantin Akimov)
31ca8a4 feat: update limit of withdrawals to flat 2000 starting from v22 (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Limit 1000 seems a bit small at the moment, while limit 2000 is still safe enough.

  ## What was done?
  Withdrawals limits in pre-v22 are:
   - if credit pool is more than 10k -> limit withdrawals to 1k
   - if credit pool is between 100 dash and 10000 dash -> let to withdraw 10%.
   - if 10% of credit pool is less than 100 dash -> no limits, let to withdraw everything.

  The fork `withdrawals` introduces higher limit:
   - 2000 dash per last 576 blocks. That's all.

  ## How Has This Been Tested?
  Updated functional test `feature_asset_locks.py`

  ## Breaking Changes
  Limits of withdrawals are increased to 2000 dash. It changes consensus rules.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK e43ca62
  UdjinM6:
    utACK e43ca62

Tree-SHA512: 77fc27b6b38105cc311ee5ea78d66edfe854600ad6fb9422c0d302dac436e9aa1dcdc394a36ccb980d42de98091c1596e01be260b3a940df4e6309842fd89065

doc/release-notes-6279.md

@@ -0,0 +1,8 @@
+# Notable changes
+
+## Asset Unlock transactions (platform transfer)
+
+This version introduces a new fork `withdrawals` that changes consensus rules.
+New logic of validation of Asset Unlock transactions's signature. It let to use all 24 active quorums and the most recent inactive, while previous version of Dash Core may refuse withdrawal with error `bad-assetunlock-not-active-quorum` even if quorum is active.
+
+Limits for withdrawals has been increased to flat 2000 Dash per 576 latest blocks.

src/chainparams.cpp

@@ -812,9 +812,9 @@ class CRegTestParams : public CChainParams {
         consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11;
         consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 0;
         consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
-        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 300;
-        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 300 / 5 * 4;     // 80% of 12
-        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 300 / 5 * 3;       // 60% of 7
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 600;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 600 / 5 * 4;     // 80% of window size
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 600 / 5 * 3;       // 60% of window size
         consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5;          // this corresponds to 10 periods
         consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true;
 

src/evo/assetlocktx.cpp

@@ -138,7 +138,10 @@ bool CAssetUnlockPayload::VerifySig(const llmq::CQuorumManager& qman, const uint
     }
 
     const auto quorum = qman.GetQuorum(llmqType, quorumHash);
-    assert(quorum);
+    // quorum must be valid at this point. Let's check and throw error just in case
+    if (!quorum) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "internal-error");
+    }
 
     const uint256 requestId = ::SerializeHash(std::make_pair(ASSETUNLOCK_REQUESTID_PREFIX, index));
 

src/evo/creditpool.cpp

@@ -134,9 +134,13 @@ CCreditPool CCreditPoolManager::ConstructCreditPool(const CBlockIndex* const blo
     if (!block) {
         // If reading of previous block is not successfully, but
         // prev contains credit pool related data, something strange happened
-        assert(prev.locked == 0);
-        assert(prev.indexes.IsEmpty());
-
+        if (prev.locked != 0) {
+            throw std::runtime_error(strprintf("Failed to create CreditPool but previous block has value"));
+        }
+        if (!prev.indexes.IsEmpty()) {
+            throw std::runtime_error(
+                strprintf("Failed to create CreditPool but asset unlock transactions already mined"));
+        }
         CCreditPool emptyPool;
         AddToCache(block_index->GetBlockHash(), block_index->nHeight, emptyPool);
         return emptyPool;
@@ -171,22 +175,30 @@ CCreditPool CCreditPoolManager::ConstructCreditPool(const CBlockIndex* const blo
         }
     }
 
-    // Unlock limits are # max(100, min(.10 * assetlockpool, 1000)) inside window
     CAmount currentLimit = locked;
     const CAmount latelyUnlocked = prev.latelyUnlocked + blockData.unlocked - distantUnlocked;
-    if (currentLimit + latelyUnlocked > LimitAmountLow) {
-        currentLimit = std::max(LimitAmountLow, locked / 10) - latelyUnlocked;
-        if (currentLimit < 0) currentLimit = 0;
+    if (DeploymentActiveAt(*block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS)) {
+        currentLimit = std::min(currentLimit, LimitAmountV22);
+    } else {
+        // Unlock limits in pre-v22 are max(100, min(.10 * assetlockpool, 1000)) inside window
+        if (currentLimit + latelyUnlocked > LimitAmountLow) {
+            currentLimit = std::max(LimitAmountLow, locked / 10) - latelyUnlocked;
+            if (currentLimit < 0) currentLimit = 0;
+        }
+        currentLimit = std::min(currentLimit, LimitAmountHigh - latelyUnlocked);
     }
-    currentLimit = std::min(currentLimit, LimitAmountHigh - latelyUnlocked);
 
-    assert(currentLimit >= 0);
+    if (currentLimit != 0 || latelyUnlocked > 0 || locked > 0) {
+        LogPrint(BCLog::CREDITPOOL, /* Continued */
+                 "CCreditPoolManager: asset unlock limits on height: %d locked: %d.%08d limit: %d.%08d "
+                 "unlocked-in-window: %d.%08d\n",
+                 block_index->nHeight, locked / COIN, locked % COIN, currentLimit / COIN, currentLimit % COIN,
+                 latelyUnlocked / COIN, latelyUnlocked % COIN);
+    }
 
-    if (currentLimit > 0 || latelyUnlocked > 0 || locked > 0) {
-        LogPrint(BCLog::CREDITPOOL, "CCreditPoolManager: asset unlock limits on height: %d locked: %d.%08d limit: %d.%08d previous: %d.%08d\n",
-               block_index->nHeight, locked / COIN, locked % COIN,
-               currentLimit / COIN, currentLimit % COIN,
-               latelyUnlocked / COIN, latelyUnlocked % COIN);
+    if (currentLimit < 0) {
+        throw std::runtime_error(
+            strprintf("Negative limit for CreditPool: %d.%08d\n", currentLimit / COIN, currentLimit % COIN));
     }
 
     CCreditPool pool{locked, currentLimit, latelyUnlocked, indexes};

src/evo/creditpool.h

@@ -117,6 +117,7 @@ class CCreditPoolManager
     static constexpr int LimitBlocksToTrace = 576;
     static constexpr CAmount LimitAmountLow = 100 * COIN;
     static constexpr CAmount LimitAmountHigh = 1000 * COIN;
+    static constexpr CAmount LimitAmountV22 = 2000 * COIN;
 
     explicit CCreditPoolManager(CEvoDB& _evoDb);
 

test/functional/feature_asset_locks.py

@@ -273,7 +273,7 @@ def run_test(self):
         self.test_asset_unlocks(node_wallet, node, pubkey)
         self.test_withdrawal_limits(node_wallet, node, pubkey)
         self.test_mn_rr(node_wallet, node, pubkey)
-        self.test_withdrawal_fork(node_wallet, pubkey)
+        self.test_withdrawal_fork(node_wallet, node, pubkey)
 
 
     def test_asset_locks(self, node_wallet, node, pubkey):
@@ -466,7 +466,9 @@ def test_asset_unlocks(self, node_wallet, node, pubkey):
 
 
     def test_withdrawal_limits(self, node_wallet, node, pubkey):
-        self.log.info("Testing withdrawal limits...")
+        self.log.info("Testing withdrawal limits before v22 'withdrawal fork'...")
+        assert not softfork_active(node_wallet, 'withdrawals')
+
         self.log.info("Too big withdrawal is expected to not be mined")
         asset_unlock_tx_full = self.create_assetunlock(201, 1 + self.get_credit_pool_balance(), pubkey)
 
@@ -611,17 +613,18 @@ def test_withdrawal_limits(self, node_wallet, node, pubkey):
 
 
         self.log.info("generate many blocks to be sure that mempool is empty after expiring txes...")
-        self.generate_batch(60)
+        self.generate_batch(HEIGHT_DIFF_EXPIRING)
         self.log.info("Checking that credit pool is not changed...")
         assert_equal(new_total, self.get_credit_pool_balance())
         self.check_mempool_size()
+        assert not softfork_active(node_wallet, 'withdrawals')
 
 
     def test_mn_rr(self, node_wallet, node, pubkey):
         self.log.info("Activate mn_rr...")
         locked = self.get_credit_pool_balance()
         self.activate_mn_rr(expected_activation_height=2500)
-        self.log.info(f'height: {node.getblockcount()} credit: {self.get_credit_pool_balance()}')
+        self.log.info(f'mn-rr height: {node.getblockcount()} credit: {self.get_credit_pool_balance()}')
         assert_equal(locked, self.get_credit_pool_balance())
 
         bt = node.getblocktemplate()
@@ -645,9 +648,10 @@ def test_mn_rr(self, node_wallet, node, pubkey):
         self.generate(node, 1)
         assert_equal(locked, self.get_credit_pool_balance())
 
-    def test_withdrawal_fork(self, node_wallet, pubkey):
+    def test_withdrawal_fork(self, node_wallet, node, pubkey):
         self.log.info("Testing asset unlock after 'withdrawal' activation...")
         assert softfork_active(node_wallet, 'withdrawals')
+        self.log.info(f'post-withdrawals height: {node.getblockcount()} credit: {self.get_credit_pool_balance()}')
 
         index = 501
         while index < 511:
@@ -684,6 +688,21 @@ def test_withdrawal_fork(self, node_wallet, pubkey):
         self.mine_quorum_2_nodes(llmq_type_name="llmq_test_platform", llmq_type=106)
         self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': False, 'reject-reason': 'bad-assetunlock-too-old-quorum'})
 
+        asset_unlock_tx = self.create_assetunlock(520, 2000 * COIN + 1, pubkey)
+        txid_in_block = self.send_tx(asset_unlock_tx)
+        self.generate(node, 1)
+        self.ensure_tx_is_not_mined(txid_in_block)
+
+        asset_unlock_tx = self.create_assetunlock(521, 2000 * COIN, pubkey)
+        txid_in_block = self.send_tx(asset_unlock_tx)
+        self.generate(node, 1)
+        block = node.getblock(node.getbestblockhash())
+        assert txid_in_block in block['tx']
+
+        asset_unlock_tx = self.create_assetunlock(522, COIN, pubkey)
+        txid_in_block = self.send_tx(asset_unlock_tx)
+        self.generate(node, 1)
+        self.ensure_tx_is_not_mined(txid_in_block)
 
 if __name__ == '__main__':
     AssetLocksTest().main()