Skip to content

Commit

Permalink
[BCF-2689] Use an in-memory keystore for tests
Browse files Browse the repository at this point in the history
- Remove unused migration methods from the keystore
- Split the keystore ORM in two, between methods which access the
  `encrypted_key_rings` table, and methods which access `evm.key_states`
- Create an in-memory version of the base ORM, which doesn't actually
  write to the DB. Methods which access `evm.key_states` are unaffected
  by this. This will prevent tests from competing to get a row-level
  lock on the singleton keystore record.
- Start using this in-memory keystore in `cltest` and in tests
  generally. Note: tests in the keystore package themselves are
  unaffected by this, since we still want to have test coverage over the
  behaviour that accesses the DB.
  • Loading branch information
cedric-cordenier committed Oct 13, 2023
1 parent 9cc576f commit ebedb38
Show file tree
Hide file tree
Showing 27 changed files with 137 additions and 465 deletions.
2 changes: 1 addition & 1 deletion core/chains/cosmos/cosmostxm/txm_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func newReaderWriterMock(t *testing.T) *tcmocks.ReaderWriter {
func TestTxm(t *testing.T) {
db := pgtest.NewSqlxDB(t)
lggr := testutils.LoggerAssertMaxLevel(t, zapcore.ErrorLevel)
ks := keystore.New(db, utils.FastScryptParams, lggr, pgtest.NewQConfig(true))
ks := keystore.NewInMemory(db, utils.FastScryptParams, lggr, pgtest.NewQConfig(true))
require.NoError(t, ks.Unlock("blah"))

for i := 0; i < 4; i++ {
Expand Down
2 changes: 1 addition & 1 deletion core/chains/cosmos/cosmostxm/txm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestTxm_Integration(t *testing.T) {
eb := pg.NewEventBroadcaster(cfg.Database().URL(), 0, 0, lggr, uuid.New())
require.NoError(t, eb.Start(testutils.Context(t)))
t.Cleanup(func() { require.NoError(t, eb.Close()) })
ks := keystore.New(db, utils.FastScryptParams, lggr, pgtest.NewQConfig(true))
ks := keystore.NewInMemory(db, utils.FastScryptParams, lggr, pgtest.NewQConfig(true))
zeConfig := sdk.GetConfig()
fmt.Println(zeConfig)
accounts, testdir, tendermintURL := cosmosclient.SetupLocalCosmosNode(t, chainID, *cosmosChain.GasToken)
Expand Down
6 changes: 2 additions & 4 deletions core/chains/evm/txmgr/confirmer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2412,8 +2412,6 @@ func TestEthConfirmer_RebroadcastWhereNecessary_TerminallyUnderpriced_ThenGoesTh
require.NoError(t, ec.RebroadcastWhereNecessary(testutils.Context(t), currentHead))
})

realKst := cltest.NewKeyStore(t, db, cfg.Database()).Eth()

t.Run("multiple gas bumps with existing broadcast attempts are retried with more gas until success in legacy mode", func(t *testing.T) {
ethClient := evmtest.NewEthClientMockWithDefaultChain(t)
ec := cltest.NewEthConfirmer(t, txStore, ethClient, evmcfg, kst, nil)
Expand All @@ -2439,7 +2437,7 @@ func TestEthConfirmer_RebroadcastWhereNecessary_TerminallyUnderpriced_ThenGoesTh
).Run(func(args mock.Arguments) {
unsignedLegacyTx := args.Get(1).(*types.Transaction)
// Use the real keystore to do the actual signing
thisSignedLegacyTx, err := realKst.SignTx(fromAddress, unsignedLegacyTx, testutils.FixtureChainID)
thisSignedLegacyTx, err := ethKeyStore.SignTx(fromAddress, unsignedLegacyTx, testutils.FixtureChainID)
require.NoError(t, err)
*signedLegacyTx = *thisSignedLegacyTx
}).Times(4) // 3 failures 1 success
Expand Down Expand Up @@ -2471,7 +2469,7 @@ func TestEthConfirmer_RebroadcastWhereNecessary_TerminallyUnderpriced_ThenGoesTh
).Run(func(args mock.Arguments) {
unsignedDxFeeTx := args.Get(1).(*types.Transaction)
// Use the real keystore to do the actual signing
thisSignedDxFeeTx, err := realKst.SignTx(fromAddress, unsignedDxFeeTx, testutils.FixtureChainID)
thisSignedDxFeeTx, err := ethKeyStore.SignTx(fromAddress, unsignedDxFeeTx, testutils.FixtureChainID)
require.NoError(t, err)
*signedDxFeeTx = *thisSignedDxFeeTx
}).Times(4) // 3 failures 1 success
Expand Down
3 changes: 1 addition & 2 deletions core/chains/evm/txmgr/txmgr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -522,9 +522,8 @@ func TestTxm_CreateTransaction_OutOfEth(t *testing.T) {
config, dbConfig, evmConfig := makeConfigs(t)

ethClient := evmtest.NewEthClientMockWithDefaultChain(t)
kst := cltest.NewKeyStore(t, db, dbConfig)
estimator := gas.NewEstimator(logger.TestLogger(t), ethClient, config, evmConfig.GasEstimator())
txm, err := makeTestEvmTxm(t, db, ethClient, estimator, evmConfig, evmConfig.GasEstimator(), evmConfig.Transactions(), dbConfig, dbConfig.Listener(), kst.Eth(), nil)
txm, err := makeTestEvmTxm(t, db, ethClient, estimator, evmConfig, evmConfig.GasEstimator(), evmConfig.Transactions(), dbConfig, dbConfig.Listener(), etKeyStore, nil)
require.NoError(t, err)

t.Run("if another key has any transactions with insufficient eth errors, transmits as normal", func(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions core/internal/cltest/cltest.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ func NewApplicationWithConfig(t testing.TB, cfg chainlink.GeneralConfig, flagsAn
}
}

keyStore := keystore.New(db, utils.FastScryptParams, lggr, cfg.Database())
keyStore := keystore.NewInMemory(db, utils.FastScryptParams, lggr, cfg.Database())

mailMon := utils.NewMailboxMonitor(cfg.AppID().String())
loopRegistry := plugins.NewLoopRegistry(lggr)
Expand Down Expand Up @@ -687,7 +687,7 @@ func (ta *TestApplication) NewAuthenticatingShell(prompter cmd.Prompter) *cmd.Sh

// NewKeyStore returns a new, unlocked keystore
func NewKeyStore(t testing.TB, db *sqlx.DB, cfg pg.QConfig) keystore.Master {
keystore := keystore.New(db, utils.FastScryptParams, logger.TestLogger(t), cfg)
keystore := keystore.NewInMemory(db, utils.FastScryptParams, logger.TestLogger(t), cfg)
require.NoError(t, keystore.Unlock(Password))
return keystore
}
Expand Down
17 changes: 0 additions & 17 deletions core/services/keystore/csa.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ type CSA interface {
Import(keyJSON []byte, password string) (csakey.KeyV2, error)
Export(id string, password string) ([]byte, error)
EnsureKey() error

GetV1KeysAsV2() ([]csakey.KeyV2, error)
}

type csa struct {
Expand Down Expand Up @@ -158,21 +156,6 @@ func (ks *csa) EnsureKey() error {
return ks.safeAddKey(key)
}

func (ks *csa) GetV1KeysAsV2() (keys []csakey.KeyV2, _ error) {
v1Keys, err := ks.orm.GetEncryptedV1CSAKeys()
if err != nil {
return keys, err
}
for _, keyV1 := range v1Keys {
err := keyV1.Unlock(ks.password)
if err != nil {
return keys, err
}
keys = append(keys, keyV1.ToV2())
}
return keys, nil
}

func (ks *csa) getByID(id string) (csakey.KeyV2, error) {
key, found := ks.keyRing.CSA[id]
if !found {
Expand Down
18 changes: 0 additions & 18 deletions core/services/keystore/csa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"github.com/smartcontractkit/chainlink/v2/core/internal/testutils/pgtest"
"github.com/smartcontractkit/chainlink/v2/core/services/keystore"
"github.com/smartcontractkit/chainlink/v2/core/services/keystore/keys/csakey"
"github.com/smartcontractkit/chainlink/v2/core/utils"
)

func Test_CSAKeyStore_E2E(t *testing.T) {
Expand Down Expand Up @@ -169,21 +168,4 @@ func Test_CSAKeyStore_E2E(t *testing.T) {
require.NoError(t, err)
require.Equal(t, 1, len(keys))
})

t.Run("returns V1 keys as V2", func(t *testing.T) {
defer reset()
defer require.NoError(t, utils.JustError(db.Exec("DELETE FROM csa_keys")))

k, err := csakey.New(cltest.Password, utils.FastScryptParams)
require.NoError(t, err)

err = utils.JustError(db.Exec(`INSERT INTO csa_keys (public_key, encrypted_private_key, created_at, updated_at) VALUES ($1, $2, NOW(), NOW())`, k.PublicKey, k.EncryptedPrivateKey))
require.NoError(t, err)

keys, err := ks.GetV1KeysAsV2()
require.NoError(t, err)

assert.Len(t, keys, 1)
assert.Equal(t, fmt.Sprintf("CSAKeyV2{PrivateKey: <redacted>, PublicKey: %s}", keys[0].PublicKey), keys[0].GoString())
})
}
34 changes: 9 additions & 25 deletions core/services/keystore/eth.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,19 @@ type Eth interface {

type eth struct {
*keyManager
keystateORM
q pg.Q
subscribers [](chan struct{})
subscribersMu *sync.RWMutex
}

var _ Eth = &eth{}

func newEthKeyStore(km *keyManager) *eth {
func newEthKeyStore(km *keyManager, orm keystateORM, q pg.Q) *eth {
return &eth{
keystateORM: orm,
keyManager: km,
q: q,
subscribers: make([](chan struct{}), 0),
subscribersMu: new(sync.RWMutex),
}
Expand Down Expand Up @@ -193,7 +197,7 @@ func (ks *eth) addKey(address common.Address, chainID *big.Int, qopts ...pg.QOpt
sql := `INSERT INTO evm.key_states (address, disabled, evm_chain_id, created_at, updated_at)
VALUES ($1, false, $2, NOW(), NOW())
RETURNING *;`
q := ks.orm.q.WithOpts(qopts...)
q := ks.q.WithOpts(qopts...)
if err := q.Get(state, sql, address, chainID.String()); err != nil {
return errors.Wrap(err, "failed to insert evm_key_state")
}
Expand All @@ -216,7 +220,7 @@ func (ks *eth) Enable(address common.Address, chainID *big.Int, qopts ...pg.QOpt
// caller must hold lock!
func (ks *eth) enable(address common.Address, chainID *big.Int, qopts ...pg.QOpt) error {
state := new(ethkey.State)
q := ks.orm.q.WithOpts(qopts...)
q := ks.q.WithOpts(qopts...)
sql := `UPDATE evm.key_states SET disabled = false, updated_at = NOW() WHERE address = $1 AND evm_chain_id = $2
RETURNING *;`
if err := q.Get(state, sql, address, chainID.String()); err != nil {
Expand All @@ -240,7 +244,7 @@ func (ks *eth) Disable(address common.Address, chainID *big.Int, qopts ...pg.QOp

func (ks *eth) disable(address common.Address, chainID *big.Int, qopts ...pg.QOpt) error {
state := new(ethkey.State)
q := ks.orm.q.WithOpts(qopts...)
q := ks.q.WithOpts(qopts...)
sql := `UPDATE evm.key_states SET disabled = false, updated_at = NOW() WHERE address = $1 AND evm_chain_id = $2
RETURNING id, address, evm_chain_id, disabled, created_at, updated_at;`
if err := q.Get(state, sql, address, chainID.String()); err != nil {
Expand Down Expand Up @@ -480,26 +484,6 @@ func (ks *eth) EnabledAddressesForChain(chainID *big.Int) (addresses []common.Ad
return
}

func (ks *eth) getV1KeysAsV2() (keys []ethkey.KeyV2, fundings []bool, _ error) {
v1Keys, err := ks.orm.GetEncryptedV1EthKeys()
if err != nil {
return nil, nil, errors.Wrap(err, "failed to get encrypted v1 eth keys")
}
if len(v1Keys) == 0 {
return nil, nil, nil
}
for _, keyV1 := range v1Keys {
dKey, err := keystore.DecryptKey(keyV1.JSON, ks.password)
if err != nil {
return nil, nil, errors.Wrapf(err, "could not decrypt eth key %s", keyV1.Address.Hex())
}
keyV2 := ethkey.FromPrivateKey(dKey.PrivateKey)
keys = append(keys, keyV2)
fundings = append(fundings, keyV1.IsFunding)
}
return keys, fundings, nil
}

// XXXTestingOnlySetState is only used in tests to manually update a key's state
func (ks *eth) XXXTestingOnlySetState(state ethkey.State) {
ks.lock.Lock()
Expand All @@ -514,7 +498,7 @@ func (ks *eth) XXXTestingOnlySetState(state ethkey.State) {
*existingState = state
sql := `UPDATE evm.key_states SET address = :address, is_disabled = :is_disabled, evm_chain_id = :evm_chain_id, updated_at = NOW()
WHERE address = :address;`
_, err := ks.orm.q.NamedExec(sql, state)
_, err := ks.q.NamedExec(sql, state)
if err != nil {
panic(err.Error())
}
Expand Down
35 changes: 0 additions & 35 deletions core/services/keystore/eth_internal_test.go

This file was deleted.

80 changes: 80 additions & 0 deletions core/services/keystore/keystoretest.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package keystore

import (
"sync"

"github.com/smartcontractkit/sqlx"

"github.com/smartcontractkit/chainlink/v2/core/logger"
"github.com/smartcontractkit/chainlink/v2/core/services/pg"
"github.com/smartcontractkit/chainlink/v2/core/utils"
)

// memoryORM is an in-memory version of the keystore. This is
// only intended to be used in tests to avoid DB lock contention on
// the single DB row that stores the key material.
//
// Note: we store `q` on the struct since `saveEncryptedKeyRing` needs
// to support DB callbacks.
type memoryORM struct {
keyRing *encryptedKeyRing
q pg.Queryer
mu sync.RWMutex
}

func (o *memoryORM) isEmpty() (bool, error) {
return false, nil
}

func (o *memoryORM) saveEncryptedKeyRing(kr *encryptedKeyRing, callbacks ...func(pg.Queryer) error) error {
o.mu.Lock()
defer o.mu.Unlock()
o.keyRing = kr
for _, c := range callbacks {
c(o.q)
}
return nil
}

func (o *memoryORM) getEncryptedKeyRing() (encryptedKeyRing, error) {
o.mu.RLock()
defer o.mu.RUnlock()
if o.keyRing == nil {
return encryptedKeyRing{}, nil
}
return *o.keyRing, nil
}

func newInMemoryORM(q pg.Queryer) *memoryORM {
return &memoryORM{q: q}
}

// NewInMemory sets up a keystore which NOOPs attempts to access the `encrypted_key_rings` table. Accessing `evm.key_states`
// will still hit the DB.
func NewInMemory(db *sqlx.DB, scryptParams utils.ScryptParams, lggr logger.Logger, cfg pg.QConfig) *master {
dbORM := NewORM(db, lggr, cfg)
memoryORM := newInMemoryORM(dbORM.q)

km := &keyManager{
orm: memoryORM,
keystateORM: dbORM,
scryptParams: scryptParams,
lock: &sync.RWMutex{},
logger: lggr.Named("KeyStore"),
}

return &master{
keyManager: km,
cosmos: newCosmosKeyStore(km),
csa: newCSAKeyStore(km),
eth: newEthKeyStore(km, dbORM, dbORM.q),
ocr: newOCRKeyStore(km),
ocr2: newOCR2KeyStore(km),
p2p: newP2PKeyStore(km),
solana: newSolanaKeyStore(km),
starknet: newStarkNetKeyStore(km),
vrf: newVRFKeyStore(km),
dkgSign: newDKGSignKeyStore(km),
dkgEncrypt: newDKGEncryptKeyStore(km),
}
}
Loading

0 comments on commit ebedb38

Please sign in to comment.