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

Address some Client SCA issues #145

Merged
merged 17 commits into from
Feb 7, 2020
Merged

Address some Client SCA issues #145

merged 17 commits into from
Feb 7, 2020

Conversation

dtebbs
Copy link
Contributor

@dtebbs dtebbs commented Jan 15, 2020

Related to #144

@dtebbs
Copy link
Contributor Author

dtebbs commented Jan 15, 2020

Currently WIP while the document is discussed and finalized.

@dtebbs dtebbs force-pushed the contract-performance branch 3 times, most recently from 896c503 to 28bb7de Compare January 21, 2020 18:30
@dtebbs dtebbs force-pushed the client-sca-issues branch 2 times, most recently from 9f8b88e to e37dedd Compare January 27, 2020 12:11
@dtebbs dtebbs force-pushed the contract-performance branch 4 times, most recently from 66bb45b to 9b0abbe Compare January 30, 2020 17:46
@dtebbs dtebbs changed the title WIP: Address some Client SCA issues (depends on #138) Address some Client SCA issues (depends on #138) Jan 31, 2020
@dtebbs dtebbs mentioned this pull request Jan 31, 2020
for out_ev in mix_result.output_events:
merkle_tree.set_entry(
out_ev.commitment_address, out_ev.commitment)
new_merkle_root = mix_result.new_merkle_root
Copy link
Contributor

Choose a reason for hiding this comment

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

Storing the entire Merkle Tree will be very expensive for the clients (2^{depth + 1} - 1 nodes to store, so if depth is set to 32, the storage requirements for the client will be very important). Instead of storing the full MK tree, the client is only interested in storing the nodes required to generate Merkle authentication paths for the commitments the client user "owns" (i.e.knows the opening for).
This may lead to various SCAs on the client itself, but won't be exploitable at the networking level which is what this PR seems to address

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this does not need to be addressed on this PR, but is something to be aware of, and document for further iterations on the client.

Copy link
Contributor Author

@dtebbs dtebbs Feb 4, 2020

Choose a reason for hiding this comment

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

Yes. Noted for future work and documentation.

@dtebbs dtebbs changed the base branch from contract-performance to develop February 5, 2020 15:57
@dtebbs dtebbs changed the title Address some Client SCA issues (depends on #138) Address some Client SCA issues Feb 5, 2020
@@ -20,3 +20,5 @@

WALLET_DIR_DEFAULT = "./notes"
WALLET_USERNAME = "zeth"

MERKLE_TREE_FILE_DEFAULT = "./merkle_tree"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be consistent with the naming of the constants:
In the same file you have:
INSTANCEFILE_DEFAULT = "zeth-instance.json", ADDRESSFILE_DEFAULT = "zeth-address.json" (FILE is concatenated directly without _), and now you declare MERKLE_TREE_FILE_DEFAULT = "./merkle_tree", with a _FILE.

Let's stick to _FILE everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, let's use file extensions in order to inform on how the data is stored and should be read (.dat can be used for generic data files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chaged to _FILE_ everywhere, and used .dat for merkle tree data.

mixer_desc = load_mixer_description_from_ctx(ctx)
zeth_instance = mixer_desc.mixer.instantiate(web3)
null = bytes(32)
merkle_tree = open_merkle_tree(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

open_merkle_tree should receive a Context, so let's switch to Context instead of Any in the signature of the ls_commit function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used Context everywhere.

@pass_context
def ls_notes(ctx: Any) -> None:
def ls_notes(ctx: Any, balance: bool, spent: bool) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, Any should be Context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(fixed)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @dtebbs. I like the changes you have made to display the balance and the spent notes!

from unittest import TestCase

MERKLE_TREE_TEST_DIR = "_merkle_tests"
MERKLE_TREE_TEST_SIZE = 16
Copy link
Contributor

@AntoineRondelet AntoineRondelet Feb 6, 2020

Choose a reason for hiding this comment

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

Let's rename this constant to be clearer. I imagine that by "size" you mean the nb of leaves, so what about MERKLE_TREE_TEST_LEAVES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the constant.

@AntoineRondelet AntoineRondelet merged commit a2ce0e3 into develop Feb 7, 2020
@AntoineRondelet AntoineRondelet deleted the client-sca-issues branch February 26, 2020 12:25
AntoineRondelet added a commit that referenced this pull request May 6, 2020
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.

2 participants