sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-26 04:22:55 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

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

Browse Source 
f44edde8fe tests: use only 2 MN and 2 Evo nodes in feature_asset_locks.py to be sure that is enough (Konstantin Akimov)
8286bdf611 fix: assert in signing_shares - amount of members can match with amount of attempts (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Currently we have several quorums which have size 3 with threshold 2 nodes: `llmq_test_instantsend`, `llmq_test_platform`, `llmq_test` and they are used on RegTest.

  For extreme case when only 2 nodes exist the assert happens:

  ```
    AssertionError: Unexpected stderr dashd: llmq/signing_shares.cpp:812: static CDeterministicMNCPtr llmq::CSigSharesManager::SelectMemberForRecovery(const llmq::CQuorumCPtr&, const uint256&, size_t): Assertion `size_t(attempt) < quorum->members.size()' failed.
    Posix Signal: Aborted
       0#: (0x5BF40CE70DA2) stl_vector.h:115       - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_copy_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data const&)
       1#: (0x5BF40CE70DA2) stl_vector.h:127       - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_swap_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data&)
       2#: (0x5BF40CE70DA2) stl_vector.h:1962      - std::vector<unsigned long, std::allocator<unsigned long> >::_M_move_assign(std::vector<unsigned long, std::allocator<unsigned long> >&&, std::integral_constant<bool, true>)
       3#: (0x5BF40CE70DA2) stl_vector.h:771       - std::vector<unsigned long, std::allocator<unsigned long> >::operator=(std::vector<unsigned long, std::allocator<unsigned long> >&&)
       4#: (0x5BF40CE70DA2) stacktraces.cpp:777    - HandlePosixSignal
       5#: (0x7664C9245320) libc_sigaction.c       - ???
       6#: (0x7664C929EB1C) pthread_kill.c:44      - __pthread_kill_implementation
       7#: (0x7664C929EB1C) pthread_kill.c:78      - __pthread_kill_internal
       8#: (0x7664C929EB1C) pthread_kill.c:89      - __GI___pthread_kill
       9#: (0x7664C924526E) raise.c:27             - __GI_raise
      10#: (0x7664C92288FF) abort.c:81             - __GI_abort
      11#: (0x7664C922881B) loadmsgcat.c:1177      - _nl_load_domain
      12#: (0x7664C923B507) <unknown-file>         - ???
      13#: (0x5BF40C6E88C8) signing_shares.cpp:823 - llmq::CSigSharesManager::SelectMemberForRecovery(std::shared_ptr<llmq::CQuorum const> const&, uint256 const&, unsigned long)
      14#: (0x5BF40C94A285) quorums.cpp:737        - operator()
      15#: (0x5BF40C94A514) std_function.h:292     - _M_invoke
      16#: (0x5BF40CE082C6) util.cpp:510           - RPCHelpMan::HandleRequest(JSONRPCRequest const&) const
      17#: (0x5BF40C89824A) univalue.h:17          - UniValue::operator=(UniValue&&)
      18#: (0x5BF40C89824A) server.h:108           - CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const
      19#: (0x5BF40C9976F4) std_function.h:591     - std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const
      20#: (0x5BF40C9976F4) server.cpp:622         - ExecuteCommand
      21#: (0x5BF40C99879F) server.cpp:511         - ExecuteCommands
      22#: (0x5BF40C99879F) server.cpp:543         - CRPCTable::execute(JSONRPCRequest const&) const
      23#: (0x5BF40CB75F24) httprpc.cpp:247        - HTTPReq_JSONRPC
  ```

  Discovered during implementation of https://github.com/dashpay/dash-issues/issues/77

  ## What was done?
  Changed condition in assert, implemented special case of using Nth element from array size N for `SelectMemberForRecovery`, added test for this case.

  ## How Has This Been Tested?
  Improved functional test `feature_asset_locks.py` to test this corner case for quorum `llmq_test_instantsend` and `llmq_test_platform`

  ## Breaking Changes
  N/A

  ## 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
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

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

Tree-SHA512: 5e8035960100778820bbbb242a8a354a27bce00c169bbebc4f3867737301dbdd3cc5993a8d806d55bf6eb467bfe1f36443087a271acc227ba7bb301c4f75bb7a
This commit is contained in:
pasta 2024-09-23 11:16:47 -05:00
commit 2b92b3e29e
No known key found for this signature in database
GPG Key ID: 52527BEDABE87984
4 changed files with 19 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/llmq/signing_shares.cpp
View File

@ -807,9 +807,9 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256&
sigman.ProcessRecoveredSig(rs); 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<std::pair<uint256, CDeterministicMNCPtr>> v; std::vector<std::pair<uint256, CDeterministicMNCPtr>> v;
v.reserve(quorum->members.size()); v.reserve(quorum->members.size());
@ -819,7 +819,7 @@ CDeterministicMNCPtr CSigSharesManager::SelectMemberForRecovery(const CQuorumCPt
} }
std::sort(v.begin(), v.end()); 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) void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToRequest)

2
src/llmq/signing_shares.h
View File

@ -434,7 +434,7 @@ public:
void HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) override; 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: private:
// all of these return false when the currently processed message should be aborted (as each message actually contains multiple messages) // all of these return false when the currently processed message should be aborted (as each message actually contains multiple messages)

2
src/rpc/quorums.cpp
View File

@ -732,7 +732,7 @@ static RPCHelpMan quorum_selectquorum()
ret.pushKV("quorumHash", quorum->qc->quorumHash.ToString()); ret.pushKV("quorumHash", quorum->qc->quorumHash.ToString());
UniValue recoveryMembers(UniValue::VARR); 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); auto dmn = llmq_ctx.shareman->SelectMemberForRecovery(quorum, id, i);
recoveryMembers.push_back(dmn->proTxHash.ToString()); recoveryMembers.push_back(dmn->proTxHash.ToString());
} }

21
test/functional/feature_asset_locks.py
View File

@ -47,7 +47,7 @@ blocks_in_one_day = 576
class AssetLocksTest(DashTestFramework): class AssetLocksTest(DashTestFramework):
def set_test_params(self): 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): def skip_test_if_missing_module(self):
self.skip_if_no_wallet() self.skip_if_no_wallet()
@ -229,6 +229,13 @@ class AssetLocksTest(DashTestFramework):
self.nodes[1].generate(batch) self.nodes[1].generate(batch)
self.sync_all() 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): def run_test(self):
node_wallet = self.nodes[0] node_wallet = self.nodes[0]
node = self.nodes[1] node = self.nodes[1]
@ -241,9 +248,9 @@ class AssetLocksTest(DashTestFramework):
self.nodes[0].sporkupdate("SPORK_2_INSTANTSEND_ENABLED", 0) self.nodes[0].sporkupdate("SPORK_2_INSTANTSEND_ENABLED", 0)
self.wait_for_sporks_same() 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) self.dynamically_add_masternode(evo=True)
node.generate(8) node.generate(8)
self.sync_blocks() self.sync_blocks()
@ -321,7 +328,7 @@ class AssetLocksTest(DashTestFramework):
self.create_and_check_block([extra_lock_tx], expected_error = 'bad-cbtx-assetlocked-amount') self.create_and_check_block([extra_lock_tx], expected_error = 'bad-cbtx-assetlocked-amount')
self.log.info("Mine a quorum...") 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) self.validate_credit_pool_balance(locked_1)
@ -407,7 +414,7 @@ class AssetLocksTest(DashTestFramework):
reason = "double copy") reason = "double copy")
self.log.info("Mining next quorum to check tx 'asset_unlock_tx_late' is still valid...") 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.log.info("Checking credit pool amount is same...")
self.validate_credit_pool_balance(locked - 1 * COIN) 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))}}) self.check_mempool_result(tx=asset_unlock_tx_late, result_expected={'allowed': True, 'fees': {'base': Decimal(str(tiny_amount / COIN))}})
@ -427,7 +434,7 @@ class AssetLocksTest(DashTestFramework):
result_expected={'allowed': False, 'reject-reason' : 'bad-assetunlock-too-late'}) 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.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.log.info("Expecting new reject-reason...")
self.check_mempool_result(tx=asset_unlock_tx_too_late, 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-not-active-quorum'})
@ -504,7 +511,7 @@ class AssetLocksTest(DashTestFramework):
self.log.info("Fast forward to the next day to reset all current unlock limits...") self.log.info("Fast forward to the next day to reset all current unlock limits...")
self.slowly_generate_batch(blocks_in_one_day) 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() total = self.get_credit_pool_balance()
coins = node_wallet.listunspent() coins = node_wallet.listunspent()