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

Add EmergencyMode #2226

Closed
wants to merge 2 commits into from
Closed

Add EmergencyMode #2226

wants to merge 2 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Jan 13, 2021

Alternative to #2205
Close #2203

Idea from: #2205 (comment)

@shargon shargon requested a review from erikzhang January 13, 2021 15:50
{
// Restore original committee because the block it's in emergency mode
var candidates = GetCandidates(snapshot);
return new CachedCommittee(Blockchain.StandbyCommittee.Select(p => (p, candidates.FirstOrDefault(k => k.PublicKey.Equals(p)).Votes)));
Copy link
Member

@vncoelho vncoelho Jan 13, 2021

Choose a reason for hiding this comment

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

This will be a complete Hard Fork.

How will other nodes and the history sync?
We need to mark this on the Ledger somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

A flag inside the block?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a flag will be enough, @shargon.
Let me talk to @igormcoelho about this situation first and discuss with him.

@@ -18,6 +18,7 @@ public class ProtocolSettings
public int MemoryPoolMaxTransactions { get; }
public uint MaxTraceableBlocks { get; }
public IReadOnlyDictionary<string, uint> NativeActivations { get; }
public IReadOnlyList<uint> EmergencyMode { get; }
Copy link
Member

Choose a reason for hiding this comment

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

It also need StartBlock, which is an index of a block that start the emergency mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought to put the block who was under emergency, in order to alter only these blocks.

Copy link
Contributor

@igormcoelho igormcoelho Jan 14, 2021

Choose a reason for hiding this comment

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

I agree with Erik, from StartBlock, and I even dare saying that we also need some EndBlock... because by changing consensus during emergency, we should guarantee a certain time to update this consensus, and come back to "normal" (at EndBlock). And since this emergency may happen many times (see my comment below), we will need a list of PastEmergencyEvents, including which height each emergency started, ended and which forced consensus it lead to. This means a big responsibility for nodes (and node maintainers) willing to perform several emergencies, which definitely shouldn't happen in a stable network.

Copy link
Contributor

@igormcoelho igormcoelho Jan 14, 2021

Choose a reason for hiding this comment

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

I thought to put the block who was under emergency, in order to alter only these blocks.

Now I got your idea @shargon... You plan on putting a list of all blocks in emergency... I think it works, but it's so much better to make a list of pairs <begin,end> instead of a direct list of blocks... because each emergency may last many blocks (due to its uncertain nature), and I guess we need some explicit EmergencyOut, for nodes to know when they should exit emergency and go back to "normal". I think we also need this in/out on blocks (as Vitor mentioned), but maybe it's better to have this on ProtocolSettings as well, to have some very certain and predictable behavior of what happened and when it happened.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand the pairs, but it could be slower to check all the pairs because we will need to iterate all the possible emergency pairs, if we have a single list, it will be faster because it could be a hashSet.

I think that we should be able to solve the emergency in one block, otherwise, we always can add a new entry in this list. What do you think?

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Edited comment, invalid.

@igormcoelho
Copy link
Contributor

igormcoelho commented Jan 14, 2021

I think this is a very fundamental feature, which I congratulate as a Mature Neo 3.0 technology. The ability of Hard Fork on emergency is necessary, because World Consensus should have higher priority than technocratic Blockchain Consensus. But that requires very high level of transparency. For example, one node joins network and realizes current block is in emergency... that should be disabled by default (automatically joining emergencies). But suppose it joins the emergency, when should it leave? So, I think it's necessary to have two markers on every block: lastEmergencyIn : uint and lastEmergencyOut : uint. Then, that's another big problem.. a node joins a network which is NOT on emergency... but it must realize that some 5 min ago, someone put an emergency, passed some blocks, changed the voting for consensus, and now it's out of emergency. Should it join this network, or another "competing" one (at same "height")? I mean, on the same P2P, with same magic, same height, we may have distinct sets of nodes following distinct networks. That's what this feature means to me.
So, I want two clarify these two points:

  • Nodes will need human intervenction to join an emergency mode (at least on "standard" C# implementation)
  • Nodes will know if any emergency happened on the past, so they will require a list of "allowed past emergencies", that include each height (in/out) and "forced consensus" that happened in the past

By having this in mind, we can keep One Block Finality, otherwise we may need to drop it during emergency, which causes many more problems (as nodes "regretting" entering "bad emergencies"... we shouldn't allow that).

The algorithm I have in mind for a node joining the network is:

  • Top block is XYZ. Is it in emergency? If yes, do I allow this emergency, at this height? Then proceed if allowed.
  • If it's not in emergency, node should ask "Did it have an emergency in the past?". Inspect lastEmergencyIn, if it's 0, then it never happened. If it happened, it will need to check that this emergency is allowed in its ProtocolSettings, then inspect block immediately before lastEmergencyIn, and see again and some emergency happened before that, and if it's accepted locally, and so on.

This means that some "frequent hard forking network" would have a large list of forced changes on its protocol, which is obviously some not welcome situation, but at least supported by the protocol.

@erikzhang
Copy link
Member

Maybe we should focus on NEO3 now. These things can be considered later.

@vncoelho
Copy link
Member

vncoelho commented Jan 14, 2021

By reading neo-project/proposals#82, I believe that this PR also accomplishes almost all of its motivations.

If this PR is merged, we need to remember to update specifications accordingly to create awareness among the community.

@steven1227
Copy link
Member

Any updates on this issue?

@erikzhang
Copy link
Member

I don't think we need to worry about it at the moment.

@shargon shargon closed this Feb 8, 2022
@shargon shargon deleted the emergency-mode branch February 8, 2022 10:05
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.

What will happen if 4 of the 7 elected consensus nodes are offline?
5 participants