-
Notifications
You must be signed in to change notification settings - Fork 19
fix pulsar info command to show correct plot size #261
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.
I'm also not following why would you send "diffs" in SummaryUpdateFields
if you override the whole file anyway. Just send up to data data structure instead 🤷♂️
@@ -38,6 +38,7 @@ pub(crate) struct SummaryUpdateFields { | |||
pub(crate) new_vote_count: u64, | |||
pub(crate) new_reward: Rewards, | |||
pub(crate) new_parsed_blocks: BlockNumber, | |||
pub(crate) maybe_updated_user_space_pledged: Option<ByteSize>, |
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.
When would it be None
? Or why is Default
needed on SummaryUpdateFields
?
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.
So, it would be None
when we are updating other fields like new_vote_count
, new_reward
etc after reading from subscribed data stream.
if it is not Option
type, we would have to query the current space pledged every time we want to update any data inside summary file due to the way the summary file is handled at present.
match file_handle { | ||
Err(e) => { |
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.
Why would you still try to write into the file in case of error during opening? I do not understand.
Old code makes not that much sense either. What needs to happen instead is you need to write to a new file and rename it back into intended name. This way you will never get truncated file in the first place.
.update(SummaryUpdateFields { | ||
is_plotting_finished: summary.initial_plotting_finished, | ||
maybe_updated_user_space_pledged: Some(user_space_pledged), | ||
// No change in other values | ||
new_authored_count: 0, | ||
new_vote_count: 0, | ||
new_reward: Rewards(0), | ||
new_parsed_blocks: 0, | ||
}) |
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.
When instantiating structs please order fields in the same exact order as in struct definition, it is confusing otherwise.
Not sure. This is done in original code. I just kept it as it is. |
I think it is worth spending some time understanding it better, you're essentially maintaining it now. And I think it makes sense to change diff to final value unless there are strong reasons to not do so. |
Description
Currently, the summary file is not updated when plot size is changed in
settings.toml
. This PR remedy this by updating theuser_space_pledged
whenfarm
command is invoked.Fixes: https://forum.subspace.network/t/pulsar-info-subcommand-outputs-an-obsolete-plot-size/1692