aarifullin/neoneo-go
1
0
Fork 0
forked from TrueCloudLab/neoneo-go
Code Pull requests Activity

notary: fix possible deadlock in UpdateNotaryNodes

Browse source 
`UpdateNotaryNodes` takes account then request mutex, and `PostPersist` takes
them in a different order. Because they are executed concurrently a deadlock
can appear.

```
2021-07-23T11:06:58.3732405Z panic: test timed out after 10m0s
2021-07-23T11:06:58.3732642Z
2021-07-23T11:06:58.3742610Z goroutine 7351 [semacquire, 9 minutes]:
2021-07-23T11:06:58.3743140Z sync.runtime_SemacquireMutex(0xc00010e4dc, 0x1100000000, 0x1)
2021-07-23T11:06:58.3743747Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/runtime/sema.go:71 +0x47
2021-07-23T11:06:58.3744222Z sync.(*Mutex).lockSlow(0xc00010e4d8)
2021-07-23T11:06:58.3744742Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/sync/mutex.go:138 +0x1c1
2021-07-23T11:06:58.3745209Z sync.(*Mutex).Lock(0xc00010e4d8)
2021-07-23T11:06:58.3745692Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/sync/mutex.go:81 +0x7d
2021-07-23T11:06:58.3746162Z sync.(*RWMutex).Lock(0xc00010e4d8)
2021-07-23T11:06:58.3746764Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/sync/rwmutex.go:98 +0x4a
2021-07-23T11:06:58.3747699Z github.com/nspcc-dev/neo-go/pkg/services/notary.(*Notary).UpdateNotaryNodes(0xc00010e480, 0xc000105b90, 0x1, 0x1)
2021-07-23T11:06:58.3748621Z 	/home/runner/work/neo-go/neo-go/pkg/services/notary/node.go:44 +0x3ba
2021-07-23T11:06:58.3749367Z github.com/nspcc-dev/neo-go/pkg/core.TestNotary(0xc0003677a0)
2021-07-23T11:06:58.3750116Z 	/home/runner/work/neo-go/neo-go/pkg/core/notary_test.go:594 +0x2dba
2021-07-23T11:06:58.3750641Z testing.tRunner(0xc0003677a0, 0x16f3308)
2021-07-23T11:06:58.3751202Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/testing/testing.go:1050 +0x1ec
2021-07-23T11:06:58.3751696Z created by testing.(*T).Run
2021-07-23T11:06:58.3752225Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/testing/testing.go:1095 +0x538
2021-07-23T11:06:58.3752573Z
2021-07-23T11:06:58.3771319Z goroutine 7340 [semacquire, 9 minutes]:
2021-07-23T11:06:58.3772048Z sync.runtime_SemacquireMutex(0xc00010e504, 0x0, 0x0)
2021-07-23T11:06:58.3772889Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/runtime/sema.go:71 +0x47
2021-07-23T11:06:58.3773581Z sync.(*RWMutex).RLock(0xc00010e4f8)
2021-07-23T11:06:58.3774310Z 	/opt/hostedtoolcache/go/1.14.15/x64/src/sync/rwmutex.go:50 +0xa4
2021-07-23T11:06:58.3775449Z github.com/nspcc-dev/neo-go/pkg/services/notary.(*Notary).getAccount(0xc00010e480, 0x0)
2021-07-23T11:06:58.3776626Z 	/home/runner/work/neo-go/neo-go/pkg/services/notary/node.go:51 +0x51
2021-07-23T11:06:58.3778270Z github.com/nspcc-dev/neo-go/pkg/services/notary.(*Notary).finalize(0xc00010e480, 0xc0003b2630, 0xa97df1bc78dd5787, 0xcc8a4d69e7f5d62a, 0x1a4d7981bd86b087, 0xbafdb720c93480b3, 0x0, 0x0)
2021-07-23T11:06:58.3779845Z 	/home/runner/work/neo-go/neo-go/pkg/services/notary/notary.go:306 +0x54
2021-07-23T11:06:58.3781022Z github.com/nspcc-dev/neo-go/pkg/services/notary.(*Notary).PostPersist(0xc00010e480)
2021-07-23T11:06:58.3782232Z 	/home/runner/work/neo-go/neo-go/pkg/services/notary/notary.go:297 +0x662
2021-07-23T11:06:58.3782989Z github.com/nspcc-dev/neo-go/pkg/services/notary.(*Notary).Run(0xc00010e480)
2021-07-23T11:06:58.3783941Z 	/home/runner/work/neo-go/neo-go/pkg/services/notary/notary.go:148 +0x3cb
2021-07-23T11:06:58.3784702Z created by github.com/nspcc-dev/neo-go/pkg/core.TestNotary
2021-07-23T11:06:58.3785451Z 	/home/runner/work/neo-go/neo-go/pkg/core/notary_test.go:132 +0x6e0
```

