Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for excess propose signals. #4816

Merged
merged 4 commits into from
Dec 23, 2024
Merged

Fix for excess propose signals. #4816

merged 4 commits into from
Dec 23, 2024

Conversation

Frozen
Copy link
Contributor

@Frozen Frozen commented Dec 14, 2024

Added block number for ReadySignal. Additional check for firing block proposing from setupForNewConsensus.

Main idea here is that we check if next key is not my - do not trigger propose.
https://github.com/harmony-one/harmony/pull/4816/files#diff-255a5cdd5516e7293e60a47a92503360676ddffb7e48e505598a589753b568c2R612

@Frozen Frozen self-assigned this Dec 14, 2024
}()

return nil
}

func (consensus *Consensus) isRotation(epoch *big.Int) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

why re-creating a new function and not use directly IsLeaderRotationInternalValidators ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't need to know which kind of rotation, i need the fact.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok and why recreating the function ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because leader rotation does not affect blockchain anyhow. It means, that we can safely remove internal and external epochs, and left only v2. Exactly in this part of code, i don't need to know which kind of rotation we use, i need to know that we have it active.

@Frozen Frozen force-pushed the fix/excess-propose-signals branch from 9b79b9f to 234e9a2 Compare December 17, 2024 01:48
@sophoah
Copy link
Contributor

sophoah commented Dec 18, 2024

@Frozen please fix travis CI:

# github.com/harmony-one/harmony/consensus [github.com/harmony-one/harmony/consensus.test]
consensus/consensus_test.go:100:27: too many arguments in call to consensus.finalCommit
	have (number, number, bool)
	want (bool)

@Frozen
Copy link
Contributor Author

Frozen commented Dec 18, 2024

fixed, lost some code during rebase

@mur-me mur-me self-requested a review December 19, 2024 16:56
Copy link
Collaborator

@mur-me mur-me left a comment

Choose a reason for hiding this comment

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

Checked against localnet via make debug-ext.

Faced both pre and post leaderRotationconsensus epochs in the !consensus.isRotation(blk.Epoch()) condition

@Frozen
Copy link
Contributor Author

Frozen commented Dec 23, 2024

This fixes certain issues, additional fixes will be in the next Prs.

@Frozen Frozen merged commit 2e1bcb6 into dev Dec 23, 2024
4 checks passed
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.

4 participants