feat!: add an implementation of DIP 0027 Credit Asset Locks by knst · Pull Request #5026 · dashpay/dash

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

probably fine

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

src/chainparams.cpp

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.

`tx.GetHash()` exclude hash from serialization at all, while previous implementation assumes zeroes on sig's place

…be changed

Co-authored-by: UdjinM6 <[email protected]>

…ed classes

…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

ping re min fee

Co-authored-by: UdjinM6 <[email protected]>

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

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 and others added 14 commits July 15, 2023 01:18

more fixes accodringly to review

1e02bcf

fix inflation bug for credit outputs in assetlock tx

4ebfcc4

improved hash calculation

4ff8e75

hidden implementation of asset locks from tx_verify

4f60fb2

cleanup quorum_count

4e6e963

fix: revert hash calculation of AssetUnlock transaction

f2c2df2

`tx.GetHash()` exclude hash from serialization at all, while previous implementation assumes zeroes on sig's place

refactor: add more const for local variables that is not supposed to …

55dff94

…be changed

fix: apply suggestions from code review

3908739

Co-authored-by: UdjinM6 <[email protected]>

refactor: move trivial getters of CAsset{Lock,Unlock}Payload to header

128ef19

refactor: moved CSkipSet from CreditPool.h to separate file

23fc477

fix: code-style adjusting for methods naming in credit pool and relat…

95b8d19

…ed classes

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

fix: add missing #define guards for skip_set

54ae92d

UdjinM6 reviewed Jul 17, 2023

View reviewed changes

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

src/evo/creditpool.cpp Outdated

Copy link

UdjinM6 Jul 17, 2023

Choose a reason for hiding this comment

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

ping re min fee

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

0e793c6

Co-authored-by: UdjinM6 <[email protected]>

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

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

thephez reviewed Aug 3, 2023

View reviewed changes

src/evo/cbtx.h Show resolved Hide resolved

knst mentioned this pull request Aug 3, 2023

refactor: rename assetLockedAmount in CbTx to creditPoolBalance #5526

Merged

5 tasks

UdjinM6 mentioned this pull request Oct 20, 2023

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

Merged

5 tasks

Conversation

knst commented Sep 23, 2022 • 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!

PastaPastaPasta commented Sep 29, 2022

Uh oh!

Uh oh!

PastaPastaPasta Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 12, 2022

Uh oh!

Uh oh!

github-actions bot commented Nov 22, 2022

Uh oh!

Uh oh!

UdjinM6 Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Jul 20, 2023

Uh oh!

knst commented Jul 20, 2023

Uh oh!

UdjinM6 commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Jul 20, 2023 • 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!

ogabrielides left a comment

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!

6 participants

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