Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BCI-2376 TXM as service. Post TX endpoint. #11256

Closed
wants to merge 21 commits into from
Closed

BCI-2376 TXM as service. Post TX endpoint. #11256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f2d5f60
Create Evm Transaction Endpoint
dhaidashenko Nov 9, 2023
0213c09
rewrite tests to use mocks
dhaidashenko Nov 9, 2023
9958ec3
improve tests coverage
dhaidashenko Nov 10, 2023
1ddc0eb
Flag to enable Txm as service
dhaidashenko Nov 10, 2023
d587ff4
require admin role to send tx
dhaidashenko Nov 10, 2023
80c4fb7
moved question into PR
dhaidashenko Nov 10, 2023
c6142c3
Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint
dhaidashenko Nov 10, 2023
565c9c3
fix config docs
dhaidashenko Nov 10, 2023
e2dde3b
address comments
dhaidashenko Nov 14, 2023
010a1ca
Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint
dhaidashenko Nov 15, 2023
c29f27d
remove redundant dependency
dhaidashenko Nov 16, 2023
1dd2430
proper status code on tx failure
dhaidashenko Nov 16, 2023
94a3c0a
wait for tx attempt to return properly populated resource
dhaidashenko Nov 16, 2023
4c23c16
remove evm config mock generation
dhaidashenko Nov 16, 2023
c70bb5d
handle fee bump
dhaidashenko Nov 16, 2023
ac995da
Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint
dhaidashenko Nov 16, 2023
0c4af4e
fixed merge
dhaidashenko Nov 16, 2023
d8bf685
switch to feature flag
dhaidashenko Nov 21, 2023
5178e0b
Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint
dhaidashenko Nov 21, 2023
be19e4f
Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint
dhaidashenko Nov 22, 2023
2d52d70
switch to sqlx.REbind for dynamic sql query
dhaidashenko Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions core/chains/evm/txmgr/evm_tx_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
nullv4 "gopkg.in/guregu/null.v4"

"github.com/smartcontractkit/chainlink-common/pkg/sqlutil"

"github.com/smartcontractkit/chainlink/v2/common/txmgr"
txmgrtypes "github.com/smartcontractkit/chainlink/v2/common/txmgr/types"
"github.com/smartcontractkit/chainlink/v2/core/chains/evm/assets"
Expand Down Expand Up @@ -55,7 +56,7 @@ type TxStoreWebApi interface {
FindTxByHash(hash common.Hash) (*Tx, error)
Transactions(offset, limit int) ([]Tx, int, error)
TxAttempts(offset, limit int) ([]TxAttempt, int, error)
TransactionsWithAttempts(offset, limit int) ([]Tx, int, error)
TransactionsWithAttempts(selector TransactionsWithAttemptsSelector) ([]Tx, int, error)
FindTxAttempt(hash common.Hash) (*TxAttempt, error)
FindTxWithAttempts(etxID int64) (etx Tx, err error)
}
Expand Down Expand Up @@ -440,18 +441,32 @@ func (o *evmTxStore) Transactions(offset, limit int) (txs []Tx, count int, err e
return
}

type TransactionsWithAttemptsSelector struct {
IdempotencyKey *string
Offset uint64
Limit uint64
}

