[GH-#125] Add lock time to transaction #155

cheezus1 · 2017-11-30T10:25:23Z

…tored code and tests

thepiwo · 2017-11-30T13:36:47Z

apps/aecore/config/dev.exs

  peers_max_count: 4
+
+config :aecore, :tx_data,
+  lock_time_block: 10


name it lock_time_coinbase

thepiwo · 2017-11-30T13:37:34Z

apps/aecore/lib/aecore/chain/chain_state.ex


-        update_block_state(updated_block_state, transaction.data.to_acc, transaction.data.value, 0)
+        add_to_amount = latest_block_height + 1 !=
+          transaction.data.lock_time_block - Application.get_env(:aecore, :tx_data)[:lock_time_block]


don't get the env here, pass it to the function

thepiwo · 2017-11-30T13:39:20Z

apps/aecore/lib/aecore/chain/chain_state.ex

-    chain_state |>
-      Enum.map(fn{_account, data} -> data.balance end) |>
-      Enum.sum()
+    chain_state


maybe return a tuple of {total_tokens, total_unlocked_tokens, total_locked_tokens}

thepiwo

can we add a test to check if locked token can't actually be spent

cytadela8

Generally good, but please address mine and thepiwo comments.

cytadela8 · 2017-12-07T13:49:36Z

apps/aecore/lib/aecore/chain/chain_state.ex

+                {amount_update_value + amount, updated_locked}
+
+              true ->
+                {amount_update_value, updated_locked}


This shouldn't happen. Let's log an error here.

cytadela8 · 2017-12-07T13:51:31Z

apps/aecore/lib/aecore/chain/chain_state.ex

+  @spec update_chain_state_locked(map(), integer()) :: map()
+  def update_chain_state_locked(chain_state, new_block_height) do
+    Enum.reduce(chain_state, %{}, fn({account, %{balance: balance, nonce: nonce, locked: locked}}, acc) ->
+      {locked_amount, updated_locked} =


Let's call locked_amount: unlocked_amount or just_unlocked_amount or to_unlock_amount to be less confusing.

cytadela8 · 2017-12-07T13:56:30Z

apps/aecore/lib/aecore/chain/chain_state.ex


-    new_account_state = %{balance: block_state_filled_empty[account].balance + value,
-                          nonce:   new_nonce}
+    new_locked = if(value > 0) do


If add_to_amount is true we add value to account twice (we duplicate tokens!). One's on current block and again on lock_time_block!

What we should do instead is check add_to_amount and do one of two things: add to balance or add to locked.

That's fixed

Ok, so if value < 0 then add_to_amount should be always true. If add_to_amount is false then the call is a no-op and we should produce an error to logs or maybe raise some exception or assert? @thepiwo What should we do if there in such a case (what is "the-best-practise")?

I guess we should raise an exception, but why handle the case at all?

@cheezus1 here is the answer

cytadela8 · 2017-12-07T14:04:04Z

apps/aecore/test/aecore_chain_state_test.exs

-      ChainState.calculate_block_state(block.txs)
+    assert %{"a" => %{balance: -9, nonce: 102,
+                      locked: [%{amount: 10, block: lock_time_block}]},
+             "b" => %{nonce: 1, balance: -11,


I see that's the same as before, but why do we actually get negative balances?

whoops, good question

Because amounts are spent this block, but the received amounts for each are locked for one block? Correct me if I am wrong

@cytadela8 this is just the block_state indicating the change this block makes to the chain_state, negative values occur for every token spent.

Ohh, right. This is not the chain_state after the block. It's delta chain_state.

thepiwo · 2017-12-12T10:03:19Z

@cheezus1 is there a test to check if locked token can't actually be spent?

cytadela8

.

cytadela8

One last issue.

cytadela8 · 2017-12-12T18:22:16Z

apps/aecore/lib/aecore/chain/chain_state.ex


-    new_account_state = %{balance: block_state_filled_empty[account].balance + value,
-                          nonce:   new_nonce}
+    new_locked = if(value > 0) do


Ok, so if value < 0 then add_to_amount should be always true. If add_to_amount is false then the call is a no-op and we should produce an error to logs or maybe raise some exception or assert? @thepiwo What should we do if there in such a case (what is "the-best-practise")?

…ate_block_state

…into GH-125

cytadela8

All good now. Great work!

cytadela8

One question + can we add a test to check if locked coins can't be spend and after locktime they can?

cytadela8 · 2017-12-15T11:02:27Z

apps/aecore/test/multiple_transactions_test.exs

-                          Map.get(Chain.chain_state, account1_pub_key, %{nonce: 0}).nonce + 1, 1)
+                          Map.get(Chain.chain_state, account1_pub_key, %{nonce: 0}).nonce + 1, 1,
+                          Chain.latest_block().header.height +
+                            Application.get_env(:aecore, :tx_data)[:lock_time_coinbase] + 1)


Why do we add locktime to all of the transactions in tests?

thepiwo · 2017-12-18T09:00:35Z

apps/aecore/lib/aecore/keys/worker.ex

  @spec sign_tx(binary(), integer(), integer(), integer(), integer()) :: {:ok, %SignedTx{}}
-  def sign_tx(to_acc, value, nonce, fee, lock_time_block) do
+  def sign_tx(to_acc, value, nonce, fee,
+              lock_time_block \\ Chain.latest_block().header.height + 1) do


why do you want to lock a transaction by default? I would set the default to 0

…orks in all cases; added test for checking if locked amount is spendable

cytadela8 · 2017-12-18T13:40:45Z

@cheezus1 We still have merge conflicts

d-velev and others added 5 commits November 23, 2017 16:30

[GH-#125] Added lock_time_block to TxData

6b124b7

[GH-#125] Changed chain state calculation

bed3e5e

[GH-#125] Unlocking locked amounts in Chain State

c8e41c4

[GH-#125] Lock time block constant added to TxData

2284e9a

[GH-#125] Fixed bug when substracting amounts from chain state; Refac…

73d6393

…tored code and tests

cheezus1 requested review from andonov, cytadela8, meivantodorov and thepiwo November 30, 2017 10:25

thepiwo reviewed Nov 30, 2017

View reviewed changes

[GH-#125] Merged master

baceab9

cytadela8 suggested changes Dec 7, 2017

View reviewed changes

cheezus1 added 3 commits December 11, 2017 16:45

[GH-#125] Small refactoring

515260c

[GH-#125] Fixed Logger message

24658aa

[GH-#125] Fixed test

fd3d906

cytadela8 suggested changes Dec 12, 2017

View reviewed changes

cheezus1 added 2 commits December 15, 2017 12:18

[GH-#125] Added exception when not substracting negative value in upd…

4775633

…ate_block_state

Merge branch 'master' of https://github.com/aeternity/elixir-research …

b6a9911

…into GH-125

cytadela8 approved these changes Dec 15, 2017

View reviewed changes

cytadela8 reviewed Dec 15, 2017

View reviewed changes

thepiwo approved these changes Dec 15, 2017

View reviewed changes

andonov approved these changes Dec 16, 2017

View reviewed changes

meivantodorov approved these changes Dec 18, 2017

View reviewed changes

[GH-#125] Added default value for lock_time_block

6bfca9c

thepiwo reviewed Dec 18, 2017

View reviewed changes

cheezus1 added 3 commits December 18, 2017 14:16

[GH-#125] Set default value for lock_time_block as 0; lock time now w…

920d71a

…orks in all cases; added test for checking if locked amount is spendable

[GH-#125] Fixed travis error

e136659

[GH-#125] Merged master

f5315c4

[GH-#125] Merged master

2399170

thepiwo merged commit 740376c into master Dec 18, 2017

thepiwo deleted the GH-125 branch October 5, 2018 11:52

[GH-#125] Add lock time to transaction #155

[GH-#125] Add lock time to transaction #155

Uh oh!

Conversation

cheezus1 commented Nov 30, 2017 • edited by thepiwo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

thepiwo left a comment

Choose a reason for hiding this comment

Uh oh!

cytadela8 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cytadela8 Dec 12, 2017 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thepiwo commented Dec 12, 2017

Uh oh!

cytadela8 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!

cytadela8 left a comment

Choose a reason for hiding this comment

Uh oh!

cytadela8 Dec 12, 2017 • 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!

cytadela8 left a comment

Choose a reason for hiding this comment

Uh oh!

cytadela8 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cytadela8 commented Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cheezus1 commented Nov 30, 2017 •

edited by thepiwo

Loading

cytadela8 Dec 12, 2017 •

edited

Loading

cytadela8 left a comment •

edited

Loading

cytadela8 Dec 12, 2017 •

edited

Loading

cytadela8 left a comment •

edited

Loading