core/txpool/blobpool: migrate billy to new slot size #31966
Conversation
MariusVanDerWijden
requested review from
fjl,
rjl493456442,
lightclient,
jwasinger and
gballet
as code owners
MariusVanDerWijden
force-pushed
the
peerdas-slotter
branch
from
2503da0 to
7539ab9
Compare
MariusVanDerWijden
changed the title
MariusVanDerWijden
changed the title
| // txBlobOverhead is an approximation of the overhead that an additional blob | ||
| // has on transaction size. This is added to the slotter to avoid | ||
| // tiny overflows causing all txs to move a shelf higher, wasting disk space. | ||
| txBlobOverhead = 2048 |
There was a problem hiding this comment.
Just curious, is this a cautious estimate or based on a specific computation?
There was a problem hiding this comment.
This is an estimation based on specific computation :D
I computed the size of a bunch of blob transactions and found that the overhead is ~1300 bytes iirc. So 2048 kinda gives us a round estimate over that
There was a problem hiding this comment.
I think we should update this value.
The size of original proof is 48 bytes, while the new version cell proof is 128 * 48 = 6144, already beyond this constant.
core/txpool/blobpool/blobpool.go
Outdated
| if p.chain.Config().IsOsaka(head.Number, head.Time) { | ||
| // Check if we need to migrate our blob db to the new slotter | ||
| oldSlotSize := 135168 | ||
| if os.Stat(filepath.Join(queuedir, fmt.Sprintf("bkt_%08d.bag", oldSlotSize))); err == nil { |
There was a problem hiding this comment.
These seems to be taking advantage of billy internals -- can we just open the db and use Infos() to get the current slot size?
There was a problem hiding this comment.
We could, but that would mean we need to open the DB. I agree that this is not a super clean solution...
Co-authored-by: lightclient <[email protected]>
|
@MariusVanDerWijden let me know what you think about this last commit. I think the best thing to do is probably make a PR to billy to improve the migration flow, but given the current interface it seems like this is the best we can do downstream without hard coding any billy-specific behavior. |
|
|
||
| return func() (size uint32, done bool) { | ||
| slotsize += blobSize + txBlobOverhead | ||
| finished := slotsize > uint32(maxBlobsPerTransaction)*blobSize+txMaxSize |
There was a problem hiding this comment.
The target should be uint32(maxBlobsPerTransaction)*(blobSize+txBlobOverhead)+txMaxSize
| // perform a migration then write the updated version of the store. | ||
| if version < storeVersion { | ||
| newSlotter := newSlotterEIP7594(eip4844.LatestMaxBlobsPerBlock(p.chain.Config())) | ||
| if err := billy.Migrate(billy.Options{Path: queuedir, Repair: true}, slotter, newSlotter); err != nil { |
There was a problem hiding this comment.
It's a bit awkward.
Before the migration, the blob transactions may still still carry the v0 sidecar. We should make the conversion first, and then migrate them to the new shelves respectively.
| // Index all transactions on disk and delete anything unprocessable | ||
| var fails []uint64 | ||
| index := func(id uint64, size uint32, blob []byte) { | ||
| if len(blob) == 1 { |
There was a problem hiding this comment.
It's actually unnecessary, we can probably keep it, but emit a big warning if it occurs.
the slotter won't contain the shelf for the metadata.
Implements a migration path for the blobpool slotter