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

Microsoft CCF based PDO transaction processor #259

Conversation

prakashngit
Copy link
Contributor

@prakashngit prakashngit commented Apr 7, 2020

This PR introduces two things:

  1. An abstraction for ledger APIs so that we can use more than just Sawtooth as the ledger choice

  2. Support for Microsoft CCF as a supported ledger for PDO. The PDO/CCF combo must be treated as an experimental effort at present. There is no docker support yet for PDO/CCF & we also haven't added any automated tests for PDO/CCF. The integration currently supports PDO in SIMULATE mode & CCF in virtual mode (meaning virtual enclaves for both PDO & CCF). Support for HW mode will be added soon. This PR integrates PDO with CCF tag 0.7.1. There are several enhancements that will be rolled out in subsequent PRs. These will be tracked via git issues.

Signed-off-by: prakashngit prakash.narayana.moorthy@intel.com

@prakashngit prakashngit requested a review from cmickeyb April 7, 2020 04:43
@prakashngit prakashngit requested a review from g2flyer April 7, 2020 04:43
@prakashngit prakashngit changed the title Ledger Submitter Abstractions to Integrate CCF submitter along with S… Microsoft CCF based PDO transaction processor Apr 7, 2020
@prakashngit prakashngit requested review from bvavala and masomel April 7, 2020 04:44
@cmickeyb cmickeyb added the enhancement New feature or request label Apr 7, 2020
@g2flyer
Copy link
Contributor

g2flyer commented Apr 8, 2020

fyi: at least as far as SGX HW-mode in docker is concerned, this PR still does run nicely with the standard tests ...

Copy link

@masomel masomel left a comment

Choose a reason for hiding this comment

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

Looks good so far. I do think the documentation needs some polishing. I'll be reviewing the files in batches.

README.md Outdated Show resolved Hide resolved
build/__tools__/run-tests.sh Outdated Show resolved Hide resolved
ccf_transaction_processor/Readme.md Show resolved Hide resolved
ccf_transaction_processor/Readme.md Show resolved Hide resolved
ccf_transaction_processor/Readme.md Show resolved Hide resolved
ccf_transaction_processor/Readme.md Show resolved Hide resolved
Copy link
Member

@bvavala bvavala left a comment

Choose a reason for hiding this comment

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

Huge PR (but nice!)

  • some naming convention issues
  • missing comments
  • and some I believe unnecessary message fields (or perhaps I just need a clarification)

ccf_transaction_processor/transaction_processor/pdo_tp.cpp Outdated Show resolved Hide resolved
ccf_transaction_processor/transaction_processor/pdo_tp.cpp Outdated Show resolved Hide resolved
ccf_transaction_processor/transaction_processor/pdo_tp.cpp Outdated Show resolved Hide resolved
python/pdo/contract/state.py Show resolved Hide resolved
python/pdo/submitter/ccf/ccf_submitter.py Show resolved Hide resolved
python/pdo/submitter/ccf/ccf_submitter.py Show resolved Hide resolved
Copy link

@masomel masomel left a comment

Choose a reason for hiding this comment

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

Mostly requesting naming changes and additional in-code documentation for the CCF TP. I do have a couple of questions about the JsonRPC fields for certain operations.

…awtooth Submitter.

Signed-off-by: prakashngit <prakash.narayana.moorthy@intel.com>

Microsfot CCF based transaction processor for PDO, in addition to HL Sawtooth based TP.
Commit includes client pieces as well as C++ code for the CCF-based PDO-TP.
The transaction processor was desgined for CCF release 0.7.1.
CCF 0.7.1 is included as a submodule. Certain python modules from this submodule are used as part of the client construction.
CF based PDO-TP includes support for proof of commit - meaining, registry reads from ledger will be signed by CCF service
 which can then be used to construct offline-verifiable proof of txn commits.

Signed-off-by: prakashngit <prakash.narayana.moorthy@intel.com>
Signed-off-by: prakashngit <prakash.narayana.moorthy@intel.com>
@prakashngit prakashngit force-pushed the ccf_pdo_transaction_processor_simulate_mode branch from 1a48b35 to 4ba4cc1 Compare April 17, 2020 17:09
@prakashngit prakashngit requested review from masomel and bvavala April 17, 2020 17:13
@lgtm-com
Copy link

lgtm-com bot commented Apr 17, 2020

This pull request introduces 16 alerts and fixes 13 when merging 4ba4cc1 into c58d420 - view on LGTM.com

new alerts:

  • 7 for Unused import
  • 5 for Explicit export is not defined
  • 2 for Unused local variable
  • 1 for Use of the return value of a procedure
  • 1 for Illegal raise

fixed alerts:

  • 9 for Unused import
  • 4 for Explicit export is not defined

Copy link

@masomel masomel left a comment

Choose a reason for hiding this comment

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

Still some concerns about handling erroneous requests in the TP, and keeping the JsonRPC interface more consistent with Sawtooth protos.

EDIT: Not sure why some of my comments are appearing as resolved. Please do take a look. Also, I'm wondering if this has to do with comments being marked as resolved previously? Not sure why github doesn't automatically re-open those threads.

Signed-off-by: prakashngit <prakash.narayana.moorthy@intel.com>
@lgtm-com
Copy link

lgtm-com bot commented Apr 18, 2020

This pull request introduces 17 alerts and fixes 13 when merging 4cac5c5 into c58d420 - view on LGTM.com

new alerts:

  • 8 for Unused import
  • 5 for Explicit export is not defined
  • 2 for Unused local variable
  • 1 for Use of the return value of a procedure
  • 1 for Illegal raise

fixed alerts:

  • 9 for Unused import
  • 4 for Explicit export is not defined

@cmickeyb
Copy link
Contributor

couple requests... first @g2flyer @bvavala and @masomel please mark issues resolved when appropriate & then approve when all of your questions are satisfied.

@prakashngit please review the LGTM analysis. Several of the issues can/should be addressed.

Signed-off-by: prakashngit <prakash.narayana.moorthy@intel.com>
@lgtm-com
Copy link

lgtm-com bot commented Apr 21, 2020

This pull request introduces 5 alerts and fixes 13 when merging d84e949 into c58d420 - view on LGTM.com

new alerts:

  • 5 for Explicit export is not defined

fixed alerts:

  • 9 for Unused import
  • 4 for Explicit export is not defined

@cmickeyb
Copy link
Contributor

There are still a number of outstanding issues here. I believe they have been (or will be) captured as issues where we can continue the discussion on a case by case basis.

@cmickeyb cmickeyb merged commit 1d73f8e into hyperledger-labs:master Apr 22, 2020
@prakashngit prakashngit deleted the ccf_pdo_transaction_processor_simulate_mode branch May 20, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants