search

chainflip-io / chainflip-backend

The Chainflip backend repo, including the Chainflip Node and CFE.
50 stars 15 forks source link

[CH-1306] Don't assume validatorship on CFE #467

Closed kylezs closed 3 years ago

kylezs commented 3 years ago

CH-1306

Currently, starting a CF node and starting a "validator" are equivalent. There is currently no way of being a node unless you're a validator.

However, there are different states the CFE can be in, dependent on their state as a node: Validator, Backup, Passive, Not Staked

Here are the 4 states:

There are two different times the CFE learns what state it should enter:

What duties run in each state

Offline Passive BackupValidator ActiveValidator
Monitor for Duty Change events :x: :heavy_check_mark: :heavy_check_mark: :heavy_check_mark:
heartbeat :x: :x: :heavy_check_mark: :heavy_check_mark:
p2p::conductor :x: :x: :x: :heavy_check_mark:
sc_observer :x: :x: :x: :heavy_check_mark:
temp_event_mapper :x: :x: :x: :heavy_check_mark:
stake_manager :x: :x: :x: :heavy_check_mark:
key_manager :x: :x: :x: :heavy_check_mark:
signing :x: :x: :x: :heavy_check_mark:

Options

After a short meeting, we came up with a few options for implementing this. Please add to the table if you think of advantages / disadvantages or more options.

Method Description Advantages Disadvantages
1) Shutdown / Startup mods At the top level of the CFE (main.rs), shutdown the mods when they are not on duty and start them up again when they are - Clean implementation at top level - How do we shutdown gracefully?
- Does the order of the modules shutting down matter? eg. channels between modules.
- could we miss messages/events if we were late to realise we are active (eg. No backlog of signing requests)
2) Stop polling mods at top level Leave all the modules alive in memory but stop polling them at the top level (main.rs) when they are not on duty - Very simple/clean to implement - Stale data in system
- No graceful pause/shutdown
3) Conditional check in mod loop Using a global variable or a channel for duty_changed_event. Get each module to check the current state and poll or ignore its functionality in its main loop. - Granular control of each mods functionality in each state. - The implementation is spread out into all the modules
- A bit of a refactor
4) Conditional wrapper for the subxt_client Leave all modules running all the time, but stop the subxt_client from communicating. @msgmaxim, maybe you can describe it better. - clean, leaves most modules unaffected. - ?
morelazers commented 3 years ago

1.5. Wait for the State Chain node to fully sync.

  1. Fetch Active and Backup Validators from the chain
  2. iii. If we are a Backup Validator then we need to be submitting heartbeats.
kylezs commented 3 years ago

Ah yeah, forgot about backup bois.

@andyjsbell @dandanlen - how do we check for node sync? Don't recall ever having to think about this for the state chain node tbh - is part of it "starting" ensuring it's synced?

andyjsbell commented 3 years ago

Not active validator == Backup Validator right?

kylezs commented 3 years ago

Not active validator == Backup Validator right?

In this doc, we're just being explicit about the node states. In the implementation, sure, there might be a single list, with some "active", but this is an implementation detail.

andyjsbell commented 3 years ago

Sync state - https://polkadot.js.org/docs/substrate/rpc/#syncstate maybe

morelazers commented 3 years ago

Not active validator == Backup Validator right?

Not quite, there are only a maximum of activeSetSize * 0.33 Backup Validators

More info: https://www.notion.so/chainflip/DRAFT-Backup-Validators-5424dbc5a6ee40029d6146f1cc26cbc9

andyjsbell commented 3 years ago

Not quite, there are only a maximum of activeSetSize * 0.33 Backup Validators

Right. I was referring to them being of the same "state"

dandanlen commented 3 years ago

I was discussing this with Andy this morning.

We mentioned the possibility of associating some classification with each on-chain account, for example an enum AccountStatus { Validator, Backup, Inactive }. This would be stored alongside the regular account data for each account. The CFE node can also query its own status directly, it doesn't have to retrieve the whole list and then filter it client-side.

I think we'll still want non-backup validators to submit heartbeats - we need to know if they're online before we consider them as validators when we resolve the auction.

