RFC 1: tBTC-specific ECDSA client actions

[Shadowfiend]

This was a reasonable first pass, and was used as the basis for the implementation
that has since gone into the client proper. It is not a 1-to-1 description of the current
implementation.

See #574.

[lukasz-zimnoch]

The approach presented here assumes we will add all app-specific behavior directly in the ECDSA client. I understand this is probably the easiest and fastest way for now. But, I have a strong feeling this will lead us towards a hard to maintain code as more applications are added in the future. Also, we will be probably forced to introduce multiple app-specific dependencies in the ECDSA client code (similar to tbtc.Deposit) which will complicate things even more. Maybe I'm not aware of all circumstances and security considerations for now, but maybe we should consider some kind of modular approach? For example using https://github.com/hashicorp/go-plugin (or similar) to implement app-specific actions as separate plugins and use them as subprocesses of the ECDSA client. Thoughts?

[pdyraga]

My opinion is that we need to design the interfaces to make them losely-coupled and later decide on how we eventually want to separate application-specific behavior from the main client. We don't need to have the final solution now and deciding on the final solution is not possible given that I am sure we will find and learn more during the implmenetation.

Having two processes for client and maintainer is a non-go in my opinion, given the additional complexity of running BTC and Electum node by stakers that we will require anyway.

What I would like to achieve is that it's a tbtc-app-specific module that listenes to keep-ecdsa interface and not the other way. I would also propose to start the work with two independent workstreams:

1. keep-ecdsa talks to Electrum and knows how to generated SPV proof.
2. keep-ecdsa imports tbtc contract Go bindings and can call retrievePubKey

Once we have those two we can work on provideRedemptionProof and increaseRedemptionFee.

[tbtc] should be a separate section in config.toml that is optional. It should have their own set of operator addresses to monitor - this could be helpful for staking providers if they want to monitor multiple operators from one client given Electrum and BTC connection is available only on that machine.

For tbtc-app-specific module that attaches to keep-ecdsa interface and not the other way piece, I think we can achieve it by watching keep created events for this node so app-specific interface is listenting to the main client's interface, and reacts by setting up the monitoring on their own. The easiest way to keep them losely-coupled is probably to organize this communication in a form of an event bus.

[lukasz-zimnoch]

Having two processes for client and maintainer is a non-go in my opinion, given the additional complexity of running BTC and Electum node by stakers that we will require anyway.

To clarify, I didn't mean forcing the operator to manage two distinct processes. Giving the example of Hashicorp's go-plugin lib, I rather had in mind running app-specific plugins as a subprocesses fully controlled by the main ECDSA client. From the operator's perspective, nothing changes. They just have new sections to manage in the config file.

Anyway, this was only a proposition for the general approach. I agree with your point to start with small steps but it will be good to know what structure we are trying to achieve eventually.

[pdyraga]

Having two processes for client and maintainer is a non-go in my opinion, given the additional complexity of running BTC and Electum node by stakers that we will require anyway.

To clarify, I didn't mean forcing the operator to manage two distinct processes. Giving the example of Hashicorp's go-plugin lib, I rather had in mind running app-specific plugins as a subprocesses fully controlled by the main ECDSA client. From the operator's perspective, nothing changes. They just have new sections to manage in the config file.

Anyway, this was only a proposition for the general approach. I agree with your point to start with small steps but it will be good to know what structure we are trying to achieve eventually.

Absolutely agree. Eventually, this should be designed in a plugin/module-like fashion. For now, let's just keep the interfaces separate and make sure tbtc module reads from the main keep process and not vice-versa. The code proposed in this RFC is an early WIP but it's not that bad and is aligned with the idea of modularization. I propose to start with point 2 (calling retrievePubKey) as it's where we answer most of those questions. Before we start working on point 1 (SPV proofs), we'll have tBTC contracts imported and we'll know how to read from config and interact with the chain.

[Shadowfiend]

This was what I was getting at here:

This proposal is very narrowly focused on shot-term changes.

My intent was to want to avoid too much refactoring right now, while laying the foundation for the functionality we need.

We're definitely in need of a refactoring in the client. Right now our interfaces in the beacon client make a clear distinction between app-specific and chain/network functionality. In the ECDSA client these are all very tightly coupled---for example, there is no distinct concept of an ECDSA keep. It's just a handful of goroutines managing the lifecycle fired directly from the client package. This is deliberate, in that we chose early on to focus on completion and refactor later. My intent with this RFC is to continue that approach for a little longer and push the broader architectural rework (which we need for upcoming Celo work anyway) a little further. I have some ideas on that refactoring as well, which I'll probably try to write up in a separate RFC for feedback… But probably not this week.

maybe we should consider some kind of modular approach

Fully agree in terms of modularity in functionality. Using subprocesses vs compiling it into the same executable I think will have to depend on how we observe runtime behavior, how likely folks are to use all applications when there are more applications, how many applications there are, and how we want recovery to work (e.g., child processes can sometimes continue operating when a parent process dies---will that be desirable or not). Probably more factors to consider, but I agree it's something we'll want to keep an eye on and think about.

[mhluongo]

Calling out that the two things we see growing on the product aide are

• More host chains
• Other versions of tBTC that will likely have slight different protocol designs

It's unclear to me how the client should map. Do operators want to stake KEEP on Ethereum and Celo?

Often having separate binaries per chain is considered a security improvement lest a less common chain have a vuln that can be chained to a more common (and valuable) chain. We've seen this pattern in exchanges and custody, and I expect it holds here.

[Shadowfiend]

@pdyraga @lukasz-zimnoch it feels like we've gone ahead and done some of this---and maybe in the way described in this RFC. Should we update, undraft, and merge?

[pdyraga]

I am good to merge it as-is on my side, let's just maybe remove the note about links to come at the bottom. The implementation we have in the client is pretty similar to what's been proposed here and we do not need to keep it 1:1 IMO, the client code is always evolving.

[Shadowfiend]

Ok, fixed things up and ready to rock here.