feat!: add an implementation of DIP 0027 Credit Asset Locks #5026

knst · 2022-09-23T16:20:19Z

Issue being fixed or feature implemented

This is an implementation of DIP0027 "Credit Asset Locks".
It's a mechanism to fluidly exchange between Dash and credits.

What was done?

This pull request includes:
- Asset Lock transaction
- Asset Unlock transaction (withdrawal)
- Credit Pool in coinbase
- Unit tests for Asset Lock/Unlock tx
- New functional test feature_asset_locks.py

RPC: currently locked amount (credit pool) is available through rpc call getblock.

How Has This Been Tested?

There added new unit tests for basic checks of transaction validity (asset lock/unlock).
Also added new functional test "feature_asset_locks.py" that cover typical cases, but not all corner cases yet.

Breaking Changes

This feature should be activated as soft-fork because:

It adds 2 new special transaction and one of them [asset unlock tx] requires update consensus rulels
It adds new data in coinbase tx (credit pool)

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
To release DIP 0027
I have assigned this pull request to a milestone

PastaPastaPasta · 2022-09-29T19:12:27Z

Please change PR title to follow conventional commits :)

src/chainparams.cpp

PastaPastaPasta · 2022-09-29T19:13:22Z

src/chainparams.cpp

+        // TODO or which one?
+        consensus.llmqTypeAssetLocks = Consensus::LLMQType::LLMQ_50_60;


probably fine

PastaPastaPasta · 2022-09-29T19:14:28Z

src/chainparams.cpp

        consensus.llmqTypeDIP0024InstantSend = Consensus::LLMQType::LLMQ_60_75;
        consensus.llmqTypePlatform = Consensus::LLMQType::LLMQ_100_67;
        consensus.llmqTypeMnhf = Consensus::LLMQType::LLMQ_50_60;
+        // TODO or which one?
+        consensus.llmqTypeAssetLocks = Consensus::LLMQType::LLMQ_50_60;


I'm confused... I thought all of these should be llmq_devnet...

github-actions · 2022-10-12T17:36:37Z

This pull request has conflicts, please rebase.

src/evo/assetlocktx.cpp

github-actions · 2022-11-22T17:35:34Z

This pull request has conflicts, please rebase.

…0 non-active

assert can be triggered if Add() is called without validation of SkipSet conditions

src/evo/specialtxman.cpp

UdjinM6 · 2023-07-17T14:21:55Z

src/evo/creditpool.cpp

+    }
+
+    index = assetUnlockTx.getIndex();
+    toUnlock = assetUnlockTx.getFee();


ping re min fee

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6

https://github.com/dashpay/dash/pull/5026/files#r1267324806

Now blocks mined and transaction are created by different nodes to be sure that transaction are relayed

UdjinM6 · 2023-07-20T15:26:05Z

Looks good! 👍

What about

Also would be nice to see a test for unlock txes with different fees (from 0 to >0.1 DASH).

?

knst · 2023-07-20T16:28:05Z

Also would be nice to see a test for unlock txes with different fees (from 0 to >0.1 DASH).

added just now, thanks for reminding!

UdjinM6 · 2023-07-20T20:01:04Z

Ok, so this

diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py
index 725d290f6e..32176e309c 100755
--- a/test/functional/feature_asset_locks.py
+++ b/test/functional/feature_asset_locks.py
@@ -295,7 +295,7 @@ class AssetLocksTest(DashTestFramework):
 
         self.log.info("Generating several txes by same quorum....")
         self.validate_credit_pool_amount(locked_1)
-        asset_unlock_tx = self.create_assetunlock(101, COIN, pubkey)
+        asset_unlock_tx = self.create_assetunlock(101, COIN, pubkey, fee=1)
         asset_unlock_tx_late = self.create_assetunlock(102, COIN, pubkey)
         asset_unlock_tx_too_late = self.create_assetunlock(103, COIN, pubkey)
         asset_unlock_tx_too_big_fee = self.create_assetunlock(104, COIN, pubkey, fee=int(Decimal("0.1") * COIN))

results in failures like

AssertionError: not([{'allowed': True, 'txid': '27d3357231f0f877d69a94d72d5f0e374a40fe8936974f868ba069439eb13aea'}] == [{'txid': '27d3357231f0f877d69a94d72d5f0e374a40fe8936974f868ba069439eb13aea', 'allowed': False, 'reject-reason': 'min relay fee not met'}])

That's exactly what I was worried about. 0f35ff3 should help. Or maybe we need to think more about unlocking vs min fees.

UdjinM6 · 2023-07-20T20:21:18Z

Why do we need to bypass limit? If fee is too low and tx is not mined - platform will generate new one after expiration. There's nothing special about asset-unlock. Tests works as I expected when implemented it...

For some txes such as quorum/etc there are nowhere to take reward/fee, so, checks are bypassed. But unlock txes are not special in this terms. If fee is too small - it's okay to request "more fee, please"

https://github.com/dashpay/dash/pull/5026/files#r1269915571

oh, so it's ok for unlock tx to fail? for example when fees are high or mempool is full

UdjinM6

utACK then :)

ogabrielides

re-utACK

PastaPastaPasta

utACK for squash merge

src/evo/cbtx.h

…nce` Bad naming is noticed in dashpay#5026 by thepez

…nce` Bad naming is noticed in dashpay#5026 by thephez

@shumkov

…5526) ## Issue being fixed or feature implemented Bad naming is noticed in #5026 by thephez ## What was done? Renamed `assetLockedAmount` in CbTx to `creditPoolBalance` Renamed also some local variables and functions to make it matched also. ## How Has This Been Tested? Run functional/unit tests - succeed Called python's rpc binding `node.getblock(block_hash)['cbTx']`: Got this result: ``` {'version': 3, 'height': 1556, 'merkleRootMNList': '978b2b4d1b884de62799b9eaee75c7812fea59f98f80d5ff9c963b0f0f195e14', 'merkleRootQuorums': 'bc7a34eb114f4e4bf38a11080b5d8ac41bdb36dd41e17467bae23c94ba06b013', 'bestCLHeightDiff': 0, 'bestCLSignature': '000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000', 'creditPoolBalance': Decimal('7.00141421')} ``` ## Breaking Changes Renamed `assetLockedAmount` in CbTx to `creditPoolBalance`. @shumkov be informed ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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

@shumkov

…ashpay#5526) ## Issue being fixed or feature implemented Bad naming is noticed in dashpay#5026 by thephez ## What was done? Renamed `assetLockedAmount` in CbTx to `creditPoolBalance` Renamed also some local variables and functions to make it matched also. ## How Has This Been Tested? Run functional/unit tests - succeed Called python's rpc binding `node.getblock(block_hash)['cbTx']`: Got this result: ``` {'version': 3, 'height': 1556, 'merkleRootMNList': '978b2b4d1b884de62799b9eaee75c7812fea59f98f80d5ff9c963b0f0f195e14', 'merkleRootQuorums': 'bc7a34eb114f4e4bf38a11080b5d8ac41bdb36dd41e17467bae23c94ba06b013', 'bestCLHeightDiff': 0, 'bestCLSignature': '000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000', 'creditPoolBalance': Decimal('7.00141421')} ``` ## Breaking Changes Renamed `assetLockedAmount` in CbTx to `creditPoolBalance`. @shumkov be informed ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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

## Issue being fixed or feature implemented The bug was introduced in the original PR #5026 and refactored later (which is good actually cause we shouldn't mix refactoring and bug-fixing :) ) ## What was done? fix conditions, add tests ## How Has This Been Tested? `feature_asset_locks.py` ## Breaking Changes n/a ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Sep 26, 2022

knst force-pushed the asset-locks branch from 9deaabf to 1664632 Compare September 29, 2022 18:48

knst changed the title ~~Added implementation of asset locks~~ Add an implementation of DIP 0027 Credit Asset Locks Sep 29, 2022

knst added this to the 19 milestone Sep 29, 2022

knst marked this pull request as ready for review September 29, 2022 18:57

knst requested a review from PastaPastaPasta September 29, 2022 18:58

PastaPastaPasta reviewed Sep 29, 2022

View reviewed changes

knst changed the title ~~Add an implementation of DIP 0027 Credit Asset Locks~~ feat!: add an implementation of DIP 0027 Credit Asset Locks Sep 29, 2022

knst force-pushed the asset-locks branch 3 times, most recently from 18e1141 to 16478ce Compare October 4, 2022 14:35

github-actions bot added the needs rebase label Oct 12, 2022

PastaPastaPasta requested a review from ogabrielides October 12, 2022 22:32

knst force-pushed the asset-locks branch from 739a86f to 6f47cb7 Compare November 1, 2022 17:58

shumkov reviewed Nov 2, 2022

View reviewed changes

src/evo/assetlocktx.cpp Outdated Show resolved Hide resolved

knst force-pushed the asset-locks branch from 6f47cb7 to 233c4d1 Compare November 11, 2022 06:09

github-actions bot removed the needs rebase label Nov 11, 2022

github-actions bot added the needs rebase label Nov 22, 2022

knst force-pushed the asset-locks branch from e5991fe to ad2f0f0 Compare November 24, 2022 16:58

knst marked this pull request as draft November 24, 2022 19:01

knst force-pushed the asset-locks branch 2 times, most recently from 53f5d52 to d2e9e20 Compare November 29, 2022 10:52

github-actions bot removed the needs rebase label Nov 29, 2022

knst marked this pull request as ready for review November 29, 2022 10:55

knst force-pushed the asset-locks branch from 57a3330 to 31b9736 Compare November 29, 2022 14:56

knst force-pushed the asset-locks branch 2 times, most recently from 879f60f to d894f13 Compare December 9, 2022 11:37

knst added 2 commits July 15, 2023 01:18

fix: fixing typos and improving comments. Fixing error message for v2…

5d29b06

…0 non-active

fix: replaced an assert to an exception in CSkipSet inside Add

2ea4251

assert can be triggered if Add() is called without validation of SkipSet conditions

knst force-pushed the asset-locks branch from ef5cec8 to 2ea4251 Compare July 14, 2023 18:25

knst requested review from ogabrielides and PastaPastaPasta July 14, 2023 18:25

fix: add missing #define guards for skip_set

54ae92d

UdjinM6 reviewed Jul 17, 2023

View reviewed changes

fix: removed duplicated return false from src/evo/specialtxman.cpp

0e793c6

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6 requested changes Jul 18, 2023

View reviewed changes

knst added 2 commits July 19, 2023 16:16

feat: check credit pool amount against all nodes in functional test

5534f64

feat: add an extra node with wallet for asset locks function test

dcd9262

Now blocks mined and transaction are created by different nodes to be sure that transaction are relayed

knst requested a review from UdjinM6 July 20, 2023 14:28

feat: new tests with asset-unlock tx absurdly high fee and zero fee

2c6aa49

UdjinM6 approved these changes Jul 20, 2023

View reviewed changes

ogabrielides approved these changes Jul 21, 2023

View reviewed changes

PastaPastaPasta approved these changes Jul 24, 2023

View reviewed changes

PastaPastaPasta merged commit 8a0e681 into dashpay:develop Jul 24, 2023

thephez reviewed Aug 3, 2023

View reviewed changes

src/evo/cbtx.h Show resolved Hide resolved

knst added a commit to knst/dash that referenced this pull request Aug 3, 2023

refactor: rename assetLockedAmount field in CbTx to `creditPoolBala…

d60cbd3

…nce` Bad naming is noticed in dashpay#5026 by thepez

knst added a commit to knst/dash that referenced this pull request Aug 3, 2023

refactor: rename assetLockedAmount field in CbTx to `creditPoolBala…

e638c00

…nce` Bad naming is noticed in dashpay#5026 by thephez

knst mentioned this pull request Aug 3, 2023

refactor: rename assetLockedAmount in CbTx to creditPoolBalance #5526

Merged

5 tasks

PastaPastaPasta added the Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. label Aug 30, 2023

UdjinM6 mentioned this pull request Oct 20, 2023

fix: actually show json for assetlock/unlock txes #5633

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add an implementation of DIP 0027 Credit Asset Locks #5026

feat!: add an implementation of DIP 0027 Credit Asset Locks #5026

knst commented Sep 23, 2022 •

edited

Loading

PastaPastaPasta commented Sep 29, 2022

PastaPastaPasta Sep 29, 2022

PastaPastaPasta Sep 29, 2022

github-actions bot commented Oct 12, 2022

github-actions bot commented Nov 22, 2022

UdjinM6 Jul 17, 2023

UdjinM6 left a comment •

edited

Loading

UdjinM6 commented Jul 20, 2023

knst commented Jul 20, 2023

UdjinM6 commented Jul 20, 2023 •

edited

Loading

UdjinM6 commented Jul 20, 2023 •

edited

Loading

UdjinM6 left a comment

ogabrielides left a comment

PastaPastaPasta left a comment

		// TODO or which one?
		consensus.llmqTypeAssetLocks = Consensus::LLMQType::LLMQ_50_60;

feat!: add an implementation of DIP 0027 Credit Asset Locks #5026

feat!: add an implementation of DIP 0027 Credit Asset Locks #5026

Conversation

knst commented Sep 23, 2022 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

PastaPastaPasta commented Sep 29, 2022

PastaPastaPasta Sep 29, 2022

Choose a reason for hiding this comment

PastaPastaPasta Sep 29, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 12, 2022

github-actions bot commented Nov 22, 2022

UdjinM6 Jul 17, 2023

Choose a reason for hiding this comment

UdjinM6 left a comment • edited Loading

Choose a reason for hiding this comment

UdjinM6 commented Jul 20, 2023

knst commented Jul 20, 2023

UdjinM6 commented Jul 20, 2023 • edited Loading

UdjinM6 commented Jul 20, 2023 • edited Loading

UdjinM6 left a comment

Choose a reason for hiding this comment

ogabrielides left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst commented Sep 23, 2022 •

edited

Loading

UdjinM6 left a comment •

edited

Loading

UdjinM6 commented Jul 20, 2023 •

edited

Loading

UdjinM6 commented Jul 20, 2023 •

edited

Loading