[NONEVM-714] - Add multiple blocks aggregation functionality to Block History Estimator #896
Conversation
|
|
SonarQube thinks some account addresses in the json file are IBM API Keys. That's why the low score.
|
|
|
||
| if desiredBlockCount > MAX_BLOCK_HISTORY_DEPTH { | ||
| // Limit the desired block count to maxblockHistoryDepth | ||
| desiredBlockCount = MAX_BLOCK_HISTORY_DEPTH |
There was a problem hiding this comment.
Maybe we should log a warning here
There was a problem hiding this comment.
The thing is this code executes once every BlockHistoryPollPeriod, so it may become really noisy.
Wdyt about refactoring and checking/logging during initialization?
There was a problem hiding this comment.
I think it might be ok not setting a max block history unless during testing we find some sort of definitive upper limit. Otherwise, we could make a recommendation to CCIP if they want to set a high value to not go over a certain value but we shouldn't make it a hard requirement in the code.
There was a problem hiding this comment.
Wdyt about refactoring and checking/logging during initialization?
If we do want to keep an upper bound, I think it's better to move this to initialization. But instead of just logging a warning, we should fail initialization so the person running the node can update the configs accordingly.
| @@ -31,6 +31,7 @@ var defaultConfigSet = Chain{ | |||
| BlockHistoryPollPeriod: config.MustNewDuration(5 * time.Second), | |||
| ComputeUnitLimitDefault: ptr(uint32(200_000)), // set to 0 to disable adding compute unit limit | |||
| EstimateComputeUnitLimit: ptr(false), // set to false to disable compute unit limit estimation | |||
| BlockHistoryDepth: ptr(uint64(20)), // number of blocks to fetch for multiple blocks fee estimation | |||
There was a problem hiding this comment.
Just to keep things consistent with EVM where we can, could we rename this config to BlockHistorySize instead?
There was a problem hiding this comment.
Also to keep existing behavior of the BHE, we should maybe set this default to 1 to mimic latest block behavior.
| type EstimationMethod int | ||
|
|
||
| const ( | ||
| LatestBlockEstimator EstimationMethod = iota | ||
| MultipleBlocksEstimator | ||
| ) | ||
|
|
||
| const MAX_BLOCK_HISTORY_DEPTH = 20 | ||
|
|
||
| func (em EstimationMethod) String() string { | ||
| switch em { | ||
| case LatestBlockEstimator: | ||
| return "LatestBlock" | ||
| case MultipleBlocksEstimator: | ||
| return "MultipleBlocks" | ||
| default: | ||
| return "UnknownMethod" | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we need to introduce an estimation method here. I think we can infer the same thing with using the BlockHistorySize config. Like if BlockHistorySize=1 then we should do the latest block logic, otherwise do the multiple blocks logic
There was a problem hiding this comment.
I think just for the unit tests we probably shouldn't commit such a big file. Feel like we should be able to get away with mocking a few blocks in code in block_history_test.go instead.
Description:
This PR Introduces the
MultipleBlocksEstimatorthat tries to get thedesiredBlockCountmost recent blocks to determine a more stable compute unit price.How It Works:
Configuration:
BlockHistoryDepthandFeeEstimatorModeare used to configure this estimator.Fetch Recent Blocks: Tries to retrieve specified number of the latest confirmed blocks from latest slots based on the configured
BlockHistoryDepth. Not all slots have a block.Concurrent Processing:: Fetches each block concurrently while limiting the number of simultaneous operations to prevent rate limiting.
Extract and Aggregate Fees: Parses each block to extract compute unit prices from transactions. Calculates the median compute unit price for each block.
Determine Overall Median: Aggregates the median prices from all fetched blocks. Computes the overall median to set as the current base compute unit price, ensuring it stays within predefined minimum and maximum config limits.