kylezs commented 3 years ago

we need to know if they're online before we consider them as validators when we resolve the auction

This introduces a fourth state. What we, or at least me, considered non-backup, is something like a full node. They may not even have a known ValidatorId (and these nodes then couldn't have an associated "on chain state"). They can join just as a peer e.g. a node that will only be used by services like a block explorer, exposing an RPC port.

non-backup validators to submit heartbeats

My understanding outlined above, applied to this statement, makes this statement confusing. There's a case for nodes not ever wanting to participate as a validator. But still want to be able to use their node for other services like block explorers and frontends.

kylezs commented 3 years ago

Sync state - https://polkadot.js.org/docs/substrate/rpc/#syncstate maybe

Returns a response as: 

{
  "startingBlock": 1725,
  "currentBlock": 1747,
  "highestBlock": null
}

Doesn't really seem that useful. Just tells you when the node joined, and what is the current highest (finalised) block.

dandanlen commented 3 years ago

There's a case for nodes not ever wanting to participate as a validator. But still want to be able to use their node for other services like block explorers and frontends.

These nodes have no reason to start up a CFE at all. If the CFE starts up and the node doesn't have an associated account on the state chain, it should probably just shut down again.

kylezs commented 3 years ago

These nodes have no reason to start up a CFE at all. If the CFE starts up and the node doesn't have an associated account on the state chain, it should probably just shut down again.

Yeah, was also thinking this is a possibility. In which case, the decision should be made at startup by the operator, and if they ever did want to be a backup or more, they'd have to restart their node manually (which I think is reasonable). If we wrap the CF node startup in a CLI - which I assume we'll do eventually, then this is not really extra work on the operator.

dandanlen commented 3 years ago

We also need to consider what the overlap/distinction is between a backup validator and a regular "wannabe" validator.

Is a backup validator implicitly also a wannabe? Ie. do they automatically participate in auctions? Is there any reason a node would want to be a wannabe but not a backup validator?

Do we really need to distinguish between the two states? I'm not sure we do.

kylezs commented 3 years ago

Do we really need to distinguish between the two states? I'm not sure we do.

Yes. At least, the CFE needs to. See my comment re: Andy's comment. It probably suffices for the state chain to only store these in one place. But the CFE does need to explicitly distinguish between the two, since it takes different actions depending on which category it falls into. An implementation detail.

In terms of the external to the chain view, I agree, backup validators should be staked, and therefore implicitly be wannabe validators, that missed out on a slot in the top 150.

These nodes have no reason to start up a CFE at all. If the CFE starts up and the node doesn't have an associated account on the state chain, it should probably just shut down again.

Also on this, the only thing that would change in the above steps is step 1.

  1. Node starts up - If the operator chooses to only be a passive node, only the state chain binary is started; else the state chain and CFE are started. (🚀 react if agreed on this and I'll update the issue desc).
dandanlen commented 3 years ago

At least, the CFE needs to. See my comment re: Andy's comment. It probably suffices for the state chain to only store these in one place. But the CFE does need to explicitly distinguish between the two, since it takes different actions depending on which category it falls into. An implementation detail.

I was just questioning the need to take different actions. Should we not merge the concept of backup validator and wannabe validator into one? Ie. any wannabe validator automatically join the set of backups (otherwise they would be excluded from emergency auctions). I don't see how it makes sense to distinguish between these two groups. 

1. If the operator chooses to only be a passive node, only the state chain binary is started; else the state chain _and_ CFE are started. (🚀  react if agreed on this and I'll update the issue desc).

I don't think it makes sense for the operator to specify this. If you don't want to run a validator, you don't need the CFE part of the package, you just run the node - we can't assume anything about how they want to run the node if they aren't running the CFE.

kylezs commented 3 years ago

I was just questioning the need to take different actions. Should we not merge the concept of backup validator and wannabe validator into one? Ie. any wannabe validator automatically join the set of backups (otherwise they would be excluded from emergency auctions). I don't see how it makes sense to distinguish between these two groups.

Yeah, I realised this is what you might be saying after thinking a little more. While I think what you're saying is true, there still has to be a distinction made somewhere - which may be out of scope for this issue (it may be on the emissions pallet / online/reputation pallet)

As per Tom's comment above, we are only to have active * 0.33 backup validators. However, I'm assuming this definition is for rewards only? @morelazers . There's nothing stopping me, if I'm the 201st person in the auction, starting my CFE and submitting a heartbeat - question is what happens if I do and what happens if I don't, since I'm staked? or am I not staked? - what happens on the contract?

It seems like you're suggesting even the 201st node's heartbeat should be registered? I'd say this makes little sense if their funds are not locked in the contract, since they can't be validators, even in an emergency rotation - but my understanding of what happens on the contracts here might be off.

So, my current top to bottom view would be:

I don't think it makes sense for the operator to specify this. If you don't want to run a validator, you don't need the CFE part of the package, you just run the node - we can't assume anything about how they want to run the node if they aren't running the CFE.

I think my above explanation should explain why I think having the operator specify it makes sense (to me).

andyjsbell commented 3 years ago

We can let the CFE keep sending heartbeats needlessly. Unless the node operator goes to their node and shuts down the CFE process manually.

Isn't there a transaction fee for this? Also, this would be one transaction every 15 minutes on the node, not so much spamming. Do we need to program around this problem? The SC would also reject these heartbeats as they aren't in either the validator group or the coming soon backup validators group.

dandanlen commented 3 years ago

It seems like you're suggesting even the 201st node's heartbeat should be registered? I'd say this makes little sense if their funds are not locked in the contract, since they can't be validators, even in an emergency rotation - but my understanding of what happens on the contracts here might be off.

Yeah I guess that's what I'm saying. If a 201st node starts up and their stake puts them in the top 200 (or even the top 150) nodes, then it seems to me they ought to take the place of the lowest-staked backup validator. Because if there is an emergency rotation, we want the highest-bidding validators to win the auction.

If they start up and don't have any stake then they can't submit transactions anyway (not even heartbeats). And if you have no stake, you have no account on the state chain. In this case it makes sense to enter some kind of passive mode whereby the CFE waits for some incoming stake to the node's account. But as soon as they have stake, they are implicitly participating in the auction process and therefore should IMO start submitting hearbeats to signal their readiness.

Next 50 are backup nodes - they have their funds locked, and they submit heartbeats, and receive some emissions for staying online.

I don't think the backup validators' funds are currently intended to be locked.

Maybe I'm wrong, and I think maybe this is one of the sources of confusion - I have been assuming that backup validators have no obligations - they can stick around and earn some fees for submitting heartbeats, or they can leave. If they are locked in, that changes things a little.

msgmaxim commented 3 years ago

My understanding is that backup validators have their funds locked, because we want them to be fully commited as they might be promoted to active at any moment. The difference is they don't perform most active validator functions and receive smaller rewards.

As per Tom's comment above, we are only to have active * 0.33 backup validators. However, I'm assuming this definition is for rewards only?

I think this is correct. I don't think we have to worry about preventing wannabe validators locking their funds. If they are not receiving rewards, they will unlock themselves. They only argument for the CFE to be aware of the limit is if we want to dissallow nodes locking 1 flip and spamming the network with heartbeats, which comes back to Andy's question about fees. Otherwise, it makes sense to remove the notion of "funded but not in the backup set" for nodes on the CFE.

morelazers commented 3 years ago

@j4m1ef0rd if you are happy with Kyle's revised issue description above and think you could implement the required changes to the CFE based on it, please remove the for approval label and add the approved label.

msgmaxim commented 3 years ago

RE the options in the updated description:

I don't consider (3) much of a refactor, it is by far the most straightforward option, with less clarity being the main disadvantage. I think of (4) as an improvement over (3) actually, where we realise that we don't need to disable all functionality and only focus on what we definitely don't want to happen, which is mainly the submitting of extrinsics to SC, I think. Further, we could build an abstraction around submitting an extrinsic that would additionally check our validatorship status, so the logic isn't as spread out as in (3).

morelazers commented 3 years ago

Yes I also prefer option 4, this way we are almost running the CFE in "simulation mode" where the only thing that really changes is that it does not try to participate in the chainflip protocol. I can see this being handy for a number of monitoring / benchmarking reasons (running mainnet nodes of an unreleased version to validate performance enhancements etc.) - @AlastairHolmes @kylezs what do you think?

kylezs commented 3 years ago

I think 4 is the cleanest from the code point of view. However, @AlastairHolmes will be looking into removing subxt. I think we go with option 3 first, because it's easiest, then when everything is completed here (in this PR) we'll be able to see what it looks like. And since we went with option 3, it's easier to go from 3 -> 4 then from 4 -> 3 (simply because 4 takes a bit more thought and work).

msgmaxim commented 3 years ago

(4) is independent of whether we use subxt or not. Either way we will have code that communicates with the SC. In my opinion (4) is just the right way of doing (3) + keeping some of the modules running. In that case it would make sense to start with (4) and then disable more modules if we want to, thus bringing it closer to (3).

dandanlen commented 3 years ago

(3) and (4) sound more or less equivalent to me - each module would listen to changes in the validator status and update their own operating mode based on this. For some processes it might mean continuing to operate without propagating messages downstream (eg. via subxt), for other is might mean ceasing to operate altogether.

Have we considered the fact there is a level of interdependency between modules? For example, the signer depends on messages from the sc_observer (?) - do we need to consider knock-on effects of shutting down a process? Maybe we need to shut things down in a particular order? If so, having central control via option (1) would be a better choice.

kylezs commented 3 years ago

Just had a long chat to @AlastairHolmes regarding this, we've reconsidered a few things.

There's currently a problem during the transition, where we, a validator in Epoch 1, may be requested to witness W, however, we transition to Epoch 2 before seeing a tx to W, and therefore are no longer in the active validator set when we submit our witness. Therefore it won't be accepted as a valid witness.

Our proposed solution to this includes:

dandanlen commented 3 years ago

State chain needs to only accept witnesses from the validators that were active when the witness-able event was emitted. (We may or may not want to allow the validators from the NewEpoch to witness these as well). This can be done in a few ways. The most obvious to be would be to emit which Epoch the event is from, on every emitted event.

Not sure I understand what you mean here by 'witnessable event'. The SC doesn't ask for things to be witnessed, the things that are witnessed are (by definition) outside of the SC's control.

Rather than the solutions provided in the issue description, which involve not submitting transactions, the CFE should instead stop listening to State chain events (that, consequently, may result in submissions), apart from those that may trigger a state transition (e.g. Validator, Backup, Passive). This ensures that the actions currently in progress (e.g. broadcasting an ETH transaction) can still occur, and are not "switched off" by prematurely changing the state. Without this, there exists a race condition where we may switch state before completing our requisite validator tasks

I'm not convinced there is a race condition here.

Remember everything is processed in blocks, all the events are ordered, and the CFE should process all events in order. So it should never happen that we process a validator's state change in block N before a broadcast in block N-1, for example.

msgmaxim commented 3 years ago

Maybe we need to shut things down in a particular order? If so, having central control via option (1) would be a better choice.

The idea was that with solutions (3) and (4) we are not shutting anything down, so there should be fewer (if any) interdepenedencies like that.

the CFE should instead stop listening to State chain events

We still want to disable some of the sending (heartbeats, for example)

State chain needs to only accept witnesses from the validators that were active when the witness-able event was emitted.

This may not be as simple as it sounds as there isn't a way to reliably map blocks from other chains to our own blocks.

kylezs commented 3 years ago

witnessable event

Anything like a deposit etc. Yes, we don't have these now. But this is exactly the kind of race condition that will be harder to a) see b) resolve later. Also, the race condition exists for broadcasting too, but witnessing I think is slightly easier to reason about / see the problem in. So, we do need to address it now anyway.

I'm not convinced there is a race condition here.

Remember everything is processed in blocks, all the events are ordered, and the CFE should process all events in order. > So it should never happen that we process a validator's state change in block N before a broadcast in block N-1, for example.

There's not a race condition if you do it the way outlined in my comment. Exactly for the reasons you outlined. There is a race condition if you have a seperate process that shoots the update state event into the future because we are then dependent on tokio's polling mechanism, and we can't control for that. This is the race condition existent in both 3 and 4.

This may not be as simple as it sounds as there isn't a way to reliably map blocks from other chains to our own blocks.

You can just say that Epoch 1 ended at ETH block 10.

We still want to disable some of the sending (heartbeats, for example)

Not sure where you see that this wouldn't be possible in the approach I've outlined?

dandanlen commented 3 years ago

witnessable event

Anything like a deposit etc. Yes, we don't have these now. But this is exactly the kind of race condition that will be harder to a) see b) resolve later. Also, the race condition exists for broadcasting too, but witnessing I think is slightly easier to reason about / see the problem in. So, we do need to address it now anyway.

Ok, so you mean something like a quote is added at block N, we rotate at block N+1 but the deposit arrives at block N+2? Shouldn't the new validators witness this?

There's not a race condition if you do it the way outlined in my comment. Exactly for the reasons you outlined. There is a race condition if you have a seperate process that shoots the update state event into the future because we are then dependent on tokio's polling mechanism, and we can't control for that. This is the race condition existent in both 3 and 4.

When we get the events we get a Vec<Event> and then iterate through them, right? So we receive them in order, and presumably they get put on whatever channel in the same order? Or is the issue that we're putting them on different channels and can't guarantee that they'll be taken off again in the same order?

Is there no way to ensure we process events in the correct order before we start adding workarounds to the protocol itself to compensate for randomness in tokio's polling mechanism?

Tbh. i think this discussion around race conditions can be addressed independently of this issue.

FWIW it still sounds to me like (3) offers the most flexibility.

AlastairHolmes commented 3 years ago

I think we agree that 3 makes the most sense. But we must be carefully about which validators (old/new) are allowed to witness things.

As if new validators are allowed to witness a previous epoch's quote, some 1/3 bad validators could witness an arbitrary tx during the old epoch. Then unstake. Then in the new epoch a different 1/3 bad validators could witness the arbitrary tx. Thereby meeting the 2/3 requirement. There are several other versions of this flaw.

Our solution is one option to solve this.

AlastairHolmes commented 3 years ago

Our soltuion being: Not allowing new validators to witness quotes from the previous epoch, and having a transition period during which old validators must still be staked and can only witness quotes from the old epoch.

AlastairHolmes commented 3 years ago

Our soltuion being: Not allowing new validators to witness quotes from the previous epoch, and having a transition period during which old validators must still be staked and can only witness quotes from the old epoch.

If we then did this, then the cfe cannot just stop sending extrinsics (when it stops being a validator), and could instead stop listening.

dandanlen commented 3 years ago

I think we agree that 3 makes the most sense. But we must be carefully about which validators (old/new) are allowed to witness things.

As if new validators are allowed to witness a previous epoch's quote, some 1/3 bad validators could witness an arbitrary tx during the old epoch. Then unstake. Then in the new epoch a different 1/3 bad validators could witness the arbitrary tx. Thereby meeting the 2/3 requirement. There are several other versions of this flaw.

Our solution is one option to solve this.

The set of validators used for counting witness votes are already segregated per-epoch so the above can't happen.

But actually this raises another issue which is where validators are honest and their witnessing is split across two epochs in such way that we don't reach 2/3 consensus in either one. Ie. Epoch N gets 1/2 the votes and epoch N+1 get 1/2 the votes, all the nodes think they have witnessed but the SC thinks it's still waiting.

dandanlen commented 3 years ago

Our soltuion being: Not allowing new validators to witness quotes from the previous epoch, and having a transition period during which old validators must still be staked and can only witness quotes from the old epoch.

FWIW I think this was already a requirement for some chains in case we receive transactions to old vaults...

AlastairHolmes commented 3 years ago

That makes sense to me. Should there be five states not four. I.e. A state to describe being in transition phase (I was previously a validator but will not be in the next epoch)?

AlastairHolmes commented 3 years ago

I think we agree that 3 makes the most sense. But we must be carefully about which validators (old/new) are allowed to witness things. As if new validators are allowed to witness a previous epoch's quote, some 1/3 bad validators could witness an arbitrary tx during the old epoch. Then unstake. Then in the new epoch a different 1/3 bad validators could witness the arbitrary tx. Thereby meeting the 2/3 requirement. There are several other versions of this flaw. Our solution is one option to solve this.

The set of validators used for counting witness votes are already segregated per-epoch so the above can't happen.

But actually this raises another issue which is where validators are honest and their witnessing is split across two epochs in such way that we don't reach 2/3 consensus in either one. Ie. Epoch N gets 1/2 the votes and epoch N+1 get 1/2 the votes, all the nodes think they have witnessed but the SC thinks it's still waiting.

Unless you don't split the votes (And do our idea), I guess we could treat it like this: if a validator has witnessed something in the old epoch it is considered to have witnessed in the future epoch. (If it is also a validator in the new epoch)

dandanlen commented 3 years ago

Unless you don't split the votes (And do our idea), I guess we could treat it like this: if a validator has witnessed something in the old epoch it is considered to have witnessed in the future epoch.

Yes, I think this would work - it would require us to copy across the votes from the previous epoch to the current one which is expensive, but I guess it only needs to happen once per epoch. And we might be able to optimise.

That makes sense to me. Should there be five states not four. I.e. A state to describe being in transition phase (I was previously a validator but will not be in the next epoch)?

Yeah maybe, like some kind of cool-down period.

AlastairHolmes commented 3 years ago

Would it be simpler to not split the vote in the first place. Then you don't need the copy. And as you say we need the transition period anyway. And doing our idea, as Kyle stated, to solve the security problems.

morelazers commented 3 years ago

a transition period during which old validators must still be staked

As Dan mentioned, this is a requirement anyway to ensure that we can get hold of transactions sent to bitcoin-like chains. We probably need to make unstaking take a couple of weeks.

Re. Voting, can't we just draw a hard line at a specific Witness Chain's block number and say "Old Set can (and should) govern events (signing/broadcast/etc) that are caused by witnesses in external-chain blocks up to this one" and "New Set can govern events (signing/broadcast/etc) from this block number and beyond"? The Witness Chain block where the VaultRotationWitnessed extrinsic is confirmed seems like a good candidate for the great firewall.

if (
  witnessed_event.block_number < previous_epoch.end_block[witnessed_event.chain]
) {
  require(old_set.contains(me));
}  else {
  require(current_set.contains(me));
}

// do stuff

I can't see how you could be malicious with two sets of 1/3 in the above. Of course unstaking Validators could turn off their machines and shirk their duties but they run the risk of being slashed by the New Set (which will contain mostly still-Active Validators from the Old Set).

dandanlen commented 3 years ago

Would it be simpler to not split the vote in the first place. Then you don't need the copy. And as you say we need the transition period anyway. And doing our idea, as Kyle stated, to solve the security problems.

Sorry, I shouldn't have used the word copy - Whether or not we split the votes between epochs, we would still need to make some sort of transition from the old to the new set, meaning you would need to remove all the old validators (and their votes), and replace them with the new validators (who haven't voted yet). This will involve iterating over the associated storage entries, which will take some careful thought to ensure it's not horribly inefficient. Anyway - this was not an objection, I think it's a problem that can be solved.

However see Tom's comment - if we can come to consensus on which blocks belong to which epoch, then maybe we don't have this witness-overlap issue in the first place.

msgmaxim commented 3 years ago

You can just say that Epoch 1 ended at ETH block 10.

You can probably say this for ETH, but will we be able to do the same for other blockchains?

morelazers commented 3 years ago

Should be able to do it for all other blockchains too. For each chain we witness the transaction which confirms the VaultRotation. The block number of this transaction can be chosen as the final block in the epoch.

morelazers commented 3 years ago

Closing in favour of a cleaner implementation plan issue which will be created by @j4m1ef0rd at some point soon.