mempool,rpc: add removetx rpc method #7047
Conversation
tychoish
requested review from
cmwaters,
creachadair,
ebuchman and
williambanfield
as code owners
There was a problem hiding this comment.
one minor comment. Love the cleanup!! thank you so much.
@adizere i think you and mircea were asking for something like this?
internal/rpc/core/routes.go
Outdated
| @@ -28,6 +28,7 @@ func (env *Environment) GetRoutes() RoutesMap { | |||
| "block_results": rpc.NewRPCFunc(env.BlockResults, "height", true), | |||
| "commit": rpc.NewRPCFunc(env.Commit, "height", true), | |||
| "check_tx": rpc.NewRPCFunc(env.CheckTx, "tx", true), | |||
| "remove_tx": rpc.NewRPCFunc(env.RemoveTx, "txkey,removeFromCache", false), | |||
There was a problem hiding this comment.
we should try to avoid putting this on the public interface. Any one could potentially constantly remove all txs. Possibly group into the unsafe section?
There was a problem hiding this comment.
I don't necessarily see how that changes anything, as the unsafe section are just as available to users as the safe section
There was a problem hiding this comment.
the unsafe section makes the operator aware they are turning it on. This change will expose functionality that could alter the nodes behaviour, that was my reasoning for suggesting unsafe.
I could crawl the network, find nodes and get a list of unconfirmed txs and remove all of them. If its a node that is a sentry or a validator it would clear the mempool. If we are fine exposing such behaviour, it should be documented somehow that this can cause problems.
tychoish
changed the title
|
We should also add to the openapi docs as well. |
There was a problem hiding this comment.
I read the code to understand the changes introduced by this PR, but my familiarity with tm-go is minimal. (Perhaps a PR description would be helpful in that regard.)
I have two questions to help me understand better the properties guaranteed by this new API:
-
Suppose I call
remove_tx(TxHashXYZ)on my full node. Is it possible that the transaction with keyTxHashXYZwill re-enter the mempool of my full node if some peer of my full node broadcasts that tx? -
Related to question above: After calling
remove_tx(TxHashXYZ)there's nothing preventing the call tobroadcast_tx_sync(TxHashXYZ), right?
| @@ -32,6 +32,10 @@ type Mempool interface { | |||
| // its validity and whether it should be added to the mempool. | |||
| CheckTx(ctx context.Context, tx types.Tx, callback func(*abci.Response), txInfo TxInfo) error | |||
|
|
|||
| // RemoveTxByKey removes a transaction, identified by its key, | |||
| // from the mempool. | |||
| RemoveTxByKey(txKey types.TxKey) error | |||
There was a problem hiding this comment.
What is a TxKey exactly, the same as hash? Ideally if we could remove the tx by hash that could be simpler.
There was a problem hiding this comment.
Yup it's the sha256 hash of the transaction
|
Hey @adizere. There is a fifo cache that can be used (see |
| return nil | ||
| } | ||
|
|
||
| return errors.New("transaction not found") |
There was a problem hiding this comment.
Should this get a variable like the already-in-cache case? It seem like the kind of thing the caller might care to distinguish.
There was a problem hiding this comment.
I guess I don't much see the point in distinguishing the error: there's no circumstance where removing a transaction would be retriable (perhaps eventually via the RPC, you might get a networking error that would make it worth retrying, but you'd want to be able to identify that that error is retriable? which seems/feels like an RPC feature.)
There was a problem hiding this comment.
I see your point, but is the answer different for the "already in cache" case?
(Separately, and not related to this change, it seems like a mistake that we don't use the error code of the JSON-RPC response to communicate errors responsibly to the client)
Addresses one of the concerns with #7041. Provides a mechanism (via the RPC interface) to delete a single transaction, described by its hash, from the mempool. The method returns an error if the transaction cannot be found. Once the transaction is removed it remains in the cache and cannot be resubmitted until the cache is cleared or it expires from the cache. (cherry picked from commit 851d2e3)
Addresses one of the concerns with #7041. Provides a mechanism (via the RPC interface) to delete a single transaction, described by its hash, from the mempool. The method returns an error if the transaction cannot be found. Once the transaction is removed it remains in the cache and cannot be resubmitted until the cache is cleared or it expires from the cache. (cherry picked from commit 851d2e3) Co-authored-by: Sam Kleinman <garen@tychoish.com>
Addresses one of the concerns with #7041.
Provides a mechanism (via the RPC interface) to delete a single transaction, described by its hash, from the mempool. The method returns an error if the transaction cannot be found. Once the transaction is removed it remains in the cache and cannot be resubmitted until the cache is cleared or it expires from the cache.