-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes Issues w/ Stream Crashing Over Large Replay #996
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update for governed-gas-pool check also.
@@ -17,6 +17,7 @@ message BlobResponse { | |||
Blob passed_through_blob = 1; | |||
Blob sequenced_blob_intent = 2; | |||
Blob sequenced_blob_block = 3; | |||
Blob heartbeat_blob = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a backwards compatible change, but we are now at the stage where the versioning starts to matter. I think you should break this out into a v1beta2.proto
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also not make this a Blob
type.
@@ -130,6 +130,22 @@ pub trait DaOperations: Send + Sync { | |||
|
|||
last_height = height; | |||
} | |||
// Already executed Height are use to send Heartbeat. | |||
Ok(Certificate::Height(height)) => { | |||
//old certificate, use to send Heartbeat block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to stream blobs from the DA here. You can just yield something that will be interpreted as a heartbeat. That is, this should basically just be yield DaBlob::heartbeat()
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I thought you want to yield blob to keep the same type of data in BlobType. I'll change to something more simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed an update with v1beta2.proto file and heartbeat with a bool.
1473fcf
to
9dd4c62
Compare
I tried syncing a follower node against the most recent commit,
This was in an EC2 instance without any special firewall permissions. The
|
Summary
protocol-units
Adds fixes and features for debugging issues with Movement DA Light Node Stream.
*Review the change log to understand useful debugging features!!!
Change Log
movement-full-node da stream-blocks
: this allows for direct monitoring of DA Stream to an arbitrary connection.Tip
As a rule of thumb, in so far as tests against the generalized version of the stream with the mock pass, issues with stream disconnects are either (a) transport issues or (b) implementation issues stemming from a case where an error in the stream should be non-fatal.
Tip
If additional issues are encountered specifically with high-contention Celestia streams, use the
disk-fifo
provider for a temporary patch.Testing
Outstanding issues