Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

fix pulsar info command to show correct plot size #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ParthDesai
Copy link
Contributor

Description

Currently, the summary file is not updated when plot size is changed in settings.toml. This PR remedy this by updating the user_space_pledged when farm command is invoked.

Fixes: https://forum.subspace.network/t/pulsar-info-subcommand-outputs-an-obsolete-plot-size/1692

Copy link
Member

@nazar-pc nazar-pc left a 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>,
Copy link
Member

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?

Copy link
Contributor Author

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.

Comment on lines +79 to +80
match file_handle {
Err(e) => {
Copy link
Member

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.

Comment on lines +123 to +131
.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,
})
Copy link
Member

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.

@ParthDesai
Copy link
Contributor Author

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 🤷‍♂️

Not sure. This is done in original code. I just kept it as it is.

@nazar-pc
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants