-
Notifications
You must be signed in to change notification settings - Fork 84
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?
Changes from 1 commit
2c68dad
aa6d928
be23e69
5f9d045
4f8f2a3
ff9136a
480bee7
7aa181b
b4dc706
40293f9
3d4ea57
881b7d4
d32fdf7
71c0ecf
a3d808a
b708ae8
2628c73
c008e29
1e288a3
54e27fa
09af380
6b43c13
81b761f
28c91f8
c6a8593
9ff4337
3969f77
00adf06
09f5c5e
0be8313
39b8755
5ea357b
922db07
1913e58
69f4d9a
acfd786
5f6de96
94c8e06
f8594da
9dd4c62
4026816
057e22d
6e00b77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
let blob_stream = self | ||
.stream_da_blobs_between_heights(height, height) | ||
.await?; | ||
tokio::pin!(blob_stream); | ||
|
||
while let Some(blob_res) = blob_stream.next().await { | ||
let (_, blob) = blob_res?; | ||
// Ack use heigth zero to identify heart beat block. | ||
// Should be changed to a type. | ||
let heart_blob = (DaHeight(0u64), blob); | ||
yield heart_blob; | ||
} | ||
} | ||
Ok(Certificate::Nolo) => { | ||
// Ignore Nolo | ||
} | ||
|
@@ -142,9 +158,9 @@ pub trait DaOperations: Send + Sync { | |
yield Err(e)?; | ||
} | ||
// If height is less than last height, ignore | ||
_ => { | ||
warn!("ignoring certificate"); | ||
} | ||
// _ => { | ||
// warn!("ignoring certificate"); | ||
// } | ||
} | ||
} | ||
}; | ||
|
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.