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

Simplify class TxHelpers #478

Closed
wants to merge 105 commits into from
Closed

Simplify class TxHelpers #478

wants to merge 105 commits into from

Conversation

barakman
Copy link
Collaborator

@barakman barakman commented Mar 22, 2024

Some of the changes in the TxHelpers class:

  1. Removed the DEFAULT_GAS_PRICE_OFFSET ("increase by 9%") factor applied on the max priority fee:

    • It is applied only in chains where the bot uses EIP1559 (Ethereum and Base), and the bot shouldn't rely on this partial solution, even if that solution appears to be working somehow.
    • It reduces the expected execution time of a transaction, making it more likely to succeed while the arbitrage opportunity is still viable. But on the same time, it makes that arbitrage opportunity less likely to be viable in the first place (and it makes it less likely to be viable, more or less by the same magnitude of 9%). So there's no clear advantage in this heuristics to begin with, and this is regardless of that specific 9% default value.
  2. Removed the parsing of the "baseFee" error-message returned from the node. This error happens when the block's baseFee becomes higher than the transaction's maxGasFee, which theoretically yields a negative tip to the miner, hence the transaction is immediately reverted. Recall that the miner gets paid maxGasFee - baseFee, regardless of the specified maxPriorityFee, which indicates the maximum tip and not the actual tip. But even before this happens, the block's baseFee may become sufficiently high to bring the effective tip paid to the miner extremely small, thus leading the transaction to linger for a very long time, and most likely become invalid (arbitrage already fulfilled) by the time it gets executed. So the only difference between these two scenarios is a reverted transaction vs a stuck transaction. Both can sporadically happen, just like front-running can sporadically invalidate arbs submitted by this bot, for example. This error can be handled in a "natural manner", at the next iteration of the bot's infinite loop, hence there is no need to tailor-make a special "handle error and retry" method for this specific error.

  3. Fetching the maxPriorityFee via web3 instead of by sending an HTTP request to the node. It shouldn't yield a notable impact, but, why invent the wheel?

  4. Fetching the gasPrice instead of the baseFee, since the latter requires fetching the entire block, which most likely takes longer to complete. In the case of chains which already support EIP1559 (e.g., Ethereum, starting from the London fork), this value indicates the sum of the current block's baseFee and the average maxPriorityFee, hence it can be used as is when setting the transaction's maxGasFee. And of course, in the case of chains which do not yet support EIP1559, it can be used as is in order to set the transaction's gasPrice.

  5. After the transaction is built, it already includes the estimated gas, so no need to then send another estimateGas request to the node.

  6. Fetching the access-list via web3 instead of by sending an HTTP request to the node, because - again - why invent the wheel?

  7. After the access-list is added to the transaction, it already includes the new estimated gas, so no need to then send another estimateGas request to the node.

  8. Removed the EXPECTED_GAS_MODIFIER ("decrease to 85%") factor used for assessing the actual gas cost of the transaction.

  9. Replaced fetching the latest block (used in order to specify max block number when sending a private transaction to Alchemy), with fetching only the block number itself, which is probably (or at least hopefully) more efficient.

  10. Removed support for legacy-transactions; all chains are assumed to be supporting EIP-1559.

  11. Sending eth_sendPrivateTransaction HTTP requests via web3 instead of via the node. This change includes the removal of the retries=3 parameter, which needs to be verified.

  12. Instead of the DEFAULT_GAS_PRICE_OFFSET ("increase by 9%") factor applied on the max priority fee, introduced a GAS_STRATEGY hook function with which the user can configure their desired gas-fee strategy.

  13. Ensured that access-list is applied on Ethereum transactions; it is not applied in the current version due to a bug in the code, and even after this PR, it will not be tested within Tenderly (when the target chain is Ethereum) because of that same bug; see issue NETWORK_TENDERLY overrides the original NETWORK #519.

  14. Constructing the tx dictionary manually and then setting its gas attribute by calling estimate_gas, instead of via build_transaction to do both of these actions; Following that, setting the accessList attribute of this dictionary, and possibly updating its gas attribute, by calling create_access_list; The gas-fee attributes (maxFeePerGas and maxPriorityFeePerGas) are set only at the end, in order to avoid getting the "max fee per gas less than block base fee" error.

