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.

Sign up for GitHub

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

Jump to bottom

Implement GA awareness of State Channel's FSM #2502

Merged

velzevur merged 6 commits into master from PT-165413555_FSM_GA_awareness-rebased

Jul 16, 2019

Member

velzevur commented Jun 9, 2019

SC FSM: GA awareness

velzevur requested review from hanssv, uwiger, lucafavatella and gorillainduction

June 9, 2019 15:23

Member Author

velzevur commented Jun 9, 2019

Supersedes #2483.
Complementary protocol changes are to be provided as well.

velzevur mentioned this pull request

Pt 165413555 fsm ga awareness #2483

Closed

velzevur force-pushed the PT-165413555_FSM_GA_awareness-rebased branch from d50c4f3 to 23058c9 Compare

June 10, 2019 07:26

velzevur commented

View reviewed changes

apps/aetx/src/aetx_env.erl

@@ @@ -244,7 +244,7 @@ tx_event(Name, #env{events = Events} = Env) -> @@
                       none -> Env;
                       {value, SignedTx} ->
                           TxHash = aetx_sign:hash(SignedTx),
-                          {Type, _} = aetx:specialize_type(aetx_sign:tx(SignedTx)),
+                          {Type, _} = aetx:specialize_type(aetx_sign:innermost_tx(SignedTx)),

Member Author

velzevur Jun 10, 2019 •

edited

Loading

This is made GA aware. For transactions wrappend in a meta authentication transaction, the old processing was emitting not channel's type but rather ga_meta_tx instead.

This is not breaking consesus but rather modifying the emitted events.

Contributor

lucafavatella Jun 14, 2019

Based on inspection of the code this shall make the aesc_fsm_min_depth_watcher work better for the has_tx scenario.

velzevur commented

View reviewed changes

apps/aeprimop/src/aeprimop.erl

-                  , tx_event_op({channel, aesc_channels:pubkey(InitiatorPubkey,
-                                                               Nonce,
-                                                               ResponderPubkey)})
+                  , tx_event_op({channel, ChannelPubkey})

Member Author

velzevur Jun 10, 2019

This is made GA aware. For channel create transactions wrappend in a meta authentication transaction(s) the channel pubkey is computed in a different GA-specific way (the nonce is 0, so we use the auth_id instead). This was emitting events with the wrong channel ID.

This is not breaking consesus but rather modifying the emitted events.

Member Author

velzevur Jul 3, 2019

This solves: Check event emited when GA channel is created

uwiger previously approved these changes

View reviewed changes

apps/aechannel/src/aesc_fsm_min_depth_watcher.erl Outdated

+                  case aesc_utils:channel_pubkey(SignedTx) of
+                      {error, not_channel_tx} ->
+                          lager:debug("Tx=~p is not a channel transaction", [SignedTx]),

Member

uwiger Jun 10, 2019

Should we really include the whole SignedTx in the debug printout?

apps/aechannel/test/aesc_fsm_SUITE.erl

+                                                    responder_is_ga, both_are_ga]) of
+                              true ->
+                                  _Config1 = stop_node(dev1, Config);
+                              false -> pass

Member

uwiger Jun 10, 2019

When does this happen?

Member Author

velzevur Jun 10, 2019

This might be a bit too implicit: the groups that belong to generalized_accounts group - namely - initator_is_ga, responder_is_ga and both_are_ga start with setting up one or two generalized accounts. Usually nodes are being stopped at each group end but since we can save some contracts upgrading: we don't stop the node when finishing any of the following groups:

  ga_sequence() ->
      [ {group, transactions}  
      , {group, errors}
      , {group, signatures}
      , {group, channel_ids}                                 
      ].

To answer your question: while finishing transactions group for example:

