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

Remove witness shutdown virtual op #2835

Merged
merged 2 commits into from Aug 23, 2018
Merged

Conversation

mvandeberg
Copy link
Contributor

No description provided.

Copy link

@goldibex goldibex left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -3603,14 +3603,17 @@ void database::update_global_dynamic_data( const signed_block& b )
modify( witness_missed, [&]( witness_object& w )
{
w.total_missed++;
if( has_hardfork( STEEM_HARDFORK_0_14__278 ) )
#ifndef IS_TEST_NET
FC_TODO( "HF 20 issue number" );

Choose a reason for hiding this comment

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

um?

@TimCliff
Copy link
Contributor

Does this change mean that after hardfork 20, witnesses will no longer be disabled for continuously missing blocks?

@mvandeberg
Copy link
Contributor Author

Yes, it is the responsibility of stake holders to vote out witnesses that do not do their job.

@VIM-Arcange
Copy link
Contributor

This might have a negative impact on the blockchain performances, because few stake holders regularly monitor witnesses block production.

@TimCliff
Copy link
Contributor

Without a way to "downvote" a witness who other stakeholders are voting on, it will be impossible to completely remove a witness who has votes from inactive stakeholders from the block production scheduling.

@mvandeberg
Copy link
Contributor Author

mvandeberg commented Aug 23, 2018

Witnesses are still able to remove themselves from the schedule by setting their signing key to null.

The Steem consensus algorithm was not designed to require 100% participation. In fact, quite the opposite. It was designed to function well when participation is <100%. This change could add missed blocks, but a very small number per day.

@mvandeberg mvandeberg merged commit fd8a9fd into develop Aug 23, 2018
@mvandeberg mvandeberg deleted the sp190-remove-witness-shutdown branch August 23, 2018 19:33
@Jolly-Pirate
Copy link
Contributor

Jolly-Pirate commented Oct 18, 2018

Please re-enable that feature. There are a few (unmaintained?) witnesses running the previous fork, getting scheduled and missing blocks since HF20 passed.

@KINGdotNET
Copy link

I second this request.

@lukestokes
Copy link

I have mixed feelings about this. At first I was thinking, "Of course we need to add this back in" but the more I think about it, the more I think the risk potential could create more incentive to support good witnesses. If a witness is missing blocks for an extended period of time and doesn't take care of it, that's a pretty strong signal for voters who need valid signals. Yes, it could happen to any witness if they are on a flight, etc, but ultimately the goal of improving our witnesses with more active voting is the right direction. Instead of hand-holding ineffective witnesses, hopefully we can educate token holders to become voters and take that responsibility seriously.

@Jolly-Pirate
Copy link
Contributor

Rarely do the voters revoke their votes because of missed blocks. They probably don't know where to look for them, and I doubt they have the dedication to monitor if their witnesses are properly set up to secure the network. Some users are more attentive to these things (whales in particular), but overall it seems to be a vote-and-forget strategy. Because of that, I think we need that feature back in some form or another.

@scr53005
Copy link

scr53005 commented Oct 18, 2018 via email

@mvandeberg
Copy link
Contributor Author

The previous shutdown logic created a vulnerability in Steem's consensus algorithm. Parts of the Steem consensus algorithm rely on a super majority. Shutting off witnesses when they miss blocks allows for 51% of witnesses to eject top20 witnesses and replace them with new witnesses. If coordinated properly, the replacements can be witnesses that side with the 51% of the top 20 enabling super majority actions to take place with only 51% control.

On the other hand NOT disabling those witnesses running the previous fork
has a serious drawback by lengthening the total duration of the schedule.

This is a compelling reason for the behavior. If this was the primary reason for re-enabling this behavior, I would propose an alternative change.

  1. Do not schedule witnesses that are not on the current hardfork version.
  2. In witness_set_properties add an extension field blockchain_version that allows a witness to announce their hardfork version without producing a block.

Another alternative proposal:

  1. At the time of the hardfork, nullify keys for witnesses not on the hardfork version.

I recommend this last as there are some performance concerns for such an action, but could potentially be optimized via clever indexing of witnesses.

@Jolly-Pirate
Copy link
Contributor

Jolly-Pirate commented Oct 18, 2018

At the time of the hardfork, nullify keys for witnesses not on the hardfork version.

That solves one aspect of the problem, I'm for it. Which leaves us with the other case where witnesses on the same HF are missing blocks for more than 24h.

@lukestokes
Copy link

Rarely do the voters revoke their votes because of missed blocks.

And that is a problem. If a witness is continually missing blocks which negatively impacts the performance of the apps using the chain and their supporters have no mechanism for knowing this, that's the real problem. Disabled witnesses don't have the same negative impact on the chain as others are scheduled in their place.

@scr53005 I wasn't thinking exclusively about forks, but I see your point. A disabled witness shown in red is just a UI feature of that particular website. A witness missing blocks for an extended period of time should (IMO) be prioritized in those displays as something voters need to take seriously as it has more negative impact on the performance of the apps using the chain. Maybe we can bring back the blink tag for them. :)

I hope the long-term impact of showing witnesses not doing their jobs (instead of just automatically disabling them) will mean backup witnesses would get full-time positions instead of a temporary increase in their scheduling due to the short period of time a witness is disabled. If a witness doesn't care about missing blocks for 24 hours straight, then that's something that should stick in voter's memory, IMO. That's something we as a community can educate people about.

I think both proposals by mvanderberg make sense to me and still leave it up to the voters to remove under-performing witnesses. The 51% attack approach seems like reason enough to be very cautious about automatically disabling witnesses.

@scr53005
Copy link

scr53005 commented Oct 22, 2018 via email

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.

None yet

8 participants