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

dhaidashenko · 2023-11-10T16:35:32Z

Marking it as draft while we are waiting for sec review

Added new endpoint that allows users to submit transaction request to TXM. Primary use case for this endpoint is config distribution via Gauntlet++
Endpoint requires Admin Role. Additionally it must be enabled via feature flag.

Open questions:

Is this tx.FeeLimit = chain.Config().EVM().GasEstimator().LimitDefault() the right way to get FeeLimit?

Issues that are not solved by this PR

It's not possible to deploy contract using this endpoint as Txm by design does not allow to set nil toAddress
Confirmed transactions are removed from db at some point. Gauntlet++ expects idempotent behavior of the endpoint on provided key. We might endup with duplicated transactions that might lead to unforeseen issues.
GET /transactions/evm returns last TxAttempt which might not be the one that is actually included into the chain

github-actions · 2023-11-10T16:35:58Z

I see that you haven't updated any README files. Would it make sense to do so?

prashantkumar1982

Thanks a lot, this was a pretty good change!

prashantkumar1982 · 2023-11-14T01:16:49Z

core/config/docs/core.toml

@@ -597,3 +597,8 @@ SamplingRatio = 1.0 # Example
 [Tracing.Attributes]
 # env is an example user specified key-value pair
 env = "test" # Example
+
+[TxmAsService]
+# Enabled turns Transaction Manager as a service feature on or off. When enabled exposes endpoint to submit arbitrary EVM transaction


nit: Also add that when enabled, regular Core Node services won't be run. Instead only TXM dependencies will be run.

Right now enabled does not affect which services are running. Should I introduce these changes as part of this PR?

prashantkumar1982 · 2023-11-14T01:27:38Z

core/web/evm_transactions_controller.go