when being ran outside of the scope of generalized_accounts group - there is no ga_group being set, so we stop the node (to start it with the next group)
when being ran in the context of any of the generalized_accounts groups - we do not stop the node (so we don't lose the GA setup). Instead we stop it once finishing the group

apps/aechannel/test/aesc_fsm_SUITE.erl Outdated

+              %    ConKey   = aect_contracts:compute_contract_pubkey(Owner, Nonce),
+              %    CallKey  = aect_call:id(Owner, Nonce, ConKey),
+              %    CallTree = aect_test_utils:calls(S1),
+              %    Call     = aect_call_state_tree:get_call(ConKey, CallKey, CallTree),

Member

uwiger Jun 10, 2019

Delete commented code?

uwiger reviewed

View reviewed changes

apps/aechannel/test/aesc_fsm_SUITE.erl Outdated

+                  ok = rpc(dev1, aec_tx_pool, push, [SignedTx]),
+                  wait_for_signed_transaction_in_block(dev1, SignedTx),
+              %    ConKey   = aect_contracts:compute_contract_pubkey(Owner, Nonce),

Member

uwiger Jun 10, 2019

Suggested change

% ConKey = aect_contracts:compute_contract_pubkey(Owner, Nonce),

apps/aechannel/test/aesc_fsm_SUITE.erl Outdated

+                  wait_for_signed_transaction_in_block(dev1, SignedTx),
+              %    ConKey   = aect_contracts:compute_contract_pubkey(Owner, Nonce),
+              %    CallKey  = aect_call:id(Owner, Nonce, ConKey),

Member

uwiger Jun 10, 2019

Suggested change

% CallKey = aect_call:id(Owner, Nonce, ConKey),

apps/aechannel/test/aesc_fsm_SUITE.erl Outdated

+              %    ConKey   = aect_contracts:compute_contract_pubkey(Owner, Nonce),
+              %    CallKey  = aect_call:id(Owner, Nonce, ConKey),
+              %    CallTree = aect_test_utils:calls(S1),

Member

uwiger Jun 10, 2019

Suggested change

% CallTree = aect_test_utils:calls(S1),

apps/aechannel/test/aesc_fsm_SUITE.erl Outdated

+              %    ConKey   = aect_contracts:compute_contract_pubkey(Owner, Nonce),
+              %    CallKey  = aect_call:id(Owner, Nonce, ConKey),
+              %    CallTree = aect_test_utils:calls(S1),
+              %    Call     = aect_call_state_tree:get_call(ConKey, CallKey, CallTree),

Member

uwiger Jun 10, 2019

Suggested change

% Call = aect_call_state_tree:get_call(ConKey, CallKey, CallTree),

velzevur dismissed uwiger’s stale review via

1fce86c

June 10, 2019 12:16

velzevur force-pushed the PT-165413555_FSM_GA_awareness-rebased branch from 23058c9 to 1fce86c Compare

June 10, 2019 12:16

uwiger previously approved these changes

View reviewed changes

Contributor

lucafavatella commented Jun 11, 2019

Related doc (so we remember):

Protocol repo may need update
Wiki assumes this in in 3.2.0.

lucafavatella reviewed

View reviewed changes

apps/aehttp/test/aehttp_integration_SUITE.erl Outdated

+                  {ok, TxBin} = aeser_api_encoder:safe_decode(transaction, EncTx),
+                  Tx = aetx:deserialize_from_binary(TxBin),
+                  SignedTx =
+                      case account_type(Pubkey) of

Contributor

lucafavatella Jun 14, 2019

I see this is changing behaviour of the test depending on the behaviour of the system under test - chain. I see that when test makes account generic it mines with strictly_follow_top. Seems good enough. Ok.

lucafavatella reviewed

View reviewed changes

apps/aehttp/test/aehttp_integration_SUITE.erl Outdated

@@ @@ -5299,24 +5324,13 @@ sc_ws_timeout_open_(Config) -> @@
               sc_ws_attach_initiator(Config) ->
                   #{initiator := #{pub_key  := Pubkey,
                                    priv_key := Privkey}} = proplists:get_value(participants, Config),
-                  attach({Pubkey, Privkey}, "simple_auth", "authorize", ["42"]),
+                  attach({Pubkey, Privkey}, "simple_auth", "authorize", [?SIMPLE_AUTH_GA_SECRET]),

Contributor

lucafavatella Jun 14, 2019

Could you please consider PR #2510 - it aims at polishing this area of the test.

velzevur dismissed uwiger’s stale review via

a0c28c7

June 16, 2019 09:37

velzevur force-pushed the PT-165413555_FSM_GA_awareness-rebased branch from a0c28c7 to 49506df Compare

