From a517e1502b3060b29efad027fa2bac08e3765150 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Mon, 16 Nov 2020 13:47:21 +0100 Subject: [PATCH 01/16] Confirmations for keep public key submission Implemented a member public key submission monitoring mechanism which allow to confirm if the chain state is same as expected after the member submits its key. --- pkg/node/node.go | 95 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/pkg/node/node.go b/pkg/node/node.go index 7cb5d4701..51f027db8 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -6,6 +6,7 @@ import ( cecdsa "crypto/ecdsa" "encoding/hex" "fmt" + "strings" "time" "github.com/keep-network/keep-ecdsa/pkg/registry" @@ -250,9 +251,103 @@ func (n *Node) publishSignerPublicKey( return fmt.Errorf("failed to submit public key: [%v]", err) } + // Trigger the member public key submission monitoring as separate goroutine. + // This way the keygen result is decoupled from the eventual outcome + // of the `SubmitKeepPublicKey` transaction. This is safer because + // keygen process will return an error only in case when the submission + // reverts at the dry-run stage. In other cases, keygen will return + // and persist the signer (and its key shares) and will treat the keep + // as normally operating which gives more chances to avoid serious errors. + go n.monitorMemberPublicKeySubmission(keepAddress, publicKey) + return nil } +func (n *Node) monitorMemberPublicKeySubmission( + keepAddress common.Address, + publicKey [64]byte, +) { + maxIterations := 3 + blockConfirmations := uint64(12) + + for iteration := 1; iteration <= maxIterations; iteration++ { + logger.Infof( + "starting iteration [%v] of member public key "+ + "submission monitoring for keep: [%v]", + iteration, + keepAddress.Hex(), + ) + + currentBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() + if err != nil { + logger.Error( + "error during iteration [%v] of member public key "+ + "submission monitoring for keep [%v]: "+ + "[could not get current block: [%v]]", + iteration, + keepAddress.Hex(), + err, + ) + continue + } + + err = n.ethereumChain.BlockCounter().WaitForBlockHeight( + currentBlock + blockConfirmations, + ) + if err != nil { + logger.Error( + "error during iteration [%v] of member public key "+ + "submission monitoring for keep [%v]: "+ + "[could not wait for block height: [%v]]", + iteration, + keepAddress.Hex(), + err, + ) + continue + } + + publicKeyAlreadySubmittedErr := "Member already submitted a public key" + + // Make a blind public key submission attempt. + err = n.ethereumChain.SubmitKeepPublicKey(keepAddress, publicKey) + if err != nil { + // Check if the error says about an already submitted public key. + // Awful, but this is the only way to check whether a particular + // member submitted the key. + if strings.Contains(err.Error(), publicKeyAlreadySubmittedErr) { + logger.Infof( + "iteration [%v] of member public key submission "+ + "monitoring for keep [%v] confirmed the member public key", + iteration, + keepAddress.Hex(), + ) + return // terminate the monitoring goroutine + } + + // If the error is not the one we're waiting for, log it and move + // to the next iteration. + logger.Error( + "error during iteration [%v] of member public key "+ + "submission monitoring for keep [%v]: "+ + "[could not submit public key: [%v]]", + iteration, + keepAddress.Hex(), + err, + ) + continue + } + + // No error so the `SubmitKeepPublicKey` transaction did not revert. + // Move to the next iteration. + logger.Infof( + "iteration [%v] of member public key submission monitoring "+ + "for keep [%v] resubmitted the member public key", + iteration, + keepAddress.Hex(), + ) + } +} + // CalculateSignature calculates a signature over a digest with threshold // signer and publishes the result to the keep associated with the signer. // From 62e701101f242dc88f3e93ee18687f56ac522954 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 17 Nov 2020 11:17:52 +0100 Subject: [PATCH 02/16] Block confirmations for keep signature Implemented a block confirmations mechanism which allow to check if the chain state after signature publication is same as expected. --- pkg/node/node.go | 53 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 51f027db8..3be270143 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -9,6 +9,8 @@ import ( "strings" "time" + "github.com/keep-network/keep-common/pkg/chain/chainutil" + "github.com/keep-network/keep-ecdsa/pkg/registry" "github.com/ethereum/go-ethereum/crypto" @@ -28,6 +30,7 @@ var logger = log.Logger("keep-ecdsa") const monitorKeepPublicKeySubmissionTimeout = 30 * time.Minute const retryDelay = 1 * time.Second +const blockConfirmations = uint64(12) // Node holds interfaces to interact with the blockchain and network messages // transport layer. @@ -267,8 +270,7 @@ func (n *Node) monitorMemberPublicKeySubmission( keepAddress common.Address, publicKey [64]byte, ) { - maxIterations := 3 - blockConfirmations := uint64(12) + const maxIterations = 3 for iteration := 1; iteration <= maxIterations; iteration++ { logger.Infof( @@ -519,6 +521,53 @@ func (n *Node) publishSignature( continue } + currentBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() + if err != nil { + logger.Errorf( + "could not get current block while confirming "+ + "signature for keep [%s]: [%v] ", + keepAddress.String(), + err, + ) + time.Sleep(retryDelay) // TODO: #413 Replace with backoff. + continue + } + + isSignatureConfirmed, err := chainutil.WaitForBlockConfirmations( + n.ethereumChain.BlockCounter(), + currentBlock, + blockConfirmations, + func() (bool, error) { + isAwaitingSignature, err := n.ethereumChain.IsAwaitingSignature( + keepAddress, + digest, + ) + if err != nil { + return false, err + } + + return !isAwaitingSignature, nil + }, + ) + if err != nil { + logger.Errorf( + "could not confirm signature for keep [%s]: [%v] ", + keepAddress.String(), + err, + ) + time.Sleep(retryDelay) // TODO: #413 Replace with backoff. + continue + } + + if !isSignatureConfirmed { + logger.Errorf( + "signature for keep [%s] is not confirmed", + keepAddress.String(), + ) + time.Sleep(retryDelay) // TODO: #413 Replace with backoff. + continue + } + logger.Infof("signature submitted for keep [%s]", keepAddress.String()) return nil } From 875e3cc3925acb5cdcfae439eb7b118baef5eadc Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 17 Nov 2020 13:38:43 +0100 Subject: [PATCH 03/16] Rework block confirmations for keep keygen --- pkg/node/node.go | 217 +++++++++++++++++++++-------------------------- 1 file changed, 97 insertions(+), 120 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 3be270143..525c0573c 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -6,7 +6,6 @@ import ( cecdsa "crypto/ecdsa" "encoding/hex" "fmt" - "strings" "time" "github.com/keep-network/keep-common/pkg/chain/chainutil" @@ -245,111 +244,16 @@ func (n *Node) publishSignerPublicKey( publicKey, ) - monitoringAbort := make(chan interface{}) - go n.monitorKeepPublicKeySubmission(monitoringAbort, keepAddress) - err := n.ethereumChain.SubmitKeepPublicKey(keepAddress, publicKey) if err != nil { - close(monitoringAbort) return fmt.Errorf("failed to submit public key: [%v]", err) } - // Trigger the member public key submission monitoring as separate goroutine. - // This way the keygen result is decoupled from the eventual outcome - // of the `SubmitKeepPublicKey` transaction. This is safer because - // keygen process will return an error only in case when the submission - // reverts at the dry-run stage. In other cases, keygen will return - // and persist the signer (and its key shares) and will treat the keep - // as normally operating which gives more chances to avoid serious errors. - go n.monitorMemberPublicKeySubmission(keepAddress, publicKey) + go n.monitorKeepPublicKeySubmission(keepAddress, publicKey) return nil } -func (n *Node) monitorMemberPublicKeySubmission( - keepAddress common.Address, - publicKey [64]byte, -) { - const maxIterations = 3 - - for iteration := 1; iteration <= maxIterations; iteration++ { - logger.Infof( - "starting iteration [%v] of member public key "+ - "submission monitoring for keep: [%v]", - iteration, - keepAddress.Hex(), - ) - - currentBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() - if err != nil { - logger.Error( - "error during iteration [%v] of member public key "+ - "submission monitoring for keep [%v]: "+ - "[could not get current block: [%v]]", - iteration, - keepAddress.Hex(), - err, - ) - continue - } - - err = n.ethereumChain.BlockCounter().WaitForBlockHeight( - currentBlock + blockConfirmations, - ) - if err != nil { - logger.Error( - "error during iteration [%v] of member public key "+ - "submission monitoring for keep [%v]: "+ - "[could not wait for block height: [%v]]", - iteration, - keepAddress.Hex(), - err, - ) - continue - } - - publicKeyAlreadySubmittedErr := "Member already submitted a public key" - - // Make a blind public key submission attempt. - err = n.ethereumChain.SubmitKeepPublicKey(keepAddress, publicKey) - if err != nil { - // Check if the error says about an already submitted public key. - // Awful, but this is the only way to check whether a particular - // member submitted the key. - if strings.Contains(err.Error(), publicKeyAlreadySubmittedErr) { - logger.Infof( - "iteration [%v] of member public key submission "+ - "monitoring for keep [%v] confirmed the member public key", - iteration, - keepAddress.Hex(), - ) - return // terminate the monitoring goroutine - } - - // If the error is not the one we're waiting for, log it and move - // to the next iteration. - logger.Error( - "error during iteration [%v] of member public key "+ - "submission monitoring for keep [%v]: "+ - "[could not submit public key: [%v]]", - iteration, - keepAddress.Hex(), - err, - ) - continue - } - - // No error so the `SubmitKeepPublicKey` transaction did not revert. - // Move to the next iteration. - logger.Infof( - "iteration [%v] of member public key submission monitoring "+ - "for keep [%v] resubmitted the member public key", - iteration, - keepAddress.Hex(), - ) - } -} - // CalculateSignature calculates a signature over a digest with threshold // signer and publishes the result to the keep associated with the signer. // @@ -577,8 +481,8 @@ func (n *Node) publishSignature( // conflicting public key is published or until keep established public key // or until key generation timed out. func (n *Node) monitorKeepPublicKeySubmission( - abort chan interface{}, keepAddress common.Address, + publicKey [64]byte, ) { monitoringCtx, monitoringCancel := context.WithTimeout( context.Background(), @@ -620,32 +524,105 @@ func (n *Node) monitorKeepPublicKeySubmission( defer subscriptionConflictingPublicKey.Unsubscribe() defer subscriptionPublicKeyPublished.Unsubscribe() - select { - case event := <-publicKeyPublished: - logger.Infof( - "public key [%x] has been accepted by keep: [%s]", - event.PublicKey, - keepAddress.String(), - ) - case event := <-conflictingPublicKey: + startingBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() + if err != nil { logger.Errorf( - "member [%x] has submitted conflicting public key for keep [%s]: [%x]", - event.SubmittingMember, - keepAddress.String(), - event.ConflictingPublicKey, - ) - case <-monitoringCtx.Done(): - logger.Warningf( - "monitoring of public key submission for keep [%s] "+ - "has been cancelled: [%v]", + "failed to get the starting block while setting up "+ + "public key submission monitoring for keep [%s]: [%v]", keepAddress.String(), - monitoringCtx.Err(), + err, ) - case <-abort: - logger.Warningf( - "monitoring of public key submission for keep [%s] "+ - "has been aborted", + return + } + + pubkeyCheckTrigger, err := n.ethereumChain.BlockCounter().BlockHeightWaiter( + startingBlock + blockConfirmations, + ) + if err != nil { + logger.Errorf( + "failed to get the check trigger while setting up "+ + "public key submission monitoring for keep [%s]: [%v]", keepAddress.String(), + err, ) + return + } + + for { + select { + case event := <-publicKeyPublished: + logger.Infof( + "public key [%x] has been accepted by keep: [%s]", + event.PublicKey, + keepAddress.String(), + ) + case event := <-conflictingPublicKey: + logger.Errorf( + "member [%x] has submitted conflicting public key for keep [%s]: [%x]", + event.SubmittingMember, + keepAddress.String(), + event.ConflictingPublicKey, + ) + case pubkeyCheckBlock := <-pubkeyCheckTrigger: + logger.Infof( + "checking public key submission for keep [%s] "+ + "at block [%v]", + keepAddress.String(), + pubkeyCheckBlock, + ) + + keepPublicKey, err := n.ethereumChain.GetPublicKey(keepAddress) + if err != nil { + logger.Errorf( + "failed to get keep public key during "+ + "public key submission monitoring for keep [%s]: [%v]", + keepAddress.String(), + err, + ) + } else { + if len(keepPublicKey) > 0 { + logger.Infof( + "public key [%x] for keep [%s] "+ + "has been confirmed", + keepPublicKey, + keepAddress.String(), + ) + return + } + } + + err = n.ethereumChain.SubmitKeepPublicKey(keepAddress, publicKey) + if err != nil { + logger.Errorf( + "keep [%s] still does not have public key "+ + "after [%v] blocks and resubmission by this member "+ + "failed with: [%v]", + keepAddress.String(), + pubkeyCheckBlock-startingBlock, + err, + ) + } + + pubkeyCheckTrigger, err = n.ethereumChain.BlockCounter().BlockHeightWaiter( + pubkeyCheckBlock + blockConfirmations, + ) + if err != nil { + logger.Errorf( + "failed to update the check trigger during "+ + "public key submission monitoring for keep [%s]: [%v]", + keepAddress.String(), + err, + ) + return + } + case <-monitoringCtx.Done(): + logger.Warningf( + "monitoring of public key submission for keep [%s] "+ + "has been cancelled: [%v]", + keepAddress.String(), + monitoringCtx.Err(), + ) + return + } } } From eaf58c89155995458f877dabd9a1ac4f0470452c Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Tue, 17 Nov 2020 16:59:58 +0100 Subject: [PATCH 04/16] Next rework of block confirmations for keep keygen --- pkg/node/node.go | 66 +++++++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 525c0573c..c50449ead 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -490,23 +490,8 @@ func (n *Node) monitorKeepPublicKeySubmission( ) defer monitoringCancel() - publicKeyPublished := make(chan *eth.PublicKeyPublishedEvent) conflictingPublicKey := make(chan *eth.ConflictingPublicKeySubmittedEvent) - subscriptionPublicKeyPublished, err := n.ethereumChain.OnPublicKeyPublished( - keepAddress, - func(event *eth.PublicKeyPublishedEvent) { - publicKeyPublished <- event - }, - ) - if err != nil { - logger.Errorf( - "failed on watching public key published event for keep [%s]: [%v]", - keepAddress.String(), - err, - ) - } - subscriptionConflictingPublicKey, err := n.ethereumChain.OnConflictingPublicKeySubmitted( keepAddress, func(event *eth.ConflictingPublicKeySubmittedEvent) { @@ -522,7 +507,6 @@ func (n *Node) monitorKeepPublicKeySubmission( } defer subscriptionConflictingPublicKey.Unsubscribe() - defer subscriptionPublicKeyPublished.Unsubscribe() startingBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() if err != nil { @@ -550,12 +534,6 @@ func (n *Node) monitorKeepPublicKeySubmission( for { select { - case event := <-publicKeyPublished: - logger.Infof( - "public key [%x] has been accepted by keep: [%s]", - event.PublicKey, - keepAddress.String(), - ) case event := <-conflictingPublicKey: logger.Errorf( "member [%x] has submitted conflicting public key for keep [%s]: [%x]", @@ -563,6 +541,7 @@ func (n *Node) monitorKeepPublicKeySubmission( keepAddress.String(), event.ConflictingPublicKey, ) + return case pubkeyCheckBlock := <-pubkeyCheckTrigger: logger.Infof( "checking public key submission for keep [%s] "+ @@ -581,13 +560,40 @@ func (n *Node) monitorKeepPublicKeySubmission( ) } else { if len(keepPublicKey) > 0 { - logger.Infof( - "public key [%x] for keep [%s] "+ - "has been confirmed", - keepPublicKey, - keepAddress.String(), + isConfirmed, err := chainutil.WaitForBlockConfirmations( + n.ethereumChain.BlockCounter(), + pubkeyCheckBlock, + blockConfirmations, + func() (bool, error) { + key, err := n.ethereumChain.GetPublicKey( + keepAddress, + ) + if err != nil { + return false, err + } + + return len(key) > 0, nil + }, ) - return + if err != nil { + logger.Errorf( + "failed to perform keep public key "+ + "confirmation during public key submission "+ + "monitoring for keep [%s]: [%v]", + keepAddress.String(), + err, + ) + } + + if isConfirmed { + logger.Infof( + "public key [%x] for keep [%s] "+ + "has been confirmed", + keepPublicKey, + keepAddress.String(), + ) + return + } } } @@ -595,10 +601,8 @@ func (n *Node) monitorKeepPublicKeySubmission( if err != nil { logger.Errorf( "keep [%s] still does not have public key "+ - "after [%v] blocks and resubmission by this member "+ - "failed with: [%v]", + "and resubmission by this member failed with: [%v]", keepAddress.String(), - pubkeyCheckBlock-startingBlock, err, ) } From 7df2aa65e5518e09e3d6e6b9f22c4c420032b825 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 18 Nov 2020 11:40:26 +0100 Subject: [PATCH 05/16] Improve docs around chain confirmations --- pkg/node/node.go | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index c50449ead..0431f3b36 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -465,7 +465,8 @@ func (n *Node) publishSignature( if !isSignatureConfirmed { logger.Errorf( - "signature for keep [%s] is not confirmed", + "signature submission for keep [%s] not confirmed; "+ + "trying to submit again", keepAddress.String(), ) time.Sleep(retryDelay) // TODO: #413 Replace with backoff. @@ -479,7 +480,23 @@ func (n *Node) publishSignature( // monitorKeepPublicKeySubmission observes the chain until either the first // conflicting public key is published or until keep established public key -// or until key generation timed out. +// or until the key generation timed out. It also tries to re-submit the public +// key if it has not been submitted in the given time frame as a way to protect +// against chain forks (i.e. transaction mined in a fork that has been dropped). +// +// When event informing about conflicting public key is emitted from the chain, +// monitoring stops immediately. Even if this event was emitted in a minority +// fork, it is clear that the operator who submitted the conflicting key is +// dishonest and it is better to abandon this keep. +// +// Function checks the chain periodically and inspects whether the public key +// has been registered on the chain. If the key is there, the function waits +// for the required number of confirmations and repeats the check. If the public +// key for the keep is still there, the monitoring exits successfully. +// In the case when public key for the keep is not yet established during the +// periodic check or it is gone after waiting for a certain number of +// confirmations (chain reorganization), this function will attempt to submit +// the public key again. func (n *Node) monitorKeepPublicKeySubmission( keepAddress common.Address, publicKey [64]byte, @@ -524,7 +541,7 @@ func (n *Node) monitorKeepPublicKeySubmission( ) if err != nil { logger.Errorf( - "failed to get the check trigger while setting up "+ + "failed to set up the check trigger while setting up "+ "public key submission monitoring for keep [%s]: [%v]", keepAddress.String(), err, @@ -535,6 +552,14 @@ func (n *Node) monitorKeepPublicKeySubmission( for { select { case event := <-conflictingPublicKey: + // Even in the case of a fork, a transaction with a conflicting + // public key submitted to the chain had to be signed with the + // operator's pubkey. For an honest operator, this situation should + // never happen, and if it happened, it is better to exit the + // monitoring immediately and do not re-attempt to submit public + // key for this node. It is clear that something is wrong with the + // operator that published the conflicting key and it is safer to + // abandon this keep. logger.Errorf( "member [%x] has submitted conflicting public key for keep [%s]: [%x]", event.SubmittingMember, @@ -550,6 +575,10 @@ func (n *Node) monitorKeepPublicKeySubmission( pubkeyCheckBlock, ) + // We check the public key periodically instead of relying on + // incoming events. The main motivation is that events could not be + // trusted because they may come from a forked chain or can + // the same event can be delivered multiple times. keepPublicKey, err := n.ethereumChain.GetPublicKey(keepAddress) if err != nil { logger.Errorf( From dc46f52852810cc5a04495e1183467951c2f86df Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 18 Nov 2020 13:28:20 +0100 Subject: [PATCH 06/16] Add logging before public key re-submission --- pkg/node/node.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 0431f3b36..366288ff3 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -626,10 +626,17 @@ func (n *Node) monitorKeepPublicKeySubmission( } } + logger.Infof( + "keep [%s] still does not have a confirmed public key; "+ + "re-submitting public key [%x]", + keepAddress.String(), + publicKey, + ) + err = n.ethereumChain.SubmitKeepPublicKey(keepAddress, publicKey) if err != nil { logger.Errorf( - "keep [%s] still does not have public key "+ + "keep [%s] still does not have a confirmed public key "+ "and resubmission by this member failed with: [%v]", keepAddress.String(), err, From 8836aef4b304d6e0246fc8b13867adbbbf1aa80c Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 18 Nov 2020 13:49:19 +0100 Subject: [PATCH 07/16] Rework signature confirmation process --- pkg/node/node.go | 105 ++++++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 43 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 366288ff3..e4930aaa6 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -375,7 +375,7 @@ func (n *Node) publishSignature( // Someone submitted the signature and it was accepted by the keep. // We are fine, leaving. - if !isAwaitingSignature { + if !isAwaitingSignature && n.confirmSignature(keepAddress, digest) { logger.Infof( "signature for keep [%s] already submitted: [%+x]", keepAddress.String(), @@ -404,7 +404,7 @@ func (n *Node) publishSignature( // Check if we failed because someone else submitted in the meantime // or because something wrong happened with our transaction. - if !isAwaitingSignature { + if !isAwaitingSignature && n.confirmSignature(keepAddress, digest) { logger.Infof( "signature for keep [%s] already submitted: [%+x]", keepAddress.String(), @@ -425,57 +425,76 @@ func (n *Node) publishSignature( continue } - currentBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() - if err != nil { - logger.Errorf( - "could not get current block while confirming "+ - "signature for keep [%s]: [%v] ", - keepAddress.String(), - err, - ) + if !n.confirmSignature(keepAddress, digest) { time.Sleep(retryDelay) // TODO: #413 Replace with backoff. continue } - isSignatureConfirmed, err := chainutil.WaitForBlockConfirmations( - n.ethereumChain.BlockCounter(), - currentBlock, - blockConfirmations, - func() (bool, error) { - isAwaitingSignature, err := n.ethereumChain.IsAwaitingSignature( - keepAddress, - digest, - ) - if err != nil { - return false, err - } + logger.Infof("signature submitted for keep [%s]", keepAddress.String()) + return nil + } +} + +func (n *Node) confirmSignature( + keepAddress common.Address, + digest [32]byte, +) bool { + logger.Infof( + "starting confirming signature submission for keep [%s]", + keepAddress.String(), + ) - return !isAwaitingSignature, nil - }, + currentBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() + if err != nil { + logger.Errorf( + "could not get current block while confirming "+ + "signature submission for keep [%s]: [%v]", + keepAddress.String(), + err, ) - if err != nil { - logger.Errorf( - "could not confirm signature for keep [%s]: [%v] ", - keepAddress.String(), - err, - ) - time.Sleep(retryDelay) // TODO: #413 Replace with backoff. - continue - } + return false + } - if !isSignatureConfirmed { - logger.Errorf( - "signature submission for keep [%s] not confirmed; "+ - "trying to submit again", - keepAddress.String(), + isSignatureConfirmed, err := chainutil.WaitForBlockConfirmations( + n.ethereumChain.BlockCounter(), + currentBlock, + blockConfirmations, + func() (bool, error) { + isAwaitingSignature, err := n.ethereumChain.IsAwaitingSignature( + keepAddress, + digest, ) - time.Sleep(retryDelay) // TODO: #413 Replace with backoff. - continue - } + if err != nil { + return false, err + } - logger.Infof("signature submitted for keep [%s]", keepAddress.String()) - return nil + return !isAwaitingSignature, nil + }, + ) + if err != nil { + logger.Errorf( + "could not confirm signature submission for keep [%s]: [%v]", + keepAddress.String(), + err, + ) + return false + } + + if !isSignatureConfirmed { + logger.Errorf( + "signature submission for keep [%s] not confirmed; "+ + "trying to submit the signature again", + keepAddress.String(), + ) + return false } + + logger.Infof( + "signature submission for keep [%s] has been confirmed", + keepAddress.String(), + ) + + return true } // monitorKeepPublicKeySubmission observes the chain until either the first From b4db118ad6401bdece3f59fc33b2bd91ca005aff Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Wed, 18 Nov 2020 15:31:48 +0100 Subject: [PATCH 08/16] Improve docs around signature confirmations --- pkg/node/node.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index e4930aaa6..0e536d0cb 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -373,7 +373,8 @@ func (n *Node) publishSignature( continue } - // Someone submitted the signature and it was accepted by the keep. + // Someone submitted the signature, it was accepted by the keep, + // and there are enough confirmations from the chain. // We are fine, leaving. if !isAwaitingSignature && n.confirmSignature(keepAddress, digest) { logger.Infof( @@ -404,6 +405,9 @@ func (n *Node) publishSignature( // Check if we failed because someone else submitted in the meantime // or because something wrong happened with our transaction. + // If someone else submitted in the meantime, wait for enough + // confirmations from the chain before making a decision about + // leaving the submission process. if !isAwaitingSignature && n.confirmSignature(keepAddress, digest) { logger.Infof( "signature for keep [%s] already submitted: [%+x]", From 4c38a633b681a11827f2d643d5fa0581f23db760 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 19 Nov 2020 13:04:52 +0100 Subject: [PATCH 09/16] Rework public key monitoring loop --- pkg/node/node.go | 88 +++++++++++++++++++----------------------------- 1 file changed, 34 insertions(+), 54 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 0e536d0cb..c5eadf7eb 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -524,12 +524,6 @@ func (n *Node) monitorKeepPublicKeySubmission( keepAddress common.Address, publicKey [64]byte, ) { - monitoringCtx, monitoringCancel := context.WithTimeout( - context.Background(), - monitorKeepPublicKeySubmissionTimeout, - ) - defer monitoringCancel() - conflictingPublicKey := make(chan *eth.ConflictingPublicKeySubmittedEvent) subscriptionConflictingPublicKey, err := n.ethereumChain.OnConflictingPublicKeySubmitted( @@ -548,29 +542,12 @@ func (n *Node) monitorKeepPublicKeySubmission( defer subscriptionConflictingPublicKey.Unsubscribe() - startingBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() - if err != nil { - logger.Errorf( - "failed to get the starting block while setting up "+ - "public key submission monitoring for keep [%s]: [%v]", - keepAddress.String(), - err, - ) - return - } + pubkeyChecksCounter := 0 + const maxPubkeyChecksCount = 6 + const pubkeyCheckTick = 10 * time.Minute - pubkeyCheckTrigger, err := n.ethereumChain.BlockCounter().BlockHeightWaiter( - startingBlock + blockConfirmations, - ) - if err != nil { - logger.Errorf( - "failed to set up the check trigger while setting up "+ - "public key submission monitoring for keep [%s]: [%v]", - keepAddress.String(), - err, - ) - return - } + pubkeyCheckTicker := time.NewTicker(pubkeyCheckTick) + defer pubkeyCheckTicker.Stop() for { select { @@ -590,12 +567,22 @@ func (n *Node) monitorKeepPublicKeySubmission( event.ConflictingPublicKey, ) return - case pubkeyCheckBlock := <-pubkeyCheckTrigger: + case <-pubkeyCheckTicker.C: + pubkeyChecksCounter++ + + if pubkeyChecksCounter > maxPubkeyChecksCount { + logger.Infof( + "monitoring of public key submission for keep [%s] "+ + "has been cancelled because maximum checks count [%v] "+ + "has been reached", + keepAddress.String(), + maxPubkeyChecksCount, + ) + } + logger.Infof( - "checking public key submission for keep [%s] "+ - "at block [%v]", + "checking public key submission for keep [%s]", keepAddress.String(), - pubkeyCheckBlock, ) // We check the public key periodically instead of relying on @@ -610,11 +597,24 @@ func (n *Node) monitorKeepPublicKeySubmission( keepAddress.String(), err, ) + continue } else { if len(keepPublicKey) > 0 { + currentBlock, err := n.ethereumChain.BlockCounter().CurrentBlock() + if err != nil { + logger.Errorf( + "failed to get the current block while "+ + "performing public key submission confirmation "+ + "for keep [%s]: [%v]", + keepAddress.String(), + err, + ) + continue + } + isConfirmed, err := chainutil.WaitForBlockConfirmations( n.ethereumChain.BlockCounter(), - pubkeyCheckBlock, + currentBlock, blockConfirmations, func() (bool, error) { key, err := n.ethereumChain.GetPublicKey( @@ -635,6 +635,7 @@ func (n *Node) monitorKeepPublicKeySubmission( keepAddress.String(), err, ) + continue } if isConfirmed { @@ -665,27 +666,6 @@ func (n *Node) monitorKeepPublicKeySubmission( err, ) } - - pubkeyCheckTrigger, err = n.ethereumChain.BlockCounter().BlockHeightWaiter( - pubkeyCheckBlock + blockConfirmations, - ) - if err != nil { - logger.Errorf( - "failed to update the check trigger during "+ - "public key submission monitoring for keep [%s]: [%v]", - keepAddress.String(), - err, - ) - return - } - case <-monitoringCtx.Done(): - logger.Warningf( - "monitoring of public key submission for keep [%s] "+ - "has been cancelled: [%v]", - keepAddress.String(), - monitoringCtx.Err(), - ) - return } } } From 5462c29126760229b965e79b13ea418185b07f86 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 19 Nov 2020 13:09:59 +0100 Subject: [PATCH 10/16] Change logging around `confirmSignature` --- pkg/node/node.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index c5eadf7eb..9034dfd33 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -434,7 +434,6 @@ func (n *Node) publishSignature( continue } - logger.Infof("signature submitted for keep [%s]", keepAddress.String()) return nil } } @@ -494,7 +493,7 @@ func (n *Node) confirmSignature( } logger.Infof( - "signature submission for keep [%s] has been confirmed", + "signature for keep [%s] successfully submitted and confirmed", keepAddress.String(), ) From 13d7696e611d39af370940d35734feb4f8a865a0 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 19 Nov 2020 13:27:54 +0100 Subject: [PATCH 11/16] Add signature waiter --- pkg/node/node.go | 57 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 9034dfd33..a8612a0fe 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -429,7 +429,7 @@ func (n *Node) publishSignature( continue } - if !n.confirmSignature(keepAddress, digest) { + if !(n.waitForSignature(keepAddress, digest) && n.confirmSignature(keepAddress, digest)) { time.Sleep(retryDelay) // TODO: #413 Replace with backoff. continue } @@ -438,6 +438,61 @@ func (n *Node) publishSignature( } } +func (n *Node) waitForSignature( + keepAddress common.Address, + digest [32]byte, +) bool { + const waitTimeout = 10 * time.Minute + const checkTick = 1 * time.Minute + + ctx, cancelCtx := context.WithTimeout(context.Background(), waitTimeout) + defer cancelCtx() + + checkTicker := time.NewTicker(checkTick) + defer checkTicker.Stop() + + logger.Infof( + "starting waiting for signature for keep [%s]", + keepAddress.String(), + ) + + for { + select { + case <-checkTicker.C: + isAwaitingSignature, err := n.ethereumChain.IsAwaitingSignature( + keepAddress, + digest, + ) + if err != nil { + logger.Error( + "failed to perform signature check while waiting "+ + "for signature for keep [%s]: [%v]", + keepAddress.String(), + err, + ) + continue + } + + if !isAwaitingSignature { + logger.Infof( + "signature waiter for keep [%s] has "+ + "detected the signature", + keepAddress.String(), + ) + return true + } + case <-ctx.Done(): + logger.Error( + "stop waiting for signature for keep [%s] because "+ + "[%v] timeout has been exceeded", + keepAddress.String(), + waitTimeout, + ) + return false + } + } +} + func (n *Node) confirmSignature( keepAddress common.Address, digest [32]byte, From 52e1e6fa5f90bf2994aa5c8e199aa7380edc7a49 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 19 Nov 2020 13:52:23 +0100 Subject: [PATCH 12/16] Minor tweaks for pubkey confirmations --- pkg/node/node.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index a8612a0fe..b94c84d14 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -597,7 +597,7 @@ func (n *Node) monitorKeepPublicKeySubmission( defer subscriptionConflictingPublicKey.Unsubscribe() pubkeyChecksCounter := 0 - const maxPubkeyChecksCount = 6 + const maxPubkeyChecksCount = 3 const pubkeyCheckTick = 10 * time.Minute pubkeyCheckTicker := time.NewTicker(pubkeyCheckTick) @@ -635,7 +635,7 @@ func (n *Node) monitorKeepPublicKeySubmission( } logger.Infof( - "checking public key submission for keep [%s]", + "confirming public key submission for keep [%s]", keepAddress.String(), ) From a8a4bd3f75237f01833e813de73694e9c2a2626f Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Thu, 19 Nov 2020 16:32:45 +0100 Subject: [PATCH 13/16] Return on keep public key re-submission error --- pkg/node/node.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/node/node.go b/pkg/node/node.go index b94c84d14..f9bfa9f71 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -719,6 +719,7 @@ func (n *Node) monitorKeepPublicKeySubmission( keepAddress.String(), err, ) + return } } } From 503e614bab7d4422ca8d3c01d6973f7ecc949396 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Fri, 20 Nov 2020 13:45:27 +0100 Subject: [PATCH 14/16] Docs and logging improvements around confirmations --- pkg/node/node.go | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index f9bfa9f71..5f4c328ec 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -452,13 +452,17 @@ func (n *Node) waitForSignature( defer checkTicker.Stop() logger.Infof( - "starting waiting for signature for keep [%s]", + "waiting for signature for keep [%s] to appear on-chain", keepAddress.String(), ) for { select { case <-checkTicker.C: + // We check the public key periodically instead of relying on + // incoming events. The main motivation is that events could not be + // trusted here because they may come from a forked chain or + // the same event can be delivered multiple times. isAwaitingSignature, err := n.ethereumChain.IsAwaitingSignature( keepAddress, digest, @@ -475,8 +479,7 @@ func (n *Node) waitForSignature( if !isAwaitingSignature { logger.Infof( - "signature waiter for keep [%s] has "+ - "detected the signature", + "signature for keep [%s] appeared on-chain", keepAddress.String(), ) return true @@ -498,7 +501,7 @@ func (n *Node) confirmSignature( digest [32]byte, ) bool { logger.Infof( - "starting confirming signature submission for keep [%s]", + "confirming on-chain signature submission for keep [%s]", keepAddress.String(), ) @@ -548,7 +551,8 @@ func (n *Node) confirmSignature( } logger.Infof( - "signature for keep [%s] successfully submitted and confirmed", + "signature for keep [%s] successfully submitted "+ + "and confirmed on-chain", keepAddress.String(), ) @@ -597,7 +601,15 @@ func (n *Node) monitorKeepPublicKeySubmission( defer subscriptionConflictingPublicKey.Unsubscribe() pubkeyChecksCounter := 0 + // There is no way to determine whether keep waits for public key submission + // from this client or some other client. Given that the consequences are + // not that serious as for not submitting a signature and to minimize gas + // expenditure in case the current client's pub key has been properly + // registered and we are waiting for someone else, we retry only three times. const maxPubkeyChecksCount = 3 + // All three operators need to submit public key to the chain so we are + // less aggressive with check ticks than in case of signature submission + // where only one signature is enough. const pubkeyCheckTick = 10 * time.Minute pubkeyCheckTicker := time.NewTicker(pubkeyCheckTick) @@ -635,13 +647,13 @@ func (n *Node) monitorKeepPublicKeySubmission( } logger.Infof( - "confirming public key submission for keep [%s]", + "confirming on-chain public key submission for keep [%s]", keepAddress.String(), ) // We check the public key periodically instead of relying on // incoming events. The main motivation is that events could not be - // trusted because they may come from a forked chain or can + // trusted here because they may come from a forked chain or // the same event can be delivered multiple times. keepPublicKey, err := n.ethereumChain.GetPublicKey(keepAddress) if err != nil { @@ -694,8 +706,8 @@ func (n *Node) monitorKeepPublicKeySubmission( if isConfirmed { logger.Infof( - "public key [%x] for keep [%s] "+ - "has been confirmed", + "public key [%x] for keep [%s] successfully "+ + "submitted and confirmed on-chain", keepPublicKey, keepAddress.String(), ) From eaf475d65b1421eb247f8d7329324b83543ce1c1 Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Fri, 20 Nov 2020 13:47:58 +0100 Subject: [PATCH 15/16] Remove `publishSignerPublicKey` method --- pkg/node/node.go | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 5f4c328ec..5ced06217 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -215,7 +215,7 @@ func (n *Node) GenerateSignerForKeep( ) } - // Serialize and publish public key to the keep. + // Serialize and submit public key to the keep. // // We don't retry in case of an error although the specific chain // implementation may implement its own retry policy. This action @@ -224,34 +224,16 @@ func (n *Node) GenerateSignerForKeep( if err != nil { return nil, fmt.Errorf("failed to serialize public key: [%v]", err) } - err = n.publishSignerPublicKey(ctx, keepAddress, publicKey) + + err = n.ethereumChain.SubmitKeepPublicKey(keepAddress, publicKey) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to submit public key: [%v]", err) } - return signer, nil // key generation succeeded. - } -} + go n.monitorKeepPublicKeySubmission(keepAddress, publicKey) -func (n *Node) publishSignerPublicKey( - ctx context.Context, - keepAddress common.Address, - publicKey [64]byte, -) error { - logger.Debugf( - "submitting public key to the keep [%s]: [%x]", - keepAddress.String(), - publicKey, - ) - - err := n.ethereumChain.SubmitKeepPublicKey(keepAddress, publicKey) - if err != nil { - return fmt.Errorf("failed to submit public key: [%v]", err) + return signer, nil // key generation succeeded. } - - go n.monitorKeepPublicKeySubmission(keepAddress, publicKey) - - return nil } // CalculateSignature calculates a signature over a digest with threshold From 676b6f6207cc76fcd3221b070616d974b964c7ea Mon Sep 17 00:00:00 2001 From: Lukasz Zimnoch Date: Fri, 20 Nov 2020 16:12:04 +0100 Subject: [PATCH 16/16] Improve log on signature wait timeout --- pkg/node/node.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/node/node.go b/pkg/node/node.go index 5ced06217..92a641993 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -450,7 +450,7 @@ func (n *Node) waitForSignature( digest, ) if err != nil { - logger.Error( + logger.Errorf( "failed to perform signature check while waiting "+ "for signature for keep [%s]: [%v]", keepAddress.String(), @@ -467,9 +467,9 @@ func (n *Node) waitForSignature( return true } case <-ctx.Done(): - logger.Error( - "stop waiting for signature for keep [%s] because "+ - "[%v] timeout has been exceeded", + logger.Errorf( + "signature for keep [%s] has not appeared on the chain "+ + "after [%v] from submitting it", keepAddress.String(), waitTimeout, )