fix pulsar info command to show correct plot size - #261
Conversation
nazar-pc
left a comment
nazar-pc
left a comment
There was a problem hiding this comment.
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 🤷♂️
| 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.
When would it be None? Or why is Default needed on SummaryUpdateFields?
There was a problem hiding this comment.
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.
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.
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. |
2 participants
Description
Currently, the summary file is not updated when plot size is changed in
settings.toml. This PR remedy this by updating theuser_space_pledgedwhenfarmcommand is invoked.Fixes: https://forum.subspace.network/t/pulsar-info-subcommand-outputs-an-obsolete-plot-size/1692