Copy link
Collaborator

@sklbancor sklbancor left a comment

Choose a reason for hiding this comment

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

some small stuff; does not necessarily need to be changed but I'd like to discuss

more generally -- it is hard to review the changes; it looks like functionality changed somewhat and things moved around a lot so the diffs are not particularly useful; with good test coverage this is not a problem -- but given how poor our test coverage seems to be: are you sure this does not introduce bugs?

fastlane_bot/bot.py Show resolved Hide resolved
fastlane_bot/events/utils.py Show resolved Hide resolved
fastlane_bot/helpers/txhelpers.py Show resolved Hide resolved
fastlane_bot/helpers/txhelpers.py Outdated Show resolved Hide resolved
fastlane_bot/helpers/txhelpers.py Show resolved Hide resolved
fastlane_bot/helpers/txhelpers.py Outdated Show resolved Hide resolved
fastlane_bot/helpers/txhelpers.py Show resolved Hide resolved
fastlane_bot/helpers/txhelpers.py Show resolved Hide resolved
@barakman
Copy link
Collaborator Author

barakman commented Mar 23, 2024

Open questions regarding the TxHelpers class:

  1. The value of self.wallet_address is set upon class initialization, but also in function _build_transaction, which permanently changes it to BINANCE14_WALLET_ADDRESS. Is this the desired behavior?

  2. In function validate_and_submit_transaction, the pending block is used when calling build_transaction_with_gas, but the latest block is used when calling submit_private_transaction. Is this the desired behavior?

  3. In function validate_and_submit_transaction, the multiplication by EXPECTED_GAS_MODIFIER is applied on the base gas cost, but not on the L1-Data part which is sometimes added to it. Is this the desired behavior?

  4. In function build_transaction_with_gas, there's an attempt to catch a "max fee per gas less than block base fee" error even though this function does actually submit any transaction. What exactly is the point in that?

@sklbancor
Copy link
Collaborator

Open questions regarding the TxHelpers class:

  1. The value of self.wallet_address is set upon class initialization, but also in function _build_transaction, which permanently changes it to BINANCE14_WALLET_ADDRESS. Is this the desired behavior?
  2. In function validate_and_submit_transaction, the pending block is used when calling build_transaction_with_gas, but the latest block is used when calling submit_private_transaction. Is this the desired behavior?
  3. In function validate_and_submit_transaction, the multiplication by EXPECTED_GAS_MODIFIER is applied on the base gas cost, but not on the L1-Data part which is sometimes added to it. Is this the desired behavior?

no idea, but 1/ - probably not would be my guess...

@barakman
Copy link
Collaborator Author

Open questions regarding the TxHelpers class:

  1. The value of self.wallet_address is set upon class initialization, but also in function _build_transaction, which permanently changes it to BINANCE14_WALLET_ADDRESS. Is this the desired behavior?
  2. In function validate_and_submit_transaction, the pending block is used when calling build_transaction_with_gas, but the latest block is used when calling submit_private_transaction. Is this the desired behavior?
  3. In function validate_and_submit_transaction, the multiplication by EXPECTED_GAS_MODIFIER is applied on the base gas cost, but not on the L1-Data part which is sometimes added to it. Is this the desired behavior?
  4. In function build_transaction_with_gas, there's an attempt to catch a "max fee per gas less than block base fee" error even though this function does actually submit any transaction. What exactly is the point in that?
  1. Fixed
  2. Fixed to use the pending block in all cases
  3. Waiting for decision
  4. No need to fix; error occurs when building a transaction, not when submitting one

@barakman
Copy link
Collaborator Author

barakman commented Apr 9, 2024

Closing and reposting as #529

@barakman barakman closed this Apr 9, 2024
@barakman barakman deleted the simplify-class-TxHelpers branch April 9, 2024 13:35
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.

3 participants