feat(quorum): private transaction support #1337

travis-payne · 2021-09-15T14:05:49Z

Fixes #951

petermetz · 2021-09-21T15:09:05Z

packages/cactus-plugin-ledger-connector-quorum/src/test/resources/network-files/member1/nodekey

@@ -0,0 +1 @@
+b9a4bd1539c15bcc83fa9078fe89200b6e9e802ae992f13cd83c853f16e8bed4


@travis-payne Where are these being used? (meaning the whole network-files directory)

As below, not all the files are being used, but figured I'd leave them all there in case someone wants to expand on functionality that I might not foresee right now. Can remove if it useless though.

https://github.com/hyperledger/cactus/blob/24e3b0dd08fff550cbba7785566eea19daee1edd/packages/cactus-plugin-ledger-connector-quorum/src/test/typescript/integration/plugin-ledger-connector-quorum/deploy-contract/v2.3.0-deploy-contract-from-json-private.test.ts#L201

@travis-payne Only saw this by accident just now, please tag me otherwise it doesn't show up on my mentions feed.
I'd throw everything out that's not being used because we don't know when that future date will come and if these files will be outdated by then which would also lead to some additional confusion.

codecov-commenter · 2021-09-28T19:16:17Z

Codecov Report

Merging #1337 (dc890f8) into main (5140fe8) will increase coverage by 0.02%.
The diff coverage is 71.17%.

@@            Coverage Diff             @@
##             main    #1337      +/-   ##
==========================================
+ Coverage   71.33%   71.35%   +0.02%     
==========================================
  Files         308      309       +1     
  Lines       11264    11371     +107     
  Branches     1383     1400      +17     
==========================================
+ Hits         8035     8114      +79     
- Misses       2475     2490      +15     
- Partials      754      767      +13

Impacted Files	Coverage Δ
...pescript/generated/openapi/typescript-axios/api.ts	`60.31% <ø> (ø)`
.../main/typescript/plugin-ledger-connector-quorum.ts	`75.18% <54.54%> (-1.62%)`	⬇️
...rc/main/typescript/quorum/quorum-mp-test-ledger.ts	`74.11% <74.11%> (ø)`
...ypescript/web-services/invoke-contract-endpoint.ts	`87.09% <100.00%> (ø)`
...ypescript/web-services/run-transaction-endpoint.ts	`87.09% <100.00%> (ø)`
...tus-test-tooling/src/main/typescript/public-api.ts	`100.00% <100.00%> (ø)`
...main/typescript/rustc-container/rustc-container.ts	`68.53% <0.00%> (+2.24%)`	⬆️
...main/typescript/plugin-factory-ledger-connector.ts	`100.00% <0.00%> (+25.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5140fe8...dc890f8. Read the comment docs.

.../cactus-plugin-ledger-connector-quorum/src/main/typescript/plugin-ledger-connector-quorum.ts

...es/cactus-plugin-ledger-connector-quorum/src/test/resources/network-files/member1/accountkey

...gin-ledger-connector-quorum/deploy-contract/v2.3.0-deploy-contract-from-json-private.test.ts

petermetz · 2021-09-30T21:32:10Z

...gin-ledger-connector-quorum/deploy-contract/v2.3.0-deploy-contract-from-json-private.test.ts

+    );
+
+    t.comment("member3 should not be able to call contract");
+    t.throws(await member3Contract.methods.getName().call());


@travis-payne Does this still works if you switch from throws to rejects ?

throws is for sync functions and I suspect the only thing it does here is asserting that a promise was returned without throwing, but it does not check if said promise would reject or resolve (even with the await keyword baked in there - could be wrong though so I'm just asking to double check)

throws works here but running rejects seems to crash out.

throws works here but running rejects seems to crash out.

@travis-payne Please attach the logs that it crashes with.

# member3 should not be able to call contract not ok 14 member3Contract.methods.getName().call() throws exception --- operator: throws expected: undefined actual: undefined at: <anonymous> (/home/travispayne/cactus-fork/node_modules/tape-promise/index.compiled.js:43:12) stack: |- Error: member3Contract.methods.getName().call() throws exception at Test.assert [as _assert] (/home/travispayne/cactus-fork/node_modules/tape/lib/test.js:311:54) at Test.bound [as _assert] (/home/travispayne/cactus-fork/node_modules/tape/lib/test.js:96:32) at Test.throws (/home/travispayne/cactus-fork/node_modules/tape/lib/test.js:676:10) at Test.bound [as throws] (/home/travispayne/cactus-fork/node_modules/tape/lib/test.js:96:32) at /home/travispayne/cactus-fork/node_modules/tape-promise/index.compiled.js:43:12 at processTicksAndRejections (internal/process/task_queues.js:95:5) at async /home/travispayne/cactus-fork/packages/cactus-plugin-ledger-connector-quorum/src/test/typescript/integration/plugin-ledger-connector-quorum/deploy-contract/v2.3.0-deploy-contract-from-json-private.test.ts:292:3

@petermetz sorry its a little delayed, but logs attached

@travis-payne The logs you attached look like they were generated by throws not rejects (based on the call stack) could you please double check if these are the logs for rejects crashing for sure?

@petermetz yup, the following code chucks the same error:

const promise = member3Contract.methods.getName().call(); await t.rejects(promise,"member3 should not be able to call contract");

@travis-payne Alright, I'll debug it on my machine then.
Please push a version of the code where it reproduces after a yarn configure && yarn ts-node packages/cactus-plugin-ledger-connector-quorum/src/test/typescript/integration/plugin-ledger-connector-quorum/deploy-contract/v2.3.0-deploy-contract-from-json-private.test.ts and then ping me here and I'll take a peek.

packages/cactus-test-tooling/src/main/typescript/quorum/quorum-mp-test-ledger.ts

petermetz · 2021-12-02T17:57:00Z

https://chat.hyperledger.org/channel/cactus?msg=H3KZMTkHwG8n2kWS2

fixes hyperledger-cacti#951 Signed-off-by: Travis Payne <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>

Signed-off-by: Peter Somogyvari <[email protected]>

Signed-off-by: johnhomantaring <[email protected]>

johnhomantaring · 2022-04-01T18:27:59Z

@petermetz can you assign me to this

fixes hyperledger-cacti#951 Signed-off-by: johnhomantaring <[email protected]>

johnhomantaring · 2022-04-05T11:28:19Z

@petermetz all changes has been merged and to give context on the changes done.

1.) Issue on multiple nonce error has been resolved.

Based on my observation this is due to getTransactionCount method that uses incorrect signing account address. Before we are using the nonce created from the test script but using a different signing account. Instead of sending it from the test script and modify openAPI I just used privateKeytoAccount() method and created the nonce from the connector.

2.) Typings for web3js-quorum has been changed as well based on the latest update. Although this is not yet available in their latest release but available on their github repo. I modified some parts as well because there are some discrepancies on the examples and documentations.

3.) Modified the last part of the test script because Node3 is getting some error due to contracts deployed only in Node1 and Node2. Instead of truthy modified it to expect error.

fixes hyperledger-cacti#951 Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

gitguardian · 2022-05-30T10:17:11Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	Secret	Commit	Filename
-	Elliptic Curve Private Key	`3f43cef`	contribs/Fujitsu-ConnectionChain/connection-chain/environment/base/cc_env/servers/cooperation/coreSide/ec1_adapter/CA/adapter.priv	View secret
-	Elliptic Curve Private Key	`3f43cef`	contribs/Fujitsu-ConnectionChain/connection-chain/environment/base/cc_env/servers/cooperation/coreSide/ec2_adapter/CA/adapter.priv	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

jagpreetsinghsasan

I see some crypto material like pub/priv keys in the code.
Can we not generate them within the code itself ?
Also, can you squash merge the commits?

johnhomantaring · 2022-06-01T13:04:17Z

I see some crypto material like pub/priv keys in the code. Can we not generate them within the code itself ? Also, can you squash merge the commits?

will do.. I'm not the original owner of the PR so I'm unable to put it in draft.. Currently working on the changes

jagpreetsinghsasan · 2022-06-02T05:07:05Z

@petermetz can we not simply close this PR and let @johnhomantaring create a fresh one?

petermetz · 2022-10-27T00:48:36Z

I see some crypto material like pub/priv keys in the code. Can we not generate them within the code itself ? Also, can you squash merge the commits?

@jagpreetsinghsasan All good ideas!

petermetz · 2022-10-27T00:50:39Z

@petermetz can we not simply close this PR and let @johnhomantaring create a fresh one?

@jagpreetsinghsasan Usually I'd not do that because then the work @travis-payne had put in would appear on the other PR, but what we can do is add him as an author to the commit that will end up being in that new PR. So yeah, @johnhomantaring Please just start a new PR so that permissions are not a problem but do make sure that @travis-payne gets credit in the commit for the contributions as well not just yourself.

Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

johnhomantaring · 2022-11-03T10:44:37Z

@petermetz can we not simply close this PR and let @johnhomantaring create a fresh one?

@jagpreetsinghsasan Usually I'd not do that because then the work @travis-payne had put in would appear on the other PR, but what we can do is add him as an author to the commit that will end up being in that new PR. So yeah, @johnhomantaring Please just start a new PR so that permissions are not a problem but do make sure that @travis-payne gets credit in the commit for the contributions as well not just yourself.

created a new branch related to this PR based from the latest code base and added the fixes done.. Will put credit to @travis-payne on the new PR once done with testing

Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

jagpreetsinghsasan · 2022-12-06T09:55:03Z

Hi @petermetz , @johnhomantaring
I was investigating this PR, and I found out that there needs to have a major, quite significant code refactoring as the implementation is wrong.

These are few major issues with the code

At all places, its assumed that the transactions are private transactions (private transactions should be supported as a feature, with something like an entirely different endpoint). In this process, lots of code generation of openapi is messed up, along with irrelevant objects getting passed through the api endpoints.
For instance

The use of privateUrl keyword in the PR code is redundant as there is no code in place supporting its use.
The RunTransactionRequest and Receipt are modified. Instead we should rather have something like RunPrivateTransactionRequest and corresponding Receipt with a seperate endpoint (like run-private-transaction)
Passing of excessive, unnecessary data to the endpoints (line 496, 507 of the screenshot has 2 huge objects)

At places privateTransactionConfig field is used as if the field is optional (first screenshot) whereas at some places its used as a mandatory field (second screenshot)

We can definitely re-use many of the code pieces from the PR, but as the refactoring needs to happen from the openapi.json level, almost all the newly generated interfaces, classes etc etc will be very different and thus a re-do is required.

petermetz · 2022-12-06T17:13:45Z

Hi @petermetz , @johnhomantaring I was investigating this PR, and I found out that there needs to have a major, quite significant code refactoring as the implementation is wrong.

These are few major issues with the code

At all places, its assumed that the transactions are private transactions (private transactions should be supported as a feature, with something like an entirely different endpoint). In this process, lots of code generation of openapi is messed up, along with irrelevant objects getting passed through the api endpoints.
For instance

The use of privateUrl keyword in the PR code is redundant as there is no code in place supporting its use.

The RunTransactionRequest and Receipt are modified. Instead we should rather have something like RunPrivateTransactionRequest and corresponding Receipt with a seperate endpoint (like run-private-transaction)

Passing of excessive, unnecessary data to the endpoints (line 496, 507 of the screenshot has 2 huge objects)

At places privateTransactionConfig field is used as if the field is optional (first screenshot) whereas at some places its used as a mandatory field (second screenshot)

We can definitely re-use many of the code pieces from the PR, but as the refactoring needs to happen from the openapi.json level, almost all the newly generated interfaces, classes etc etc will be very different and thus a re-do is required.

@jagpreetsinghsasan Fair enough. If that's the case then a re-work can be done on it eventually but first I really want to see the tests passing with a minimal set of fixes to the original work and then work our way backwards from there. Otherwise we'll fight the war on 2 fronts (one is trying to make the tests work to begin with and the second one is refactoring everything at the same time).
So, first please work with @johnhomantaring and figure out why the tests are not passing and then make not of a specific commit hash on the branch that has the "minimum viable fix" for making private txes work.
Then move on to the larger refactoring before we do a final review.

johnhomantaring · 2022-12-07T14:41:55Z

@petermetz The issue of txCount reoccur again because of the same issue of trying to do the old flow of private txn.. I'm also checking how besu is doing the private txn and their code is more simplier without excessive codes. I think it would be best to do the recode like what @jagpreetsinghsasan mentioned since we could based it more with "web3js-quorum" rather than the old "web3js-eea".

Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

jagpreetsinghsasan · 2022-12-15T12:07:00Z

@johnhomantaring I have found out the issue (removed the previous messages as this is my final finding).
For quorum specifically, the Web3JsQuorum library initialisation is different.
https://consensys.net/docs/goquorum/en/stable/develop/client-libraries/#initialize-the-web3js-quorum-client

This wasnt the case with Besu, as by default isQuorum is set to false.

I tried our code and it works perfectly fine with the above change, the only challenge is with the typescript code of this (The js version works fine for me).
The enclave options are not optional and are not getting passed properly here,

But they are working fine in the corresponding js code (the highlighted code)

jagpreetsinghsasan · 2022-12-20T09:00:18Z

I made it work using the quorum-js library.
Needed the validator privateKey as well. Attaching the complete test case for reference.
https://github.com/jagpreetsinghsasan/cactus/blob/feature-1337/packages/cactus-plugin-ledger-connector-quorum/src/test/typescript/integration/plugin-ledger-connector-quorum/deploy-contract/alpha.ts

(Maybe we need the validator priv key for web3js-quorum as well, but currently facing technical challenges with the initialisation itself, as mentioned above)

aldousalvarez · 2022-12-21T04:00:56Z

Good Day @petermetz and @jagpreetsinghsasan.

I would like to work on this one and @johnhomantaring assigned this to me to continue working on this issue

jagpreetsinghsasan · 2023-01-02T06:43:55Z

Good Day @petermetz and @jagpreetsinghsasan.

I would like to work on this one and @johnhomantaring assigned this to me to continue working on this issue

Hi there!
So for this, are you continuing using the web3jsQuorum lib or quorum-js lib ?
Also, I have made the code work with web3jsQuorum as well.
You can check this commit for reference (Added accountKeyStore of validator1)
jagpreetsinghsasan@7dadf7b

jagpreetsinghsasan · 2023-01-12T07:27:58Z

Hi @aldousalvarez
Any updates on this PR?

aldousalvarez · 2023-01-12T08:10:35Z

@jagpreetsinghsasan After using some of your codes and minor modifications on my end of the private transaction is already passing with web3jsQuorum. Do you have any remaining tests on your end that I should include because I will be cleaning up the codes to start the PR and commit my changes.

jagpreetsinghsasan · 2023-01-12T08:24:47Z

Cool @aldousalvarez . The current tests included in the packages/cactus-plugin-ledger-connector-quorum/src/test/typescript/integration/plugin-ledger-connector-quorum/ path are self-sufficient. Just one point while cleaning up, is to have your changes in a single commit, on the top of the currently existing commits (the latest commit ID being f8bef7a). Once done, we can have all the previous commits squashed (the current 9 commits) into one (will need @petermetz help in this) and add your fix+cleanup (single) commit over it. This way, we will neither loose any previous contributions nor yours. Thankyou for the update!

jagpreetsinghsasan · 2023-01-19T06:59:17Z

Hi @aldousalvarez
Any updates on this PR?

aldousalvarez · 2023-01-20T09:23:16Z

Hello @jagpreetsinghsasan currently working with the merging and clean up. As of now I am encountering issue with some conflicts on merging of previous commits. Targeting to complete the cleanup by Jan 27 or earlier and will commit once done with my testing.

jagpreetsinghsasan · 2023-01-23T07:07:48Z

Hello @jagpreetsinghsasan currently working with the merging and clean up. As of now I am encountering issue with some conflicts on merging of previous commits. Targeting to complete the cleanup by Jan 27 or earlier and will commit once done with my testing.

Thankyou for the update!

jagpreetsinghsasan · 2023-02-10T07:16:16Z

Hi @aldousalvarez
Any further updates on this ?

petermetz · 2023-03-28T17:42:23Z

@johnhomantaring @jagpreetsinghsasan @aldousalvarez Closing this so that the discussion is moved over to the new PR at #2293

@travis-payne We'll credit you for that one as well, thanks for putting in the initial work on this one.

travis-payne force-pushed the quorum-private-transaction branch 2 times, most recently from 5e57b11 to fed1aff Compare September 15, 2021 14:12

travis-payne marked this pull request as ready for review September 15, 2021 14:20

travis-payne requested review from jonathan-m-hamilton and petermetz as code owners September 15, 2021 14:20

takeutak approved these changes Sep 15, 2021

View reviewed changes

petermetz reviewed Sep 21, 2021

View reviewed changes

travis-payne force-pushed the quorum-private-transaction branch from 7d61bff to 493e20d Compare September 29, 2021 10:24

petermetz requested changes Sep 30, 2021

View reviewed changes

petermetz changed the title ~~Quorum private transaction~~ feat(quorum): private transaction support Oct 4, 2021

petermetz self-assigned this Dec 3, 2021

petermetz added the Quorum label Dec 3, 2021

petermetz force-pushed the quorum-private-transaction branch 2 times, most recently from 70f6042 to af28daa Compare December 29, 2021 02:10

petermetz removed the request for review from jonathan-m-hamilton December 29, 2021 02:11

petermetz force-pushed the quorum-private-transaction branch from af28daa to 220c2c0 Compare December 29, 2021 04:52

petermetz added 3 commits December 29, 2021 11:26

feat(quorum): private transaction support

d06e978

fixes hyperledger-cacti#951 Signed-off-by: Travis Payne <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>

squash! - work in progress

381d1f5

Signed-off-by: Peter Somogyvari <[email protected]>

squash! - work in progress 2

3a31e5d

Signed-off-by: Peter Somogyvari <[email protected]>

petermetz force-pushed the quorum-private-transaction branch from 220c2c0 to 3a31e5d Compare January 14, 2022 01:14

petermetz and others added 2 commits January 13, 2022 17:26

squash! - work in progress 3

55bc0a6

Signed-off-by: Peter Somogyvari <[email protected]>

feat(quorum): private transaction support

b62c0f7

Signed-off-by: johnhomantaring <[email protected]>

johnhomantaring force-pushed the quorum-private-transaction branch 2 times, most recently from b049089 to 39b0133 Compare April 5, 2022 09:58

feat(quorum): private transaction support

3844682

fixes hyperledger-cacti#951 Signed-off-by: johnhomantaring <[email protected]>

johnhomantaring force-pushed the quorum-private-transaction branch from 39b0133 to 3844682 Compare April 5, 2022 10:34

feat(quorum): private transaction support

f8bef7a

fixes hyperledger-cacti#951 Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

travis-payne requested review from izuru0, jagpreetsinghsasan and sandeepnRES as code owners May 30, 2022 10:16

jagpreetsinghsasan requested changes May 31, 2022

View reviewed changes

johnhomantaring added a commit to johnhomantaring/cactus that referenced this pull request Nov 3, 2022

feat(quorum): private transaction support hyperledger-cacti#1337

2ca2a49

Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this pull request Dec 6, 2022

feat(quorum): private transaction support hyperledger-cacti#1337

147933e

Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

jagpreetsinghsasan pushed a commit to jagpreetsinghsasan/cactus that referenced this pull request Dec 13, 2022

feat(quorum): private transaction support hyperledger-cacti#1337

2efbac7

Signed-off-by: john.h.o.mantaring-at-475704139995 <[email protected]>

petermetz closed this Mar 28, 2023

		@@ -0,0 +1 @@
		b9a4bd1539c15bcc83fa9078fe89200b6e9e802ae992f13cd83c853f16e8bed4

feat(quorum): private transaction support #1337

feat(quorum): private transaction support #1337

Conversation

travis-payne commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

travis-payne Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travis-payne Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petermetz commented Dec 2, 2021

johnhomantaring commented Apr 1, 2022

johnhomantaring commented Apr 5, 2022

gitguardian bot commented May 30, 2022

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

johnhomantaring commented Jun 1, 2022

jagpreetsinghsasan commented Jun 2, 2022

petermetz commented Oct 27, 2022

petermetz commented Oct 27, 2022

johnhomantaring commented Nov 3, 2022

jagpreetsinghsasan commented Dec 6, 2022 • edited Loading

petermetz commented Dec 6, 2022

johnhomantaring commented Dec 7, 2022 • edited Loading

jagpreetsinghsasan commented Dec 15, 2022

jagpreetsinghsasan commented Dec 20, 2022

aldousalvarez commented Dec 21, 2022

jagpreetsinghsasan commented Jan 2, 2023 • edited Loading

jagpreetsinghsasan commented Jan 12, 2023

aldousalvarez commented Jan 12, 2023

jagpreetsinghsasan commented Jan 12, 2023

jagpreetsinghsasan commented Jan 19, 2023

aldousalvarez commented Jan 20, 2023 • edited Loading

jagpreetsinghsasan commented Jan 23, 2023

jagpreetsinghsasan commented Feb 10, 2023

petermetz commented Mar 28, 2023

codecov-commenter commented Sep 28, 2021 •

edited

Loading

travis-payne Oct 4, 2021 •

edited

Loading

travis-payne Nov 2, 2021 •

edited

Loading

jagpreetsinghsasan commented Dec 6, 2022 •

edited

Loading

johnhomantaring commented Dec 7, 2022 •

edited

Loading

jagpreetsinghsasan commented Jan 2, 2023 •

edited

Loading

aldousalvarez commented Jan 20, 2023 •

edited

Loading