[Besu-Plugin] Use blob compressor selector by timestamp#2569
[Besu-Plugin] Use blob compressor selector by timestamp#2569gauravahuja wants to merge 11 commits intomainfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...equencer/sequencer/src/main/java/net/consensys/linea/utils/CachingTransactionCompressor.java
Outdated
Show resolved
Hide resolved
...r/sequencer/src/main/java/net/consensys/linea/config/LineaTransactionSelectorCliOptions.java
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2569 +/- ##
============================================
+ Coverage 58.89% 59.28% +0.39%
- Complexity 1620 1627 +7
============================================
Files 454 453 -1
Lines 18530 18528 -2
Branches 2014 2014
============================================
+ Hits 10913 10985 +72
+ Misses 6948 6867 -81
- Partials 669 676 +7
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
| @@ -25,25 +26,30 @@ | |||
| @Slf4j | |||
| public class CachingTransactionCompressor implements TransactionCompressor { | |||
There was a problem hiding this comment.
It looks like once we're past a certain timestamp:
- The caller of
CachingTransactionCompressormay get a cached compression size, compressed by an old compressor, because the key is only tx's hash - Cache is not invalidated after a switch timestamp is reached. However we don't need old cached entries for an old compressor version, so they will linger in memory for the expiry timeout
There was a problem hiding this comment.
CachingTransactionCompressor is not aware of the switch timestamps, neither does the BlobCompressorSelectorByTimestamp exposes this detail.So I don't know how cace invalidation could be accomplished. However there is TTL for the cache entry so it should automatically be invalidated after that.
I see two options:
- We might need to change the interface of BlobCompressorSelectorByTimestamp to be able to tell the caller that it has switched or
- The caller can always compare the previous BlobCompressor object with the one returned by BlobCompressorSelectorByTimestamp each time and if the object is different, invalidate the cache.
Which option do you prefer?
There was a problem hiding this comment.
I think we need a deeper restructuring.
- We can introduce a cache with a new key:
(Blob compressor version, tx hash). Since the cache key is different, this cache can't conform toTransactionCompressor, it needs to acceptBlob compressor versionalong with every transaction. This shared cache will be created inAbstractLineaSharedPrivateOptionsPluginand will be referenced by the other instances as backend - This new cache can be used by the new implementation of
TransactionCompressorwhich will accept the blob version on instantiation. This new implementation ofTransactionCompressorwill abstract users from the switch logic and will pass a constant blob compressor version, which was set in the constructor to the shared cache - Then we'll need custom implementations of
BlobCompressorSelectorByTimestampthat would be aware of Blob compressor versions, the switch logic and would instantiate theTransactionCompressorimplementation I described above
Instantiation
- For tx selection it can be done in
LineaTransactionSelector, because it's the first class that is aware ofTransactionEvaluationContextwhich has the pending block's header, which has the timestamp - For txpool validation, instantiation will be in
LineaTransactionPoolValidatorFactory::createTransactionValidator, by the current timestamp. It's called for every transaction before validation
BlobCompressorSelectorByTimestamp can be used in LineaTransactionSelector to instantiate BlobCompressor and pass it to CompressionAwareTransactionSelector's constructor
There was a problem hiding this comment.
BlobCompressorSelectorByTimestamp can be used in LineaTransactionSelector to instantiate BlobCompressor and pass it to CompressionAwareTransactionSelector's constructor
It is not clear to me how CompressionAwareTransactionSelector be reinstantiated with new version of BlobCompressor when the new compressor version timestamp is reached.
There was a problem hiding this comment.
You're right. We can't do it with the API we have, but in practice the timestamp we need is 1 frame away from the PluginTransactionSelectorFactory.create(...) call. So the way you create BlobCompressor inside CompressionAwareTransactionSelector is already optimal
@fab-10 Do you think it makes sense to pass some pending block metadata to PluginTransactionSelectorFactory? It would allow making the switch logic like that cleaner
There was a problem hiding this comment.
Could be a hard fork, could be a soft one. In this case it's a ground to switch a blob compressor. The idea is similar to the hard forks. Starting from a certain block timestamp we change the blob compressor version
There was a problem hiding this comment.
I think that's feasible, since selectors are created for each new block, they can take advantage of the pending block header.
Let me propose a quick PR
There was a problem hiding this comment.
In any case we won't be able to use your change in this PR, because we first need to finish the work on upgrading the Besu version
There was a problem hiding this comment.
The new API is now on Besu main
| Instant timestamp = | ||
| Instant.Companion.fromEpochSeconds(pendingHeader.getTimestamp(), 0L); | ||
| BlobCompressor blobCompressor = | ||
| blobCompressorSelectorByTimestamp.getBlobCompressor(timestamp); |
There was a problem hiding this comment.
I think that's wrong. Since blobCompressor can be injected into CompressionAwareTransactionSelector's constructor, why would we need to get it by timestamp here?
There was a problem hiding this comment.
Since this is the first class in the call stack which has access to evaluationContext, I think it makes the most sense to get a specific transaction compressor here. It would make even more sense in LineaTransactionSelectorFactory, but it seems that you can't access pending block's header from there
...equencer/acceptance-tests/src/test/kotlin/linea/plugin/acc/test/rpc/linea/EstimateGasTest.kt
Outdated
Show resolved
Hide resolved
...r/sequencer/src/main/java/net/consensys/linea/config/LineaTransactionSelectorCliOptions.java
Outdated
Show resolved
Hide resolved
...r/sequencer/src/main/java/net/consensys/linea/config/LineaTransactionSelectorCliOptions.java
Outdated
Show resolved
Hide resolved
| description = | ||
| "Comma-separated map of BlobCompressorVersion to Instant, e.g. V1_2=2025-01-01T00:00:00Z,V2=2026-01-01T00:00:00Z") | ||
| private String blobCompressorVersionTimestampsRaw = | ||
| String.format("%s=%s", BlobCompressorVersion.V2.name(), java.time.Instant.MIN); |
There was a problem hiding this comment.
Private constructor removed breaking encapsulation of CLI options
Low Severity
The private no-arg constructor LineaTransactionSelectorCliOptions() was removed (previously on line 140 of old code) when the new blobCompressorVersionTimestampsRaw field was added. This changes the constructor from private to the implicit package-private default, allowing direct instantiation from test code (new LineaTransactionSelectorCliOptions()) but weakening the intended factory pattern enforced by create().
There was a problem hiding this comment.
I do not know what issue this comment is referring to.
...va/net/consensys/linea/sequencer/txpoolvalidation/validators/ProfitabilityValidatorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
...linea-sequencer/sequencer/src/main/java/net/consensys/linea/utils/TransactionCompressor.java
Show resolved
Hide resolved
| hidden = true, | ||
| paramLabel = "<MAP>", | ||
| description = | ||
| "Comma-separated map of BlobCompressorVersion to Instant, e.g. V1_2=2025-01-01T00:00:00Z,V2=2026-01-01T00:00:00Z") |
There was a problem hiding this comment.
It would be nice to include all supported versions in the description so user knows what is available and how to write versions exactly
| apply from: lineaSequencerProject.file("gradle/dependency-management.gradle") | ||
| apply from: lineaSequencerProject.file("gradle/lint.gradle") | ||
|
|
||
| configurations.all { |
There was a problem hiding this comment.
Can you please add a clarifying comment on why we need this and what's the resolution strategy we do here?
5 participants
This PR implements issue(s) #2503
Checklist
PR.
Note
Medium Risk
Touches sequencer transaction selection/compression paths and introduces timestamp-based compressor selection, which can affect block building and profitability calculations if activation times or caching keys are wrong. Changes are scoped and covered by updated/new unit tests, but impact runtime behavior under blob size limits.
Overview
Switches sequencer compression from a single
GoBackedBlobCompressorinstance toBlobCompressorSelectorByTimestamp, enabling compressor version selection by block timestamp and threading that timestamp throughTransactionCompressor/CachingTransactionCompressor(including cache keys now incorporating compressor version).Adds a hidden CLI option
--plugin-linea-blob-compressor-version-timestampsparsed intoLineaTransactionSelectorConfiguration, wires it into shared plugin initialization, and updatesCompressionAwareTransactionSelectorto use the pending block’s timestamp for both per-tx size estimates and full-block compression checks.Bumps
blob-compressorto3.0.2, adds Kotlin stdlib dependency alignment in Gradle, and updates/extends tests (including new parsing tests) to use the selector-based compressor.Written by Cursor Bugbot for commit d0af4b0. This will update automatically on new commits. Configure here.