Skip to content

[WIP] Full circle of Ursula's Identity evidence#996

Closed
cygnusv wants to merge 10 commits intonucypher:masterfrom
cygnusv:identity-evidence
Closed

[WIP] Full circle of Ursula's Identity evidence#996
cygnusv wants to merge 10 commits intonucypher:masterfrom
cygnusv:identity-evidence

Conversation

@cygnusv
Copy link
Member

@cygnusv cygnusv commented May 21, 2019

This PR closes the gap between learning about Ursulas and requesting a slashing to one of them, in particular wrt to Ursula's identity evidence (#962).

@codecov
Copy link

codecov bot commented May 21, 2019

Codecov Report

Merging #996 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #996      +/-   ##
==========================================
+ Coverage   84.98%   85.01%   +0.02%     
==========================================
  Files          65       65              
  Lines        7908     7921      +13     
==========================================
+ Hits         6721     6734      +13     
  Misses       1187     1187
Impacted Files Coverage Δ
nucypher/network/server.py 88.12% <ø> (ø) ⬆️
nucypher/crypto/api.py 97.4% <100%> (+0.25%) ⬆️
nucypher/blockchain/eth/policies.py 58.53% <100%> (-0.67%) ⬇️
nucypher/characters/lawful.py 89.16% <100%> (+0.02%) ⬆️
nucypher/policy/models.py 92.3% <100%> (+0.11%) ⬆️
nucypher/characters/unlawful.py 96.55% <100%> (ø) ⬆️
nucypher/network/nodes.py 83% <100%> (ø) ⬆️

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 965654f...0404f09. Read the comment docs.

// TODO: This will depend on the outcome of #962
address miner = SignatureVerifier.recover(
SignatureVerifier.hash(_minerPublicKey, hashAlgorithm), _minerPublicKeySignature);
SignatureVerifier.hash(abi.encodePacked(precomp.lostBytes[4], miner_xcoord),
Copy link
Member

Choose a reason for hiding this comment

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

...4?

Copy link
Member Author

Choose a reason for hiding this comment

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

See

// Lost bytes: a bytes5 variable holding the following byte values:

BTW, there's a typo in the linked comment, and it should be 4 where it says 5. I'll correct it.

SignatureVerifier.HashAlgorithm.KECCAK256),
_minerPublicKeySignature);
// Check that miner can be slashed
(uint256 minerValue,,,,,) = escrow.minerInfo(miner);
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only way to know that the miner can be slashed?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK, what this actually checks is that the miner is registered by MinersEscrow. @szotov is that right?

Copy link
Member

@vzotova vzotova May 23, 2019

Choose a reason for hiding this comment

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

And also that miner has something that we can take: miner can be slashed if he has stake,
in #949 this line was changed to getAllTokens

ursula = self.known_nodes[node_id]
for ursula_checksum_address, arrangement_id in treasure_map_to_use:
ursula = self.known_nodes[ursula_checksum_address]
assert ursula.checksum_public_address == ursula_checksum_address
Copy link
Member

Choose a reason for hiding this comment

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

Production assert? This will be the third one. Perhaps a comment will help.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! My bad, I was just checking something and I forgot about it.

@cygnusv
Copy link
Member Author

cygnusv commented Jun 7, 2019

Closed. Will try again as part of Alhambra Verde EPIC (#1029)

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.

3 participants