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.

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

Add txmgr rpc api #10632

Closed
wants to merge 8 commits into from
Closed

Add txmgr rpc api #10632

wants to merge 8 commits into from

Conversation

bitwiseguy
Copy link
Contributor

@bitwiseguy bitwiseguy commented May 23, 2024

Description

Chain operators have hit issues in the past where txs get stuck in their nodes. This PR adds a new txmgr_ json rpc interface for nodes that use the TxManager (i.e. op-batcher, op-challenger, op-proposer) to give them more flexibility/insight when troubleshooting tx submission issues.

Tests

Added unit tests to the op-service/txmgr package. Updated op-e2e tests to ensure their use of mocked TxManager adheres to the new interface with its additional methods.

Additional context

Config rpc methods:

  • txmgr_getMinBaseFee
  • txmgr_setMinBaseFee
  • txmgr_getPriorityFee
  • txmgr_setPriorityFee
  • txmgr_getMinBlobFee
  • txmgr_setMinBlobFee
  • txmgr_getFeeThreshold
  • txmgr_setFeeThreshold
  • txmgr_getBumpFeeRetryTime
  • txmgr_setBumpFeeRetryTime

Pending tx rpc method:

  • txmgr_getPendingTxs
  • txmgr_cancelPendingTx

Metadata

@@ -124,7 +141,8 @@ type SimpleTxManager struct {
nonce *uint64
nonceLock sync.RWMutex

pending atomic.Int64
numPending atomic.Int64
pendingTxs map[uint64]*PendingTxWithCancel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this data structure provide everything we need? Namely, does the assumption that there will only ever be one pendingTx per nonce hold true? Or would we want to nest a map or slice to allow for multiple in-flight pending txs for a single nonce, e.g.: pendingTxs map[uint64]map[common.Hash]*PendingTxWithCancel

Copy link
Contributor

Choose a reason for hiding this comment

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

We resubmit at the same nonce with bumped gas, but have a single outer loop which handles that resubmit which could cancel all of the variations of the transaction. Per nonce is probably the easiest way to manually cancel a transaction

Copy link
Contributor

semgrep-app bot commented May 23, 2024

Semgrep found 2 invalid-usage-of-modified-variable findings:

Variable unsafeInNode is likely modified and later used on error. In some cases this could result in panics due to a nil dereference

Ignore this finding from invalid-usage-of-modified-variable.

Semgrep found 1 todos_require_linear finding:

  • packages/core-utils/src/common/bn.ts

Please create a GitHub ticket for this TODO.

Ignore this finding from todos_require_linear.

@@ -249,6 +249,7 @@ func (ps *ProposerService) initRPCServer(cfg *CLIConfig) error {
if cfg.RPCConfig.EnableAdmin {
adminAPI := rpc.NewAdminAPI(ps.driver, ps.Metrics, ps.Log)
server.AddAPI(rpc.GetAdminAPI(adminAPI))
server.AddAPI(txmgr.NewTxmgrApi(ps.TxManager, ps.Metrics, ps.Log))
Copy link
Contributor Author

@bitwiseguy bitwiseguy May 23, 2024

Choose a reason for hiding this comment

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

Currently the op-challenger does not have an rpc server. Should we create one (similar to how the op-proposer has an rpcServer implemented here) so that we also add the txmgr_ (and perhaps admin_) rpc methods to that service? If we add an rpc server to op-challenger, I'm assuming it would be part of this struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ajsutton. I think it would helpful to have one. I think when we first created the challenge it wasn't easy to quickly add the RPC service b/c of how startup was done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall ever looking at adding RPC to the op-challenger. Certainly happy to have it added since it seems useful and if someone can take on that work. Otherwise we should log an issue to track it but I don't think the proofs team will have capacity for it in the short term.

@bitwiseguy bitwiseguy marked this pull request as ready for review May 24, 2024 02:54
@bitwiseguy bitwiseguy requested a review from a team as a code owner May 24, 2024 02:54
@@ -196,6 +217,12 @@ type TxCandidate struct {
Value *big.Int
}

type PendingTxWithCancel struct {
tx *types.Transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to extend this to an array because the key for this is a nonce & it will often have multiple transactions (different gas prices + sig, same data) per nonce when resubmitting.

return receipt, err
}

type PendingTxRPC struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to include blobs here?

Copy link
Contributor

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

@protolambda
Copy link
Contributor

Can we close this, since it is being replaced by #10897 ?

@bitwiseguy
Copy link
Contributor Author

bitwiseguy commented Jun 21, 2024

Can we close this, since it is being replaced by #10897 ?

Yes. Closing this PR now

@bitwiseguy bitwiseguy closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

txmgr: admin rpc
5 participants