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

Support quorum tessera blockchain node #310

Conversation

rodion-lim-partior
Copy link
Contributor

@rodion-lim-partior rodion-lim-partior commented Jun 21, 2024

This PR is raised to add Quorum and Tessera support for firefly-cli. We would like to ease the creation of a development environment for networks using the combination of Blockchain and Private Manager mentioned, and in future, build capabilities in Firefly stack such as extending the blockchain connector to cater for additional functionalities exposed by the combination.

This section describes some of the functionalities implemented from both an external and internal command line perspective:

External ff command line perspective
Unsupported combinations / algorithms will be explicitly disabled on the command line flags.

Supported Consensus Mechanism (only when Quorum is selected)

  • Clique

Unsupported Consensus Mechanism (only when Quorum is selected)

  • RAFT (Intend to support in future, needs command line tweaks, different discovery mechanism)
  • IBFT (Intend to support in future, needs Genesis file tweaks)
  • QBFT (Intend to support in future, needs Genesis file tweaks)

Supported Client Combinations

  • Quorum + EthConnect + Tessera
  • Quorum + EvmConnect
  • Quorum + EthConnect

Unsupported Client Combinations

  • Quorum + EvmConnect + Tessera (Intend to work on this next)
  • Besu + EvmConnect + Tessera
  • Besu + EthConnect + Tessera

Internal ff command line perspective

  1. There are many dependencies to spin up a Quorum/Tessera pair, one example is Quorum having to wait for Tessera to be in a ready state before starting up, we implemented separate docker entrypoint scripts to handle these dependencies for both Quorum and Tessera instead of passing docker args to the binary, so that it is easier to extend more complicated configurations in future. These entry point scripts are copied into the docker volumes during the first time setup
  2. To spin up a Tessera instance, we need to generate a private/public key pair. When sending private transactions, Quorum/Third party client needs to use the Tessera public key as an identifier to determine the party to send the unencrypted payload to. The keypairs are pre-generated and then copied into the docker volumes during the first time setup. For this implementation, Tessera configuration takes in a path where the keys are stored, and the same configuration is passed to the binary
  3. All Tessera keypairs are dynamically generated, so we need to store this in the stack state / persist them down to disk so that applications built on top have a reference point
  4. On top of the Tessera keypairs, private payloads are being stored in the Tessera database which is a configuration property. For the dev implementation, we chose to go with a h2 database that is automatically created within the docker volume if it does not exists. There are other ways of storing this data, such as using a Postgresql database, but for the purpose of a development environment, we feel that h2 is sufficient
  5. Tessera instances need to constantly poll each other on the p2p port to determine the status of the members that are connected to each other. The list of members and their endpoints are passed in as a configuration file when we call the Tessera binary. This configuration file is generated in the shell script of the docker entry-point file
  6. There are three API servers exposed for Tessera, q2t (9101), p2p (9000) and tp (9080). All three servers are on different ports and need to be exposed internally. Third party port is deliberately exposed on the host, since clients outside of the docker network can choose to use an external signer to hit the /storeraw endpoint, or require to perform health checks on the /upcheck endpoint
  7. Based on the number of members passed in to the CLI, that will determine the number of Quorum and Tessera containers that will be added to the docker compose. Each Quorum has it's own docker volume to store it's signer keys. Instead of having all keys for all members in a single Geth containers, each Quorum only has it's own signer keys. The same is being implemented for Tessera. Similar to the default implementation, we unlock the keys within the Quorum node itself
  8. EvmConnect needs to point to the correct Quorum node as per previous bullet point, instead of pointing to a single Geth node in the default implementation
  9. Discovery mechanism needs to be taken care of since there are multiple Blockchain nodes, this can be done either via bootnodes or a static list of quorum connection identifiers. We have implemented all Quorum nodes to use the first member's node as a bootstrap node to discover each other

Resolves #309

@rodion-lim-partior rodion-lim-partior changed the title Feature/set516 support quorum tessera Support quorum tessera blockchain node Jun 21, 2024
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Thank you @rodion-lim-partior for the contribution here.

I have spent some time doing the first round of code level understanding of the proposal, and you will see it lead to a significant number of questions that are about the architecture and purpose of this PR.

So I have paused my review for you to take my high level feedback on board.

I would summarize it as:

  • There are three things in this PR, and they need to be separately choosable and clear on the ff CLI options:
    1. Choosing Quorum (IBFT/QBFT/Clique/RAFT) rather than Besu/Geth as the ethereum blockchain runtime for my dev environment
    2. Choosing to enable the Tessera off-chain private transaction manager on top of my EVM base blockchain
    3. Choosing to pass a set of environment variables to all containers in a stack
  • I believe at the end of this PR being ready to merge, from reading the code the following combinations should be possible:
    - Quorum + EVMConnect
    - Quorum + EthConnect
    - Quorum + Tessera + EthConnect
  • Unless you do significantly more work, I think the following combinations will fail and need to explicitly fail on the ff command line
    - Quorum + Tessera + EVMConnect
    - Besu + Tessera + EthConnect
    - Besu + Tessera + EVMConnect
  • The comments on the PR ahead of the next review, I would request come spelling out what you have done both from:
    1. An external ff command line perspective
    2. An internal CLI perspective on how what you've done fits with what exists (ports, storage, keys etc.)

