Add gas bumping txmgr #284
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
samlaf
commented
Jun 28, 2024
samlaf
force-pushed
the
samlaf-avs-588-upstream-tx-manager-to-eigensdk-go
branch
from
7c57f9c to
ce05793
Compare
samlaf
force-pushed
the
samlaf-avs-588-upstream-tx-manager-to-eigensdk-go
branch
from
bb33f19 to
c8fef0f
Compare
samlaf
force-pushed
the
samlaf-avs-588-upstream-tx-manager-to-eigensdk-go
branch
2 times, most recently
from
b54c915 to
5616d83
Compare
ian-shim
reviewed
Aug 7, 2024
added gas oracle refactored txmgr module added geometric txmgr make fmt log -> logger in simple txmgr add TODO + rename constructor update geometric txmgr to use sync api gasoracle refactor chore: switch to pointer to gasoracle in geometric txmgr Also added some comments example: add example test for geometric txmgr fix(geometric_example_test): start anvil container refactor(geometric txmgr): delete gasoracle and add its methods to geometric txmgr directly make fmt fix(lint): remove no longer used variable hundred wip moving geometric to its own subdir simplify privatekey_wallet delete old moved files add geometric txmgr metrics interface + noop metrics docs: add default values in geometric txmgr params comment test(geometric txmgr): add first simple test for geometric txmgr test(geometric txmgr): add some basic tx sending tests feat(ecdsa): add KeyAndAddressFromHexKey helper function feat(geometric txmgr): add integration test scaffold with 1 simple tx sending test fix(geometric txmgr): remove unneeded mutex test(geometric txmgr): add parallel mining logic to fakeEthBackend.SendTransaction to test parallel sending of txs feat(geometrix txmgr): refactor queryTicketDuration to be a param instead of a fixed const refactor(txmgr test harness): refactor into a mining goroutine fix(geometric txmgr test): use mutex to get rid of race condition refactor(geometric txmgr params): made all params public fields feat(geometric txmgr test): added congestedBlocks field to test network congestion where txs need to get bumped also made tests run fast by setting txmgr's GetTxReceiptTickerDuration to 100ms instead of the default 3s fix(geometric txmgr): bug in ensureAnyTxBroadcasted ethereum.NotFound errors were not handled properly, causing txs to never get bumped feat(geometric txmgr test): added congested network test
samlaf
force-pushed
the
samlaf-avs-588-upstream-tx-manager-to-eigensdk-go
branch
from
a302e22 to
4c94ecd
Compare
…tead of percentages
…roadcasted for fireblock wallet
ian-shim
reviewed
Aug 8, 2024
|
|
||
| // processTransaction sends the transaction and runs the monitoring loop which will bump the gasPrice until the tx get | ||
| // included. | ||
| func (t *GeometricTxManager) processTransaction(ctx context.Context, req *txnRequest) (*types.Receipt, error) { |
There was a problem hiding this comment.
Is this method thread safe?
Could a transaction with higher nonce end up published before one with lower nonce?
There was a problem hiding this comment.
Oh btw also had to add 3a12fcc#diff-f3751b0e1d13e5e4c58c0ae0d936505427becc99a1c52514628bdaf32df6d370R488 to slow down the gas bumping loop, otherwise txs were getting bumped like crazy if a txs with the wrong (too-high) nonce was sent. Is that normal? Was there a better way to deal with this?
…is goroutine safe
ian-shim
previously approved these changes
Aug 9, 2024
chainio/txmgr/geometric/geometric.go
Outdated
Comment on lines
331
to
332
| // t.logger.Debug("Transaction not yet mined", "nonce", tx.Nonce(), "txID", txID, "txHash", | ||
| // tx.Hash().Hex(), "err", err) |
There was a problem hiding this comment.
What do we want to do here? We can remove this block if nothing is being done
There was a problem hiding this comment.
oops commented while debugging and committed by mistake. Uncommented in 63832fc
| // Remove the transaction from the list of transactions to query. | ||
| // go seemingly allows deleting from a map while iterating over it: | ||
| // https://groups.google.com/g/golang-nuts/c/rEmaoxi11_A | ||
| // although the official spec and faq don't seem to mention this anywhere... |
There was a problem hiding this comment.
There was a problem hiding this comment.
This doesn't seem to mention that deleting items while iterating is safe?
ian-shim
approved these changes
Aug 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also in passing Fixes #153 .
What Changed?
Also updated chainio and signer READMEs + some code comments
Reviewer Checklist