June 16, 2019 12:53

Member Author

velzevur commented Jun 16, 2019

I am still to provide a PR in the protocol repo

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_create_tx.erl

@@ @@ -153,7 +155,8 @@ process(#channel_create_tx{} = Tx, Trees, Env) -> @@
                         lock_period(Tx),
                         fee(Tx),
                         nonce(Tx),
-                        round(Tx)),
+                        round(Tx),
+                        ChannelPubkey),

Contributor

lucafavatella Jun 17, 2019

This is independent refactoring and not strictly needed, right?

It is ok to keep here - I am asking simply because I am considering regression testing across tags using this module as interface.

Member Author

velzevur Jun 17, 2019

Without this, the wrong channel_id event is being sent for GAs - if the initiator is a generalized account one's nonce is not being used in channel_id computation. This fixes it

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_fsm.erl Outdated Show resolved Hide resolved

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_offchain_state.erl Outdated Show resolved Hide resolved

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_utils.erl

+                  case aetx:specialize_callback(aetx_sign:tx(SignedTx)) of
+                      {aega_meta_tx, MetaTx} ->
+                          case aega_meta_tx:ga_pubkey(MetaTx) =:= Signer of
+                              true -> aega_meta_tx:auth_id(MetaTx);

Contributor

lucafavatella Jun 18, 2019

This seems to take the auth id of outermost ga meta tx related to the account, while an on-chain processing of tx takes the innermost one (see aega_meta_tx:process). Is this intended?

I am still getting my head around this, and this may have no impact if multiple ga meta tx for same account prevented somewhere else. Though there may be room for opportunity in readability (maybe in other point in code) hence this note.

Member Author

velzevur Jun 18, 2019

Different transaction types have different origin and the origin of channel_create_transaction is the initiator. This function is intended to get the initiator's nonce or auth_id out from channel_create_transaction so it can later on be used in channel_id computation. In case that the outermost transaction indeed is a meta one, it could be either the initiator or the responder authentication method, thus the aega_meta_tx:ga_pubkey(MetaTx) =:= Signer check - we return only the Signer's auth_id.

What botters me now is the other option - if none of the participants is a GA, the funciton returns Mod:nonce(Tx). This could return unexpected result if the get_nonce_or_auth_id/2 function is called for the responder instead.

I am to rework and rename it to express more clearly the intention regarding the transaction type and the participant.

Member Author

velzevur Jun 18, 2019 •

edited

Loading

Addressed in bf327a8

Contributor

lucafavatella Jun 18, 2019

Change is improvement thanks.

Doubt re outermost-vs-innermost still stays. For the same account-meant-to-be-authenticated, multiple ga meta txs can be nested within each other.

velzevur mentioned this pull request

Explain GAs in State Channel FSM aeternity/protocol#367

Merged

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_utils.erl

+              verify_signatures_onchain(SignedTx, Trees, Env) ->
+                  verify_signatures_onchain(SignedTx, Trees, Env, []).
+              verify_signatures_onchain(SignedTx, Trees, Env, CheckedSigners) ->

Contributor

lucafavatella Jun 18, 2019

