Location: [client]

Problem: Having logging output like this is not useful:

time="2021-06-07T11:59:58+02:00" level=debug msg="channelConn: publishing message: &{[65 118 88 117 50 30 136 25 23 181 229 133 193 244 89 48 252 220 160 8 68 111 236 240 215 1 236 233 187 133 123 215] 0 [236 88 2 227 250 155 243 158 46 97 169 75 86 125 210 9 34 189 195 213 195 28 222 82 123 62 78 20 133 132 5 235 89 165 161 71 31 229 119 245 179 117 90 231 243 144 55 62 172 210 79 237 137 157 9 175 98 165 218 90 209 210 248 167 54]}" 

Solution: Also log msg.Type().

Problem: The ERC20Depositor sends two transactions (increase allowance and deposit) in the same block.
This seems to be causing issues with how the nonce is determined. Sometimes its transactions revert.

Possible solutions:

1. Manually increasing the nonce by one after the first TX
2. Wait for the first TX to be mined (which is actually the job of the Funder)
3. Poll the nonce multiple times until a new one is returned

I opt for 1. as it seems the easiest and fastest.

Location: [backend/eth/channel]

Motivation: After #39 is done, the generic EventSub can be used in all places where Watch… or Filter… is used currently to monitor events. This also helps to finalize the design of #39, which is currently in review.

Testing: The tests should not need any modification.

Location: [backend/eth/channel/errors]

Problem: The current error wrapping functions are private, and reside in package [backend/eth/channel].
The [backend/eth/channel/subscription] (#39) package now also needs these functions so instead of copy-pasting them, re-using them would be better.

Solution: Move (Check)IsChainNotReachableError into a new [backend/eth/channel/errors] package and use it in [backend/eth/channel(/subscription] .

Testing: No changes needed.

Parameters are not checked for consistency when created by deserialization. In particular, it is not checked whether the ID is correct. However, this may be crucial for the security of our program. Hence, we must check that params.id == CalcID(params) after deserialization.

Location: funder.go, timeout.go

When we fund, we wait for blocktime + challengeDuration before we cancel the context. In some situations this can cause unwanted behavior. For example, when the block tick time is larger than the challenge duration, the BlockTimeout is immediately canceled with the next block, even it the next block arrives within the expected time frame.

Currently, channel watching must be started manually, which can be forgotten. I suggest that we automatically start the channel watcher by default.

Moreover, the watcher event handler should be optional. All security relevant event handling should be done automatically.

Location: New Package [backend/eth/glue]

Motivation: As part of #19 we need a generic event subscription. It will then be used to build the generic reorg resistant event subscription.

Design: The current prototype implementation looks like this:

uml.txt
EventFactory is defined as func() EventType since otherwise i would need to use reflections to clone the EventType.

Possible extensions:

• Gas bumper
• Rebroadcasting
• Receipt polling
• Persistence

Usage: Subscribing to all ERC20Approval events of the token contract would look like this:

factory := func() glue.EventType {
	return glue.EventType{
		Name: "Approval",
		Data: new(peruntoken.ERC20Approval),
	}
}

sub, _ := glue.NewEventSub(token.Contract(), factory, nil /* Start block is 0 */) // Error handling omitted
defer sub.Close()
sink := make(chan *glue.Event, 1)
go sub.Read(ctx, sink) // Error handling omitted

for _event := range sink {
	event := _event.Data.(*peruntoken.ERC20Approval)
	// Use event…
}

Testing: I already have a test to check that emitted events are received. This should be enough in combination with the additional reorg tests that i added.

Location: [backend/eth/channel] contractbackend.go

Problem: TX are assumed to be final after one block, which is not the case for a reorg.

Solution: Implement a modified version of ContractBackend.ConfirmTransaction which checks that the Transaction is included in at least TxBlockFinality blocks.
TxBlockFinality is a new package constant of [backend/eth/channel]. Value to be discussed in #98

There are at least three different types of channels that we consider in go-perun:

• A ledger channel is a channel that it is funded on-chain. It can be established between any participants in the network.
• A sub-channel is a channel that is funded off-chain from a parent channel. Its participants are the same as in the parent channel.
• A virtual channel is a channel that is funded off-chain from a set of parent channels. It can be established between participants that have a channel to a common intermediary.

Currently, go-perun supports ledger channels and sub-channels. The goal of this issue is to enable virtual channels in go-perun. Virtual channels are essential for establishing a channel network.

Currently, in a channel proposal, we have the convention that the proposer has index 0. However, this is not documented well and not evident. Furthermore, the framework does not give an indication of that fact via, for example, a property on the proposal object (e.g., proposal.ProposerIndex) or the client package (e.g., client.ProposerIndex).

I think implicit assumptions like that are bad style. For example, if we, for whatever reason, decide to change away from this assumption, it will be difficult to make the framework user aware of that change. If we, at least, indicate this assumption via a property on some object, transitioning to a different behavior will be easier.

Location: [backend/eth/channel] deploy.go

Problem: While using the Kovan4 endpoint of Arbitrum, i sometimes got an error when deploying our contracts:
setting up contracts: contract setup failed: deploying eth assetholder: deploying ETHAssetHolder: no contract code after deployment.
go-ethereum queries the code of a deployed contract immediately after deploying it.
It seems that either by design or because go-ethereum just waits one block after deploying, it sometimes returns no code.

Solution: go-etherum returns an ErrNoCodeAfterDeploy error in this case.
Checking for it in a retry loop with 3 tries and a 2 seconds delay should make up for it.

Testing: I would test it again with the demo client.

Description

This is regarding the closure of channel in non collaborative manner as described below. Lets assume Alice initiates closure of a channel with bob. The channel state is not marked final.

1. Alice: calls register.

2. Alice and Bob received RegisteredEvent.

3. Now there are few possibilities:
   a. Both Alice and Bob do not want to conclude the channel, they continue to do off-chain transactions. This however will eventually lead to case b or c.
   b. Bob expects Alice to call conclude, so Bob ignores the event and waits; Alice calls conclude within expected duration.
   c. Same as b, but Alice doesn't call conclude within the expected time period, so Bob calls conclude. Unfortunately it was exactly during the same time that Alice also calls conclude.

4. Case b is not a problem. But in case c, the conclude call that is processed second by the blockchain will fail and the channel will already have been concluded. This case also is not covered by secondary logic is applied only when the state of the channel is final (i understand it this way by looking here).

This issue also relates to #7 created by @ggwpez. I think one of the solution could be to create a provision to wait for N blocks (first by adding/updating an interface in the core of the framework, then by adding the corresponding implementation in the ethereum backend). This could also remove the need for secondary logic, which now is propagated across many functions/methods in the go-perun framework.

Steps to reproduce:

To reproduce this issue:

1. Clone this version of perun-node.
2. Comment out lines 172-174 in session/channel.go
3. Start ganache-cli node using the below command (in a separate terminal):

ganache-cli -b 1 --account="0x1fedd636dbc7e8d41a0622a2040b86fea8842cef9d4aa4c582aad00465b7acff,100000000000000000000" --account="0xb0309c60b4622d3071fad3e16c2ce4d0b1e7758316c187754f4dd0cfb44ceb33,100000000000000000000"

4. Run Test_Integ_Role with the below command:

cd session
cd session && go test -cover -p 1 -count=1 -tags=integration -run Test_Integ_Role

5. The following phrase can be found in the tests logs. By looking at the failed transaction ID in the ganache-cli logs and the transaction IDs in the test log, the transaction can be found corresponding to conclude call.

At the moment, creating a channel allocation for channel opening is not very convenient and can be confusing. Even for just a single asset channel it looks like the following.

channel.Allocation{
	Assets:   []channel.Asset{asset},
	Balances: [][]*big.Int{etherToWei(myBalEth, peerBalEth)},
}

We may want to change the Allocation struct and provide helper methods for that.

The allocation struct could hold a map[Asset]AccountBalances, where AccountBalances is a map from accounts to integers.

Location: [backend/eth/bindings/] abi.go

Motivation: #39 needs the *bind.BoundContract of a contract to listen on events of a contract.

Design: Add a new file abi.go which contains all parsed ABI definitions.
They should be parsed in the init method and panic upon failure.

Testing: I already tried it out in #39 and it works there.

Context: Parameter secondary bool of function Channel.Settle is part of a protocol optimization that aims to reduce client gas fees. The Ethereum contracts require only one client to call conclude. The other clients can skip the conclude call and thereby save some gas fees.

Problem: I would like to discuss the following issues with the current implementation:

1. The secondary parameter is not document well and it is not clear to a new framework user how to use it.
2. It is arguable whether the complexity introduced by the settle secondary logic outweighs its benefits.
3. It is arguable whether implementing the settle secondary logic on the framework level is preferable over implementing it on the application level.

Suggestion:
I see the following options to improve on the situation:

• Keep the logic: Make parameter secondary optional and document it well.
• Drop the logic: If required, implement the secondary logic on the application level.

Our backend/ethereum/wallet/hd uses SignText(Hash(data)), while backend/ethereum/wallet/keystore uses SignHash(PrefixedHash(data)), which is equivalent to SignText(data). Please unify this.
Also, our wallets add 27 to the v byte of the signature when signing, when the Ethereum standard says that signatures should have a v in [0,1].

The GitLab issue templates are still in the .gitlab folder. I think we should migrate them to GitHub templates if we still want to use that workflow.
This ties in with #12.

Location  [client], [channel], [backend/eth/channel].

Discussed in  DST/dev chat.

Context  When both users try to close a finalized channel at the same time, one of their transactions can revert with channel already concluded.
This results from both parties sending the conclude transaction Thanks to @manoranjith for finding this.
channel.Settle has a secondary flag which can be used to prevent this from happening.

A makeshift solution is to let one party wait two block duration.

Changes 
There are at least two possibilities here:

1. Add a Secondary flag to channel.Register which is only used for final states.
2. Add some kind of wait function that would be called before Register.

I think we should keep settle and register consistent, so either use two wait functions or two secondary flags.

Acceptance criteria  The user can prevent go-perun from sending a reverting register transaction.

Testing  All of our Two-party tests use a simulated ETH-chain which makes this unlikely to trigger.
It is still be worth trying it out though.

Implementation hints  A sleep can be added here to make it more likely to fail.

It is currently rather complicated to setup a funder.

The constructor NewFunder takes as input accounts map[Asset]accounts.Account and depositors map[Asset]Depositor, both mapping from the same list of assets to a set of objects. For ease of use, these two maps could be combined into one map by integrating the account into the depositor.

Furthermore, there are no constructors for these maps and not much documentation on NewFunder, so it is hard to know how exactly the maps should be created. For ease of use, we should extend the documentation of NewFunder and we should make it easier to create a depositor map, e.g., by adding a special type for it with respective helper methods.

It seems like if no logger is set up, runtime errors happening in the backend are not visible. This is problematic because relevant information might be hidden from a user. For example, when the funder fails to perform a channel funding operation, the user will just see "did not fund channel in time", while the logger actually has some more information "out of gas" or "failed to increase allowance".

I suggest that we make such error level logs visible to the user by default, for example, by printing them to stdout if no logger is set.

Currently, NewFundingTimeoutErrror, the constructor for error type FundingTimeoutError returns a pointer instead of value. This makes the usage of the error type tricky.

For example, the client.Proposal returns PeerRejectedError, RequestTimedoutError, FundingTimedoutError and few more.

Since each of the other errors are generated as values by their respective constructors, then can be retrieved as below:

peerRejectedError := pclient.PeerRejectedError{}
ok := errors.As(err, &peerRejectedError)

But for FundingTimeoutError, a double pointer should be used

fundingTimeoutError := &pchannel.FundingTimeoutError{}
ok := errors.As(err, &fundingTimeoutError)

Is there a specific reason for returning a pointer in this case ?

If not, I think constructor of FundingTimeoutError can be updated to return a value, so that it is more idiomatic and does not requires an exception when handling the error.

In update.go, machine.CheckUpdate is only called after handling subchannel funding and withdrawal updates. Hence, the check is not done in these cases. This may lead the client to accepting an update where the signatures are invalid. machine.CheckUpdate must be called before any response.

Problem
As mentioned in #20, a channel opening can fail while channel funding is in progress (e.g., if the funding confirmation is not received within the specified waiting period). There is currently no convenient method to withdraw the funds in that case.

Solution

1. Persist the channel before submitting the funding transaction.
2. If the channel opening fails, wait until the funding transaction is confirmed and initiate channel settling afterwards.

Location: funder.go, func Fund

When something goes wrong during funding, the funder prioritizes error messages depending on their type. If there is an error of type AssetFundingError, any other errors are discarded. This is not a good behavior. For example, I just had an error during an IncreaseAllowance in the ERC20Depositor. The error was that the nonce could not be determined. However, I did not receive any information about this error as it was discarded. I just get:

time="2021-05-05T10:13:44+02:00" level=error msg="Could not fund asset" asset=0xb5c37Ee9C816D2DB2f6A9A020ce6a80af3993AD3 error="sending transaction: "

We should always log all errors.

• Cache dependecies
• Run in parallel
  • vetting
  • tests
  • race tests

We currently do not have a CONTRIBUTING.md.
It would be important to have one so that new and external developers can get familiar with our workflow.

There is a template available from GitHub.

Currently, method Client.Handle does not return an error when there is a connection failure. It only logs the error and returns. We should consider returning the error so that the framework user can act on it.

Problem: [channel] params.go
Function ValidateParameters does not check for nil values in the participants slice since it is only passed len(parts) instead of parts.
This can subsequently cause a panic in CalcID which assumes that all values are valid.

Solution: Pass the participants as parts []wallet.Address to ValidateParameters and check that none of the values is nil.

In backend/eth/channel/withdraw.go loop variables index and asset are used in a go routine. This means that they change while the go routine is running, which is not intended.

The fix is to redefine these variables locally before starting the go routine.

Location: Package [backend/eth/glue], part of #19

Motivation: The Resistant Event Sub tracks the state of an event in the case of a chain reorganization.

Design: Analogous to #39 the design is very simple. It wraps an EventSub and accepts a finalityDepth. The necessary ChainReader is provided by the ContractBackend.

It tracks the status of an Event by subscribing to new block chain headers and then checking all events for finality.
Once an Event is final, it is emitted on the sink channel that the user passed to Read.

uml.txt

Testing: So far i am testing that final Events arrive even after reorgs. Still needs to be slightly extended.

Problem: We need some reasonable block finality depth when confirming Events or Transactions.
Larger values take longer to confirm but are safer, smaller values are faster but unsafer.
Crypto exchanges use 2-20 blocks, Chainlink seems to use 50.

Solution: Depends on the security that we need, so please discuss here.

PS: The value in question is backend/ethereum/channel/TxFinalityDepth

Related to #46 (comment)

Our generate.sh script has the following issues:

1. We run solc twice (once with abigen for the bindings, once for the binary runtime), potentially creating inconsistencies.
2. solc with abigen does not allow configuration parameters.

The parameters of client.NewLedgerChannelProposal are not documented. We should add documentation.

Location: generate.sh

Description:
Use the following snippet to get rid of the awk call and simplify the script.

Snippet from @sebastianst, link:

#!/bin/bash

# SPDX-License-Identifier: Apache-2.0

set -e

# Configuration
solpath="../contracts/contracts"
bindpath="../../bindings" # reverse to solpath
solc="solc-static-linux"

# Download solc.
wget -nc "https://github.com/ethereum/solidity/releases/download/v0.8.3/${solc}"
chmod +x $solc
echo -e "⚠ Ensure that the newest version of abigen is installed"

# Compile in solpath
cd "${solpath}"
"${bindpath}/${solc}" --combined-json abi,bin,bin-runtime \
  --allow-paths '.,../node_modules/@openzeppelin' \
  --optimize --optimize-runs 10000 \
  --overwrite -o "${bindpath}" \
  "@openzeppelin=../node_modules/@openzeppelin" \
  *.sol

# Bind
cd "${bindpath}"
abigen --pkg bindings --out bindings.go --combined-json combined.json \
  --exc Address,Bytes,Context,ECDSA,ERC165,ERC20,ERC721,IERC165,IERC721Enumerable,IERC721Metadata,IERC721Receiver,Ownable,Sig,Strings

echo -e "🎉 Bindings generated"

The Ethereum backend is currently build on the assumption that a transaction cannot be reverted. While this may be true for local testnets, it is not true for public testnets and the Ethereum mainnet, where block reorgs can happen.

We should make the Ethereum backend work with Ethereum nets where reorgs appear. An idea how to implement this is as follows: We introduce a parameter ConfirmationBlockDepth. Whenever go-perun accesses on-chain state, we only provide on-chain state if it is at least ConfirmationBlockDepth blocks old.

The following appear to be relevant places in the code where on-chain state is accessed:

• Contract validation: ValidateAdjudicator, ValidateAssetHolder
• Event subscription: Watch, Filter
• Contract deployment: WaitDeployed
• Transaction confirmation: ConfirmTransaction

Added by @ggwpez:
The current road map for this issue looks like this:

• Reorg resistance in go-perun
  • Simulated reorgs in go-ethereum ethereum/go-ethereum#22624
    • Tests
  • Resistant Event Subs #40
    • Use in go-perun #85
    • Expose BoundContract #41 (done but not merged)
    • Generic Event Subs #39 (done but not merged)
      • Use in go-perun #84
  • Resistant TX confirming
    • Generic TX confirming #97 (partially done with ConfirmTx and WaitDeployed)
    • Resistant contract deployment #99
    • Fix finality depth #98
  • Reorg Tests
    • Reverting
    • Reordering Events
    • Reordering TX
    • Header subscription

The Accept and Reject functions of ProposalResponder and UpdateResponder can only be called once and panic on a second call.
I think this logic should only apply in the success case, since when an error occurs, the user probably wants to retry it without panic.

Travis CI has changed how open source projects are supported, and Hyperledger can no longer build there. I suggest GitHub Actions as a replacement.

Location  New package, maybe [client/simple], [client/payment] or [payment/client].

Description 
The Payment Client API could look like this:

Compared to go-perun, the proposed API is synchronous and lets the user write linear programs without confusing callback magic.
I think this is important for making it more understandable.
In detail:

• HandleProposal is replaced by Client.Proposals() <-chan ChannelProposal from which the user can read the proposals that were sent to him.
• HandleUpdate is replaced by Channel.ReceivedPayments() <- chan amount from which the user can read the payments that he received.
• HandleNewChannel is removed. Its functionality is replaced by Client.Restore, Client.ProposeChannel and ChannelProposal.Accept.
• HandleAdjudicatorEvent is removed. Adjudicator events will be handled by the Client internally.

The callback version would look like this:

Context  We want to make go-perun easier to use for new developers.
This approach minimizes and simplifies the API at the cost of its versatility.
It would allow developers to create two-party-single-asset-payment-ledger channels.

Open questions:

• Do you like it 😄 ?
• Sub-Channels Y/N?
• Should the asset be fixed to ETH? (Allowing ERC20 would complicate the Bals struct)
• Do we need Payment requests?

Testing  A End-to-End test à la HappyAliceBobTest would be great.

Motivation: Ensure that all assumptions that we make about how geth handles reorgs are correct.
Since reorgs are a complicated topic, i wanted to make sure that we understand them right.

This was done mainly to ensure that i implemented the SimBackend.Reorg function correctly, but it could also help us understand how geth handles this case.
-> It is therefore up for discussion if we want to include this code in go-perun or not.

Design: I wrote a test that creates an ERC20 setup, sends Transactions and then reorganizes them.
Afterwards it asserts that the events are emitted and removed correctly.

Type  IMPROVEMENT

Affects  [log]

Current situation 
It is often difficult to tell multiple parties apart in the log output.
For example we have Alices logger:
log.WithField("role", "Alice").Info("test")
which produces:
INFO[0000] test role=Alice

Suggestion 
I suggest to add a WithPrefix(string) function, such that the above would become
log.WithPrefix("Alice").Info("test")
which produces:
INFO[0000] Alice: test

Recursively it would look like this:
log.WithPrefix("Alice").WithPrefix("client").Info("test"):
INFO[0000] Alice.client: test or
INFO[0000] Alice: client: test

To simplify the above, a WithPrefixes([]string) or WithPrefices([]string) should be added.

Justification  Logs are more readable with a prefix and the entity that does the logging is not a real Field in the first place.

Location: [backend/ethc/channel/subscription]

Task: Instead of having three different read functions: ReadAll, ReadPast and ReadFuture we only want to have a single Read function.
This simplifies the event sub code a lot and the cases that the other functions cover are not needed right now.

Location  [client]

Changes  Add field CurrentState to client.ChannelUpdate to make the current state of a channel accessible.
For version 1 updates it could either be the same as State or null.

Context  channel.State() can not be called in an ongoing HandleUpdate call.
This makes it harder to use, since the user has to track the current state themself. Imagine:

func (c *Client) HandleUpdate(update client.ChannelUpdate, _ *client.UpdateResponder) {
    channel := c.getChannel(update.State.ID)
    current := channel.State()   // NOT POSSIBLE
}

It is not possible since the mutex of the machine that is associated with the channel is already locked.

Acceptance criteria  The state of a channel is accessible inside of an ongoing update in a threadsafe way.

In the optimistic case we can settle a subchannel into its parent channel without having to go on-chain. However, our state machine logic currently does not allow to settle and withdraw a channel from the final state (no valid transition from phase final to phase withdrawing). We should change the state machine behavior so that it allows settling subchannels without channel registration on-chain.

Currently, Client.Handle returns only if the underlying connection returns an error. I think it would be convenient if the Handle method is provided with a Context so that the user is assured that the method returns once the context is done.

When we send an on-chain transaction (fund, register, conclude, withdraw), we wait for a fixed time period for the transaction to be mined. The time period is set by using a context that gets canceled after a given timeout. When the context expires, the top level function call returns with an error and the protocol execution is stopped. However, the transaction can eventually get mined after some more time and the on-chain state of the channel will then be changed to one that is different from the state seen by the go-perun framework .

In this scenario, there is no option for the user to continue the protocol from the intermediate state to either progress it forward or revert it back. For example, in the case of funding transaction:
When the funding transaction gets mined eventually, the funds would be locked in the contract and as per the perun protocol, the user should be able to withdraw his funds back. However, the current implementation lacks support for this.

While there could be multiple ways to provide a mechanism to continue the protocol from the intermediate state, I think we need to explicitly identify this scenario has occurred by introducing a named error (say TxTimedOutError). Based on the above described points, I think it will also be useful to include the name of the transaction that has failed ("fund", "register", "conclude" or "withdraw") for the user to be able to efficiently handle this condition.

In the current implementation of ethereum blockchain backend, the user (of the framework) is expected to initialize a connection to the blockchain node and pass it to the NewContractBackend function in channel package.

If the connection to blockchain node is lost after a ContractBackend is created, then it is detected only when one of the method calls (invoked at later point of time) that interact with the blockchain node fails. The resulting error message is directly returned to the user.

I think, in this scenario, the user should be

1. Receiving a named error (say ChainNotReachableError).
2. Able to restart the protocol from the intermediate state of where it stopped after fixing the connection)

The second point described above is also related to #20.
For the first point, one suggestion is that we can add the named error to the core of the SDK and update the functions in the backend implementations to return this error in appropriate scenarios. As far as see, this error is not returned as a named error by the ethclient library. So there could be multiple approaches on how we want to detect this error in the ethereum backend.

Location: [backend/eth/channel]

Motivation: After #40 is done, the ResistantEventSub can be used in all places where EventSub is currently used to monitor events. This also helps to finalize the design of #40, which is currently in progress.

Testing: Testing would take a bit of work, since the SimulatedBackend does not produce blocks on its own.
A hacky PoW were a go-routine mines a block every 20ms did not work. I will look into to again, hopefully it can be done without much effort.
Besides this, all simulated on-chain tests will need to be checked if they still work as intended.

The simple.Wallet implementation holds a struct Accounts. This is currently exported and not protected against concurrent access.

We should unexport it (i.e., Accounts -> accounts) and protect it with a read-write mutex.

The current way of accepting a ledger channel proposal lcp is as follows.

acceptance := lcp.Accept(part, nonce)
r.Accept(h.conn.Context(), acceptance)

1. It is confusing that accept needs to be called on the proposal and the proposal responder.
2. In most cases we provide a nonce via client.WithRandomNonce(), which does not depend on any parameters.

Regarding 2.: We can make the nonce parameter optional and let the default behavior be that the nonce is chosen as client.WithRandomNonce().

Regarding 1.: We should consider combining lcp.Accept and r.Accept into one function. We might even consider combining lcp and r into one object. Alternatively, we should at least rename lcp.Accept to make it clear, that it does not accept the proposal, but only generates the acceptance message.