BCI-2376 TXM as service. Post TX endpoint. #11256
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import ( | |
| "strings" | ||
| "time" | ||
|
|
||
| sq "github.com/Masterminds/squirrel" | ||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/google/uuid" | ||
| "github.com/jackc/pgconn" | ||
|
|
@@ -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(selector TransactionsWithAttemptsSelector) ([]Tx, int, error) | ||
| FindTxAttempt(hash common.Hash) (*TxAttempt, error) | ||
| FindTxWithAttempts(etxID int64) (etx Tx, err error) | ||
| } | ||
|
|
@@ -440,18 +441,38 @@ 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(selector TransactionsWithAttemptsSelector) (txs []Tx, count int, err error) { | ||
| stmt := sq.Select("count(*)").From("evm.txes"). | ||
| Where("id IN (SELECT DISTINCT eth_tx_id FROM evm.tx_attempts)").PlaceholderFormat(sq.Dollar) | ||
| if selector.IdempotencyKey != nil { | ||
| stmt = stmt.Where("idempotency_key = ?", *selector.IdempotencyKey) | ||
|
There was a problem hiding this comment. Why not just use string concatenation instead of squirrel? |
||
| } | ||
|
|
||
| query, args, err := stmt.ToSql() | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("failed to build count query: %w", err) | ||
| } | ||
|
|
||
| if err = o.q.Get(&count, query, args...); err != nil { | ||
| return nil, 0, fmt.Errorf("failed to exec count query: %w, sql: %s", err, query) | ||
| } | ||
|
|
||
| query, args, err = stmt.RemoveColumns().Columns("*"). | ||
| OrderBy("id desc").Limit(selector.Limit).Offset(selector.Offset).ToSql() | ||
|
There was a problem hiding this comment. @samsondav @reductionista WDYT about this github.com/Masterminds/squirrel library? It certainly helps in dynamic cases, but I'm worried about readability and efficiency. There was a problem hiding this comment. 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 There was a problem hiding this comment. @jmank88 @samsondav for this case, do you prefer strings builder/concatenation to handle the optional There was a problem hiding this comment. I think it would be worth writing out to compare side-by-side at least. There was a problem hiding this comment. 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()
}There was a problem hiding this comment. Postgres uses numeric place holders for query arguments, so instead of There was a problem hiding this comment. Ah OK. So then is this correct? Is there any way to simplify it more? Maybe a 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
}There was a problem hiding this comment. 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. There was a problem hiding this comment. I'm not sure. @samsondav WDYT? There was a problem hiding this comment. sqlx comes with a helper to replace |
||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("failed to build select query: %w", err) | ||
| } | ||
| var dbTxs []DbEthTx | ||
| if err = o.q.Select(&dbTxs, query, args...); err != nil { | ||
| return nil, 0, fmt.Errorf("failed to exec select query: %w, sql: %s", err, query) | ||
| } | ||
| txs = dbEthTxsToEvmEthTxs(dbTxs) | ||
| err = o.preloadTxAttempts(txs) | ||
|
|
||
|
There was a problem hiding this comment. What is the rationale for feature flagging this service? If is meant to be temporary only, then we should use the existing There was a problem hiding this comment. 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. There was a problem hiding this comment.
Even knowing that the ability to send tokens is already available? I am claiming that this is inconsistent. There was a problem hiding this comment.
Can you elaborate on this? It sounds like an anti-pattern. There was a problem hiding this comment. Regardless, it sounds like [Feature]
TransactionService = trueThere was a problem hiding this comment.
There are tradeoffs, and these are not public.
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. There was a problem hiding this comment. 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.
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. There was a problem hiding this comment.
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.
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. There was a problem hiding this comment. When "TransactionService = false", which will be the default, the service will operate as current Core Node. 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? There was a problem hiding this comment. 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 |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package config | ||
|
|
||
| type TxmAsService interface { | ||
| Enabled() bool | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package chainlink | ||
|
|
||
| import ( | ||
| "github.com/smartcontractkit/chainlink/v2/core/config" | ||
| "github.com/smartcontractkit/chainlink/v2/core/config/toml" | ||
| ) | ||
|
|
||
| var _ config.TxmAsService = (*txmAsServiceConfig)(nil) | ||
|
|
||
| type txmAsServiceConfig struct { | ||
| c toml.TxmAsService | ||
| } | ||
|
|
||
| func (t *txmAsServiceConfig) Enabled() bool { | ||
| return t.c.Enabled != nil && *t.c.Enabled | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -232,3 +232,6 @@ Enabled = false | |
| CollectorTarget = '' | ||
| NodeID = '' | ||
| SamplingRatio = 0.0 | ||
|
|
||
| [TxmAsService] | ||
| Enabled = false | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine some interesting uses though, and @krehermann might want to take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.