Cache submitter gas block #1465
Conversation
WalkthroughThis update introduces caching for gas blocks in different chains to improve performance. It also refines the GitHub workflow for label management, replacing a third-party action with a built-in GitHub action for better maintainability. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- ethergo/submitter/export_test.go (4 hunks)
- ethergo/submitter/submitter.go (7 hunks)
- ethergo/submitter/submitter_test.go (3 hunks)
Additional comments (Suppressed): 11
ethergo/submitter/submitter_test.go (3)
2-9: The import of
math/bighas been moved from the bottom of the import block to the top. This change is acceptable as it does not affect the functionality of the code. However, it's a good practice to group standard library imports together and separate them from third-party imports for better readability.22-25: The import of
math/bighas been removed from this block. Ensure that this does not affect any functionality in the code that relies on this package.76-107: The new
TestGetGasBlockfunction tests theGetGasBlockmethod's behavior in both successful and failedHeaderByNumberRPC call scenarios, and verifies the caching behavior when the RPC call fails. This is a good addition as it covers different scenarios and ensures that the caching mechanism works as expected.ethergo/submitter/export_test.go (4)
3-9: The import of
math/bighas been removed. Ensure that this does not affect any existing functionality that relies on this package.13-16: The import of
math/bighas been removed. Ensure that this does not affect any existing functionality that relies on this package.86-93: The
GetGasBlockfunction has been added to theTestTransactionSubmitterinterface. Ensure that all implementations of this interface have been updated accordingly.100-106: The
GetGasBlockfunction has been implemented in thetxSubmitterImplstruct. This function is a wrapper around thegetGasBlockfunction, which is presumably a private function in the same struct.ethergo/submitter/submitter.go (4)
64-71: The addition of
gasBlockCacheto thetxSubmitterImplstruct is a good move for performance optimization. It allows the caching of gas blocks for different chains, which can reduce the number of RPC calls when theHeaderByNumbercall fails. However, please ensure that the cache is properly managed to avoid potential memory leaks or stale data.80-93: The
NewTransactionSubmitterfunction has been updated to initialize thegasBlockCache. This is a good practice as it ensures that the cache is ready for use when the transaction submitter is created.375-382: The
getGasBlockfunction now accepts achainIDparameter. This is a breaking change and all calls to this function throughout the codebase should be updated to match the new signature. Also, the error handling has been improved to return the error when the gas block retrieval fails, which prevents a potential null pointer exception.412-434: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [397-434]
The
getGasBlockfunction has been updated to support the caching mechanism. If theHeaderByNumberRPC call fails, the function will attempt to load the gas block from the cache. If the gas block is not in the cache, an error is returned. This is a good approach as it provides a fallback mechanism when the RPC call fails. However, please ensure that the cache is updated regularly to avoid serving stale data.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1465 +/- ##
===================================================
+ Coverage 50.69210% 50.84105% +0.14894%
===================================================
Files 356 366 +10
Lines 24274 25147 +873
Branches 267 268 +1
===================================================
+ Hits 12305 12785 +480
- Misses 10775 11122 +347
- Partials 1194 1240 +46
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- ethergo/submitter/submitter.go (7 hunks)
Additional comments (Suppressed): 6
ethergo/submitter/submitter.go (6)
64-71: The addition of
gasBlockCacheto thetxSubmitterImplstruct is a good approach to mitigate the risk of a null pointer when failing to get the gas block. It's a map that stores the gas block for different chains, which can be used as a fallback when fetching the gas block from the chain fails.80-93: The
NewTransactionSubmitterfunction is updated to initialize thegasBlockCachemap. This is a necessary step to ensure that the cache is ready for use when the transaction submitter is created.374-381: The
setGasPricefunction now passes thechainIDto thegetGasBlockfunction. This is a change in the function signature ofgetGasBlock, so ensure that all calls to this function throughout the codebase have been updated to match the new signature.398-402: The
getGasBlockfunction signature has been updated to include thechainIDparameter. This is necessary for the function to fetch the correct gas block from the cache when theHeaderByNumberRPC call fails.411-426: The
getGasBlockfunction has been modified to retrieve the gas block from the cache if theHeaderByNumberRPC call fails. This is a good fallback mechanism that can prevent null pointer errors when the RPC call fails. However, it's important to note that the cached gas block might not be the latest one if the cache hasn't been updated recently.428-431: The
getGasBlockfunction now caches the latest gas block after successfully fetching it from the chain. This ensures that the cache is always updated with the latest gas block, which can be used as a fallback when theHeaderByNumberRPC call fails.
…cns/sanguine into feat/submitter-gas-block
|
Trying to fix add-label action, as it seems the andymckay/labeler action has been deprecated for over a year: Whatever caused this was probably an issue between #1399 and now 
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files ignored due to filter (10)
- agents/go.mod
- agents/go.sum
- contrib/promexporter/go.mod
- contrib/promexporter/go.sum
- ethergo/go.mod
- ethergo/go.sum
- services/cctp-relayer/go.mod
- services/cctp-relayer/go.sum
- services/omnirpc/go.mod
- services/omnirpc/go.sum
Files selected for processing (1)
- ethergo/submitter/submitter.go (7 hunks)
Additional comments (Suppressed): 6
ethergo/submitter/submitter.go (6)
4-17: The import of
github.com/puzpuzpuz/xsync/v2is new and is used for thelastGasBlockCachein thetxSubmitterImplstruct. Ensure that this package is added to the project dependencies.65-72: The
lastGasBlockCachefield has been added to thetxSubmitterImplstruct. This cache is used to store the last gas block for a given chain. This is a good addition as it can improve performance by reducing the number of RPC calls to get the gas block.81-94: The
NewTransactionSubmitterfunction has been updated to initialize thelastGasBlockCachefield in thetxSubmitterImplstruct. This is a necessary change to ensure that the cache is properly initialized when a new transaction submitter is created.376-382: The
getGasBlockfunction now takes an additionalchainIDparameter. This change is necessary to support the new caching behavior. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.399-403: The
getGasBlockfunction signature has been updated to include achainIDparameter. This change is necessary to support the new caching behavior. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.412-433: The
getGasBlockfunction has been updated to use thelastGasBlockCachewhen theHeaderByNumberRPC call fails. This is a good change as it can improve performance by reducing the number of RPC calls. However, it's important to note that this could potentially lead to using stale gas block data if the cache is not updated frequently enough.
|
are agents flaking here? Or did I introduce a regression? |
Hard to say- the retry settings on the agents CI may be too lenient. Checking locally |
Description
Adds a
gasBlockCacheto submitter so that we can use a cached value in the event that theHeaderByNumberRPC call fails.Additional context
Also fixes a bug where we fail to get the gas block properly, but continue with setting gas price and are at risk of getting nullptr.
Summary by CodeRabbit