Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: assert in signing_shares for quorums with 3 members but 2 nodes only #6261

Merged
merged 2 commits into from
Sep 23, 2024
Merged

fix: assert in signing_shares for quorums with 3 members but 2 nodes only #6261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8286bdf
fix: assert in signing_shares - amount of members can match with amou…
knst Sep 5, 2024
f44edde
tests: use only 2 MN and 2 Evo nodes in feature_asset_locks.py to be …
knst Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/llmq/signing_shares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: clang format complains

Suggested change
CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256 &id, int attempt)
CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPtr& quorum, const uint256& id, int attempt)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix it if need to force-push again.

{
assert(size_t(attempt) < quorum->members.size());
assert(attempt < quorum->params.recoveryMembers);

std::vector<std::pair<uint256, CDeterministicMNCPtr>> v;
v.reserve(quorum->members.size());
Expand All @@ -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<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToRequest)
Expand Down
2 changes: 1 addition & 1 deletion src/llmq/signing_shares.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/rpc/quorums.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
21 changes: 14 additions & 7 deletions test/functional/feature_asset_locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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]
Expand All @@ -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()
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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))}})
Expand All @@ -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'})
Expand Down Expand Up @@ -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()
Expand Down
Loading