Move away from no blobs placeholders to storing earliestBlobSidecar separately #7128
Conversation
…art of storage comparison)
…from HistoricalBatchFetcher
…ng test from StoreTest and improve ChainStorageTest
|
Ready for review |
| new ConcurrentHashMap<>(); | ||
| private Optional<UInt64> maybeEarliestBlobSidecarSlot = Optional.empty(); |
There was a problem hiding this comment.
better to put it as volatile? can different threads access this non-final field?
There was a problem hiding this comment.
I think if alwaysRun fragment is executed in the same time (and it's the only other usage) when we do import, we definitely have problems in the whole class. I'm not sure we need volatile here
| if (!blobSidecars.isEmpty()) { | ||
| this.blobSidecars.put(block.getSlotAndBlockRoot(), blobSidecars); | ||
| } | ||
| if (maybeEarliestBlobSidecarTransactionSlot.isEmpty()) { |
There was a problem hiding this comment.
shouldn't be maybeEarliestBlobSidecarSlot from method param?
If I am right, why no tests are failing?
There was a problem hiding this comment.
No
We don't set it again if it's already set, that's an idea.
We set it only once.
We could override it later only when pruning.
I think it's a correct logic as we don't want to move it to the right on every batch
There was a problem hiding this comment.
As an option, we could check if new value is smaller and change it only in this condition.
Also we could consider moving earliestBlobSidecar to the separate method but I dislike it as I want to force anyone saving blobSidecars to provide it. I think it's safer
| this.blobSidecars.put(block.getSlotAndBlockRoot(), blobSidecars); | ||
| } | ||
| if (maybeEarliestBlobSidecarTransactionSlot.isEmpty()) { | ||
| maybeEarliestBlobSidecarTransactionSlot = maybeEarliestBlobSidecarSlot; |
| @@ -164,6 +164,11 @@ public int getBlobSidecarsCount(final Optional<SignedBeaconBlock> signedBeaconBl | |||
| .orElse(0); | |||
| } | |||
|
|
|||
| public UInt64 computeDenebStartSlot() { | |||
There was a problem hiding this comment.
I think we should use a feature not a milestone in the name. Something like computeFirstSlotWithBlobSupport
| @@ -408,10 +409,20 @@ private BlockImportResult importBlockAndState( | |||
| } | |||
|
|
|||
| final StoreTransaction transaction = recentChainData.startStoreTransaction(); | |||
| final UInt64 earliestAffectedSlot = | |||
There was a problem hiding this comment.
I'd definitely put a comment here to explain why we are doing this way. Or move the computation on a dedicated private method with a self-explanatory name
There was a problem hiding this comment.
Added comment to applyBlockToStore method, do you think it could be enough?
| final Optional<UInt64> maybeEarliestBlobSidecarSlotDb = dao.getEarliestBlobSidecarSlot(); | ||
| if (maybeEarliestBlobSidecarSlotDb.isEmpty() && maybeEarliestBlobSidecar.isPresent()) { | ||
| updater.setEarliestBlobSidecarSlot(maybeEarliestBlobSidecar.get()); | ||
| } | ||
| updater.commit(); |
There was a problem hiding this comment.
what about
maybeEarliestBlobSidecar.ifPresent(earliestBlobSidecar -> {
if(dao.getEarliestBlobSidecarSlot().isEmpty()) {
updater.setEarliestBlobSidecarSlot(earliestBlobSidecar);
}
});
so we query only if need?
There was a problem hiding this comment.
I've changed to the same idea but I like this code more
if (maybeEarliestBlobSidecar.isPresent() && dao.getEarliestBlobSidecarSlot().isEmpty()) {
updater.setEarliestBlobSidecarSlot(maybeEarliestBlobSidecar.get());
}
…smaller than existing tx/db value
PR Description
TODO:
deal with TODOsadd testsFixed Issue(s)
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog