[GH-759] Channel Snapshot #791

gorbak25 · 2018-11-16T00:30:02Z

This PR implements channel snapshots and tests for these transactions.
This was made on top of #789.

Resolves #759

…SoloTx

[GH-491] Channel snapshot

…ring

…GH-759

cytadela8

Good code, but fixes needed.

cytadela8 · 2018-11-17T23:35:52Z

apps/aecore/lib/aecore/channel/channel_state_on_chain.ex

@@ -246,9 +271,9 @@ defmodule Aecore.Channel.ChannelStateOnChain do
             inspect(Poi.calculate_root_hash(poi))
           }"}

-        channel.sequence >= offchain_tx.sequence ->
+        channel.sequence > offchain_tx.sequence ->


Why was the check changed? I don't think equal sequence is a valid slash.

I checked in epoch. As stated in line 107 of aesc_utils.erl I believe we should have >= here.

I changed it in order to allow solo closing a channel which was snapshoted(E.g. Peers A and B open a channel, A transfers funds to B, peer B snapshots the state and then disappears). As we have the force progress transaction we can force an update and then solo close with the forced state, but this will require 3 txs instead of 2 txs.

cytadela8 · 2018-11-17T23:39:26Z

apps/aecore/lib/aecore/channel/tx/channel_deposit_tx.ex

@@ -221,8 +221,14 @@ defmodule Aecore.Channel.Tx.ChannelDepositTx do
      !ChannelStateOnChain.active?(channel) ->
        {:error, "#{__MODULE__}: Can't deposit from inactive channel."}

+      !ChannelStateOnChain.is_peer?(channel, depositing_account) ->


Why do you move this check from ChannelStateOnChain.validate_deposit to transaction?

In order to avoid passing the sender to the on chain channel code as it is used only in one place.

apps/aecore/lib/aecore/channel/channel_state_on_chain.ex

