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.

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

feat: increased withdrawal limits to flat 2000 from v22 #6369

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Oct 28, 2024

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:

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

@knst knst added this to the 22 milestone Oct 28, 2024
@UdjinM6
Copy link

UdjinM6 commented Oct 28, 2024

clang format:

--- src/evo/creditpool.cpp	(before formatting)
+++ src/evo/creditpool.cpp	(after formatting)
@@ -187,10 +187,11 @@
     assert(currentLimit >= 0);
 
     if (currentLimit > 0 || latelyUnlocked > 0 || locked > 0) {
-        LogPrint(BCLog::CREDITPOOL, "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);
+        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);
     }
 
     CCreditPool pool{locked, currentLimit, latelyUnlocked, indexes};

block_index->nHeight, locked / COIN, locked % COIN,
currentLimit / COIN, currentLimit % COIN,
latelyUnlocked / COIN, latelyUnlocked % COIN);
LogPrint(BCLog::CREDITPOOL,
Copy link

@UdjinM6 UdjinM6 Oct 28, 2024

Choose a reason for hiding this comment

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

All calls to LogPrintf() and LogPrint() should be terminated with \n

src/evo/creditpool.cpp:        LogPrint(BCLog::CREDITPOOL,
Suggested change
LogPrint(BCLog::CREDITPOOL,
LogPrint(BCLog::CREDITPOOL, /* Continued */

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can we disable clang-format explicitely for LogPrint somehow?

UdjinM6
UdjinM6 previously approved these changes Oct 28, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK a51ade5

@knst
Copy link
Collaborator Author

knst commented Oct 28, 2024

tsan CI failed due to too large size of log.
Indeed, feature_asset_locks.py became the slowest functional tests with the biggest size of log after this PR.

Small optimizations are coming....

It is impossible situation which will never happen.
But better to change it to exception for better error-prune implementation
in case someone will decide to change this code:

    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-too-old-quorum");
    }
    ...
    const auto quorum = qman.GetQuorum(llmqType, quorumHash);
    assert(quorum); <-- for sure exist because we just scanned quorums
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK e43ca62

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK e43ca62

@PastaPastaPasta
Copy link
Member

Guix builds locally; gonna merge

@PastaPastaPasta PastaPastaPasta merged commit 79ced62 into dashpay:develop Oct 28, 2024
27 of 35 checks passed
@knst knst deleted the v22-withdrawal-limit branch October 28, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants