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

knst · 2024-09-18T21:02:41Z

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:

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

thephez · 2024-09-23T12:47:36Z

src/chainparams.cpp

+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1704067200;   // January 1, 2024
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = 1767225600; // January 1, 2026


Start time and timeout values for each network should be updated to something more relevant before merging

nStartTime should be somewhere in the (near) future

nStartTime and nTimeout should be 1 year apart on mainnet

github-actions · 2024-09-23T16:18:21Z

This pull request has conflicts, please rebase.

PastaPastaPasta · 2024-09-27T16:59:14Z

Needs rebase?

knst · 2024-09-27T17:48:09Z

Needs rebase?

it depends on #6275 (bury mn_rr) also, which is not merged yet; will rebase after

github-actions · 2024-10-01T19:00:24Z

This pull request has conflicts, please rebase.

UdjinM6 · 2024-10-01T20:41:28Z

src/chainparams.cpp

+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1704067200;   // January 1, 2024
+        consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = 1767225600; // January 1, 2026


nStartTime should be somewhere in the (near) future

nStartTime and nTimeout should be 1 year apart on mainnet

UdjinM6 · 2024-10-01T20:41:47Z

src/evo/assetlocktx.cpp

+    assert(llmq_params_opt.has_value());
+
+    // after deployment WITHDRAWALS activated we check not to quorum, but all active quorums + 1 the latest inactive
+    const int quorums_to_scan = DeploymentActiveAt(*pindexTip, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS) ? (llmq_params_opt->signingActiveQuorumCount + 1) : 2;


could use keepOldConnections here

Suggested change

const int quorums_to_scan = DeploymentActiveAt(*pindexTip, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS) ? (llmq_params_opt->signingActiveQuorumCount + 1) : 2;

const int quorums_to_scan = DeploymentActiveAt(*pindexTip, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS) ? llmq_params_opt->keepOldConnections : 2;

could use keepOldConnections here

technically, yes

But logically I can't explain myself how signingActiveQuorumCount and keepOldConnections are related.

// Used for intra-quorum communication. This is the number of quorums for which we should keep old connections. // For non-rotated quorums it should be at least one more than the active quorums set. // For rotated quorums it should be equal to 2 x active quorums set. int keepOldConnections;

https://github.com/dashpay/dash/blob/develop/src/llmq/params.h#L102-L105

i.e. active set + 1 quorum that became inactive recently, exactly what you need here

I'd still says, that in some particular cases it is matched with "+1" indeed, but I can't justify use "keep old connections" as amount of quorums which makes signature valid; also "2x active quorums" for rotating is not relevant for this situation.

UdjinM6 · 2024-10-03T11:01:09Z

tests failed

…wals'

knst · 2024-10-08T08:30:46Z

@knst knst force-pushed the fix-withdrawal-quorums branch from 7bc3698 to 5fd7b07

fixed test, activation date and rebased to the newer commit - base commit was pretty old

UdjinM6

pls see https://github.com/UdjinM6/dash/commits/pr_6279/

knst · 2024-10-15T13:33:17Z

int CAssetUnlockPayload::getHeightToExpiry() const
{
    const auto& llmq_params_opt = Params().GetLLMQ(Params().GetConsensus().llmqTypePlatform);
    assert(llmq_params_opt.has_value());
    return requestedHeight + llmq_params_opt->dkgInterval * (llmq_params_opt->signingActiveQuorumCount + 1);
}

@UdjinM6 no, accordingly DIP and by design, getHeightToExpire is not related to the amount of active quorums at all, it's a mechanism to refuse withdrawal in a short time (48 blocks) if it has not been mined, for example due to limits, or due to small fee; so it can be re-issued without waiting 24+ hours

UdjinM6

pls see 0c8b377

UdjinM6

invalid syntax in while attempts < 10 (missing:), also pls see below

test/functional/feature_asset_locks.py

UdjinM6

utACK d690309

PastaPastaPasta · 2024-10-21T15:12:50Z

src/evo/assetlocktx.cpp

