4x400: Early pass at a relay maintainer initcontainer #575
Conversation
This is incomplete and requires a bit more massaging, both in terms of where and how we get the operator key and in terms of populating the remainder of the environment in the initcontainer vs via kube config.
Currently we set up Ropsten to deploy a Bitcoin testnet relay and mainnet to deploy a Bitcoin mainnet relay. The main distinction are the initialization parameters passed to the relay itself for the maintainer to build on.
There are still 5 missing bits of configuration, but the rest are more or less plugged in. Some of the remaining 5 may need to be dropped, and some filled in via config map or elsewhere.
| envTemplate | ||
| .replace(/^(SUMMA_RELAY_ETHER_HOST=).*$/, `\\1${ethURL.hostname}`) | ||
| .replace(/^(SUMMA_RELAY_ETHER_PORT=).*$/, `\\1${ethURL.port}`) | ||
| .replace(/^(SUMMA_RELAY_OPERATOR_KEY=).*$/, `\\1${operatorKey}`) |
There was a problem hiding this comment.
Am I reading this right in that the only thing the relay maintainer needs is the eth account private key?
There was a problem hiding this comment.
Yeah, but I think this is an unencrypted hex key?
There was a problem hiding this comment.
Yeah - we set these in a Secret, can pull one out from there.
| .replace(/^(SUMMA_RELAY_ETH_CHAIN_ID=).*$/, `\\1${ethNetworkId}`) | ||
| .replace(/^(SUMMA_RELAY_BCOIN_HOST=).*$/, `\\1${bcoinHost}`) | ||
| .replace(/^(SUMMA_RELAY_BCOIN_PORT=).*$/, `\\1${bcoinPort}`) | ||
| .replace(/^(SUMMA_RELAY_BCOIN_API_KEY=).*$/, `\\1${bcoinApiKey}`) |
There was a problem hiding this comment.
Not sure about this one, for our bitcoin node we generate an rpc user and password. I don't think we have a key, will double check.
There was a problem hiding this comment.
https://github.com/bitcoin/bitcoin/blob/master/share/examples/bitcoin.conf#L93-L94 this is what we set in bitcoind conf.
There was a problem hiding this comment.
Yeah this looks geared towards bcoin: https://bcoin.io/api-docs/index.html#authentication
| console.log('########### Provisioning relay maintainer! ###########') | ||
|
|
||
| console.log(`\n<<<<<<<<<<<< Read operator address from key file >>>>>>>>>>>>`) | ||
| const operatorAddress = readAddressFromKeyFile(operatorKeyFile) |
There was a problem hiding this comment.
It doesn't look like we need the keyfile on the relay-maintainer..rather we're just grepping data out of the keyfile.
We can read the account address directly from a ConfigMap., so this can go away in favor of an env var. Will post an example in the Deployment file
| async function createRelayMaintainerConfig() { | ||
| const envTemplate = fs.readFileSync('/tmp/env-template', 'utf8') | ||
|
|
||
| const operatorKey = readKeyFromKeyFile(keyFilePath) |
There was a problem hiding this comment.
It doesn't look like we need the keyfile on the relay-maintainer..rather we're just grepping data out of the keyfile.
We can read the account private key directly from a ConfigMap., so this can go away in favor of an env var. Will post an example in the Deployment file
| .replace(/^(SUMMA_RELAY_BCOIN_HOST=).*$/, `\\1${bcoinHost}`) | ||
| .replace(/^(SUMMA_RELAY_BCOIN_PORT=).*$/, `\\1${bcoinPort}`) | ||
| .replace(/^(SUMMA_RELAY_BCOIN_API_KEY=).*$/, `\\1${bcoinApiKey}`) | ||
| .replace(/^(SUMMA_RELAY_INFURA_KEY=).*$/, `\\1${infuraKey}`) |
There was a problem hiding this comment.
I'm not sure what this is.
There was a problem hiding this comment.
We need to get a real Infura set up heh. Infura lets you require an API key alongside your requests so that others can't use your Infura info, basically.
There was a problem hiding this comment.
Do we need to do this before we try deploying?
This is an early draft of what a relay-maintainer Deployment may look like. There are still a few configuration values that need to be sorted out, along with start commands, etc.
| @@ -0,0 +1,107 @@ | |||
| --- | |||
There was a problem hiding this comment.
I put some space between a few values for readability while we work through this.
|
I recall you working on a Dockerfile somewhere..but i can't seem to find it. Do we have a published image to use? |
The values we were deriving from the keyfile are available directly via ConfigMap and Secret. We read those here via env vars. I didn't see the keyfile used in its own right so I went ahead and removed it.
|
Yah, it's at https://github.com/keep-network/relays/blob/deployment/Dockerfile but haven't published the build anywhere. |
No idea if this is going to work since we run a bitcoind node, but going to give it a shot.
Standard build and publish jobs that we use for app InitContainers.
I build it locally and published to: |
We usually terminate the initcontainer template dir path with a directory for the particular initcontainer, incase we have a day where a particular app has more than one initcontainer. Not going to split hairs now, just going with what exists.
This is the first image we build, so we need to create the image workspace dir.
We need one more conditional here to make sure we deploy the summa relay to keep-dev while testing...not sure if we'll keep this but I need it for now.
We need npm <_<
0.13.0 went out but the initcontainer stayyyed.
Alpine gives a smaller image, though it requires a multistage build since the base Alpine image doens't include git.
Mostly to align with how this is handled elsewhere in the script.
These exist now, and needed to be pulled in.
Had a look at how the app constructs the Bitcoin URL used for connections and we can maybeeee get things working with Bitcoind by setting the RPC user/pass to SUMMA_RELAY_BCOIN_API_KEY.
This reverts commit b4de285.
Sledgehammering different Bitcoin host configs to see what may go. Trying to avoid using something other than Bitcoind, which we're already running.
After some experimenting it looks like we wont be able to get our already running Bitcoind node working with the relay-maintainer. This is due to the maintainer using some RPC method calls that just don't exist on the Bitcoind API. Here we provide a Kube config to run a bcoin node. Bcoin is a Javascript implementation of Bitcoin and has the api methods we need. The Docker image that's published by the maintianers of Bcoin is two years old. The image we're using here was build locally on the latest Bcoin release and uploaded to our container registries. Not a huge fan but it's what we've got.
We've got a bcoin node going now. As part of that we now have an api key for RPC requests. We'll set this value in Kube config.
This app requires a connection to a Bcoin Bitcoin node, other options such as Bitcoind don't work. To reenforce this notion we're updating the env var configs for Bcoin host/port/api to BCOIN from BITCOIN. This naming also matches closely with the related relayer vars.
Include configuration value for BCOIN_API_KEY now that we're running a Bcoin node. Changing BITCOIN to BCOIN to reflect that these are connetions for a Bcoin node.
Here we set a Docker image tag for the relay InitContainer. It uses the Circle Context env var DOCKER_IMAGE_TAG.
FundingScript and RedemptionScript use mload to cast the first bytes of a byte array to bytes4. Because mload deals with 32-byte chunks, the resulting bytes4 value may contain dirty lower-order bits.
This is more consistent with our existing setters.
In BondedECDSAKeep, function _keep.submitSignatureFraud returns true if signature is fraudulent or reverts if the signature is not a fraud.
Here we include configs for deploying bcoin and the relay-maintainer to the keep-test env.
| volumeMounts: | ||
| - name: relay-maintainer-env | ||
| mountPath: /mnt/relay-maintainer | ||
| command: ['node', '/tmp/provision-relay-maintainer.js'] |
There was a problem hiding this comment.
The Dockerfile already sets an appropriate entry point, yah?
There was a problem hiding this comment.
I go back and forth on this, it feels good to be explicit on the app config with what command is being run for a container, even if it's the default entry point.
There was a problem hiding this comment.
I feel like this is probably true for external containers, since they can change out from under you, but for containers we control it feels like we're just doubling our workload whenever we need to change something… Either way, it's fine for this PR, especially if it's what we've been doing elsewhere.
| type: bcoin | ||
| spec: | ||
| replicas: 1 | ||
| serviceName: tbtc-electrumx-server |
| "dependencies": { | ||
| "@keep-network/tbtc": ">0.14.0-pre <0.14.0-rc", | ||
| "@truffle/hdwallet-provider": "^1.0.25", | ||
| "web3": "1.0.0-beta.55" |
There was a problem hiding this comment.
Oof. Probably we can hold here, but we should really bump this to a >1.0 version of web3.
Set to bcoin for both environments.
Touchups Small set of renames, removing superfluous require's.
A Bit Sale - Clear dirty bits FundingScript and RedemptionScript use mload to cast the first bytes of a byte array to bytes4. Because mload deals with 32-byte chunks, the resulting bytes4 value may contain dirty lower-order bits.
Shadowfiend
force-pushed
the
four-by-four-hundred
branch
from
6a61a13
to
d10c78a
Compare
Well, this is why I get for reviewing PR's whilst tired. In #575 we did a fair amount of testing for a new app which required some Circle build filter surgery. Normally we squash all that nonsense before the PR is merged....but it was missed. Here we correct it.
This is incomplete and requires a bit more massaging, both in terms of
where and how we get the operator key and in terms of populating the
remainder of the environment in the initcontainer vs via kube config.
Also to be included here: migration of the relay contracts for keep-test
and quite possibly for keep-dev.