I arrived here from clause awaiting_signature(cast, {?SIGNED, create_tx, Tx} = Msg, in the FSM.

I have the impression that this function does not prevent not relevant extra ga meta txs - i.e. ga meta tx authenticating accounts not relevant to operation. Though I see that this is checked on chain, and FSM waits for tx in chain anyway. So ok.

Though an assertion aesc_utils:count_authentications(...) =:= 1 would improve readability in code and reduce chance of user(s?) preparing wrong tx. Maybe could be considered as future improvement (outside of this PR).

Contributor

lucafavatella Jun 18, 2019

Similarly for code path from half_signed(cast, {?FND_SIGNED, Msg}, #data{role = initiator,. Though ok.

lucafavatella reviewed

View reviewed changes

Contributor

lucafavatella left a comment

I need to check combine_sigs_or_meta.

Pending comment: outermost-vs-innermost ga meta tx auth id.

I prepared a couple of minor improvements in #2516 - totally optional.

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_fsm.erl Outdated

		end.

		combine_sigs_or_meta(HalfSigned1, HalfSigned2) ->

Contributor

lucafavatella Jun 19, 2019

I see that a correct ga auth fun should enquire the inner tx hash, otherwise the auth can be used for forging other tx.

(Trying to understand how this combining works.)

lucafavatella approved these changes

View reviewed changes

Contributor

lucafavatella left a comment

I need to check combine_sigs_or_meta.

I did not check it.

Pending comment: outermost-vs-innermost ga meta tx auth id.

If multiple meta tx for same account, I expect effect is that the watcher does not observe right tx on chain. Though I expect same effect may happen in other way (e.g. I expect party receiving half signed tx to have option to publish to mempool tx different from the one later provided by other party). I expect party doing so would not gain anything... apart from locking the funds of the other party until he/she realizes.

I have the impression that this function does not prevent not relevant extra ga meta txs - i.e. ga meta tx authenticating accounts not relevant to operation. Though I see that this is checked on chain, and FSM waits for tx in chain anyway. So ok.

Though an assertion aesc_utils:count_authentications(...) =:= 1 would improve readability in code and reduce chance of user(s?) preparing wrong tx. Maybe could be considered as future improvement (outside of this PR).

If this restriction were introduced maybe the code would be easier to follow - though make nested meta tx for same account not allowed. Out of scope.

velzevur added the status/wip label

velzevur force-pushed the PT-165413555_FSM_GA_awareness-rebased branch from 630efff to 098a210 Compare

July 2, 2019 14:46

mpowaga mentioned this pull request

[WIP] Implement GA awareness of State Channels aeternity/aepp-sdk-js#529

Closed

3 tasks


          Implement GA awareness of State Channel's FSM

54367b1

velzevur force-pushed the PT-165413555_FSM_GA_awareness-rebased branch from 098a210 to 54367b1 Compare

July 14, 2019 12:39


          Make FSM checks Fate aware

924cfa5

velzevur removed the status/wip label

lucafavatella reviewed

View reviewed changes

apps/aecore/test/aecore_suite_utils.erl Outdated

+                      catch _:_ ->
+                          aetx_sign:new(InnerTx0, [])
+                      end,
+                  Options1 = maps:merge(#{%fee => 50000 * aec_test_utils:min_gas_price(),

Contributor

lucafavatella Jul 15, 2019

Shall this comment be deleted?

lucafavatella reviewed

View reviewed changes

apps/aehttp/test/aehttp_sc_SUITE.erl Outdated

+                  AuthTx =
+                      case aetx:specialize_type(InnerMostTx) of
+                          {channel_offchain_tx, _} -> InnerMostTx;
+                          _ -> InnerTx

Contributor

lucafavatella Jul 15, 2019

Totally optional. I find it more readable in this way.

Suggested change

      
                        _ -> InnerTx
          
                        {_, _} -> InnerTx

lucafavatella reviewed

View reviewed changes

Contributor

lucafavatella left a comment

Could you please confirm that aest_channels_SUITE runs? I would not be surprised if some s/tx/signed_tx/g needed.

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_fsm.erl

		@@ -936,55 +940,49 @@ awaiting_signature(cast, {?SIGNED, withdraw_tx, Tx} = Msg,
		awaiting_signature(cast, {?SIGNED, ?FND_CREATED, SignedTx} = Msg,

Contributor

lucafavatella Jul 15, 2019

It feels that the macros like FND_CREATED in aesc_codec.hrl (-define(FND_CREATED , funding_created).) shall be shared with sc_ws_api_jsonrpc - as part of encoding/decoding in user API.

Though orthogonal to this PR.

lucafavatella reviewed

View reviewed changes

apps/aechannel/src/aesc_fsm.erl

               awaiting_signature(cast, {?SIGNED, ?FND_CREATED, SignedTx} = Msg,
                                  #data{role = responder,
                                        latest = {sign, ?FND_CREATED, HSCTx, Updates}} = D) ->
-                  NewSignedTx = aetx_sign:add_signatures(
-                                  HSCTx, aetx_sign:signatures(SignedTx)),

Contributor

lucafavatella Jul 15, 2019

Who does now check that SignedTx is related to HSCTx rather than being a completely distinct transaction?

Contributor

lucafavatella Jul 15, 2019

Is the assumption maybe that the user of the user API is trusted?

Member Author

velzevur Jul 15, 2019

Assumption is that if it is a different:

new transaction - then authentication method will not match
old co-signed transaction - the check round will catch it

lucafavatella previously approved these changes

View reviewed changes


          System tests

eac7cb1

velzevur dismissed lucafavatella’s stale review via

eac7cb1

July 15, 2019 16:46


          Fix system tests

6ab8f3b

Member Author

velzevur commented Jul 15, 2019

We used to have the following flow:

Alice requests a change from her FSM_A
the FSM_A prepares a set of updates as well as a new transaction
the FSM_A requests Alice to authenticate the aetx:tx() (1)
Alice returnns to the FSM_A the solo-authenticated tx
FSM_A sends the solo-authenticated tx to FSM_B
the FSM_B requests Alice to authenticate the aetx:tx() (2)
the FSM_B sends the solo-authenticated tx to FSM_B (3)

This has changed to (differences are 1, 2 and 3):

Alice requests a change from her FSM_A
the FSM_A prepares a set of updates as well as a new transaction
the FSM_A requests Alice to authenticate the aetx_sign:signed_tx() (1)
Alice returnns to the FSM_A the solo-authenticated tx
FSM_A sends the solo-authenticated tx to FSM_B
the FSM_B requests Alice to sign the solo-authenticated aetx_sign:signed_tx() (2)
the FSM_B sends the co-authenticated tx to FSM_B (3)

Note that 3 changes the noise protocol message and expectations for the message format itself, thus making it non-backwards compatible with older versions.

uwiger previously approved these changes

View reviewed changes

apps/aega/test/aega_test_utils.erl Outdated

@@ @@ -184,10 +184,14 @@ make_calldata(Code, Fun, Args) -> @@
                   {ok, Calldata} = aeso_compiler:create_calldata(Code, Fun, Args, [{backend, Backend}]),
                   Calldata.
-              get_contract(Name) ->
+              get_contract(Name0) ->

Member

uwiger Jul 15, 2019

Why Name0 here? By convention, this is normally used when some preparatory operation converts it to Name.


          Cleanup

531ae60

velzevur dismissed uwiger’s stale review via

531ae60

July 15, 2019 19:11

Member Author

velzevur commented Jul 15, 2019

From the remaining System Tests for State Channels, commit eac7cb1 allowed runing aest_channels_SUITE as smoke tests and commit 6ab8f3b fixed a small issue in it. The latter passed.

The normal smoke tests behaviour is returned in commit 531ae60.

uwiger previously approved these changes

View reviewed changes

lucafavatella previously approved these changes

View reviewed changes

docs/release-notes/next/PT-165413555_FSM_GA_awareness.md

@@ @@ -0,0 +1,8 @@ @@
+              * Introduces State Channel FSM Generalized Accounts awareness. It now fully
+                supports creation of meta transactions and verifies them.
+              * Changes WebSocket API regarding signing transactions: now sign requests

Contributor

lucafavatella Jul 16, 2019

I read this line mentions the change in the user API. Ok.

docs/release-notes/next/PT-165413555_FSM_GA_awareness.md

@@ @@ -0,0 +1,8 @@ @@
+              * Introduces State Channel FSM Generalized Accounts awareness. It now fully
+                supports creation of meta transactions and verifies them.
+              * Changes WebSocket API regarding signing transactions: now sign requests

Contributor

lucafavatella Jul 16, 2019

the FSM_B sends the co-authenticated tx to FSM_B (3)

Note that 3 changes the noise protocol message and expectations for the message format itself, thus making it non-backwards compatible with older versions.

Shall there be somewhere in this file at least a mention of the channels Noise node-to-node protocol breakage/change?

Contributor

lucafavatella commented Jul 16, 2019

Amended wiki with target release.


          Add a description regardin noise changes

c9c38ce

velzevur dismissed stale reviews from lucafavatella and uwiger via

c9c38ce

July 16, 2019 11:15

lucafavatella approved these changes

View reviewed changes

uwiger approved these changes

View reviewed changes

velzevur merged commit e164fc4 into master

velzevur deleted the PT-165413555_FSM_GA_awareness-rebased branch

July 16, 2019 12:03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet