Conversation
|
|
This PR changes implementation code, but doesn't include a changeset. Did you forget to add one? |
| // If the range of blocks serialized to be too large, restore | ||
| // blocks that could not be included inside the channel | ||
| if len(leftOverBlocks) != 0 { | ||
| s.blocks = append(leftOverBlocks, s.blocks...) |
There was a problem hiding this comment.
This looks right, I think a test case hitting this particular branch would be nice to see
There was a problem hiding this comment.
yea, this code also worried me a bit
trianglesphere
force-pushed
the
jg/clean_up_load_into_state
branch
from
67aa6d6 to
4f8d797
Compare
trianglesphere
force-pushed
the
jg/full_channel_manger
branch
from
95a5d91 to
2545bdf
Compare
| if err != nil { | ||
| return nil, txID{}, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Perhaps sanity check on the length of frames being less than math.MaxUint16 to prevent an overflow with the typecast, it should never happen but it would be a nice sanity check
There was a problem hiding this comment.
Gonna leave a TODO comment about this. It's a good idea, but has some trickiness when implementing it.
tynes
left a comment
tynes
left a comment
There was a problem hiding this comment.
Will re-review after comments addressed
|
Hey @trianglesphere! This PR has merge conflicts. Please fix them before continuing review. |
The channel manager is now responsible for tracking in flight frames and properly handling different error conditions.
trianglesphere
force-pushed
the
jg/full_channel_manger
branch
from
2545bdf to
29273c6
Compare
|
Looks like not all comments have been addressed yet |
Nope. I didn't re-request a review. The force push was a rebase. |
protolambda
left a comment
protolambda
left a comment
There was a problem hiding this comment.
Changes look good, but I would prefer to defer the creation of frames until we include them into transactions, so we can better utilize the packing, instead of committing to a specific size of frame before we know the tx we can put them in.
And long-term I would like if we always pack frames from one or two channels per transaction: if a channel leaves data-tx space underutilized, we can start encoding the next channel in the same tx. Maybe it's too deep of a change for this PR, but we can keep the option more open to do so later? Outputting the frames early and using single channel txIDs makes that quite hard.
It is too large a change for this PR. It's tricky enough to handle multiple frames per channel with a single channel (you have to keep track of which block were submitted where). In terms of the IDs, they're opaque IDs on purpose & when it becomes worth it pack multiple frames into a single L1 transaction we will be able to do that by only updating the channel manager. |
|
@trianglesphere LGTM - you'll need to fix a misspelling in order to get the go tests to pass. Otherwise once @protolambda has a look I'll merge this while we update + fix hive. |
trianglesphere
force-pushed
the
jg/full_channel_manger
branch
from
68f3eb2 to
8d6e1a6
Compare
|
I fixed the lint |
|
trianglesphere
force-pushed
the
jg/full_channel_manger
branch
from
8d6e1a6 to
a3d0466
Compare
gah, I guess the first attempt at fixing this didn't work. I force pushed something that's trying to fix it. |
protolambda
left a comment
protolambda
left a comment
There was a problem hiding this comment.
LGTM. Can merge after CI passes (except hive, which I'll work on fixing as soon as the geth rebase lands on develop)
4 participants
Description
The channel manager is now responsible for tracking in flight frames and properly handling different error conditions.
A channel is a collection of frames. We have a single frame per L1 transaction.
When frames are submitted to L1 even if all frames are successfully included, their
inclusion may not be timely. The tx confirmation method handles resubmitting frames
if the transaction fails or voiding the entire channel if the channel times out.
It has hook points for how large a range of blocks to select, but does not use them
yet. It simply chooses up to 100 blocks (but it will also not submit channels that
are too large).
Tests
No unit tests yet. op-e2e still passing.