fix: add temporary flag to disable storing headers #1668
Conversation
njgheorghita
requested review from
KolbyML and
morph-dev
and removed request for
KolbyML
looks good, there was a check missing for HeaderByNumber, I assume the cluster repo will be updated with --disable-storing-header=true| // temporarily disable storing all block headers | ||
| if self.disable_storing_headers { | ||
| if let HistoryContentKey::BlockHeaderByHash(_) = key { | ||
| return Ok(ShouldWeStoreContent::NotWithinRadius); | ||
| } | ||
| } |
There was a problem hiding this comment.
| // temporarily disable storing all block headers | |
| if self.disable_storing_headers { | |
| if let HistoryContentKey::BlockHeaderByHash(_) = key { | |
| return Ok(ShouldWeStoreContent::NotWithinRadius); | |
| } | |
| } | |
| // temporarily disable storing all block headers | |
| if self.disable_storing_headers { | |
| if let HistoryContentKey::BlockHeaderByHash(_) | HistoryContentKey::BlockHeaderByNumber(_) = key { | |
| return Ok(ShouldWeStoreContent::NotWithinRadius); | |
| } | |
| } |
Don't you need to add a check for BlockHeaderByNumber as well
There was a problem hiding this comment.
I think we should disable storing all history content (for the time being).
Reason for not storing new bodies and receipts until migration is completed, is that I'm not sure we can verify them if we can't fetch correct headers from the network. Meaning, we might end up doing a lot of unnecessary work..
Also, I'm not sure this content will be gossiped until migration is complete either.
bin/trin/src/cli.rs
Outdated
|
|
||
| #[arg( | ||
| long = "disable-storing-headers", | ||
| help = "Disable storing headers in the database. This is a temporary flag used for upgrading the network, and should be removed once the upgrade is complete.", |
There was a problem hiding this comment.
nit: maybe add link to tracking issue
There was a problem hiding this comment.
I added this pr to the list of todos in the tracking issue so I'm confident this won't slip through the cracks
crates/storage/src/config.rs
Outdated
| @@ -127,6 +129,7 @@ pub struct PortalStorageConfig { | |||
| pub distance_fn: DistanceFunction, | |||
| pub sql_connection_pool: Pool<SqliteConnectionManager>, | |||
| pub max_radius: Distance, | |||
| pub disable_storing_headers: bool, | |||
There was a problem hiding this comment.
I know that this is only temporarily, but I don't like that this is history network specific thing and it's present in the PortalStorageConfig.
Instead, I believe we can:
- add parameter to
initialize_history_network - pass it to
HistoryNetwork::new - pass it to
HistoryStorage::new
This would make change smaller and also easier to revert.
Again, since this is temporarily, I won't block/insist on this.
njgheorghita
force-pushed
the
disable-storing-headers
branch
from
cdf8d3e to
3949755
Compare
What was wrong?
Part of #1666 ...
How was it fixed?
HeaderWithProoftypesTo-Do