From 48c8ab598f060a4a7e56224e0e9be60cb36b7c74 Mon Sep 17 00:00:00 2001 From: Leonard Lyubich Date: Wed, 23 Aug 2023 23:16:48 +0400 Subject: [PATCH] sidechain/deploy: Fix and improve Notary request re-signing routine Prevent double processing. Support different number and order of signers. Skip transactions that have already been signed by the local account. Log main transaction hash. Signed-off-by: Leonard Lyubich --- pkg/morph/deploy/notary.go | 182 ++++++++++++++++++++++++++----------- 1 file changed, 130 insertions(+), 52 deletions(-) diff --git a/pkg/morph/deploy/notary.go b/pkg/morph/deploy/notary.go index 09955e5a95d..44c944239cb 100644 --- a/pkg/morph/deploy/notary.go +++ b/pkg/morph/deploy/notary.go @@ -1030,6 +1030,13 @@ func listenCommitteeNotaryRequests(ctx context.Context, prm listenCommitteeNotar return fmt.Errorf("compose committee multi-signature account: %w", err) } + ver, err := prm.blockchain.GetVersion() + if err != nil { + return fmt.Errorf("read protocol configuration: %w", err) + } + + netMagic := ver.Protocol.Network + localAccID := prm.localAcc.ScriptHash() validatorMultiSigAccID := prm.validatorMultiSigAcc.ScriptHash() committeeMultiSigAccID := committeeMultiSigAcc.ScriptHash() chNotaryRequests := make(chan *result.NotaryRequestEvent, 100) // secure from blocking @@ -1053,6 +1060,7 @@ func listenCommitteeNotaryRequests(ctx context.Context, prm listenCommitteeNotar prm.logger.Info("listening to committee notary requests...") + upperLoop: for { select { case <-ctx.Done(): @@ -1067,115 +1075,185 @@ func listenCommitteeNotaryRequests(ctx context.Context, prm listenCommitteeNotar // for simplicity, requests are handled one-by one. We could process them in parallel // using worker pool, but actions seem to be relatively lightweight - const expectedSignersCount = 3 // sender + committee|validator + Notary mainTx := notaryEvent.NotaryRequest.MainTransaction // note: instruction above can throw NPE and it's ok to panic: we confidently // expect that only non-nil pointers will come from the channel (NeoGo // guarantees) srcMainTxHash := mainTx.Hash() + l := prm.logger.With(zap.Stringer("tx", srcMainTxHash)) _, processed := mProcessedMainTxs[srcMainTxHash] + if processed { + l.Info("main transaction of the notary request has already been processed, skip") + continue + } + + mProcessedMainTxs[srcMainTxHash] = struct{}{} // revise severity level of the messages // https://github.com/nspcc-dev/neofs-node/issues/2419 switch { - case processed: - prm.logger.Info("main transaction of the notary request has already been processed, skip", - zap.Stringer("ID", srcMainTxHash)) - continue case notaryEvent.Type != mempoolevent.TransactionAdded: - prm.logger.Info("unsupported type of the notary request event, skip", + l.Info("unsupported type of the notary request event, skip", zap.Stringer("got", notaryEvent.Type), zap.Stringer("expect", mempoolevent.TransactionAdded)) continue - case len(mainTx.Signers) != expectedSignersCount: - prm.logger.Info("unsupported number of signers of main transaction from the received notary request, skip", - zap.Int("expected", expectedSignersCount), zap.Int("got", len(mainTx.Signers))) + case len(mainTx.Scripts) != len(mainTx.Signers): + l.Info("different number of signers and scripts of main transaction from the received notary request, skip") continue - case !mainTx.Signers[1].Account.Equals(committeeMultiSigAccID): - if !mainTx.Signers[1].Account.Equals(validatorMultiSigAccID) { - prm.logger.Info("2nd signer of main transaction from the received notary request is neither committee nor validator multi-sig account, skip") - continue + case len(mainTx.Signers) == 0 || !mainTx.Signers[len(mainTx.Signers)-1].Account.Equals(notary.Hash): + l.Info("Notary contract is not the last signer of main transaction from the received notary request, skip") + continue + } + + localAccSignerIndex := -1 + committeeMultiSigSignerIndex := -1 + validatorMultiSigSignerIndex := -1 + notaryContractSignerIndex := -1 + + for i := range mainTx.Signers { + switch mainTx.Signers[i].Account { + case notary.Hash: + notaryContractSignerIndex = i + case localAccID: + if len(mainTx.Scripts[i].InvocationScript) > 0 { + l.Info("main transaction from the received notary request already has local account's signature, skip") + continue upperLoop // correctness doesn't matter + } + + localAccSignerIndex = i + case committeeMultiSigAccID: + // simplified: we know binary format, so may match faster + if bytes.Contains(mainTx.Scripts[i].InvocationScript, committeeMultiSigAcc.SignHashable(netMagic, mainTx)) { + l.Info("main transaction from the received notary request already has local account's committee signature, skip") + continue upperLoop // correctness doesn't matter + } + + // we cannot differ missing signature from the incorrect one in this case + + committeeMultiSigSignerIndex = i + case validatorMultiSigAccID: + // simplified: we know binary format, so may match faster + if bytes.Contains(mainTx.Scripts[i].InvocationScript, prm.validatorMultiSigAcc.SignHashable(netMagic, mainTx)) { + l.Info("main transaction from the received notary request already has local account's committee signature, skip") + continue upperLoop // correctness doesn't matter + } + + // we cannot differ missing signature from the incorrect one in this case + + validatorMultiSigSignerIndex = i } - case mainTx.HasSigner(prm.localAcc.ScriptHash()): - prm.logger.Info("main transaction from the received notary request is signed by a local account, skip") + } + + if notaryContractSignerIndex < 0 { + l.Info("Notary contract is not a signer of main transaction of the received notary request, skip") continue - case len(mainTx.Scripts) == 0: - prm.logger.Info("missing scripts of main transaction from the received notary request, skip") + } + + if localAccSignerIndex < 0 && committeeMultiSigSignerIndex < 0 && validatorMultiSigSignerIndex < 0 { + l.Info("local account is not a signer of main transaction of the received notary request, skip") continue } - var payerAcc *wallet.Account + signers := make([]actor.SignerAccount, 0, len(mainTx.Signers)-1) // Notary contract added by actor - bSenderKey, ok := vm.ParseSignatureContract(mainTx.Scripts[0].VerificationScript) - if ok { - senderKey, err := keys.NewPublicKeyFromBytes(bSenderKey, elliptic.P256()) - if err != nil { - prm.logger.Info("failed to decode sender's public key from first script of main transaction from the received notary request, skip", zap.Error(err)) + for i := range mainTx.Signers { + if i == notaryContractSignerIndex { continue } - payerAcc = notary.FakeSimpleAccount(senderKey) - } else { - payerAcc = notary.FakeContractAccount(mainTx.Signers[0].Account) + var acc *wallet.Account + switch i { + case localAccSignerIndex: + acc = prm.localAcc + case committeeMultiSigSignerIndex: + acc = committeeMultiSigAcc + case validatorMultiSigSignerIndex: + acc = prm.validatorMultiSigAcc + default: + if len(mainTx.Scripts[i].VerificationScript) > 0 { + if bSenderKey, ok := vm.ParseSignatureContract(mainTx.Scripts[i].VerificationScript); ok { + senderKey, err := keys.NewPublicKeyFromBytes(bSenderKey, elliptic.P256()) + if err != nil { + l.Info("failed to decode public key from simple signature contract verification script of main transaction from the received notary request, skip", + zap.Int("script#", i), zap.Error(err)) + continue + } + + acc = notary.FakeSimpleAccount(senderKey) + } else if m, bKeys, ok := vm.ParseMultiSigContract(mainTx.Scripts[i].VerificationScript); ok { + pKeys := make(keys.PublicKeys, len(bKeys)) + for j := range bKeys { + err := pKeys[j].DecodeBytes(bKeys[j]) + if err != nil { + l.Info("failed to decode public key from multi-sig contract verification script of main transaction from the received notary request, skip", + zap.Int("script#", i), zap.Int("key#", j), zap.Error(err)) + continue + } + } + + acc, err = notary.FakeMultisigAccount(m, pKeys) + if err != nil { + l.Info("failed to build fake multi-sig account from verification script of main transaction from the received notary request, skip", + zap.Int("script#", i), zap.Error(err)) + continue + } + } else { + l.Info("got invalid/unsupported verification script in main transaction from the received notary request, skip", + zap.Int("script#", i)) + continue upperLoop + } + } else { + acc = notary.FakeContractAccount(mainTx.Signers[i].Account) + } + } + + signers = append(signers, actor.SignerAccount{ + Signer: mainTx.Signers[i], + Account: acc, + }) } // copy transaction to avoid pointer mutation mainTxCp := *mainTx - mainTxCp.Scripts = nil - mainTx = &mainTxCp // source one isn't needed anymore // it'd be safer to get into the transaction and analyze what it is trying to do. // For simplicity, now we blindly sign it. Track https://github.com/nspcc-dev/neofs-node/issues/2430 - prm.logger.Info("signing main transaction from the received notary request by the local account...") + l.Info("signing main transaction from the received notary request by the local account...") - var multiSigAcc *wallet.Account - if mainTx.Signers[1].Account.Equals(committeeMultiSigAccID) { - multiSigAcc = committeeMultiSigAcc - } else { - // note: switch-case above denies any other cases - multiSigAcc = prm.validatorMultiSigAcc - } + // reset all existing script because Notary actor adds itself + mainTx.Scripts = nil // create new actor for current signers. As a slight optimization, we could also // compare with signers of previously created actor and deduplicate. // See also https://github.com/nspcc-dev/neofs-node/issues/2314 - notaryActor, err := notary.NewActor(prm.blockchain, []actor.SignerAccount{ - { - Signer: mainTx.Signers[0], - Account: payerAcc, - }, - { - Signer: mainTx.Signers[1], - Account: multiSigAcc, - }, - }, prm.localAcc) + notaryActor, err := notary.NewActor(prm.blockchain, signers, prm.localAcc) if err != nil { // not really expected - prm.logger.Error("failed to init Notary request sender with signers from the main transaction of the received notary request", zap.Error(err)) + l.Error("failed to init Notary request sender with signers from the main transaction of the received notary request", zap.Error(err)) continue } err = notaryActor.Sign(mainTx) if err != nil { - prm.logger.Error("failed to sign main transaction from the received notary request by the local account, skip", zap.Error(err)) + l.Error("failed to sign main transaction from the received notary request by the local account, skip", zap.Error(err)) continue } - prm.logger.Info("sending new notary request with the main transaction signed by the local account...") + l.Info("sending new notary request with the main transaction signed by the local account...") _, _, _, err = notaryActor.Notarize(mainTx, nil) if err != nil { if isErrNotEnoughGAS(err) { - prm.logger.Info("insufficient Notary balance to send new Notary request with the main transaction signed by the local account, skip") + l.Info("insufficient Notary balance to send new Notary request with the main transaction signed by the local account, skip") } else { - prm.logger.Error("failed to send new Notary request with the main transaction signed by the local account, skip", zap.Error(err)) + l.Error("failed to send new Notary request with the main transaction signed by the local account, skip", zap.Error(err)) } continue } - prm.logger.Info("main transaction from the received notary request has been successfully signed and sent by the local account") + l.Info("main transaction from the received notary request has been successfully signed and sent by the local account") } } }()