Restore compatibility between go-perun post-0.10.6 and eth-backend #44
Conversation
iljabvh
force-pushed
the
fix-for-goperun-0.10.7
branch
3 times, most recently
from
80432b3 to
64796c1
Compare
channel/contractbackend.go
Outdated
| @@ -73,11 +82,24 @@ type ContractBackend struct { | |||
| // txFinalityDepth defines in how many consecutive blocks a TX has to be | |||
| // included to be considered final. Must be at least 1. | |||
| func NewContractBackend(cf ContractInterface, chainID ChainID, tr Transactor, txFinalityDepth uint64) ContractBackend { | |||
| // Check if the shared maps are initialized, if not, initialize them. | |||
There was a problem hiding this comment.
Shouldn't this be under a SharedMutex lock?
If it is allowed to call this function in paralell (as the == nil checks suggest) you would end up with a race condition when writing to the maps in line 95 and 96.
channel/contractbackend.go
Outdated
| @@ -45,6 +45,15 @@ const ( | |||
| // create a TxTimedoutError with additional context. | |||
| var errTxTimedOut = errors.New("") | |||
|
|
|||
| // SharedExpected Nonce is a map of each expected next nonce of all clients. | |||
| var ( | |||
| SharedExpectedNonces map[ChainID]map[common.Address]uint64 | |||
There was a problem hiding this comment.
Any reason those two are separate maps instead of a map[ChainID]map[common.Address]struct{uint64,*sync.Mutex} (assuming a per-item mutex is needed)? As far as I can tell those two always work in tandem and you basically never need/use SharedExpectedNonces[a][b] without using SharedNonceMtx[a][b] (though I could be wrong and different parts of perun already use separate maps for a different reason).
Even then the following could be an option, too, grouping them and removing an extra lookup due to having two outer maps:
type PerChainData struct {
expectedNonces map[common.Address]uint64
mutexes map[common.Address]*sync.Mutex
}
map[chainID]PerChainData
There was a problem hiding this comment.
Another note: Do those need to be public? Might be easier to reason about the mutex behavior if only this package can access them.
channel/contractbackend.go
Outdated
| // Look up expected next nonce locally. | ||
| c.nonceMtx.Lock() | ||
| defer c.nonceMtx.Unlock() | ||
| SharedMutex.Lock() |
There was a problem hiding this comment.
The previous defer Unlock is probably more idiomatic Go (as it enforces the unlock should a panic happen, too).
Though as I don't think the code in between can panic (besides an out of memory error) they should be functionally equivalent. I don't know which one you usually use in perun code.
channel/erc20_depositor.go
Outdated
| func lockAndUnlock(lock *sync.Mutex, fn func()) { | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| lock.Lock() |
There was a problem hiding this comment.
I'm confused: Why do we have individual mutexes if they are only locked while mu is locked? Shouldn't it be sufficient to always lock mu and throw away the individual mutexes? As the mu.Lock() already guarantees that only a single thread/goroutine is in fn, thus guaranteeing that only a single individual mutex (in locks) is ever locked at the same time, making the locks mutexes effectively useless?
iljabvh
force-pushed
the
fix-for-goperun-0.10.7
branch
from
a3f1a6e to
0948c6a
Compare
… uses the AdjudicatorReq object during conclusion of a channel. Also remove functions that are necessary for the Secondary option. Update(go.mod/go.sum): Update go.mod and go.sum to the most recent go-perun commit. Signed-off-by: Ilja von Hoessle <[email protected]>
…ce since v0.10.7. * fix(channel/test): Include Randomizer for AppID type to generate random AppIDs for tests. * fix(client/client_test.go): Adapt appID to the new AppID type. * fix(channel/backend.go): Adapt app generation to new AppID type, include NewAppID method. * fix(channel/subscription.go): Adapt convertEvent to new AppID type. * chore(go.mod, go.sum): Update go-perun version to a v0.10.7 commit. Signed-off-by: Ilja von Hoessle <[email protected]>
Signed-off-by: Ilja von Hoessle <[email protected]>
Signed-off-by: Ilja von Hoessle <[email protected]>
Signed-off-by: Ilja von Hoessle <[email protected]>
…nction Signed-off-by: Ilja von Hoessle <[email protected]>
Signed-off-by: Ilja von Hoessle <[email protected]>
iljabvh
force-pushed
the
fix-for-goperun-0.10.7
branch
from
263ecd6 to
b5f8a7f
Compare
Two changes have been implemented in go-perun that break the eth-backend. This PR restores the compatibility by adapting to the changes made in go-perun:
Secondaryfield inchannel.AdjudicatorReqhere.Note that this PR aims to close issue #38 .