With the above addressed, it will be much easier for me and other maintainers to understand what is being proposed in a large PR like this. It's very hard for me to review without this guiding outline to review the code against.

I hope that's helpful, and thanks again for what looks like a significant contribution for established networks that have built upon Quorum and/or Tessera and are looking for a development environment setup.

cmd/init.go Show resolved Hide resolved
cmd/init.go Show resolved Hide resolved
internal/blockchain/ethereum/ethereum.go Outdated Show resolved Hide resolved
@rodion-lim-partior rodion-lim-partior force-pushed the feature/set516_support_quorum_tessera branch 3 times, most recently from 6690a16 to f545bf6 Compare June 28, 2024 06:11
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
… pair

Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
@rodion-lim-partior rodion-lim-partior force-pushed the feature/set516_support_quorum_tessera branch from f545bf6 to 55a8385 Compare June 28, 2024 06:58
@rodion-lim-partior
Copy link
Contributor Author

rodion-lim-partior commented Jun 28, 2024

@peterbroadhurst, I addressed your review comments and refactored the code. Also left a high level overview in the description of this PR, hope this helps to give a clearer picture on the changes that are being proposed. Thank you for taking the time to look through the large PR and the feedback provided.

Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Thanks @rodion-lim-partior, great contribution! I spent some time going over the lower level details as well as reviewing Peter's high level architecture decisions.

I realise the test coverage has significantly dropped in this repo in the past and was wondering if you could add test coverage for your bits as much as you can without significant refactoring and then I will raise a follow up issue to clean the debt that was caused my previous contributions!

Finally, I do not want to miss any future work mentioned as part of this PR so could you please raise the changed needed for these as issues to this repo please:

Unsupported Consensus Mechanism (only when Quorum is selected)
RAFT (Intend to support in future, needs command line tweaks, different discovery mechanism)
IBFT (Intend to support in future, needs Genesis file tweaks)
QBFT (Intend to support in future, needs Genesis file tweaks)

and

Unsupported Client Combinations

Besu + EVMConnect + Tessera (Intend to support in future)

cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Show resolved Hide resolved
internal/blockchain/ethereum/ethereum.go Outdated Show resolved Hide resolved
internal/blockchain/ethereum/quorum/genesis.go Outdated Show resolved Hide resolved
internal/blockchain/ethereum/quorum/genesis.go Outdated Show resolved Hide resolved
internal/blockchain/ethereum/quorum/geth_provider.go Outdated Show resolved Hide resolved
internal/blockchain/ethereum/quorum/geth_provider.go Outdated Show resolved Hide resolved
internal/blockchain/ethereum/quorum/quorum.go Outdated Show resolved Hide resolved
internal/blockchain/ethereum/quorum/quorum.go Show resolved Hide resolved
@EnriqueL8
Copy link
Contributor

A few lint errors to fix as well:

Error: internal/blockchain/ethereum/quorum/private_transaction_manager.go:157:31: Error return value is not checked (errcheck)
	CopyTesseraEntrypointToVolume(ctx, outputDirectory, volumeName)
	                             ^
Error: internal/blockchain/ethereum/quorum/quorum.go:103:30: Error return value is not checked (errcheck)
	CopyQuorumEntrypointToVolume(ctx, outputDirectory, volumeName)
	                            ^
Error: internal/blockchain/ethereum/quorum/private_transaction_manager.go:76:43: unslice: could simplify pubKeyBytes[:] to pubKeyBytes (gocritic)
	return privateKeyData.Data.Bytes, string(pubKeyBytes[:]), path, nil
	                                         ^
Error: internal/blockchain/ethereum/quorum/quorum.go:28:121: ST1003: func parameter chainId should be chainID (stylecheck)
func CreateQuorumEntrypoint(ctx context.Context, outputDirectory, volumeName, memberIndex, consensus, stackName string, chainId int, tesseraEnabled bool) error {
                                                                                                                        ^
make: *** [Makefile:32: lint] Error 1

Signed-off-by: rodion <rodion.lim@partior.com>
@rodion-lim-partior
Copy link
Contributor Author

@EnriqueL8, addressed your comments and refactored most of the code accordingly. Will add additional tests for my bits in the next commit. Also raised two more issues on the remaining bits of the PR.

@EnriqueL8
Copy link
Contributor

Thanks, will give it another pass

Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
@rodion-lim-partior rodion-lim-partior force-pushed the feature/set516_support_quorum_tessera branch from 86c076d to c33fd98 Compare July 1, 2024 09:59
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Thanks @rodion-lim-partior this is looking great and for following up with the issues! I would like @peterbroadhurst to give this another pass now

@rodion-lim-partior rodion-lim-partior force-pushed the feature/set516_support_quorum_tessera branch from c33fd98 to aecd489 Compare July 2, 2024 08:51
Signed-off-by: rodion <rodion.lim@partior.com>
@rodion-lim-partior rodion-lim-partior force-pushed the feature/set516_support_quorum_tessera branch from aecd489 to 2a12af0 Compare July 2, 2024 09:33
@rodion-lim-partior
Copy link
Contributor Author

@EnriqueL8, added more tests to increase the coverage. Had to do some refactoring on the non quorum packages to allow for that.

Signed-off-by: rodion <rodion.lim@partior.com>
@peterbroadhurst
Copy link
Contributor

@rodion-lim-partior - would like if it's ok to check these items, as I think this will need to go into the docs and I need a little more help understanding the options you're proposing:

Quorum + EvmConnect + Tessera
Quorum + EthConnect + Tessera (Haven't fully tested this, still undecided if this should be supported since we noticed the redirection in EthConnect repository to go with EvmConnect instead)

I'm not sure I understand how this fits into the first delivery here for a dev environment.
The code that makes Tessera private smart contracts work with FireFly, is only in EthConnect (not in EVMConnect):
https://github.com/hyperledger/firefly-ethconnect/blob/d3d27b26f0344dcb76f59a043523ae17d3f2141b/internal/eth/send.go#L225-L242

Wouldn't a "Quorum + EvmConnect + Tessera" stack be non-functional for users?

No problem for your PR to enable only one combination, but it seems to me like it should be "Quorum + EthConnect + Tessera".

Quorum + EvmConnect (There is no strong use case to use Quorum without Tessera, we recommend going directly with Geth / Besu instead even though this is supported)
Quorum + EthConnect (Same as above)

I understand that the future of the Quorum codebase is in-flux at the moment, but I do know from experience that the adoption of Quorum without Tessera (or Constellation) is very significant for permissioned PoA (IBFT etc.) based networks. The momentum has certainly shifted very significantly to Hyperledger Besu for new networks.

The argument that Geth is in the box is valid so there's go-ethereum based runtime support, although that's also in even more doubt from a community perspective as Clique support is being removed from Geth.

In net - I'm very happy with your contribution not supporting Quorum+EvmConnect if you're not able to take on that work, but if you do so I think I'd likely go ahead and propose a follow-on commit before we include this in the same release to avoid needing to document why it's not supported.

@rodion-lim-partior
Copy link
Contributor Author

rodion-lim-partior commented Jul 9, 2024

@peterbroadhurst, very valid comments, thank you. EVMConnect is something I am currently working on to bring Quorum / Tessera support. I intend to raise the PRs to the other repositories (firefly-signer, firefly-transaction-manager and firefly-evmconnect). As you mentioned, this includes back-porting some of the work that is already in EthConnect.

I'm happy to change the wording to add future support for Quorum + EvmConnect / EthConnect, without a Tessera instance, since the use case you mentioned in your previous comment sounds valid. As of now, the PR allows the spinning up of the two combinations, and the containers have been tested to be working properly with our own set of scripts.

It will be great if this PR can be merged before I attempt on the remaining ones to bring Quorum / Tessera support to the entire Firefly stack. Let me know your proposal on the follow-on commit, I'm happy to work on it.

@peterbroadhurst
Copy link
Contributor

Sounds like @rodion-lim-partior if the change you could make, is to disable the EVMConnect support in this PR (which could then go in quickly), and have a follow-on to turn it on once we've worked through the considerations in EVMConnect.

I'll then work on getting Quorum only support in, as that should be straight forward.

I think there will be some architectural complexity there, so if maybe you could start with an architecture proposal in FFTM for how you are thinking about it, that would be helpful. FFTM is blockchain agnostic, so we need to try and preserve that.

Doing a detail PR review of the other items now.

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Think the only required changes for this to merge are covered in this review:

  • Tweaks to spelling of args
  • Only allowing evmconnect until work is done elsewhere to support it in the framework

With those two small tweaks, very happy to see this go in. I'll look to help with the making of tessera optional.

cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
Signed-off-by: rodion <rodion.lim@partior.com>
Signed-off-by: rodion <rodion.lim@partior.com>
@rodion-lim-partior
Copy link
Contributor Author

@peterbroadhurst, made the init parameters generic. Also disabled evmconnect support temporarily. I'm still looking through the transaction-manager and signer implementation to understand the extent of the changes required to bring Quorum specific support to evmconnect, will get back on the proposal in a separate issue / PR and see if it's possible to make it Blockchain agnostic

@EnriqueL8
Copy link
Contributor

Thanks @rodion-lim-partior

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Thanks @rodion-lim-partior - great to see this go in.

@peterbroadhurst peterbroadhurst merged commit c0fa99a into hyperledger:main Jul 11, 2024
11 checks passed
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.

Add support for Quorum/Tessera
3 participants