Merge #6279: fix: new fork 'withdrawals' and fixes for asset unlock signature verification

d690309513 fix: limit amount of attempts for test `test_withdrawal_fork` (Konstantin Akimov)
ecd0a96f63 test: fix test_withdrawal_fork (UdjinM6)
6fbd128947 chore: clang format/reword/typos (UdjinM6)
5fd7b07ddc chore: test new validation of asset unlocks tests after fork 'withdrawals' (Konstantin Akimov)
27040030e9 fix: using proper quorums for asset unlock validation (Konstantin Akimov)
0253438503 feat: new fork WITHDRAWALS introduced (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  https://github.com/dashpay/dash-issues/issues/83

  ## What was done?
  Introduces new fork "withdrawals" which let Asset Unlock be valid for any active quorum + 1 extra inactive (in opposite of hard-coded 2 of them).

  ## How Has This Been Tested?
  See new test section `test_withdrawals_fork` in functional test feature_asset_locks.py

  ## Breaking Changes
   - new fork "withdrawals"
   - new logic of validation of Asset Unlock transactions's signature. Even no asset-unlock is mined yet, previous version of clients will refuse blocks which is fine for current system

  ## 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:
  UdjinM6:
    utACK d690309513
  PastaPastaPasta:
    utACK d690309513

Tree-SHA512: cac28cf974e97d4a4100c22d5aa07b398b15413235383c68a0f4ff005005b28892c7ac8e6b703582c126cb6e0ff80a950f190dd32268655fac534ed6f2a90d03

src/chainparams.cpp

@@ -209,6 +209,15 @@ public:
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1728864000;   // October 14, 2024
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = 1760400000; // October 14, 2025
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 4032;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 3226;     // 80% of 4032
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 2420;       // 60% of 4032
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5;          // this corresponds to 10 periods
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true;
+
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000988117deadb0db9cd5b8"); // 2109672
 
@@ -395,6 +404,15 @@ public:
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1728864000;   // October 14, 2024
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 100;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 80;     // 80% of 100
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 60;       // 60% of 100
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5;          // this corresponds to 10 periods
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true;
+
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x000000000000000000000000000000000000000000000000031779704a0f54b4"); // 1069875
 
@@ -556,6 +574,15 @@ public:
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].bit = 11;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1704067200;   // January 1, 2024
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nWindowSize = 120;
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdStart = 80;     // 80% of 100
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nThresholdMin = 60;       // 60% of 100
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nFalloffCoeff = 5;          // this corresponds to 10 periods
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true;
+
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x000000000000000000000000000000000000000000000000000000000000000");
 
@@ -782,6 +809,15 @@ public:
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
         consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].min_activation_height = 0; // No activation delay
 
+        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].nFalloffCoeff = 5;          // this corresponds to 10 periods
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].useEHF = true;
+
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x00");
 

src/consensus/params.h

@@ -39,6 +39,7 @@ constexpr bool ValidDeployment(BuriedDeployment dep) { return dep <= DEPLOYMENT_
 
 enum DeploymentPos : uint16_t {
     DEPLOYMENT_TESTDUMMY,
+    DEPLOYMENT_WITHDRAWALS, // Deployment of Fix for quorum selection for withdrawals
     // NOTE: Also add new deployments to VersionBitsDeploymentInfo in deploymentinfo.cpp
     MAX_VERSION_BITS_DEPLOYMENTS
 };

src/deploymentinfo.cpp

@@ -11,6 +11,10 @@ const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_B
         /*.name =*/ "testdummy",
         /*.gbt_force =*/ true,
     },
+    {
+        /*.name =*/"withdrawals",
+        /*.gbt_force =*/true,
+    },
 };
 
 std::string DeploymentName(Consensus::BuriedDeployment dep)

src/evo/assetlocktx.cpp

@@ -12,6 +12,7 @@
 #include <chainparams.h>
 #include <consensus/params.h>
 #include <consensus/validation.h>
+#include <deploymentstatus.h>
 #include <logging.h>
 #include <tinyformat.h>
 #include <util/ranges_set.h>
@@ -113,13 +114,21 @@ bool CAssetUnlockPayload::VerifySig(const llmq::CQuorumManager& qman, const uint
     // and at the quorumHash must be active in either the current or previous quorum cycle
     // and the sig must validate against that specific quorumHash.
 
+
     Consensus::LLMQType llmqType = Params().GetConsensus().llmqTypePlatform;
 
-    // We check at most 2 quorums
+    const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
-    const auto quorums = qman.ScanQuorums(llmqType, pindexTip, 2);
+    assert(llmq_params_opt.has_value());
+
+    // We check two quorums before DEPLOYMENT_WITHDRAWALS activation
+    // and "all active quorums + 1 the latest inactive" after activation.
+    const int quorums_to_scan = DeploymentActiveAt(*pindexTip, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS)
+                                    ? (llmq_params_opt->signingActiveQuorumCount + 1)
+                                    : 2;
+    const auto quorums = qman.ScanQuorums(llmqType, pindexTip, quorums_to_scan);
 
     if (bool isActive = std::any_of(quorums.begin(), quorums.end(), [&](const auto &q) { return q->qc->quorumHash == quorumHash; }); !isActive) {
-        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-not-active-quorum");
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-too-old-quorum");
     }
 
     if (static_cast<uint32_t>(pindexTip->nHeight) < requestedHeight || pindexTip->nHeight >= getHeightToExpiry()) {

src/rpc/blockchain.cpp

@@ -1841,6 +1841,7 @@ RPCHelpMan getblockchaininfo()
     SoftForkDescPushBack(tip, softforks, consensusParams, Consensus::DEPLOYMENT_V19);
     SoftForkDescPushBack(tip, softforks, consensusParams, Consensus::DEPLOYMENT_V20);
     SoftForkDescPushBack(tip, softforks, consensusParams, Consensus::DEPLOYMENT_MN_RR);
+    SoftForkDescPushBack(tip, ehfSignals, softforks, consensusParams, Consensus::DEPLOYMENT_WITHDRAWALS);
     SoftForkDescPushBack(tip, ehfSignals, softforks, consensusParams, Consensus::DEPLOYMENT_TESTDUMMY);
 
     obj.pushKV("softforks", softforks);

test/functional/feature_asset_locks.py

@@ -38,12 +38,14 @@ from test_framework.util import (
     assert_equal,
     assert_greater_than,
     assert_greater_than_or_equal,
+    softfork_active,
 )
 from test_framework.wallet_util import bytes_to_wif
 
 llmq_type_test = 106 # LLMQType::LLMQ_TEST_PLATFORM
 tiny_amount = int(Decimal("0.0007") * COIN)
 blocks_in_one_day = 576
+HEIGHT_DIFF_EXPIRING = 48
 
 class AssetLocksTest(DashTestFramework):
     def set_test_params(self):
@@ -271,6 +273,7 @@ class AssetLocksTest(DashTestFramework):
         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)
 
 
     def test_asset_locks(self, node_wallet, node, pubkey):
@@ -345,7 +348,7 @@ class AssetLocksTest(DashTestFramework):
         asset_unlock_tx_duplicate_index = copy.deepcopy(asset_unlock_tx)
         # modify this tx with duplicated index to make a hash of tx different, otherwise tx would be refused too early
         asset_unlock_tx_duplicate_index.vout[0].nValue += COIN
-        too_late_height = node.getblockcount() + 48
+        too_late_height = node.getblockcount() + HEIGHT_DIFF_EXPIRING
 
         self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})
         self.check_mempool_result(tx=asset_unlock_tx_too_big_fee,
@@ -430,8 +433,9 @@ class AssetLocksTest(DashTestFramework):
         self.log.info("Checking that two quorums later it is too late because quorum is not active...")
         self.mine_quorum_2_nodes(llmq_type_name='llmq_test_platform', llmq_type=106)
         self.log.info("Expecting new reject-reason...")
+        assert not softfork_active(self.nodes[0], 'withdrawals')
         self.check_mempool_result(tx=asset_unlock_tx_too_late,
-                result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-not-active-quorum'})
+                result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-too-old-quorum'})
 
         block_to_reconsider = node.getbestblockhash()
         self.log.info("Test block invalidation with asset unlock tx...")
@@ -445,7 +449,8 @@ class AssetLocksTest(DashTestFramework):
         self.validate_credit_pool_balance(locked - 2 * COIN)
 
         self.log.info("Forcibly mining asset_unlock_tx_too_late and ensure block is invalid")
-        self.create_and_check_block([asset_unlock_tx_too_late], expected_error = "bad-assetunlock-not-active-quorum")
+        assert not softfork_active(self.nodes[0], 'withdrawals')
+        self.create_and_check_block([asset_unlock_tx_too_late], expected_error = "bad-assetunlock-too-old-quorum")
 
         self.generate(node, 1)
 
@@ -636,6 +641,45 @@ class AssetLocksTest(DashTestFramework):
         self.generate(node, 1)
         assert_equal(locked, self.get_credit_pool_balance())
 
+    def test_withdrawal_fork(self, node_wallet, pubkey):
+        self.log.info("Testing asset unlock after 'withdrawal' activation...")
+        assert softfork_active(node_wallet, 'withdrawals')
+
+        index = 501
+        while index < 511:
+            self.log.info(f"Generating new Asset Unlock tx, index={index}...")
+            asset_unlock_tx = self.create_assetunlock(index, COIN, pubkey)
+            asset_unlock_tx_payload = CAssetUnlockTx()
+            asset_unlock_tx_payload.deserialize(BytesIO(asset_unlock_tx.vExtraPayload))
+
+            self.log.info("Check that Asset Unlock tx is valid for current quorum")
+            self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})
+
+            quorumHash_str = format(asset_unlock_tx_payload.quorumHash, '064x')
+            assert quorumHash_str in node_wallet.quorum('list')['llmq_test_platform']
+
+            while quorumHash_str != node_wallet.quorum('list')['llmq_test_platform'][-1]:
+                self.log.info("Generate one more quorum until signing quorum becomes the last one in the list")
+                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': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})
+
+            self.log.info("Generate one more quorum after which signing quorum is gone but Asset Unlock tx is still valid")
+            assert quorumHash_str in node_wallet.quorum('list')['llmq_test_platform']
+            self.mine_quorum_2_nodes(llmq_type_name="llmq_test_platform", llmq_type=106)
+            assert quorumHash_str not in node_wallet.quorum('list')['llmq_test_platform']
+
+            if asset_unlock_tx_payload.requestedHeight + HEIGHT_DIFF_EXPIRING > node_wallet.getblockcount():
+                self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})
+                break
+            else:
+                self.check_mempool_result(tx=asset_unlock_tx, result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-too-late'})
+                self.log.info("Asset Unlock tx expired, let's try again...")
+                index += 1
+
+        self.log.info("Generate one more quorum after which signing quorum becomes too old")
+        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'})
+
 
 if __name__ == '__main__':
     AssetLocksTest().main()

test/functional/rpc_blockchain.py

@@ -173,6 +173,17 @@ class BlockchainTest(BitcoinTestFramework):
             'v19': { 'type': 'buried', 'active': False, 'height': 900},
             'v20': { 'type': 'buried', 'active': False, 'height': 900},
             'mn_rr': { 'type': 'buried', 'active': False, 'height': 900},
+            'withdrawals': {
+                'type': 'bip9',
+                'bip9': {
+                    'status': 'defined',
+                    'start_time': 0,
+                    'timeout': 9223372036854775807,  # "withdrawals" does not have a timeout so is set to the max int64 value
+                    'since': 0,
+                    'min_activation_height': 0,
+                    'ehf': True
+                },
+                'active': False},
             'testdummy': {
                 'type': 'bip9',
                 'bip9': {