-        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-not-active-quorum");
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-too-old-quorum");


why change this?

because (active quorums + the latest inactive) are matched. So, quorum can be not-active, but still valid.

? (llmq_params_opt->signingActiveQuorumCount + 1)

PastaPastaPasta

utACK d690309

knst added this to the 22 milestone Sep 18, 2024

knst force-pushed the fix-withdrawal-quorums branch from 7b33395 to 1870106 Compare September 19, 2024 19:20

knst changed the title ~~WIP: fix withdrawal quorums~~ fix: new fork 'withdrawals' and fixes for asset unlock signature verification Sep 19, 2024

thephez reviewed Sep 23, 2024

View reviewed changes

github-actions bot added the needs rebase label Sep 23, 2024

knst force-pushed the fix-withdrawal-quorums branch from b5237f3 to 4a473a1 Compare September 30, 2024 08:55

github-actions bot removed the needs rebase label Sep 30, 2024

github-actions bot added the needs rebase label Oct 1, 2024

knst force-pushed the fix-withdrawal-quorums branch from 4a473a1 to 7bc3698 Compare October 1, 2024 19:10

knst added the Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. label Oct 1, 2024

knst requested review from PastaPastaPasta and UdjinM6 October 1, 2024 19:11

github-actions bot removed the needs rebase label Oct 1, 2024

UdjinM6 requested changes Oct 1, 2024

View reviewed changes

knst added 3 commits October 8, 2024 03:15

feat: new fork WITHDRAWALS introduced

0253438

fix: using proper quorums for asset unlock validation

2704003

chore: test new validation of asset unlocks tests after fork 'withdra…

5fd7b07

…wals'

knst force-pushed the fix-withdrawal-quorums branch from 7bc3698 to 5fd7b07 Compare October 7, 2024 20:16

knst requested a review from UdjinM6 October 8, 2024 09:58

UdjinM6 requested changes Oct 11, 2024

View reviewed changes

knst requested a review from UdjinM6 October 15, 2024 13:34

chore: clang format/reword/typos

6fbd128

UdjinM6 requested changes Oct 16, 2024

View reviewed changes

test: fix test_withdrawal_fork

ecd0a96

UdjinM6 requested changes Oct 17, 2024

View reviewed changes

test/functional/feature_asset_locks.py Outdated Show resolved Hide resolved

fix: limit amount of attempts for test test_withdrawal_fork

d690309

knst force-pushed the fix-withdrawal-quorums branch from fea8821 to d690309 Compare October 17, 2024 16:10

knst requested a review from UdjinM6 October 18, 2024 06:22

UdjinM6 approved these changes Oct 18, 2024

View reviewed changes

PastaPastaPasta reviewed Oct 21, 2024

View reviewed changes

PastaPastaPasta approved these changes Oct 21, 2024

View reviewed changes

PastaPastaPasta merged commit b7d5430 into dashpay:develop Oct 21, 2024
34 of 39 checks passed

knst deleted the fix-withdrawal-quorums branch October 24, 2024 15:14

		consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nStartTime = 1704067200; // January 1, 2024
		consensus.vDeployments[Consensus::DEPLOYMENT_WITHDRAWALS].nTimeout = 1767225600; // January 1, 2026

	const int quorums_to_scan = DeploymentActiveAt(*pindexTip, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS) ? (llmq_params_opt->signingActiveQuorumCount + 1) : 2;
	const int quorums_to_scan = DeploymentActiveAt(*pindexTip, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS) ? llmq_params_opt->keepOldConnections : 2;

		return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-not-active-quorum");
		return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-too-old-quorum");

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

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

Uh oh!

Conversation

knst commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 23, 2024

Uh oh!

PastaPastaPasta commented Sep 27, 2024

Uh oh!

knst commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Oct 3, 2024

Uh oh!

knst commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst commented Oct 15, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knst commented Sep 18, 2024 •

edited

Loading

knst commented Sep 27, 2024 •

edited

Loading

knst commented Oct 8, 2024 •

edited

Loading