hyperledger-labs / go-perun Goto Github PK
View Code? Open in Web Editor NEWπ Perun's Blockchain-Agnostic State Channels Framework in Go.
Home Page: https://perun.network/
License: Apache License 2.0
π Perun's Blockchain-Agnostic State Channels Framework in Go.
Home Page: https://perun.network/
License: Apache License 2.0
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.
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.
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β [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:
Secondary
flag to channel.Register
which is only used for final states.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.
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
.
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.
The parameters of client.NewLedgerChannelProposal
are not documented. We should add documentation.
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:
Added by @ggwpez:
The current road map for this issue looks like this:
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.
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.
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
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.
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.
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
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.
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.
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.
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.
Alice
: calls register
.
Alice
and Bob
received RegisteredEvent.
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
.
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.
To reproduce this issue:
perun-node
.172-174
in session/channel.go
ganache-cli -b 1 --account="0x1fedd636dbc7e8d41a0622a2040b86fea8842cef9d4aa4c582aad00465b7acff,100000000000000000000" --account="0xb0309c60b4622d3071fad3e16c2ce4d0b1e7758316c187754f4dd0cfb44ceb33,100000000000000000000"
Test_Integ_Role
with the below command:cd session
cd session && go test -cover -p 1 -count=1 -tags=integration -run Test_Integ_Role
conclude
call.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:
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.
Typeβ IMPROVEMENT
Affectsβ [log]
Current situationβ
It is often difficult to tell multiple parties apart in the log output.
For example we have Alice
s 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.
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β [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.
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.
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.
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.
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:
Client.Proposals() <-chan ChannelProposal
from which the user can read the proposals that were sent to him.Channel.ReceivedPayments() <- chan amount
from which the user can read the payments that he received.Client.Restore
, Client.ProposeChannel
and ChannelProposal.Accept
.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:
Bals
struct)Testingβ A End-to-End test Γ la HappyAliceBobTest
would be great.
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
The current way of accepting a ledger channel proposal lcp
is as follows.
acceptance := lcp.Accept(part, nonce)
r.Accept(h.conn.Context(), acceptance)
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.
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.
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:
Funder
)I opt for 1.
as it seems the easiest and fastest.
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"
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()
.
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:
secondary
parameter is not document well and it is not clear to a new framework user how to use it.Suggestion:
I see the following options to improve on the situation:
secondary
optional and document it well.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.
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.
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.
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.
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.
Related to #46 (comment)
Our generate.sh
script has the following issues:
solc
twice (once with abigen for the bindings, once for the binary runtime), potentially creating inconsistencies.solc
with abigen does not allow configuration parameters.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].
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.
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.
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.
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.
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.
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.
There are at least three different types of channels that we consider in go-perun:
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.
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
ChainNotReachableError
).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: 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
.
Testing: So far i am testing that final Events arrive even after reorgs. Still needs to be slightly extended.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. πππ
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google β€οΈ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.