Skip to content

fix(node): fix updating node stats during synchronization#1771

Merged
lrubiorod merged 1 commit intowitnet:masterfrom
lrubiorod:fix_node_stats
Dec 10, 2020
Merged

fix(node): fix updating node stats during synchronization#1771
lrubiorod merged 1 commit intowitnet:masterfrom
lrubiorod:fix_node_stats

Conversation

@lrubiorod
Copy link
Copy Markdown
Contributor

@lrubiorod lrubiorod commented Dec 9, 2020

Before to stop it:
image

After clean storage and restarting again:
image

Comment thread node/src/actors/chain_manager/mining.rs Outdated
// This will run all the validations again

let block_hash = block.hash();
// TODO. Currently last_block_proposed is not used, but removing it is a breaking change
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change the TODO as FIXME to track it and change it when there is a new release with breaking changes. :)

Comment thread node/src/actors/chain_manager/mod.rs Outdated
Comment on lines +717 to +718
// During synchronization, we assume that every consolidated block has,
// at least, one proposed block.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// During synchronization, we assume that every consolidated block has,
// at least, one proposed block.
// During synchronization, we assume that every consolidated block has, at least, one proposed block.

Comment thread node/src/actors/chain_manager/mod.rs Outdated
@@ -2140,21 +2160,19 @@ fn update_pools(
transactions_pool.clear_reveals();

// Update own_utxos:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Update own_utxos:
// Update own_utxos

let own_pkh = match self.own_pkh {
Some(x) => x,
None => {
log::error!("No OwnPkh loaded in ChainManager");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the kind of error that we should never see, isn't it? :P What happens if it is None?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is None, something wrong happened. We have different ways to handle those "non optional" option fields (match, if let, unwrap...) So, we should unify in a refactor PR, but by the moment...

} else {
// During synchronization, we assume that every consolidated block has,
// at least, one proposed block.
self.chain_state.node_stats.block_proposed_count += 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this is the count of blocks proposed by this node? So it should be assumed to be 0 unless the miner pkh is the same as the node pkh?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block_proposed_count is a counter for all the proposed blocks by the node, in order that you can check how many times you create and broadcast a valid block and how many times it was included it in the blockchain, the block_mined_count

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, if you detect a mined block by you during synchronization, you assume that you proposed a candidate in the past

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I would assume that the number of proposed blocks is 0, because the probability of proposing a block is very low. Right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind, I missed the if miner_pkh == own_pkh

Comment thread node/src/actors/chain_manager/mining.rs Outdated
// This will run all the validations again

let block_hash = block.hash();
// FIXME(#1773). Currently last_block_proposed is not used, but removing it is a breaking change
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// FIXME(#1773). Currently last_block_proposed is not used, but removing it is a breaking change
// FIXME(#1773): Currently last_block_proposed is not used, but removing it is a breaking change

@lrubiorod lrubiorod merged commit 08cdd38 into witnet:master Dec 10, 2020
@lrubiorod lrubiorod deleted the fix_node_stats branch February 4, 2021 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants