From 8286bdf611949e248dd5cc3e090634577512c070 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 5 Sep 2024 16:58:14 +0700 Subject: [PATCH 1/2] fix: assert in signing_shares - amount of members can match with amount of attempts Co-Authored-By: UdjinM6 --- src/llmq/signing_shares.cpp | 6 +++--- src/llmq/signing_shares.h | 2 +- src/rpc/quorums.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 3667e593bc3a9..da63536c9969a 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -807,9 +807,9 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256& sigman.ProcessRecoveredSig(rs); } -CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256 &id, size_t attempt) +CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256 &id, int attempt) { - assert(size_t(attempt) < quorum->members.size()); + assert(attempt < quorum->params.recoveryMembers); std::vector> v; v.reserve(quorum->members.size()); @@ -819,7 +819,7 @@ CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPt } std::sort(v.begin(), v.end()); - return v[attempt].second; + return v[attempt % v.size()].second; } void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map>& sigSharesToRequest) diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 3725d295be806..b1f0acbf310b7 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -434,7 +434,7 @@ class CSigSharesManager : public CRecoveredSigsListener void HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) override; - static CDeterministicMNCPtr SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256& id, size_t attempt); + static CDeterministicMNCPtr SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256& id, int attempt); private: // all of these return false when the currently processed message should be aborted (as each message actually contains multiple messages) diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index b33d42e4f4260..0ccff814a9465 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -732,7 +732,7 @@ static RPCHelpMan quorum_selectquorum() ret.pushKV("quorumHash", quorum->qc->quorumHash.ToString()); UniValue recoveryMembers(UniValue::VARR); - for (size_t i = 0; i < size_t(quorum->params.recoveryMembers); i++) { + for (int i = 0; i < quorum->params.recoveryMembers; ++i) { auto dmn = llmq_ctx.shareman->SelectMemberForRecovery(quorum, id, i); recoveryMembers.push_back(dmn->proTxHash.ToString()); } From f44edde8fea9d993c76b22033dcc458602fb3f04 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 17 Sep 2024 03:16:06 +0700 Subject: [PATCH 2/2] tests: use only 2 MN and 2 Evo nodes in feature_asset_locks.py to be sure that is enough --- test/functional/feature_asset_locks.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index 8dd11ecb12694..23b5d03e1fa15 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -47,7 +47,7 @@ class AssetLocksTest(DashTestFramework): def set_test_params(self): - self.set_dash_test_params(5, 3, [["-whitelist=127.0.0.1", "-llmqtestinstantsenddip0024=llmq_test_instantsend"]] * 5, evo_count=3) + self.set_dash_test_params(4, 2, [["-whitelist=127.0.0.1", "-llmqtestinstantsenddip0024=llmq_test_instantsend"]] * 4, evo_count=2) def skip_test_if_missing_module(self): self.skip_if_no_wallet() @@ -229,6 +229,13 @@ def slowly_generate_batch(self, count): self.nodes[1].generate(batch) self.sync_all() + # This functional test intentionally setup only 2 MN and only 2 Evo nodes + # to ensure that corner case of quorum with minimum amount of nodes as possible + # does not cause any issues in Dash Core + def mine_quorum_2_nodes(self, llmq_type_name, llmq_type): + self.mine_quorum(llmq_type_name=llmq_type_name, expected_members=2, expected_connections=1, expected_contributions=2, expected_commitments=2, llmq_type=llmq_type) + + def run_test(self): node_wallet = self.nodes[0] node = self.nodes[1] @@ -241,9 +248,9 @@ def run_test(self): self.nodes[0].sporkupdate("SPORK_2_INSTANTSEND_ENABLED", 0) self.wait_for_sporks_same() - self.mine_quorum(llmq_type_name='llmq_test_instantsend', llmq_type=104) + self.mine_quorum_2_nodes(llmq_type_name='llmq_test_instantsend', llmq_type=104) - for _ in range(3): + for _ in range(2): self.dynamically_add_masternode(evo=True) node.generate(8) self.sync_blocks() @@ -321,7 +328,7 @@ def test_asset_locks(self, node_wallet, node, pubkey): self.create_and_check_block([extra_lock_tx], expected_error = 'bad-cbtx-assetlocked-amount') self.log.info("Mine a quorum...") - self.mine_quorum(llmq_type_name='llmq_test_platform', llmq_type=106, expected_connections=2, expected_members=3, expected_contributions=3, expected_complaints=0, expected_justifications=0, expected_commitments=3 ) + self.mine_quorum_2_nodes(llmq_type_name='llmq_test_platform', llmq_type=106) self.validate_credit_pool_balance(locked_1) @@ -407,7 +414,7 @@ def test_asset_unlocks(self, node_wallet, node, pubkey): reason = "double copy") self.log.info("Mining next quorum to check tx 'asset_unlock_tx_late' is still valid...") - self.mine_quorum(llmq_type_name="llmq_test_platform", llmq_type=106) + self.mine_quorum_2_nodes(llmq_type_name='llmq_test_platform', llmq_type=106) self.log.info("Checking credit pool amount is same...") self.validate_credit_pool_balance(locked - 1 * COIN) self.check_mempool_result(tx=asset_unlock_tx_late, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}}) @@ -427,7 +434,7 @@ def test_asset_unlocks(self, node_wallet, node, pubkey): result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-too-late'}) self.log.info("Checking that two quorums later it is too late because quorum is not active...") - self.mine_quorum(llmq_type_name="llmq_test_platform", llmq_type=106) + self.mine_quorum_2_nodes(llmq_type_name='llmq_test_platform', llmq_type=106) self.log.info("Expecting new reject-reason...") self.check_mempool_result(tx=asset_unlock_tx_too_late, result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-not-active-quorum'}) @@ -504,7 +511,7 @@ def test_withdrawal_limits(self, node_wallet, node, pubkey): self.log.info("Fast forward to the next day to reset all current unlock limits...") self.slowly_generate_batch(blocks_in_one_day) - self.mine_quorum(llmq_type_name="llmq_test_platform", llmq_type=106) + self.mine_quorum_2_nodes(llmq_type_name='llmq_test_platform', llmq_type=106) total = self.get_credit_pool_balance() coins = node_wallet.listunspent()