Signed-off-by: Evgeniy Stratonikov <evgeniy@nspcc.ru>
This commit is contained in:
Evgeniy Stratonikov 2021-07-23 14:28:11 +03:00
parent 3507f52c32
commit e2f2addf95
1 changed files with 8 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
pkg/services/notary/notary.go
View file

@ -157,7 +157,8 @@ func (n *Notary) Stop() {
// OnNewRequest is a callback method which is called after new notary request is added to the notary request pool. // OnNewRequest is a callback method which is called after new notary request is added to the notary request pool.
func (n *Notary) OnNewRequest(payload *payload.P2PNotaryRequest) { func (n *Notary) OnNewRequest(payload *payload.P2PNotaryRequest) {
if n.getAccount() == nil { acc := n.getAccount()
if acc == nil {
return return
} }
@ -243,7 +244,7 @@ func (n *Notary) OnNewRequest(payload *payload.P2PNotaryRequest) {
} }
} }
if r.typ != Unknown && r.nSigsCollected == nSigs && r.minNotValidBefore > n.Config.Chain.BlockHeight() { if r.typ != Unknown && r.nSigsCollected == nSigs && r.minNotValidBefore > n.Config.Chain.BlockHeight() {
if err := n.finalize(r.main, payload.MainTransaction.Hash()); err != nil { if err := n.finalize(acc, r.main, payload.MainTransaction.Hash()); err != nil {
n.Config.Log.Error("failed to finalize main transaction", zap.Error(err)) n.Config.Log.Error("failed to finalize main transaction", zap.Error(err))
} }
} }
@ -276,7 +277,8 @@ func (n *Notary) OnRequestRemoval(pld *payload.P2PNotaryRequest) {
// PostPersist is a callback which is called after new block event is received. // PostPersist is a callback which is called after new block event is received.
// PostPersist must not be called under the blockchain lock, because it uses finalization function. // PostPersist must not be called under the blockchain lock, because it uses finalization function.
func (n *Notary) PostPersist() { func (n *Notary) PostPersist() {
if n.getAccount() == nil { acc := n.getAccount()
if acc == nil {
return return
} }
@ -285,7 +287,7 @@ func (n *Notary) PostPersist() {
currHeight := n.Config.Chain.BlockHeight() currHeight := n.Config.Chain.BlockHeight()
for h, r := range n.requests { for h, r := range n.requests {
if !r.isSent && r.typ != Unknown && r.nSigs == r.nSigsCollected && r.minNotValidBefore > currHeight { if !r.isSent && r.typ != Unknown && r.nSigs == r.nSigsCollected && r.minNotValidBefore > currHeight {
if err := n.finalize(r.main, h); err != nil { if err := n.finalize(acc, r.main, h); err != nil {
n.Config.Log.Error("failed to finalize main transaction", zap.Error(err)) n.Config.Log.Error("failed to finalize main transaction", zap.Error(err))
} }
continue continue
@ -294,7 +296,7 @@ func (n *Notary) PostPersist() {
for _, fb := range r.fallbacks { for _, fb := range r.fallbacks {
if nvb := fb.GetAttributes(transaction.NotValidBeforeT)[0].Value.(*transaction.NotValidBefore).Height; nvb <= currHeight { if nvb := fb.GetAttributes(transaction.NotValidBeforeT)[0].Value.(*transaction.NotValidBefore).Height; nvb <= currHeight {
// Ignore the error, wait for the next block to resend them // Ignore the error, wait for the next block to resend them
_ = n.finalize(fb, h) _ = n.finalize(acc, fb, h)
} }
} }
} }
@ -302,11 +304,7 @@ func (n *Notary) PostPersist() {
} }
// finalize adds missing Notary witnesses to the transaction (main or fallback) and pushes it to the network. // finalize adds missing Notary witnesses to the transaction (main or fallback) and pushes it to the network.
func (n *Notary) finalize(tx *transaction.Transaction, h util.Uint256) error { func (n *Notary) finalize(acc *wallet.Account, tx *transaction.Transaction, h util.Uint256) error {
acc := n.getAccount()
if acc == nil {
panic(errors.New("no available Notary account")) // unreachable code, because all callers of `finalize` check that acc != nil
}
notaryWitness := transaction.Witness{ notaryWitness := transaction.Witness{
InvocationScript: append([]byte{byte(opcode.PUSHDATA1), 64}, acc.PrivateKey().SignHashable(uint32(n.Network), tx)...), InvocationScript: append([]byte{byte(opcode.PUSHDATA1), 64}, acc.PrivateKey().SignHashable(uint32(n.Network), tx)...),
VerificationScript: []byte{}, VerificationScript: []byte{},