-
Notifications
You must be signed in to change notification settings - Fork 57
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
Support quorum tessera blockchain node #310
Conversation
There was a problem hiding this 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:- Choosing Quorum (IBFT/QBFT/Clique/RAFT) rather than Besu/Geth as the ethereum blockchain runtime for my dev environment
- Choosing to enable the Tessera off-chain private transaction manager on top of my EVM base blockchain
- 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:
- An external
ff
command line perspective - An internal CLI perspective on how what you've done fits with what exists (ports, storage, keys etc.)
- An external
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.
6690a16
to
f545bf6
Compare
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
… pair Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
f545bf6
to
55a8385
Compare
@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. |
There was a problem hiding this 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)
internal/blockchain/ethereum/quorum/private_transaction_manager.go
Outdated
Show resolved
Hide resolved
A few lint errors to fix as well:
|
Signed-off-by: rodion <[email protected]>
@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. |
Thanks, will give it another pass |
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
86c076d
to
c33fd98
Compare
There was a problem hiding this 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
c33fd98
to
aecd489
Compare
Signed-off-by: rodion <[email protected]>
aecd489
to
2a12af0
Compare
@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 <[email protected]>
@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:
I'm not sure I understand how this fits into the first delivery here for a dev environment. 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".
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 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. |
@peterbroadhurst, very valid comments, thank you. 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. |
Sounds like @rodion-lim-partior if the change you could make, is to disable the 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. |
There was a problem hiding this 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.
Signed-off-by: rodion <[email protected]>
Signed-off-by: rodion <[email protected]>
@peterbroadhurst, made the init parameters generic. Also disabled |
Thanks @rodion-lim-partior |
There was a problem hiding this 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.
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)
Unsupported Consensus Mechanism (only when Quorum is selected)
Supported Client Combinations
Unsupported Client Combinations
Internal ff command line perspective
/storeraw
endpoint, or require to perform health checks on the/upcheck
endpointbootnodes
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 otherResolves #309