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

Minimal viable ledger configuration strategy #1844

Closed
bonomat opened this issue Jan 9, 2020 · 11 comments
Closed

Minimal viable ledger configuration strategy #1844

bonomat opened this issue Jan 9, 2020 · 11 comments

Comments

@bonomat
Copy link
Member

@bonomat bonomat commented Jan 9, 2020

Problem

At the moment, users of cnd can configure the connection to a bitcoind node and an ethereum node implementing the web3 endpoints.
In addition, the user can specify a network but it is not used for anything.
This is confusing and should be resolved.

Goal

Define a minimally viable ledger configuration strategy for cnd.
The inner workings of cnd are designed to work against anything that implements BlockByHash and LatestBlock.
Choosing a connector is non-trivial though and depends a lot on the usecase of the user.
To not make cnd overly complex, we should reduce the configuration surface to a bare minimum at only keep the flexibility on the inside (and for a future library).
The goal of this spike is to figure out, what the best way of configuring cnd with two fullnodes is. Take into account:

  • how/whether to specify, which blockchain network we are connected to
  • the configuration surface should be minimal

We can assume that cnd will always run against full-nodes for now.

Note: Think about how to derive blockchain network from connection details (or the opposite), what should be the mandatory information (socket or blockchain network), what should be the default if something/anything is not specified.

@D4nte

This comment has been minimized.

Copy link
Member

@D4nte D4nte commented Jan 10, 2020

What is the difference with #1503 ?

@thomaseizinger thomaseizinger changed the title Minimal viable configuration strategy Minimal viable ledger configuration strategy Jan 10, 2020
@thomaseizinger

This comment has been minimized.

Copy link
Member

@thomaseizinger thomaseizinger commented Jan 10, 2020

What is the difference with #1503 ?

I've updated the issue description.

@thomaseizinger

This comment has been minimized.

Copy link
Member

@thomaseizinger thomaseizinger commented Jan 23, 2020

@D4nte Seems like you had a typo in the issue number :)

@D4nte

This comment has been minimized.

Copy link
Member

@D4nte D4nte commented Jan 27, 2020

Yep, sorry

@bonomat

This comment has been minimized.

Copy link
Member Author

@bonomat bonomat commented Feb 17, 2020

@thomaseizinger : Can we remove the blocker (or turn it around) of #1952?
I think that we can do either independently or use the learnings from #1952 in this spike.

@thomaseizinger

This comment has been minimized.

Copy link
Member

@thomaseizinger thomaseizinger commented Feb 17, 2020

@thomaseizinger : Can we remove the blocker (or turn it around) of #1952?
I think that we can do either independently or use the learnings from #1952 in this spike.

@bonomat see #1952 (comment).

How do you see technical-debt being cleaned up if not at the time when we need to touch stuff that needs changing?

@thomaseizinger

This comment has been minimized.

Copy link
Member

@thomaseizinger thomaseizinger commented Feb 17, 2020

I guess this could be downgraded from a spike to an actual ticket. I know it is not estimated now but it was created before we had the talk about "what should spikes be".

Thoughts?

@bonomat

This comment has been minimized.

Copy link
Member Author

@bonomat bonomat commented Feb 18, 2020

Assumptions

  • It should be easy for a user to define what networks (blockchain networks) he wants to connect to
    • In best case there is no need for a configuration at all, i.e. cnd is able to derive smart defaults
    • The user should have the freedom configure everything he wants
  • If a connection url and network string/chain_id is provided, cnd should verify those two fit together (sanity test)
  • The config should be extensible for non-blockchain node blocksources (e.g. blockchain explorers)

Proposal (10% feedback)

Simple

I propose an approach inspired by starting blockchain nodes:

A user can provide the network for cnd in a whole, i.e.

  • cnd or cnd --mainnet:
    cnd derives default settings to connect to local blockchain nodes in mainnet mode.

  • cnd --testnet:
    cnd derives default settings to connect to local blockchain nodes in testnet mode.

  • cnd --devnet:
    cnd derives default settings to connect to local blockchain nodes in devchain/regtest mode.

Advanced

Additionally, for more fine grained configs, the user can change the config file:

For that I propose to allow a combination of settings. This complicates the code a bit but allows for maximum configuration freedom:

I also propose to not generalize bitcoin and ethereum config but explcitily state the node, e.g. bitcoind and parity. This allows us to use features provided by that particular implementation, e.g. derive network

e.g.

[bitcoind]
network = "regtest"

cnd derives node_url = "http://localhost:18443/" and sets username/password to empty.

[bitcoind]
node_url = "http://localhost:18443/"

cnd connects to blockchain under given url and needs to learn the network string from it. Doable with:

curl --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getblockchaininfo", "params": [] }' -H 'content-type: text/plain;' http://localhost:18443 --user __cookie__:080f7f07a463f5e41c29643b6285b9eb7f2a366e0feb5e60413080318140178e  | jq .result.chain

Full config:

[bitcoind]
node_url = "http://localhost:18443/"
network = "mainnet"

--> cnd should do a sanity test to verify the network from the provided url. In this case, the sanity test should fail as we don't know what the user really wanted.

for parity the chain_id can be derived with this allowing for the same config strategy as for bitcoind.

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_chainId","params":[],"id":1}' -H "Content-type:application/json" localhost:18454 | jq .

additionally, I propose new optional fields to configure online resources e.g.

[bitcoin]
[blockchain.info]
api_key=TopSecret

[ethereum]
[infura]
chain_id=0x07
api_key=0x1337

Notes:

  • I propose a sanity test if cnd is connected to the configured blockchains during startup (this can be done in a separate ticket)
  • parity and geth are similar, and get_chainId work probably for both. But given we are testing with parity I would stick to parity for now and state that this setting might work with geth as well
  • I did not consider what to do if multiple blockchain sources are defined, e.g. [bitcoind] and [bitcoin]

@thomaseizinger & @tcharding : please provide 10% feedback (probably already 50% as I provided some code snippets)

@thomaseizinger

This comment has been minimized.

Copy link
Member

@thomaseizinger thomaseizinger commented Feb 18, 2020

Nice, I like the overall approach.

cnd connects to blockchain under given url and needs to learn the network string from it. Doable with:

Not sure if that is a good idea. I think we should force the user to always say, which network they want.
If this happens:

[bitcoind]
node_url = "http://localhost:18443/"
network = "mainnet"

then I think we should assume mainnet is the intended network and fail because the given node doesn't provide access to mainnet.

cnd or cnd --mainnet:

Which ledgers does this assume? All of them? Should the user be able to opt in/out of certain ledgers?

cnd --devnet:

If the emphasis here is on local, we could include that in the name to make it crystal clear.

Note:

[bitcoin]
[blockchain.info]
api_key=TopSecret

[ethereum]
[infura]
chain_id=0x07
api_key=0x1337

This is not valid toml but the general idea is good :)


EDIT:

Now that I am thinking about the last toml snippet, what about something like this:

[bitcoin]
network = "mainnet"

[bitcoin.connector.bitcoind]
node_url = "..."

[bitcoin.connector.blockchain_info]
api_key="TopSecret"

[ethereum]
chain_id = "0x07"

[ethereum.connector.infura]
api_key="0x1337"

Basically, the idea is to have a general bitcoin and ethereum section to configure the network.

The connector sub-section would then have different possibilities for the various connectors.

@bonomat

This comment has been minimized.

Copy link
Member Author

@bonomat bonomat commented Feb 19, 2020

cnd or cnd --mainnet:

Which ledgers does this assume? All of them? Should the user be able to opt in/out of certain ledgers?

Good point. I would say all of them and print an error message for those which are not available?
This would allow us later on to just decline swaps with a ledger which is not connected. Obviously, this does not give us any lifetime guarantees as a connection could go down over time.
However, the oposite holds: cnd won't randomly connect to a chain which was not configured.

```toml
[bitcoind]
node_url = "http://localhost:18443/"
network = "mainnet"

then I think we should assume mainnet is the intended network and fail because the given node doesn't provide access to mainnet.

The result will be the same right? Cnd will fail as the two don't fit together.

cnd --devnet:

If the emphasis here is on local, we could include that in the name to make it crystal clear.

fair, can put an emphasis on local here.

This is not valid toml but the general idea is good :)

10% review ;)


// EDIT //
I like your proposal for in the edit section.

@thomaseizinger

This comment has been minimized.

Copy link
Member

@thomaseizinger thomaseizinger commented Feb 19, 2020

10% review ;)

But you said "probably 50" :D

However, the oposite holds: cnd won't randomly connect to a chain which was not configured.

What if I don't have a configuration file and start cnd with cnd --mainnet. I think it is very likely for users to start start something and not check the configuration first.

Does that mean, without a config file, cnd --mainnet doesn't do anything? Good be a fair default.

The result will be the same right? Cnd will fail as the two don't fit together.

Yes the result is the same but the error message is different:

  1. The node is wrong
  2. The network is wrong

One of the two has to be the "primary" configuration. I think, it is unlikely that the user configures the wrong network, hence we can assume that the mistakes is in the node they provided. For that reason, I would make the network the primary config and derive other things from it. If the user is explicit about those and they don't match, we treat those config values as wrong (and not the network).

@D4nte D4nte added this to the Sprint 28 ❤💸 milestone Feb 21, 2020
@bors bors bot closed this in 3589d43 Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.