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.

Sign up for GitHub

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

Jump to bottom

Sawtooth in-depth removal #380

Merged
merged 3 commits into from
Nov 8, 2022
Merged

Sawtooth in-depth removal #380

merged 3 commits into from
Nov 8, 2022

Conversation

bvavala
Copy link
Member

@bvavala bvavala commented Oct 17, 2022 •
edited
Loading

This PR completes the removal of Sawtooth and depends on #379 for testing (at least the eservice) in HW mode.

This PR removes the following Sawtooth-related things:

  • references in docs and toml files
  • build flags and procedures
  • scripts for SKF files
  • configuration flags and procedures in services

Also, it updates:

  • the default ledger port 8008 -> 6600
  • the default ledger type sawtooth -> ccf
  • the registration with the ledger -- which is now empty since CCF is not set up for checking proof-data in HW mode
  • the default service keys *.skf -> *.pem
  • the TransactionKeys class by deprecating the methods

@bvavala
remove in-file sawtooth references, functions, keys, docs, etc.
4bad6cc 
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
marcelamelara
marcelamelara reviewed Oct 21, 2022
View reviewed changes
Copy link
Member

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I have a couple clarification questions, that may just be because I don't fully understand CCF.

README.md Show resolved Hide resolved
client/setup.py Show resolved Hide resolved
docs/host_install.md Show resolved Hide resolved
docs/install.md Show resolved Hide resolved
eservice/bin/register-with-ledger.sh Show resolved Hide resolved
@bvavala bvavala mentioned this pull request Oct 24, 2022
Closed
@bvavala
remove protobuf dependencies
ee0a100 
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
@bvavala bvavala force-pushed the bruno.221013.sawtooth-in-depth-removal branch from e86274a to ee0a100 Compare October 24, 2022 16:42
@bvavala bvavala mentioned this pull request Oct 25, 2022
Closed
prakashngit
prakashngit reviewed Nov 7, 2022
View reviewed changes
python/pdo/common/keys.py
@@ -60,6 +57,7 @@ class TransactionKeys(object) :
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need TransactionKeys after Sawtooth removal? (the comment says " Wrapper for managing Sawtooth transaction keys").

Also, please see ContractMessage init. Channel_id is no longer TransactionKeys. The derivation of channel_id depends on the ledger type, for CCF, it's just nonce (AR: change the comment on line 30 of message.py)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need TransactionKeys after Sawtooth removal? (the comment says " Wrapper for managing Sawtooth transaction keys").

It does not seem so, I have deprecated it. Upon removal, the dependency on secp256k1 can be discarded as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, please see ContractMessage init. Channel_id is no longer TransactionKeys. The derivation of channel_id depends on the ledger type, for CCF, it's just nonce (AR: change the comment on line 30 of message.py)

Committed 039e413

build/Makefile
@@ -90,7 +90,6 @@ $(PYTHON_DIR) :
. $(abspath $(DSTDIR)/bin/activate) && pip install --upgrade twisted
. $(abspath $(DSTDIR)/bin/activate) && pip install --upgrade pyyaml
. $(abspath $(DSTDIR)/bin/activate) && pip install --upgrade google
. $(abspath $(DSTDIR)/bin/activate) && pip install --upgrade protobuf==3.19.0
. $(abspath $(DSTDIR)/bin/activate) && pip install secp256k1==0.13.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need secp256k1 after Sawtooth removal?

Copy link
Member Author

@bvavala bvavala Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(see #380 (comment))

build/__tools__/make-keys
pf.write(stl_private_key.pubkey.serialize().hex())

elif options.format == 'pem' :
if options.format == 'pem' :
import pdo.common.crypto as crypto
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to retain secp256k1 after Sawtooth removal?

build/common-config.sh
@@ -85,7 +85,7 @@ var_set() {
env_val[PDO_DEFAULT_SIGCURVE]="${PDO_DEFAULT_SIGCURVE:-SECP384R1}"
env_desc[PDO_DEFAULT_SIGCURVE]="
PDO_DEFAULT_SIGCURVE is the ECDSA curve used by PDO for generating signatures.
Choose SECP384R1 for ccf. If not set, cyrpto library uses SECP256K1 as default which works for sawtooth"
Choose SECP384R1 for ccf. If not set, the crypto library uses SECP256K1 by default."
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps remove "If not set, the crypto library uses SECP256K1 by default"?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's still the default.

docker/ccf-pdo.debugging.yaml
@@ -0,0 +1,47 @@
# Copyright 2018 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this image?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not an image. It's the equivalent of what we had with Sawtooth (see here https://github.com/hyperledger-labs/private-data-objects/blob/a95abcf2892c61d96b02cc2026f7137efada174b/docker/sawtooth-pdo.debugging.yaml) to enable the PDO_DEBUG_BUILD feature in docker compose. I can't recall if used it in my tests.

python/pdo/contract/transaction.py
@@ -301,7 +301,7 @@ def __init__(self, ledger_config, commit_id, wait_parameter_for_ledger = None):

self.ledger_config = ledger_config
# add the wait parameter to the ledger config, if there is one.
# Used only with Sawtooth. Defaults to 30s in Sawtooth submitter
# Question: does CCF (or the submitter) use this parameter?
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid I don't remember. Sawtooth showed a weird bug that @cmickeyb found out, and this was my fix. I cannot recall what the issue was.

@bvavala
misc clarifications
039e413 
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
@prakashngit prakashngit merged commit f7646fb into hyperledger-labs:PDO-v0.2.0 Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prakashngit prakashngit prakashngit left review comments

@marcelamelara marcelamelara marcelamelara left review comments

Assignees

@prakashngit prakashngit

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bvavala @prakashngit @marcelamelara