From e2f2addf954a313c13b4e62f5d80fb7c85e494f1 Mon Sep 17 00:00:00 2001 From: Evgeniy Stratonikov Date: Fri, 23 Jul 2021 14:28:11 +0300 Subject: [PATCH] notary: fix possible deadlock in `UpdateNotaryNodes` `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 --- pkg/services/notary/notary.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/pkg/services/notary/notary.go b/pkg/services/notary/notary.go index 702c178fb..a99b7a359 100644 --- a/pkg/services/notary/notary.go +++ b/pkg/services/notary/notary.go @@ -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. func (n *Notary) OnNewRequest(payload *payload.P2PNotaryRequest) { - if n.getAccount() == nil { + acc := n.getAccount() + if acc == nil { 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 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)) } } @@ -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 must not be called under the blockchain lock, because it uses finalization function. func (n *Notary) PostPersist() { - if n.getAccount() == nil { + acc := n.getAccount() + if acc == nil { return } @@ -285,7 +287,7 @@ func (n *Notary) PostPersist() { currHeight := n.Config.Chain.BlockHeight() for h, r := range n.requests { 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)) } continue @@ -294,7 +296,7 @@ func (n *Notary) PostPersist() { for _, fb := range r.fallbacks { if nvb := fb.GetAttributes(transaction.NotValidBeforeT)[0].Value.(*transaction.NotValidBefore).Height; nvb <= currHeight { // 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. -func (n *Notary) finalize(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 - } +func (n *Notary) finalize(acc *wallet.Account, tx *transaction.Transaction, h util.Uint256) error { notaryWitness := transaction.Witness{ InvocationScript: append([]byte{byte(opcode.PUSHDATA1), 64}, acc.PrivateKey().SignHashable(uint32(n.Network), tx)...), VerificationScript: []byte{},