Implementation ideas
Context
#1550 added tests to the block submission that highlighted the following edge case.
Given a set of blocks, if any of the blocks has a marshaling error, none of the previous blocks would be submitted.
For example, given the following test case:
name: "A and B are fair blocks, but C has a marshalling error. So C never gets submitted",
blocks: func() []*types.Block {
numBlocks, numTxs := 3, 5
blocks := make([]*types.Block, numBlocks)
blocks[0] = types.GetRandomBlock(uint64(1), numTxs)
blocks[1] = types.GetRandomBlock(uint64(2), numTxs)
blocks[2] = types.GetRandomBlock(uint64(3), numTxs)
invalidateBlockHeader(blocks[2])
return blocks
}(),
isErrExpected: true,
expectedPendingBlocksLength: 1,
This is the ideal end result, A and B would be submitted, while C isn't due to it's marshaling error.
However, currently no blocks will be submitted because a marshal error immediately returns an error as oppose to using a break like exceeding the blob size.
REF: https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L113
Proposal
There are a few things that I think can be cleaned up in the SubmitBlocks method.
-
submitted
The submitted variable is not needed and is a confusing variable. https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L109
We can simply use len(blobs) here https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L128
-
Break on marshal error
I think we should break on marshal error so we can submit any good blocks ahead of the bad block https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L113
The result of 1 and 2 would be something like this:
var (
blobs [][]byte
blobSize uint64
code StatusCode
message string
)
for i := range blocks {
blob, err := blocks[i].MarshalBinary()
if err != nil {
code = StatusError
message = fmt.Sprint("failed to serialize block", err)
dac.Logger.Info(message)
break
}
if blobSize+uint64(len(blob)) > maxBlobSize {
code = StatusError
message = fmt.Sprint(ErrBlobSizeOverLimit.Error(), "blob size limit reached", "maxBlobSize", maxBlobSize, "index", i, "blobSize", blobSize, "len(blob)", len(blob))
dac.Logger.Info(message)
break
}
blobSize += uint64(len(blob))
blobs = append(blobs, blob)
}
if len(blobs) == 0 {
return ResultSubmitBlocks{
BaseResult: BaseResult{
Code: code,
Message: "failed to submit blocks: no blobs generated" + message,
},
}
}
- Improve handling of marshal error and blob limit exceeded with submit errors.
In the case where len(blobs) > 0 any errors or issues in the blob generation is forgotten about. We should either properly propagate those errors after any error handling of dac.da.Submit, or we should have a comment explaining that since we break out of the blob generating loop on the first error, that block will be cause the same error the next submission resulting in the error propagating at that point because len(blobs) will be zero the next time around.
Implementation ideas
Context
#1550 added tests to the block submission that highlighted the following edge case.
Given a set of blocks, if any of the blocks has a marshaling error, none of the previous blocks would be submitted.
For example, given the following test case:
This is the ideal end result, A and B would be submitted, while C isn't due to it's marshaling error.
However, currently no blocks will be submitted because a marshal error immediately returns an error as oppose to using a
breaklike exceeding the blob size.REF: https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L113
Proposal
There are a few things that I think can be cleaned up in the
SubmitBlocksmethod.submittedThe
submittedvariable is not needed and is a confusing variable. https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L109We can simply use
len(blobs)here https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L128Break on marshal error
I think we should
breakon marshal error so we can submit any good blocks ahead of the bad block https://github.com/rollkit/rollkit/blob/f61fa09916be6d25bc87654bf09d4d0eab847a3c/da/da.go#L113The result of 1 and 2 would be something like this:
In the case where
len(blobs) > 0any errors or issues in the blob generation is forgotten about. We should either properly propagate those errors after any error handling ofdac.da.Submit, or we should have a comment explaining that since we break out of the blob generating loop on the first error, that block will be cause the same error the next submission resulting in the error propagating at that point becauselen(blobs)will be zero the next time around.