+type EvmTransactionController struct {
+	Enabled     bool
+	Logger      logger.SugaredLogger
+	TxmStorage  txmgr.EvmTxStore


nit: lets name it as TxmStore. The previous name TxmStorageService was an old one, which we replaced with TxmStore, but I see there are still some places using the old name.

prashantkumar1982 · 2023-11-14T01:34:18Z

core/web/evm_transactions_controller.go

+	}
+
+	if tx.IdempotencyKey == "" {
+		jsonAPIError(c, http.StatusBadRequest, errors.New("idempotencyKey must be set"))


Actually this is optional in the txm service.
Did you mean here that any caller using the rest api should set it, just in case their connection gets lost, and we create the Tx anyways?
So it will be optional in the service, but required for rest clients?

Yes, the idea here is that if consumer of the API does not provide Idempotency key - most probably it's due to wrong design or bug. If they actually want to have non idempotent behaviour it's easily achieved by providing random key for each of the calls.

prashantkumar1982 · 2023-11-14T01:36:21Z

core/web/evm_transactions_controller.go

+	}
+
+	if tx.FromAddress == utils.ZeroAddress {
+		tx.FromAddress, err = tc.KeyStore.GetRoundRobinAddress(tx.ChainID.ToInt())


For now this is ok.
But I want to add a new feature, where the caller of TXM gives no fromAddress. And then TXM can choose the best fromAddress on its own.

core/web/evm_transactions_controller.go

core/store/models/common.go

core/web/evm_transactions_controller.go

prashantkumar1982 · 2023-11-14T01:48:32Z

core/web/router.go

@@ -279,6 +279,8 @@ func v2Routes(app chainlink.Application, r *gin.RouterGroup) {

 		txs := TransactionsController{app}
 		authv2.GET("/transactions/evm", paginatedRequest(txs.Index))
+		evmTxs := NewEVMTransactionController(app)
+		authv2.POST("/transactions/evm", auth.RequiresAdminRole(evmTxs.Create))


Do we know what is the admin role here?
Who owns this, and how can we ensure that caller can be an admin?
Would we need to discuss this with EngOps?
I think EngOps could be the admins, and they may not want to delegate admin role to the Config Distribution client. We may perhaps need another role or something equivalent.

For now this code is ok.

Admin role is property of the user entity that can be set via API endpoint by other admin or on core setup (first user is always admin).
Endpoints for keys management and transfers also require admin role, so it seems to be appropriate for create transaction.

Caller needs to specify API key of user with admin role, which can be created via API and by providing password and clsession cookies.

prashantkumar1982 · 2023-11-14T01:50:18Z

docs/CONFIG.md

+```toml
+Enabled = false # Default
+```
+Enabled turns Transaction Manager as a service feature on or off. When enabled exposes endpoint to submit arbitrary EVM transaction


nit: same as a comment earlier. Also add context that when this is true, the node doesn't run as a regular core node, and only starts services that are needed for TXM to work.

jmank88 · 2023-11-15T17:44:13Z

core/chains/evm/config/config.go

@@ -10,6 +10,7 @@ import (
 	"github.com/smartcontractkit/chainlink/v2/core/config"
 )

+//go:generate mockery --quiet --name EVM --output ./mocks/ --case=underscore


We have moved away from mocking config interfaces since they were cause a lot of problems. How is this one being used?

chainlink/core/web/evm_transactions_controller.go

Line 142 in e2dde3b

tx.FeeLimit = chain.Config().EVM().GasEstimator().LimitDefault()

This is the only way that I could get access to the default gas limit.
Is it better to introduce new method on chain.GasEstimator() that would return default gas limit?

I don't understand what you mean. Are we able to use a fake config type? Or a real config type populated with the values that we need? Mockery types comes with a ton of extra baggage.

For example:

chainlink/core/chains/evm/txmgr/test_helpers.go

Line 43 in 1208fb3

type TestEvmConfig struct {

jmank88 · 2023-11-15T17:48:41Z

core/config/docs/core.toml

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).

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.

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.

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.

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

[Feature] TransactionService = true

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.

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:

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.

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.

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.

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.

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.

go.mod

core/web/evm_transactions_controller.go

# Conflicts: # core/web/evm_transactions_controller_test.go

jmank88 · 2023-11-21T03:32:30Z

core/chains/evm/txmgr/evm_tx_store.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"

+	sq "github.com/Masterminds/squirrel"


Is this important to have? It doesn't look like we are using it for anything fancy, and it comes with some extra dependencies. Could we use regular SQL instead?

I can imagine some interesting uses though, and @krehermann might want to take a look

Yes, it's possible to implement a workaround, but it will be messier.
Let me know, if you'd like me to drop this dep

I didn't see all the logic at first, so I can see how it is helpful now, but for me it is still a little difficult to read and it seems inefficient too. Especially when reusing for the second query.

jmank88 · 2023-11-21T13:59:46Z

core/chains/evm/txmgr/evm_tx_store.go

+	query, args, err = stmt.RemoveColumns().Columns("*").
+		OrderBy("id desc").Limit(selector.Limit).Offset(selector.Offset).ToSql()


@samsondav @reductionista WDYT about this github.com/Masterminds/squirrel library? It certainly helps in dynamic cases, but I'm worried about readability and efficiency.

In my experience, SQL generators usually make things harder to read and understand than writing raw SQL. They sound nice in theory but don't work well in practice. Just my 2c

@jmank88 @samsondav for this case, do you prefer strings builder/concatenation to handle the optional idempotency_key?

I think it would be worth writing out to compare side-by-side at least.

@jmank88 @samsondav

func (o *evmTxStore) TransactionsWithAttempts(selector TransactionsWithAttemptsSelector) (txs []Tx, count int, err error) { countQuery := "SELECT count(*) FROM evm.txes WHERE id IN (SELECT DISTINCT eth_tx_id FROM evm.tx_attempts)" selectQuery := "SELECT * 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) // NOTE: we are lucky to have only one optional filter and WHERE is the root statement, so we can use AND selectQuery += " AND idempotency_key = ?" countQuery += " AND idempotency_key = ?" } countQuery = replacePositionalPlaceholders(countQuery) 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) } args = append(args, selector.Limit, selector.Offset) selectQuery += " ORDER BY id desc LIMIT ? OFFSET ?" var dbTxs []DbEthTx selectQuery = replacePositionalPlaceholders(selectQuery) 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) return } // Copy-paste from squirrel to replace ? with $1 func replacePositionalPlaceholders(sql string) string { buf := &bytes.Buffer{} i := 0 for { p := strings.Index(sql, "?") if p == -1 { break } if len(sql[p:]) > 1 && sql[p:p+2] == "??" { // escape ?? => ? buf.WriteString(sql[:p]) buf.WriteString("?") if len(sql[p:]) == 1 { break } sql = sql[p+2:] } else { i++ buf.WriteString(sql[:p]) fmt.Fprintf(buf, "$%d", i) sql = sql[p+1:] } } buf.WriteString(sql) return buf.String() }

Postgres uses numeric place holders for query arguments, so instead of ? we need to use $1, $2, etc.. So on the line
selectQuery += " ORDER BY id desc LIMIT ? OFFSET ?" we do not know which placeholders to use , as idempotency key is optional

Ah OK. So then is this correct? Is there any way to simplify it more? Maybe a Sprintf would be easier to read?

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) // NOTE: we are lucky to have only one optional filter and WHERE is the root statement, so we can use AND query += " AND idempotency_key = $1" } countQuery := "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 := "SELECT * " + query + " ORDER BY id desc " if len(args) == 0 { selectQuery += "LIMIT $1 OFFSET $2" } else { selectQuery += "LIMIT $2 OFFSET $3" } args = append(args, selector.Limit, selector.Offset) var dbTxs []DbEthTx 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) return }

Looks good, but I'm worried about this part

if len(args) == 0 { selectQuery += "LIMIT $1 OFFSET $2" } else { selectQuery += "LIMIT $2 OFFSET $3" }

What if we need to add another optional parameter? It does not seem to be easy to extend.
Why do you prefer this to the placeholder replacement?

I'm not sure. @samsondav WDYT?

sqlx comes with a helper to replace ? with $x. Does this solve your problem? https://github.com/jmoiron/sqlx/blob/master/bind.go#L60

# Conflicts: # integration-tests/types/config/node/core.go

cl-sonarqube-production · 2023-11-22T16:59:23Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

92.2% Coverage
0.0% Duplication

samsondav

Agree with Jordan's remarks, would rather not see us pull in a third party SQL generation library just for this

samsondav · 2023-11-28T14:08:21Z

core/chains/evm/txmgr/evm_tx_store.go

+	if selector.IdempotencyKey != nil {
+		stmt = stmt.Where("idempotency_key = ?", *selector.IdempotencyKey)


Why not just use string concatenation instead of squirrel?

github-actions · 2024-01-29T00:12:26Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2024-04-09T00:12:55Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

dhaidashenko and others added 7 commits November 9, 2023 17:42

Create Evm Transaction Endpoint

f2d5f60

rewrite tests to use mocks

0213c09

improve tests coverage

9958ec3

Flag to enable Txm as service

1ddc0eb

require admin role to send tx

d587ff4

moved question into PR

80c4fb7

Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint

c6142c3

fix config docs

565c9c3

prashantkumar1982 reviewed Nov 14, 2023

View reviewed changes

address comments

e2dde3b

dhaidashenko added the do-not-merge-yet label Nov 15, 2023

dhaidashenko marked this pull request as ready for review November 15, 2023 17:30

dhaidashenko requested review from a team, jkongie, samsondav and jmank88 as code owners November 15, 2023 17:30

jmank88 reviewed Nov 15, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint

010a1ca

jmank88 reviewed Nov 15, 2023

View reviewed changes

core/web/evm_transactions_controller.go Outdated Show resolved Hide resolved

dhaidashenko added 7 commits November 16, 2023 13:20

remove redundant dependency

c29f27d

proper status code on tx failure

1dd2430

wait for tx attempt to return properly populated resource

94a3c0a

remove evm config mock generation

4c23c16

handle fee bump

c70bb5d

Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint

ac995da

# Conflicts: # core/web/evm_transactions_controller_test.go

fixed merge

0c4af4e

jmank88 reviewed Nov 21, 2023

View reviewed changes

switch to feature flag

d8bf685

dhaidashenko requested a review from a team as a code owner November 21, 2023 15:46

Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint

5178e0b

# Conflicts: # integration-tests/types/config/node/core.go

dhaidashenko removed the do-not-merge-yet label Nov 21, 2023

Merge branch 'develop' into feature/BCI-2376-post-tx-endpoint

be19e4f

samsondav requested changes Nov 28, 2023

View reviewed changes

dhaidashenko marked this pull request as draft November 29, 2023 13:19

switch to sqlx.REbind for dynamic sql query

2d52d70

github-actions bot added the Stale label Jan 29, 2024

github-actions bot closed this Feb 6, 2024

dhaidashenko reopened this Feb 8, 2024

dhaidashenko removed the Stale label Feb 8, 2024

github-actions bot added the Stale label Apr 9, 2024

github-actions bot closed this Apr 17, 2024

		query, args, err = stmt.RemoveColumns().Columns("*").
		OrderBy("id desc").Limit(selector.Limit).Offset(selector.Offset).ToSql()

		if selector.IdempotencyKey != nil {
		stmt = stmt.Where("idempotency_key = ?", *selector.IdempotencyKey)

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

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

Conversation

dhaidashenko commented Nov 10, 2023 • edited Loading

github-actions bot commented Nov 10, 2023

prashantkumar1982 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaidashenko Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantkumar1982 Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaidashenko Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Nov 22, 2023

samsondav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2024

github-actions bot commented Apr 9, 2024

dhaidashenko commented Nov 10, 2023 •

edited

Loading

dhaidashenko Nov 15, 2023 •

edited

Loading

jmank88 Nov 15, 2023 •

edited

Loading

jmank88 Nov 15, 2023 •

edited

Loading

prashantkumar1982 Nov 20, 2023 •

edited

Loading

dhaidashenko Nov 21, 2023 •

edited

Loading