+      }) do
+    %ChannelStateOnChain{
+      channel
+      | sequence: sequence,


thepiwo · 2018-11-22T17:27:23Z

@cytadela8 can you approve?
@d-velev could you review as well?

d-velev · 2018-11-22T20:17:11Z

apps/aecore/lib/aecore/channel/tx/channel_snapshot_solo_tx.ex

+  - offchain_tx - off chain transaction mutually signed by both parties of the channel
+  """
+  defstruct [:channel_id, :offchain_tx]
+  use ExConstructor


Not needed I assume?

@grzegorz225 You might have copied this from some other code. We generally removed ExConstructor from many files as it only provides some functions that we don't use.

d-velev · 2018-11-22T20:23:18Z

apps/aecore/lib/aecore/channel/worker.ex

@@ -67,6 +68,11 @@ defmodule Aecore.Channel.Worker do
    closed(tx)
  end

+  def new_tx_mined(%SignedTx{data: %DataTx{type: ChannelSnapshotSoloTx}} = _tx) do


_tx not needed

cytadela8

Small comments, I just found while working with your code.

cytadela8 · 2018-11-22T22:47:19Z

apps/aecore/lib/aecore/channel/channel_state_on_chain.ex

@@ -147,6 +147,31 @@ defmodule Aecore.Channel.ChannelStateOnChain do
    {initiator_pubkey, responder_pubkey}
  end

+  @spec is_peer?(ChannelStateOnChain.t(), Keys.pubkey()) :: boolean()
+  def is_peer?(


"is_" is not needed. Boolean-ness is represented by "?". This follows the convention we have in our code. (see: active?, settled?)

cytadela8 · 2018-11-22T22:47:30Z

apps/aecore/lib/aecore/channel/channel_state_on_chain.ex

+  end
+
+  @spec is_peer_or_delegate?(ChannelStateOnChain.t(), Keys.pubkey()) :: boolean()
+  def is_peer_or_delegate?(


also remove "is_"

cytadela8 · 2018-11-23T20:32:05Z

apps/aecore/lib/aecore/channel/worker.ex

@@ -67,6 +68,11 @@ defmodule Aecore.Channel.Worker do
    closed(tx)
  end

+  def new_tx_mined(%SignedTx{data: %DataTx{type: ChannelSnapshotSoloTx}} = _tx) do
+    # ChannelSnapshotSoloTx requires no action


Untrue. You have snapshot sequence in ChannelStatePeer

gorbak25 and others added 21 commits August 10, 2018 23:15

[GH-491] SnapshotSoloTx implemented. Tests

09f7a0d

Merge remote-tracking branch 'origin/master' into GH-491

a6fee0d

[GH-491] More tests

7bb42f1

[GH-491] Changed comment for new_tx_mined handling of ChannelSnapshot…

78ac4bc

…SoloTx

[GH-491] More relevant error messages

fc11b32

[GH-491] Worker replies with pool error. Consistency fixes.

d64bb11

[GH-491] Consistency fix of filename

083daca

[GH-491] Fixed channels test setup

26454eb

[GH-491] Removed changes to mix.exs and mix.lock

2fdccc2

Updated README, implemented transaction blacklists

796ebf6

[GH-491] Small fix

e16afee

[GH-491] Removed transaction blacklist

1a36b28

[GH-491] Added ChannelSnapshotSoloTx to allowed transactions

de77959

Merge pull request #525 from aeternity/GH-491

8aa153a

[GH-491] Channel snapshot

Merge remote-tracking branch 'origin/master-channels' into GH-759

82efbd4

[GH-759] mix format

6ba26cf

[GH-759] Adjusted ChannelSnapshotSoloTx

9aebc2f

[GH-759] Adjusted ChannelStateOnChain, small Withdraw/deposit refacto…

d09837c

…ring

[GH-759] Adjusted ChannelStatePeer and Worker

968a735

[GH-759] Adjusted tests, more tests

6417a5b

[GH-759] Spellcheck

4e571ba

gorbak25 added blocked channel labels Nov 16, 2018

gorbak25 self-assigned this Nov 16, 2018

gorbak25 requested review from cytadela8, d-velev, thepiwo and DanielaIvanova November 16, 2018 00:30

gorbak25 added 2 commits November 16, 2018 18:33

[GH-759] Mix credo

a15f34d

Merge branch 'GH-759' of https://github.com/aeternity/elixir-node into …

a4f29ce

…GH-759

[GH-759] Dialyzer fix

e52b8b5

gorbak25 force-pushed the GH-759 branch from 3d4ee09 to e52b8b5 Compare November 16, 2018 21:09

cytadela8 reviewed Nov 17, 2018

View reviewed changes

gorbak25 added 4 commits November 20, 2018 01:51

[GH-759] Small changes

8b63df0

Merge remote-tracking branch 'origin/master' into GH-759

f519f76

[GH-759] Solo close with an snapshoted state

1f8baa0

Merge remote-tracking branch 'origin/master' into GH-759

1a5c469

cytadela8 approved these changes Nov 22, 2018

View reviewed changes

cytadela8 removed the blocked label Nov 22, 2018

d-velev reviewed Nov 22, 2018

View reviewed changes

cytadela8 reviewed Nov 22, 2018

View reviewed changes

cytadela8 mentioned this pull request Nov 22, 2018

[GH-758] ChannelMutalClose + ChannelSettle adjustments #799

Merged

cytadela8 reviewed Nov 23, 2018

View reviewed changes

DanielaIvanova approved these changes Nov 26, 2018

View reviewed changes

thepiwo merged commit 1a5c469 into master Nov 26, 2018

cytadela8 mentioned this pull request Nov 26, 2018

Channels style fixes (left from PR) #805

Open

thepiwo deleted the GH-759 branch November 30, 2018 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-759] Channel Snapshot #791

[GH-759] Channel Snapshot #791

gorbak25 commented Nov 16, 2018

cytadela8 left a comment

cytadela8 Nov 17, 2018

cytadela8 Nov 17, 2018

gorbak25 Nov 20, 2018

cytadela8 Nov 17, 2018

gorbak25 Nov 20, 2018 •

edited

Loading

This comment was marked as resolved.

thepiwo commented Nov 22, 2018

d-velev Nov 22, 2018

cytadela8 Nov 22, 2018

d-velev Nov 22, 2018

cytadela8 left a comment

cytadela8 Nov 22, 2018

cytadela8 Nov 22, 2018

cytadela8 Nov 23, 2018

[GH-759] Channel Snapshot #791

[GH-759] Channel Snapshot #791

Conversation

gorbak25 commented Nov 16, 2018

cytadela8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorbak25 Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

thepiwo commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cytadela8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorbak25 Nov 20, 2018 •

edited

Loading