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

Add freezing capability to UTXO #326

Merged
merged 3 commits into from Sep 26, 2019
Merged

Add freezing capability to UTXO #326

merged 3 commits into from Sep 26, 2019

Conversation

MichaelKim20
Copy link
Member

@MichaelKim20 MichaelKim20 commented Sep 11, 2019

Add freezing capability to UTXO #204 of part

Transaction & UTXO in Freeze

The design was based on Definition of Done.
Illustrated the changing situation over time for Block / Transaction / UTXOSet.
Transaction & UTXO in Freeze

Implement validation when receive transaction 'freeze' in melted and , receive transaction 'payment' in frozen.(TX2, TX3 in upper diagram)
Implement validation when receive transaction 'payment' in various status (melting, melted)(TX4, TX5 in upper diagram)

@MichaelKim20 MichaelKim20 added the status-blocked label Sep 11, 2019
@codecov
Copy link

@codecov codecov bot commented Sep 12, 2019

Codecov Report

Merging #326 into v0.x.x will increase coverage by 1.03%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x     #326      +/-   ##
==========================================
+ Coverage   83.73%   84.76%   +1.03%     
==========================================
  Files          48       48              
  Lines        2766     2973     +207     
==========================================
+ Hits         2316     2520     +204     
- Misses        450      453       +3
Flag Coverage Δ
#unittests 84.76% <97.22%> (+1.03%) ⬆️
Impacted Files Coverage Δ
source/agora/test/Ledger.d 98.47% <ø> (ø) ⬆️
source/agora/node/Ledger.d 96.34% <100%> (+1.96%) ⬆️
source/agora/consensus/Validation.d 93.9% <95.13%> (+0.9%) ⬆️
source/agora/consensus/data/UTXOSet.d 88.05% <0%> (+1.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2844fa...a3e8cf4. Read the comment docs.

@MichaelKim20 MichaelKim20 changed the title Implement validation when receive transaction 'freeze', 'payment' in various status Add freezing capability to UTXO Sep 12, 2019
@MichaelKim20 MichaelKim20 added type-feature and removed status-blocked labels Sep 12, 2019
@MichaelKim20 MichaelKim20 added this to the 2. Validator milestone Sep 13, 2019
@MichaelKim20 MichaelKim20 added this to In progress (Max 4) in Kanban period (August-September 2019) via automation Sep 13, 2019
@MichaelKim20 MichaelKim20 removed this from In progress (Max 4) in Kanban period (August-September 2019) Sep 13, 2019
@MichaelKim20
Copy link
Member Author

@MichaelKim20 MichaelKim20 commented Sep 16, 2019

Ready for review

@MichaelKim20
Copy link
Member Author

@MichaelKim20 MichaelKim20 commented Sep 16, 2019

Copy link
Contributor

@Geod24 Geod24 left a comment

Wow okay this is a lot of code, we're going to have to break it down a bit.
I see some commit are doing back-and-forth, e.g. a duplicated method is added in one commit and removed 2 commits later, which is a red flag.
Additionally, some functions are doing the following:

void myFunc ()
{
   if (A)
   {

    }
    else
    {

     }
}

This essentially means you have 2 functions in one.

source/agora/consensus/data/UTXOSet.d Outdated Show resolved Hide resolved
source/agora/consensus/data/UTXOSet.d Outdated Show resolved Hide resolved
source/agora/consensus/Validation.d Outdated Show resolved Hide resolved
source/agora/consensus/Validation.d Outdated Show resolved Hide resolved
@MichaelKim20
Copy link
Member Author

@MichaelKim20 MichaelKim20 commented Sep 16, 2019

Wow okay this is a lot of code, we're going to have to break it down a bit.
I see some commit are doing back-and-forth, e.g. a duplicated method is added in one commit and removed 2 commits later, which is a red flag.
Additionally, some functions are doing the following:

void myFunc ()
{
   if (A)
   {

    }
    else
    {

     }
}

This essentially means you have 2 functions in one.

This may be due to the addition of a function to 'isInvalidReason' to remove redundant functions from 'UTXOSet.updateUTXOCache'.
It was to speed up the process.
But now it will be difficult to confirm, as the whole changed source is committed.

TxType type;

/// Previous transaction type
TxType prev_type;
Copy link
Contributor

@Geod24 Geod24 Sep 17, 2019

Choose a reason for hiding this comment

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

What is this field for ?

Copy link
Member Author

@MichaelKim20 MichaelKim20 Sep 17, 2019

Choose a reason for hiding this comment

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

Type of previous transaction.
To verify that the freeze is a UTXO just melted.

Copy link
Member Author

@MichaelKim20 MichaelKim20 Sep 17, 2019

Choose a reason for hiding this comment

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

Please refer to the table below.
Freezing status

@bpalaggi bpalaggi added this to Review/Testing (Max 2) in Sprint #6 (2019-09-17 to 2019-09-30) Sep 17, 2019
@Geod24 Geod24 removed this from the 2. Validator milestone Sep 17, 2019
@MichaelKim20
Copy link
Member Author

@MichaelKim20 MichaelKim20 commented Sep 18, 2019

@Geod24 Ready for review

@Geod24
Copy link
Contributor

@Geod24 Geod24 commented Sep 18, 2019

@MukeunKim : I think the first 3 commits can go in their own PR and be merged already, if you want.

@Geod24
Copy link
Contributor

@Geod24 Geod24 commented Sep 18, 2019

I'm not so sure about the last two commits. Do they belong to this PR ?

@MichaelKim20 MichaelKim20 added status-blocked and removed status-blocked labels Sep 18, 2019
@MichaelKim20
Copy link
Member Author

@MichaelKim20 MichaelKim20 commented Sep 19, 2019

The first three commit are already included in PR #337.
Please check there first.

@MichaelKim20
Copy link
Member Author

@MichaelKim20 MichaelKim20 commented Sep 23, 2019

Block_Tx_UTXO 001

Block_Tx_UTXO

FreezingAndUTXO

@MichaelKim20
Copy link
Member Author

@MichaelKim20 MichaelKim20 commented Sep 23, 2019

Ready for review ~

@MichaelKim20 MichaelKim20 requested a review from Geod24 Sep 23, 2019
Block height is used to check if it has changed from 'melting' to 'melted'

Co-Authored-By: HyeonyeobKim <hyeonyeob.kim@bpfkorea.org>
MichaelKim20 and others added 2 commits Sep 25, 2019
when receive transaction 'freeze' in melted
when receive transaction 'payment' in frozen
when receive transaction 'payment' in melting, melted

Co-Authored-By: HyeonyeobKim <hyeonyeob.kim@bpfkorea.org>
Co-Authored-By: HyeonyeobKim <hyeonyeob.kim@bpfkorea.org>
Geod24
Geod24 approved these changes Sep 26, 2019
Copy link
Contributor

@Geod24 Geod24 left a comment

LGTM. Any last comment @AndrejMitrovic ?

@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Sep 26, 2019

LGTM. Any last comment @AndrejMitrovic ?

Yes, I will review it now.

@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Sep 26, 2019

I suggest to add this to Amount.d:

struct Amount
{
    ...
    /// Exact amount that needs to be staked to make a freezing transaction
    public static immutable Amount FreezeAmount =
    Amount(UnitPerCoin.value * 40_000, true);
...

And also improving the validation function a little bit, to remove code duplication:

public string isInvalidReason (const Transaction tx, UTXOFinder findUTXO,
    const ulong height)
    @safe nothrow
{
    import std.conv;

    if (tx.inputs.length == 0)
        return "Transaction: No input";

    if (tx.outputs.length == 0)
        return "Transaction: No output";

    // disallow negative amounts
    foreach (output; tx.outputs)
        if (!output.value.isValid())
            return "Transaction: Output(s) overflow or underflow";

    const tx_hash = hashFull(tx);

    string isInvalidInput (Input input, ref UTXOSetValue utxo_value,
        ref Amount sum_unspent)
    {
        if (!findUTXO(input.previous, input.index, utxo_value))
            return "Transaction: Input ref not in UTXO";

        if (!utxo_value.output.address.verify(input.signature, tx_hash[]))
            return "Transaction: Input has invalid signature";

        if (!sum_unspent.add(utxo_value.output.value))
            return "Transaction: Input overflow";

        return null;
    }

    Amount sum_unspent;

    if (tx.type == TxType.Freeze)
    {
        foreach (input; tx.inputs)
        {
            UTXOSetValue utxo_value;
            if (auto fail_reason = isInvalidInput(input, utxo_value, sum_unspent))
                return fail_reason;

            if (utxo_value.type != TxType.Payment)
                return "Transaction: Can only freeze a Payment transaction";
        }

        if (sum_unspent != Amount.FreezeAmount)
            return "Transaction: Only available when the amount is 40,000";
    }
    else if (tx.type == TxType.Payment)
    {
        uint count_freeze = 0;
        foreach (input; tx.inputs)
        {
            UTXOSetValue utxo_value;
            if (auto fail_reason = isInvalidInput(input, utxo_value, sum_unspent))
                return fail_reason;

            // when status is frozen, it will begin to melt
            // In this case, all inputs must be frozen.
            if (utxo_value.type == TxType.Freeze)
                count_freeze++;

            // when status is (frozen->melting->melted) or (frozen->melting)
            if (utxo_value.type == TxType.Payment)
            {
                // when status is still melting
                if (height < utxo_value.unlock_height)
                    return "Transaction: Not available when melting UTXO";
            }
        }

        // current limitation: if any UTXO is frozen, they all must be frozen
        if ((count_freeze > 0) && (count_freeze != tx.inputs.length))
            return "Transaction: Rejected combined inputs (freeze & payment)";
    }
    else assert(0, "Unhandled transaction type: " ~ tx.to!string);

    Amount new_unspent;
    if (!tx.getSumOutput(new_unspent))
        return "Transaction: Referenced Output(s) overflow";

    if (!sum_unspent.sub(new_unspent))
        return "Transaction: Output(s) are higher than Input(s)";

    return null;
}

Here is the diff:

diff --git a/source/agora/common/Amount.d b/source/agora/common/Amount.d
index 26912a9b..911dca3c 100644
--- a/source/agora/common/Amount.d
+++ b/source/agora/common/Amount.d
@@ -41,6 +41,9 @@ public struct Amount
     /// Maximum amount of money that can ever be in circulation
     public static immutable Amount MaxUnitSupply =
         Amount(UnitPerCoin.value * MaxCoinSupply, true);
+    /// Exact amount that needs to be staked to make a freezing transaction
+    public static immutable Amount FreezeAmount =
+        Amount(UnitPerCoin.value * 40_000, true);
 
     /// Helper type for `toString`
     private alias SinkT = void delegate(scope const(char)[] v) @safe;
diff --git a/source/agora/consensus/Validation.d b/source/agora/consensus/Validation.d
index 0b35bfd8..fa16ff63 100644
--- a/source/agora/consensus/Validation.d
+++ b/source/agora/consensus/Validation.d
@@ -40,9 +40,12 @@ public alias UTXOFinder = scope bool delegate (Hash hash, size_t index,
 
 *******************************************************************************/
 
-public string isInvalidReason (const Transaction tx, UTXOFinder findUTXO, const ulong height)
+public string isInvalidReason (const Transaction tx, UTXOFinder findUTXO,
+    const ulong height)
     @safe nothrow
 {
+    import std.conv;
+
     if (tx.inputs.length == 0)
         return "Transaction: No input";
 
@@ -54,73 +57,76 @@ public string isInvalidReason (const Transaction tx, UTXOFinder findUTXO, const
         if (!output.value.isValid())
             return "Transaction: Output(s) overflow or underflow";
 
-    Amount sum_unspent;
-
     const tx_hash = hashFull(tx);
 
+    string isInvalidInput (Input input, ref UTXOSetValue utxo_value,
+        ref Amount sum_unspent)
+    {
+        if (!findUTXO(input.previous, input.index, utxo_value))
+            return "Transaction: Input ref not in UTXO";
+
+        if (!utxo_value.output.address.verify(input.signature, tx_hash[]))
+            return "Transaction: Input has invalid signature";
+
+        if (!sum_unspent.add(utxo_value.output.value))
+            return "Transaction: Input overflow";
+
+        return null;
+    }
+
+    Amount sum_unspent;
+
     if (tx.type == TxType.Freeze)
     {
         foreach (input; tx.inputs)
         {
-            // all referenced outputs must be present
             UTXOSetValue utxo_value;
-            if (!findUTXO(input.previous, input.index, utxo_value))
-                return "Transaction: Input ref not in UTXO";
-
-            if (!utxo_value.output.address.verify(input.signature, tx_hash[]))
-                return "Transaction: Input has invalid signature";
-
-            if (!sum_unspent.add(utxo_value.output.value))
-                return "Transaction: Input overflow";
+            if (auto fail_reason = isInvalidInput(input, utxo_value, sum_unspent))
+                return fail_reason;
 
             if (utxo_value.type != TxType.Payment)
-                return "Transaction: Only available freeze on TxType.Payment";
+                return "Transaction: Can only freeze a Payment transaction";
         }
 
-        if (sum_unspent != Amount(400_000_000_000L))
+        if (sum_unspent != Amount.FreezeAmount)
             return "Transaction: Only available when the amount is 40,000";
     }
     else if (tx.type == TxType.Payment)
     {
         uint count_freeze = 0;
-
         foreach (input; tx.inputs)
         {
-            // all referenced outputs must be present
             UTXOSetValue utxo_value;
-            if (!findUTXO(input.previous, input.index, utxo_value))
-                return "Transaction: Input ref not in UTXO";
+            if (auto fail_reason = isInvalidInput(input, utxo_value, sum_unspent))
+                return fail_reason;
 
-            if (!utxo_value.output.address.verify(input.signature, tx_hash[]))
-                return "Transaction: Input has invalid signature";
-
-            if (!sum_unspent.add(utxo_value.output.value))
-                return "Transaction: Input overflow";
-
-            //  when status is frozen, it will begin to melt
-            //  In this case, all inputs must be frozen.
+            // when status is frozen, it will begin to melt
+            // In this case, all inputs must be frozen.
             if (utxo_value.type == TxType.Freeze)
                 count_freeze++;
 
-            //  when status is (frozen->melting->melted) or (frozen->melting)
+            // when status is (frozen->melting->melted) or (frozen->melting)
             if (utxo_value.type == TxType.Payment)
             {
-                //  when status is still melting
+                // when status is still melting
                 if (height < utxo_value.unlock_height)
                     return "Transaction: Not available when melting UTXO";
             }
         }
 
-        //  In this case, all inputs must be frozen.
+        // current limitation: if any UTXO is frozen, they all must be frozen
         if ((count_freeze > 0) && (count_freeze != tx.inputs.length))
-            return "Transaction: TxType.Freeze and Payment are not handled together in Input";
+            return "Transaction: Rejected combined inputs (freeze & payment)";
     }
+    else assert(0, "Unhandled transaction type: " ~ tx.to!string);
 
     Amount new_unspent;
     if (!tx.getSumOutput(new_unspent))
         return "Transaction: Referenced Output(s) overflow";
+
     if (!sum_unspent.sub(new_unspent))
         return "Transaction: Output(s) are higher than Input(s)";
+
     return null;
 }

@@ -576,7 +578,7 @@ unittest
Transaction[] transactions;

// always use the same amount, for simplicity
const Amount AmountPerTx = Amount(40_000);
const Amount AmountPerTx = Amount(400_000_000_000L);
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Sep 26, 2019

Choose a reason for hiding this comment

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

Once the freeze amount is added to the Amount struct, it can be used here directly.

@AndrejMitrovic AndrejMitrovic merged commit a8d3f9f into bosagora:v0.x.x Sep 26, 2019
3 checks passed
Sprint #6 (2019-09-17 to 2019-09-30) automation moved this from Review/Testing (Max 2) to Done Sep 26, 2019
@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Sep 26, 2019

I will submit a PR that slightly improves the code.

@MichaelKim20 MichaelKim20 deleted the utxo-validation branch Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants