From 6f1c9af76b711d17962ecc928550ff22b583700d Mon Sep 17 00:00:00 2001 From: Samuel Hawksby-Robinson Date: Wed, 12 Jul 2023 23:29:38 +0100 Subject: [PATCH] Added functionality to local pairing that makes client calls (more) idempotent --- VERSION | 2 +- api/geth_backend.go | 45 +++++---- mobile/status.go | 40 ++++---- .../pairing/statecontrol/state_management.go | 93 +++++++++++++++++++ .../statecontrol/state_management_test.go | 58 ++++++++++++ 5 files changed, 194 insertions(+), 44 deletions(-) create mode 100644 server/pairing/statecontrol/state_management.go create mode 100644 server/pairing/statecontrol/state_management_test.go diff --git a/VERSION b/VERSION index 9704c9bc9..5874cec80 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.163.11 +0.163.12 diff --git a/api/geth_backend.go b/api/geth_backend.go index 7800b4b9b..3303d5feb 100644 --- a/api/geth_backend.go +++ b/api/geth_backend.go @@ -12,10 +12,6 @@ import ( "sync" "time" - "github.com/status-im/status-go/common/dbsetup" - "github.com/status-im/status-go/images" - "github.com/status-im/status-go/walletdatabase" - "github.com/imdario/mergo" "github.com/ethereum/go-ethereum/common" @@ -27,9 +23,11 @@ import ( "github.com/status-im/status-go/account" "github.com/status-im/status-go/account/generator" "github.com/status-im/status-go/appdatabase" + "github.com/status-im/status-go/common/dbsetup" "github.com/status-im/status-go/connection" "github.com/status-im/status-go/eth-node/crypto" "github.com/status-im/status-go/eth-node/types" + "github.com/status-im/status-go/images" "github.com/status-im/status-go/logutils" "github.com/status-im/status-go/multiaccounts" "github.com/status-im/status-go/multiaccounts/accounts" @@ -43,6 +41,7 @@ import ( "github.com/status-im/status-go/protocol/identity/colorhash" "github.com/status-im/status-go/protocol/requests" "github.com/status-im/status-go/rpc" + "github.com/status-im/status-go/server/pairing/statecontrol" "github.com/status-im/status-go/services/ext" "github.com/status-im/status-go/services/personal" "github.com/status-im/status-go/services/rpcfilters" @@ -50,6 +49,7 @@ import ( "github.com/status-im/status-go/signal" "github.com/status-im/status-go/sqlite" "github.com/status-im/status-go/transactions" + "github.com/status-im/status-go/walletdatabase" ) var ( @@ -81,18 +81,18 @@ type GethStatusBackend struct { walletDB *sql.DB config *params.NodeConfig - statusNode *node.StatusNode - personalAPI *personal.PublicAPI - multiaccountsDB *multiaccounts.Database - account *multiaccounts.Account - accountManager *account.GethManager - transactor *transactions.Transactor - connectionState connection.State - appState appState - selectedAccountKeyID string - log log.Logger - allowAllRPC bool // used only for tests, disables api method restrictions - localPairing bool // used to disable login/logout signalling + statusNode *node.StatusNode + personalAPI *personal.PublicAPI + multiaccountsDB *multiaccounts.Database + account *multiaccounts.Account + accountManager *account.GethManager + transactor *transactions.Transactor + connectionState connection.State + appState appState + selectedAccountKeyID string + log log.Logger + allowAllRPC bool // used only for tests, disables api method restrictions + LocalPairingStateManager *statecontrol.ProcessStateManager } // NewGethStatusBackend create a new GethStatusBackend instance @@ -116,7 +116,8 @@ func (b *GethStatusBackend) initialize() { b.personalAPI = personalAPI b.statusNode.SetMultiaccountsDB(b.multiaccountsDB) b.log = log.New("package", "status-go/api.GethStatusBackend") - b.localPairing = false + b.LocalPairingStateManager = new(statecontrol.ProcessStateManager) + b.LocalPairingStateManager.SetPairing(false) } // StatusNode returns reference to node manager @@ -493,7 +494,7 @@ func (b *GethStatusBackend) StartNodeWithKey(acc multiaccounts.Account, password return err } // get logged in - if !b.localPairing { + if !b.LocalPairingStateManager.IsPairing() { return b.LoggedIn(acc.KeyUID, err) } return nil @@ -761,7 +762,7 @@ func (b *GethStatusBackend) StartNodeWithAccount(acc multiaccounts.Account, pass _ = b.StopNode() } // get logged in - if !b.localPairing { + if !b.LocalPairingStateManager.IsPairing() { return b.LoggedIn(acc.KeyUID, err) } return err @@ -1659,7 +1660,7 @@ func (b *GethStatusBackend) stopNode() error { if b.statusNode == nil || !b.IsNodeRunning() { return nil } - if !b.localPairing { + if !b.LocalPairingStateManager.IsPairing() { defer signal.SendNodeStopped() } @@ -2267,10 +2268,6 @@ func (b *GethStatusBackend) SwitchFleet(fleet string, conf *params.NodeConfig) e return nil } -func (b *GethStatusBackend) SetLocalPairing(value bool) { - b.localPairing = value -} - func (b *GethStatusBackend) getAppDBPath(keyUID string) string { return filepath.Join(b.rootDataDir, fmt.Sprintf("%s-v4.db", keyUID)) } diff --git a/mobile/status.go b/mobile/status.go index f53db17e8..260ae7c0d 100644 --- a/mobile/status.go +++ b/mobile/status.go @@ -9,8 +9,6 @@ import ( "sync" "unsafe" - "github.com/status-im/status-go/logutils" - validator "gopkg.in/go-playground/validator.v9" "github.com/ethereum/go-ethereum/log" @@ -27,6 +25,7 @@ import ( "github.com/status-im/status-go/exportlogs" "github.com/status-im/status-go/extkeys" "github.com/status-im/status-go/images" + "github.com/status-im/status-go/logutils" "github.com/status-im/status-go/multiaccounts" "github.com/status-im/status-go/multiaccounts/accounts" "github.com/status-im/status-go/multiaccounts/settings" @@ -1051,9 +1050,9 @@ func GetConnectionStringForBeingBootstrapped(configJSON string) string { return makeJSONResponse(fmt.Errorf("no config given, PayloadSourceConfig is expected")) } - statusBackend.SetLocalPairing(true) + statusBackend.LocalPairingStateManager.SetPairing(true) defer func() { - statusBackend.SetLocalPairing(false) + statusBackend.LocalPairingStateManager.SetPairing(false) }() cs, err := pairing.StartUpReceiverServer(statusBackend, configJSON) @@ -1080,9 +1079,9 @@ func GetConnectionStringForBootstrappingAnotherDevice(configJSON string) string return makeJSONResponse(fmt.Errorf("no config given, SendingServerConfig is expected")) } - statusBackend.SetLocalPairing(true) + statusBackend.LocalPairingStateManager.SetPairing(true) defer func() { - statusBackend.SetLocalPairing(false) + statusBackend.LocalPairingStateManager.SetPairing(false) }() cs, err := pairing.StartUpSenderServer(statusBackend, configJSON) @@ -1101,22 +1100,23 @@ func GetConnectionStringForBootstrappingAnotherDevice(configJSON string) string // Example: A mobile device (device with a camera) receiving account data from // a device with a screen (mobile or desktop devices) func InputConnectionStringForBootstrapping(cs, configJSON string) string { + var err error if configJSON == "" { return makeJSONResponse(fmt.Errorf("no config given, ReceiverClientConfig is expected")) } - statusBackend.SetLocalPairing(true) - defer func() { - statusBackend.SetLocalPairing(false) - }() - - err := pairing.StartUpReceivingClient(statusBackend, cs, configJSON) + err = statusBackend.LocalPairingStateManager.StartPairing(cs) + defer func() { statusBackend.LocalPairingStateManager.StopPairing(cs, err) }() if err != nil { return makeJSONResponse(err) } - err = statusBackend.Logout() - return makeJSONResponse(err) + err = pairing.StartUpReceivingClient(statusBackend, cs, configJSON) + if err != nil { + return makeJSONResponse(err) + } + + return makeJSONResponse(statusBackend.Logout()) } // InputConnectionStringForBootstrappingAnotherDevice starts a pairing.SendingClient @@ -1127,16 +1127,18 @@ func InputConnectionStringForBootstrapping(cs, configJSON string) string { // // Example: A mobile (device with camera) sending account data to a desktop device (device without camera) func InputConnectionStringForBootstrappingAnotherDevice(cs, configJSON string) string { + var err error if configJSON == "" { return makeJSONResponse(fmt.Errorf("no config given, SenderClientConfig is expected")) } - statusBackend.SetLocalPairing(true) - defer func() { - statusBackend.SetLocalPairing(false) - }() + err = statusBackend.LocalPairingStateManager.StartPairing(cs) + defer func() { statusBackend.LocalPairingStateManager.StopPairing(cs, err) }() + if err != nil { + return makeJSONResponse(err) + } - err := pairing.StartUpSendingClient(statusBackend, cs, configJSON) + err = pairing.StartUpSendingClient(statusBackend, cs, configJSON) return makeJSONResponse(err) } diff --git a/server/pairing/statecontrol/state_management.go b/server/pairing/statecontrol/state_management.go new file mode 100644 index 000000000..9e0de40d0 --- /dev/null +++ b/server/pairing/statecontrol/state_management.go @@ -0,0 +1,93 @@ +package statecontrol + +// TODO refactor into the pairing package once the backend dependencies have been removed. + +import ( + "fmt" + "sync" +) + +var ( + ErrProcessStateManagerAlreadyPairing = fmt.Errorf("cannot start new LocalPairing session, already pairing") + ErrProcessStateManagerAlreadyPaired = func(sessionName string) error { + return fmt.Errorf("given connection string already successfully used '%s'", sessionName) + } +) + +// ProcessStateManager represents a g +type ProcessStateManager struct { + pairing bool + pairingLock sync.Mutex + + // sessions represents a map[string]bool: + // where string is a ConnectionParams string and bool is the transfer success state of that connection string + sessions sync.Map +} + +// IsPairing returns if the ProcessStateManager is currently in pairing mode +func (psm *ProcessStateManager) IsPairing() bool { + psm.pairingLock.Lock() + defer psm.pairingLock.Unlock() + return psm.pairing +} + +// SetPairing sets the ProcessStateManager pairing state +func (psm *ProcessStateManager) SetPairing(state bool) { + psm.pairingLock.Lock() + defer psm.pairingLock.Unlock() + psm.pairing = state +} + +// RegisterSession stores a sessionName with the default false value. +// In practice a sessionName will be a ConnectionParams string provided by the server mode device. +// The boolean value represents whether the ConnectionParams string session resulted in a successful transfer. +func (psm *ProcessStateManager) RegisterSession(sessionName string) { + psm.sessions.Store(sessionName, false) +} + +// CompleteSession updates a transfer session with a given transfer success state only if the session is registered. +func (psm *ProcessStateManager) CompleteSession(sessionName string) { + r, c := psm.GetSession(sessionName) + if r && !c { + psm.sessions.Store(sessionName, true) + } +} + +// GetSession returns two booleans for a given sessionName. +// These represent if the sessionName has been registered and if it has resulted in a successful transfer +func (psm *ProcessStateManager) GetSession(sessionName string) (bool, bool) { + completed, registered := psm.sessions.Load(sessionName) + if !registered { + return registered, false + } + return registered, completed.(bool) +} + +// StartPairing along with StopPairing are the core functions of the ProcessStateManager +// This function takes a sessionName, which in practice is a ConnectionParams string, and attempts to init pairing state management. +// The function will return an error if the ProcessStateManager is already currently pairing or if the sessionName was previously successful. +func (psm *ProcessStateManager) StartPairing(sessionName string) error { + if psm.IsPairing() { + return ErrProcessStateManagerAlreadyPairing + } + + registered, completed := psm.GetSession(sessionName) + if completed { + return ErrProcessStateManagerAlreadyPaired(sessionName) + } + if !registered { + psm.RegisterSession(sessionName) + } + + psm.SetPairing(true) + return nil +} + +// StopPairing takes a sessionName and an error, if the error is nil the sessionName will be recorded as successful +// the pairing state of the ProcessStateManager is set to false. +func (psm *ProcessStateManager) StopPairing(sessionName string, err error) { + if err == nil { + psm.CompleteSession(sessionName) + } + psm.SetPairing(false) +} diff --git a/server/pairing/statecontrol/state_management_test.go b/server/pairing/statecontrol/state_management_test.go new file mode 100644 index 000000000..dfb05b06f --- /dev/null +++ b/server/pairing/statecontrol/state_management_test.go @@ -0,0 +1,58 @@ +package statecontrol + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +const ( + cs = "cs2:4FHRnp:Q4:uqnn" + cs1 = cs + "1234" + cs2 = cs + "qwer" +) + +func TestProcessStateManager_StartPairing(t *testing.T) { + psm := new(ProcessStateManager) + + // A new psm should start with no error + err := psm.StartPairing(cs) + require.NoError(t, err) + + // A started psm should return an ErrProcessStateManagerAlreadyPairing if another start is attempted + err = psm.StartPairing(cs) + require.EqualError(t, err, ErrProcessStateManagerAlreadyPairing.Error()) + + // A psm should start without error if the pairing process has been stopped with an error + psm.StopPairing(cs, err) + err = psm.StartPairing(cs) + require.NoError(t, err) + + // A psm should return an error if starting with a conn string that previously succeeded (a nil error) + psm.StopPairing(cs, nil) + err = psm.StartPairing(cs) + require.EqualError(t, err, ErrProcessStateManagerAlreadyPaired(cs).Error()) + + // A psm should be able to start with a new connection string if the psm has been stopped + err = psm.StartPairing(cs1) + require.NoError(t, err) + + // A started psm should return an ErrProcessStateManagerAlreadyPairing if another start is attempted + err = psm.StartPairing(cs1) + require.EqualError(t, err, ErrProcessStateManagerAlreadyPairing.Error()) + + // A started psm should return an ErrProcessStateManagerAlreadyPairing if another start is attempted regardless of + // the given connection string. + err = psm.StartPairing(cs2) + require.EqualError(t, err, ErrProcessStateManagerAlreadyPairing.Error()) + + // A psm should start without error if the pairing process has been stopped with an error + psm.StopPairing(cs1, err) + err = psm.StartPairing(cs2) + require.NoError(t, err) + + // A psm should return an error if starting with a conn string that previously succeeded (a nil error) + psm.StopPairing(cs2, nil) + err = psm.StartPairing(cs2) + require.EqualError(t, err, ErrProcessStateManagerAlreadyPaired(cs2).Error()) +}