// TransactionsWithAttempts returns all eth transactions with at least one attempt
// limited by passed parameters. Attempts are sorted by id.
func (o *evmTxStore) TransactionsWithAttempts(offset, limit int) (txs []Tx, count int, err error) {
sql := `SELECT count(*) FROM evm.txes WHERE id IN (SELECT DISTINCT eth_tx_id FROM evm.tx_attempts)`
if err = o.q.Get(&count, sql); err != nil {
return
func (o *evmTxStore) TransactionsWithAttempts(selector TransactionsWithAttemptsSelector) (txs []Tx, count int, err error) {
query := " FROM evm.txes WHERE id IN (SELECT DISTINCT eth_tx_id FROM evm.tx_attempts)"
var args []interface{}
if selector.IdempotencyKey != nil {
args = append(args, *selector.IdempotencyKey)
query += " AND idempotency_key = ?"
}

sql = `SELECT * FROM evm.txes WHERE id IN (SELECT DISTINCT eth_tx_id FROM evm.tx_attempts) ORDER BY id desc LIMIT $1 OFFSET $2`
countQuery := sqlx.Rebind(sqlx.DOLLAR, "SELECT count(*)"+query)
if err = o.q.Get(&count, countQuery, args...); err != nil {
return nil, 0, fmt.Errorf("failed to exec count query: %w, sql: %s", err, countQuery)
}

selectQuery := sqlx.Rebind(sqlx.DOLLAR, "SELECT * "+query+" ORDER BY id desc LIMIT ? OFFSET ?")
args = append(args, selector.Limit, selector.Offset)
var dbTxs []DbEthTx
if err = o.q.Select(&dbTxs, sql, limit, offset); err != nil {
return
if err = o.q.Select(&dbTxs, selectQuery, args...); err != nil {
return nil, 0, fmt.Errorf("failed to exec select query: %w, sql: %s", err, selectQuery)
}
txs = dbEthTxsToEvmEthTxs(dbTxs)
err = o.preloadTxAttempts(txs)
Expand Down
20 changes: 18 additions & 2 deletions core/chains/evm/txmgr/evm_tx_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ func TestORM_TransactionsWithAttempts(t *testing.T) {
require.NoError(t, err)
require.Equal(t, 3, count)

txs, count, err := txStore.TransactionsWithAttempts(0, 100) // should omit tx3
txs, count, err := txStore.TransactionsWithAttempts(txmgr.TransactionsWithAttemptsSelector{
Offset: 0,
Limit: 100,
}) // should omit tx3
require.NoError(t, err)
assert.Equal(t, 2, count, "only eth txs with attempts are counted")
assert.Len(t, txs, 2)
Expand All @@ -70,11 +73,24 @@ func TestORM_TransactionsWithAttempts(t *testing.T) {
assert.Equal(t, int64(3), *txs[0].TxAttempts[0].BroadcastBeforeBlockNum, "attempts should be sorted by created_at")
assert.Equal(t, int64(2), *txs[0].TxAttempts[1].BroadcastBeforeBlockNum, "attempts should be sorted by created_at")

txs, count, err = txStore.TransactionsWithAttempts(0, 1)
txs, count, err = txStore.TransactionsWithAttempts(txmgr.TransactionsWithAttemptsSelector{
Offset: 0,
Limit: 1,
})
require.NoError(t, err)
assert.Equal(t, 2, count, "only eth txs with attempts are counted")
assert.Len(t, txs, 1, "limit should apply to length of results")
assert.Equal(t, evmtypes.Nonce(1), *txs[0].Sequence, "transactions should be sorted by nonce")

txs, count, err = txStore.TransactionsWithAttempts(txmgr.TransactionsWithAttemptsSelector{
IdempotencyKey: tx2.IdempotencyKey,
Offset: 0,
Limit: 10,
})
require.NoError(t, err)
assert.Equal(t, 1, count, "only eth tx with specified idempotency key")
assert.Equal(t, tx2.ID, txs[0].ID)
assert.Equal(t, tx2.IdempotencyKey, txs[0].IdempotencyKey)
}

func TestORM_Transactions(t *testing.T) {
Expand Down
24 changes: 13 additions & 11 deletions core/chains/evm/txmgr/mocks/evm_tx_store.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions core/config/docs/core.toml
View file
Open in desktop
Copy link
Contributor

@jmank88 jmank88 Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rationale for feature flagging this service? If is meant to be temporary only, then we should use the existing [Feature]. If it is meant to be precautionary, and make sending txes opt-in only, then I think we have an inconsistency, because the existing endpoints to send tokens are not opt-in only (meaning we could just drop this and have it be enabled by default).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's meant to be precautionary. Right now only planned user of this endpoint is Mercury Config Distribution project. Enabling by default this endpoint for all the nodes increases attack surface and we are skeptical that it'll pass security review.

Additionally it's planned to use this flag to only start services that are directly used by TXM to reduce resource redundant resources consumption and reduce RPC calls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are skeptical that it'll pass security review.

Even knowing that the ability to send tokens is already available? I am claiming that this is inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally it's planned to use this flag to only start services that are directly used by TXM to reduce resource redundant resources consumption and reduce RPC calls.

Can you elaborate on this? It sounds like an anti-pattern.

Copy link
Contributor

@jmank88 jmank88 Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, it sounds like [Feature] is more appropriate, and since this is user facing we might prefer TransactionService:

[Feature]
TransactionService = true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No point unnecessarily adding new endpoints publicly when not needed. (even though send tokens already exists)

There are tradeoffs, and these are not public.

Secondly, we wanted to use this feature also as a way of knowing when to only start TXM and dependencies, vs the regular Core Node dependencies. Are you saying that needs yet another config or feature?

This is misguided. A feature flag is scoped to a feature. This feature flag should not act as a global switch that runs the node in a different type of "mode". This is undesirable for the same reasons as a separate subcommand.

I'm not opposed to gating the new routes, but I'm still not clear on whether it would be temporary or not.

I am opposed to having this act like anything other than a simple gate for the new routes.

Copy link
Contributor

@prashantkumar1982 prashantkumar1982 Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the new routes, what is the Foundation team's recommendation to use?

Treat this as permanent. We may spin off a new separate service in future, but for now, that is not even being planned.
So we need a way for Core Node to run in "TXM as a Service" mode, where:

  1. We only need TXM and its dependencies to be run. Other services are not needed, and preferably not started.
  2. New API capabilities (like Tx creation) to be only enabled in this mode, not in a regular Core Node mode.

There are tradeoffs, and these are not public

I'm talking from a Security point of view. Our code is open source. So anyone can see which endpoints are enabled by default. Thus we don't want to enable the newly created endpoints on all nodes run by all NOPs. In any case, we are fine to choose any option that Security team recommends here, as part of the Security Review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the new routes, what is the Foundation team's recommendation to use?

Regardless, it sounds like [Feature] is more appropriate, and since this is user facing we might prefer TransactionService:

[Feature]
TransactionService = true

Treat this as permanent. We may spin off a new separate service in future, but for now, that is not even being planned.

I mean, if this is feature flagged, is the feature flag switch permanent, or just temporary until the new routes are phased in and eventually enabled by default - after which we could remove the feature flag. I'm not sure it actually matters though, since there aren't any other config fields involved.

So we need a way for Core Node to run in "TXM as a Service" mode, where:

We only need TXM and its dependencies to be run. Other services are not needed, and preferably not started.
New API capabilities (like Tx creation) to be only enabled in this mode, not in a regular Core Node mode.

In what specific ways does a feature flag simply gating the routes fail to meet these goals? It should not be necessary to wire any other logic to this feature flag for unrelated services. If there are services running and using resources when not configured, we should fix those.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "TransactionService = false", which will be the default, the service will operate as current Core Node.
When "TransactionService = true", which will deliberately be set like this by our internal EngOps, when they want to deploy the TransactionService, then it will run TXM as a Service, and enable new routes.

I would say, this setting will remain like this indefinitely. In future, when and if we take TXM as a Service into entirely a different service than Core Node, then we should be able to remove this feature flag from the Core Node code.

For other services, you are saying, they should be left untouched. If they cause issues, like resource consumption, then we ought to fix that?
Yes, we can start with that. Ultimately, when this scales to 100+ EVM chains supported, and 1 instance has to support multiple chains, we might have to revisit this. If un-needed services are causing issues, like resources, or unnecessary logging or anything else, we may want to explicitly turn them off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can revisit this later. But my claim is that having "modes" at all is an anti-pattern and solves a problem which doesn't actually exist. We have the ability to configure many services independently, which enables complete flexibility for configuring any custom "mode" that is desired. There simply is no need to compromise this model.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ FeedsManager = true # Default
LogPoller = false # Default
# UICSAKeys enables CSA Keys in the UI.
UICSAKeys = false # Default
# TransactionService enables `POST /v2/transactions/evm` endpoint.
TransactionService = false # Default

[Database]
# DefaultIdleInTxSessionTimeout is the maximum time allowed for a transaction to be open and idle before timing out. See Postgres `idle_in_transaction_session_timeout` for more details.
Expand Down
1 change: 1 addition & 0 deletions core/config/feature_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ type Feature interface {
FeedsManager() bool
UICSAKeys() bool
LogPoller() bool
TransactionService() bool
}
11 changes: 8 additions & 3 deletions core/config/toml/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,10 @@ func (p *PrometheusSecrets) validateMerge(f *PrometheusSecrets) (err error) {
}

type Feature struct {
FeedsManager *bool
LogPoller *bool
UICSAKeys *bool
FeedsManager *bool
LogPoller *bool
UICSAKeys *bool
TransactionService *bool
}

func (f *Feature) setFrom(f2 *Feature) {
Expand All @@ -306,6 +307,10 @@ func (f *Feature) setFrom(f2 *Feature) {
if v := f2.UICSAKeys; v != nil {
f.UICSAKeys = v
}

if v := f2.TransactionService; v != nil {
f.TransactionService = v
}
}

type Database struct {
Expand Down
2 changes: 2 additions & 0 deletions core/internal/cltest/factories.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ func EmptyCLIContext() *cli.Context {
}

func NewEthTx(t *testing.T, fromAddress common.Address) txmgr.Tx {
idempotencyKey := uuid.New().String()
return txmgr.Tx{
IdempotencyKey: &idempotencyKey,
FromAddress: fromAddress,
ToAddress: testutils.NewAddress(),
EncodedPayload: []byte{1, 2, 3},
Expand Down
4 changes: 4 additions & 0 deletions core/services/chainlink/config_feature.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,7 @@ func (f *featureConfig) LogPoller() bool {
func (f *featureConfig) UICSAKeys() bool {
return *f.c.UICSAKeys
}

func (f *featureConfig) TransactionService() bool {
return *f.c.TransactionService
}
1 change: 1 addition & 0 deletions core/services/chainlink/config_feature_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ func TestFeatureConfig(t *testing.T) {
assert.True(t, f.LogPoller())
assert.True(t, f.FeedsManager())
assert.True(t, f.UICSAKeys())
assert.True(t, f.TransactionService())
}
4 changes: 4 additions & 0 deletions core/services/chainlink/config_general.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,10 @@ func (g *generalConfig) FeatureUICSAKeys() bool {
return *g.c.Feature.UICSAKeys
}

func (g *generalConfig) FeatureTransactionService() bool {
return *g.c.Feature.TransactionService
}

func (g *generalConfig) AutoPprof() config.AutoPprof {
return &autoPprofConfig{c: g.c.AutoPprof, rootDir: g.RootDir}
}
Expand Down
8 changes: 5 additions & 3 deletions core/services/chainlink/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,10 @@ func TestConfig_Marshal(t *testing.T) {
}

full.Feature = toml.Feature{
FeedsManager: ptr(true),
LogPoller: ptr(true),
UICSAKeys: ptr(true),
FeedsManager: ptr(true),
LogPoller: ptr(true),
UICSAKeys: ptr(true),
TransactionService: ptr(true),
}
full.Database = toml.Database{
DefaultIdleInTxSessionTimeout: models.MustNewDuration(time.Minute),
Expand Down Expand Up @@ -703,6 +704,7 @@ Headers = ['Authorization: token', 'X-SomeOther-Header: value with spaces | and
FeedsManager = true
LogPoller = true
UICSAKeys = true
TransactionService = true
`},
{"Database", Config{Core: toml.Core{Database: full.Database}}, `[Database]
DefaultIdleInTxSessionTimeout = '1m0s'
Expand Down
1 change: 1 addition & 0 deletions core/services/chainlink/testdata/config-empty-effective.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ ShutdownGracePeriod = '5s'
FeedsManager = true
LogPoller = false
UICSAKeys = false
TransactionService = false

[Database]
DefaultIdleInTxSessionTimeout = '1h0m0s'
Expand Down
1 change: 1 addition & 0 deletions core/services/chainlink/testdata/config-full.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ ShutdownGracePeriod = '10s'
FeedsManager = true
LogPoller = true
UICSAKeys = true
TransactionService = true

[Database]
DefaultIdleInTxSessionTimeout = '1m0s'
Expand Down
1 change: 1 addition & 0 deletions core/services/chainlink/testdata/config-multi-chain-effective.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ ShutdownGracePeriod = '5s'
FeedsManager = true
LogPoller = false
UICSAKeys = false
TransactionService = false

[Database]
DefaultIdleInTxSessionTimeout = '1h0m0s'
Expand Down
12 changes: 12 additions & 0 deletions core/store/models/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,3 +662,15 @@ func (h ServiceHeader) Validate() (err error) {
}
return
}

// CreateEVMTransactionRequest represents a request to create request to submit evm transaction.
type CreateEVMTransactionRequest struct {
IdempotencyKey string `json:"idempotencyKey"`
ChainID *utils.Big `json:"chainID"`
ToAddress *common.Address `json:"toAddress"`
FromAddress common.Address `json:"fromAddress"` // optional, if not set we use one of accounts available for specified chain
EncodedPayload string `json:"encodedPayload"` // hex encoded payload
Value *utils.Big `json:"value"`
ForwarderAddress common.Address `json:"forwarderAddress"`
FeeLimit uint32 `json:"feeLimit"`
}
Loading