adds parity_verifySignature RPC method #9507

seunlanlege · 2018-09-10T09:53:38Z

parity-cla-bot · 2018-09-10T09:53:41Z

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

niklasad1 · 2018-09-10T10:06:00Z

rpc/src/v1/helpers/mod.rs

@@ -34,8 +34,10 @@ mod signer;
 mod signing_queue;
 mod subscribers;
 mod subscription_manager;
+mod verify_signature;


this file doesn't exist change it to mod signature or change the file name itself?

niklasad1 · 2018-09-10T10:07:52Z

rpc/src/v1/helpers/errors.rs

+pub fn verification_error<T: fmt::Display>(data: T) -> Error {
+	Error {
+		code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST),
+		message: format!("{}", data).into(),


Needless into()

tomusdrw

Looks promising, but would be nice to re-use existing methods.

tomusdrw · 2018-09-10T10:10:49Z

rpc/src/v1/helpers/signature.rs

+pub fn verify_signature(is_prefixed: bool, message: Bytes, signature: H520) -> Result<RichBasicAccount> {
+	let mut buf = [0; 32];
+	buf.copy_from_slice(&message.0[..]);
+	let mut message = H256(buf);


Message is pre-maturely casted to H256, it should be just keccaked without prefix.

did not think of this 😅

tomusdrw · 2018-09-10T10:20:31Z

rpc/src/v1/helpers/signature.rs

+
+	if is_prefixed {
+		let mut buf = [0; 32];
+		let mut keccak = Keccak::new_keccak256();


You can re-use: https://github.com/paritytech/parity-ethereum/blob/2177a0179e67717adfb7b00e7fddc59df6cfa8f4/rpc/src/v1/helpers/dispatch.rs#L243

tomusdrw · 2018-09-10T10:23:02Z

rpc/src/v1/helpers/signature.rs

+use ethereum_types::H256;
+use tiny_keccak::Keccak;
+
+pub fn verify_signature(is_prefixed: bool, message: Bytes, signature: H520) -> Result<RichBasicAccount> {


The method should support chain replay protected signatures with chain_id encoded in the value of v.

rpc/src/v1/traits/parity.rs

seunlanlege · 2018-09-10T16:49:31Z

rpc/src/v1/helpers/signature.rs

+	let signature = Signature::from(signature.0);
+	let is_valid_for_current_chain = chain_id.map(|chain| {
+		let v = signature.v();
+		if v > 1 && (v as u64) == chain {


I'm pretty sure this is wrong

niklasad1 · 2018-09-10T21:07:44Z

rpc/src/v1/helpers/signature.rs

+	let mut hash = keccak(message.0.clone());
+	if is_prefixed {
+		hash = eth_data_hash(message.0);
+	}


Since you are not using the message after you have computed the hash the clone seems needless!

Consider:
let hash = if is_prefixed { eth_data_hash(message.0) } else { keccak(message.0) };

Also I think keccak supports slices so should be possible to pass-by-reference too!

niklasad1 · 2018-09-10T21:08:56Z

rpc/src/v1/impls/light/parity.rs

 use v1::helpers::dispatch::LightDispatcher;
 use v1::helpers::light_fetch::LightFetch;
 use v1::metadata::Metadata;
 use v1::traits::Parity;
 use v1::types::{
-	Bytes, U256, U64, H160, H256, H512, CallRequest,
+	Bytes, U256, U64, H160,H520, H256, H512, CallRequest,


space after comma 😸

seunlanlege · 2018-09-11T13:37:18Z

@tomusdrw @niklasad1 i think this is ready to be merged

niklasad1 · 2018-09-12T13:01:17Z

rpc/src/v1/helpers/signature.rs

+	let is_valid_for_current_chain = chain_id.map(|chain_id| {
+		let v = signature.v() as u64;
+		if v > 36  {
+			let decoded_v = (v - 35) - (chain_id * 2);


this can still overflow if chain_id*2 is bigger than v-35 (maybe only theoretical but still ...)

Consider to use saturating_sub/checked_sub and saturated_mul for this use-case instead!

EDIT:
for example v.saturating_sub(35).saturating_sub(chain_id.saturating_mul(2)) < 2

not sure why we need saturating_mul with chain_id, isn't it supposed to be a compile-time constant?

niklasad1 · 2018-09-12T13:02:21Z

rpc/src/v1/helpers/signature.rs

+	let hash = if is_prefixed {
+		eth_data_hash(message.0)
+	} else {
+		keccak(message.0.clone())


sorry to nag but please remove clone() here 🥇

niklasad1

Minor things to address, see comments below

niklasad1 · 2018-09-12T14:59:49Z

rpc/src/v1/helpers/signature.rs

@@ -9,16 +9,16 @@ pub fn verify_signature(is_prefixed: bool, message: Bytes, signature: H520, chai
 	let hash = if is_prefixed {
 		eth_data_hash(message.0)
 	} else {
-		keccak(message.0.clone())
+		keccak(message.0)
 	};

 	let signature = Signature::from(signature.0);
 	let is_valid_for_current_chain = chain_id.map(|chain_id| {
 		let v = signature.v() as u64;
 		if v > 36  {


@seunlanlege

this if expression is unnecessary now so remove it and return false from the match instead 👍

Also I would prefer if you can perform saturated_mul on chain_id * 2 because it can also overflow sorry if I was unclear

tomusdrw · 2018-09-13T08:14:50Z

rpc/src/v1/helpers/signature.rs

+
+	let signature = Signature::from(signature.0);
+	let is_valid_for_current_chain = chain_id.map(|chain_id| {
+		let  result = (signature.v() as u64)


Should be just (signature.v().checked_sub(35) / 2) as u64 == chain_id - it's simpler, doesn't overflow and does not require matching 0 and 1.
Also if chain_id.is_none() the signature is valid if it's not replay-protected (currently we return false).

BTW This function needs tests, we should also clearly document what format of signature we expect here. (I know there is one RPC test for this, but we should just test this function with different signatures to make sure to cover all branches and that replay protection part works correctly)
Note that signature in transaction is actually using u64 + U256 + U256 to store the replay protection, we later remove replay protection (subtract and divide) and convert the entire signature to H520.
So the fact that we require replay protection to fit to u8 when submitted over RPC limits the number of chain_ids one can safely use for this scheme.

To make it clear: have a look at ec_recover method that we have in personal namespace.
It expects signature in electrum notation and doesn't handle replay protection (I think replay protection is not implemented for prefixed signing (i.e. eth_sign - please check)).
This method should work exactly like ec_recover for prefixed data, but for non-prefixed data (i.e. transaction hash) we might consider checking replay-protection (but then we need to change the input type to something else than H520) or we need to clearly state that the signature should be passed in electrum format.

tomusdrw · 2018-09-13T08:19:26Z

rpc/src/v1/impls/light/parity.rs

 	Peers, Transaction, RpcSettings, Histogram,
 	TransactionStats, LocalTransactionStatus,
 	BlockNumber, ConsensusCapability, VersionInfo,
 	OperationsInfo, ChainStatus,
 	AccountInfo, HwAccountInfo, Header, RichHeader,
 };
 use Host;
+use v1::types::RichBasicAccount;


Move to other v1::types imports

tomusdrw · 2018-09-13T08:20:00Z

rpc/src/v1/helpers/signature.rs

+	let public = recover(&signature, &hash).map_err(errors::encryption)?;
+	let address = public_to_address(&public);
+	let account = BasicAccount { address, public_key: public, is_valid_for_current_chain };
+	Ok(RichBasicAccount { inner: account, extra_info: Default::default() })


Why do we return RichBasicAccount if extra_info is always empty?

tomusdrw · 2018-09-13T08:20:40Z

rpc/src/v1/types/account_info.rs

+pub struct BasicAccount {
+	pub address: Address,
+	pub public_key: Public,
+	pub is_valid_for_current_chain: Option<bool>


missing trailing comma & docs

tomusdrw · 2018-09-17T07:06:51Z

rpc/src/v1/tests/helpers/signature.rs

@@ -0,0 +1,57 @@
+use std::sync::Arc;


New files need a preamble, but I think it would be much better to add the tests in the same file as the function-under-test. Just add #[cfg(test)] mod tests { and put the content there.

tomusdrw · 2018-09-17T07:13:15Z

rpc/src/v1/tests/helpers/signature.rs

+
+#[test]
+fn test_verify_signature_prefixed_mainnet() {
+	setup(true, Some(1), Some(1), true)


Test in a current form is pretty unreadable - I think it would be way better to avoid passing so many bools and numbers without some additional description.
I think the setup should only be producing the signature, the assertion and call to verify_signature should be inlined and duplicated in every test, like this:

#[test] fn test_verify_signature_not_prefixed_mainnet() { let should_prefix = true; let data = vec![5u8]; let chain_id = Some(1); let (address, v, r, s) = sign(should_prefix, &data, chain_id); let account = verify_signature(should_prefix, &data, v, r, s, chain_id).unwrap(); assert_eq!(account.inner.address, address); assert_eq!(account.inner.is_valid_for_current_chain, true); }

IMHO that would really help readability/understandability of the test. If you want to keep the de-duplication, then consider using this:

#[test] fn test_verify_signature_not_prefixed_mainnet() { run_test(TestCase { should_prefix: true, signing_chain_id: Some(1), rpc_chain_id: Some(1), is_valid_for_current_chain: true, }); }

tomusdrw · 2018-09-17T07:14:18Z

rpc/src/v1/tests/mocked/parity.rs

+			"0xe552acf4caabe9661893fd48c7b5e68af20bf007193442f8ca05ce836699d75e",
+			"0x2089e84151c3cdc45255c07557b349f5bf2ed3e68f6098723eaa90a0f8b2b3e5",
+			"0x5f70e8df7bd0c4417afb5f5a39d82e15d03adeff8796725d8b14889ed1d1aa8a",
+			1


In RPC we only use non hex-encoded numbers for indices/limits. For constistency this should be U64 type (hex-encoded ser/de)

tomusdrw · 2018-09-17T07:15:38Z

rpc/src/v1/traits/parity.rs

+
+		/// ecrecover signature
+		#[rpc(name = "parity_verifySignature")]
+		fn verify_signature(&self, bool, Bytes, H256, H256, u64) -> Result<RichBasicAccount>;


I still don't understand why we need RichBasicAccount here, if we don't pass any "rich" data. Just return the BasicAccount and get rid of the to_rich_struct helper func.

tomusdrw

👌

5chdn · 2018-09-18T07:39:29Z

Please, rebase on master

dvdplm · 2018-09-27T19:29:38Z

rpc/src/v1/helpers/signature.rs

+use v1::helpers::dispatch::eth_data_hash;
+use hash::keccak;
+
+pub fn verify_signature(is_prefixed: bool, message: Bytes, v: U64, r: H256, s: H256, chain_id: Option<u64>) -> Result<BasicAccount> {


Need docs (anything pub does). Consider moving params to own lines as well, and perhaps use the rsv order to make it clearer what the params are?

dvdplm · 2018-09-27T19:31:36Z

rpc/src/v1/helpers/signature.rs

+
+	let v = if v > 36 {
+		(v - 1) % 2
+	} else { v };


Funny whitespace here. Either put it all on one line or both arms on own lines.

dvdplm · 2018-09-27T19:45:05Z

rpc/src/v1/helpers/signature.rs

+	let v: u64 = v.into();
+	let is_valid_for_current_chain = match (chain_id, v) {
+		(None, v) if v == 0 || v == 1 => true,
+		(Some(chain_id), v) if v > 36 => (v - 35) / 2 == chain_id,


Can you help me understand what is going on here?

if there's no chain_id, then v must either be 1/0 to be a valid for the current chain.
otherwise, if there's a chain_id and v > 36; (37 is the lowest value, where v = 0, chain_id = 1)
we check if the encoded chain_id is valid for the current chain.

(v - 35) / 2 == chain_id

This is related to extract to chain_id and remove the replay protection right?

This code is hard to read, can you add an inline comment to refer to EIP 155 and perhaps add a one-liner function named remove_replay_protection or something similar?

Also, I think it should be v >= 35 to support chain_id 0

dvdplm · 2018-09-27T19:46:50Z

rpc/src/v1/helpers/signature.rs

+		let accounts = Arc::new(AccountProvider::transient_provider());
+		let address = accounts.new_account(&"password123".into()).unwrap();
+		let sig = accounts.sign(address, Some("password123".into()), hash).unwrap();
+		let (v, r, s) = (sig.v(), sig.r(), sig.s());


I'd prefer let (r, s, v) = … for consistency with other code, e.g. from_rsv()

dvdplm · 2018-09-27T20:04:32Z

rpc/src/v1/traits/parity.rs

@@ -218,5 +217,9 @@ build_rpc_trait! {
 		/// Call contract, returning the output data.
 		#[rpc(name = "parity_call")]
 		fn call(&self, Vec<CallRequest>, Trailing<BlockNumber>) -> Result<Vec<Bytes>>;
+
+		/// ecrecover signature


The docs here are a bit…short? I don't see the params being documented, but perhaps you'll address that in a separate PR? Maybe something like: "/// Extracts Address and public key from signature using the r, s and v params. Equivalent to Solidity erecover"?

dvdplm · 2018-09-27T20:06:15Z

rpc/src/v1/types/account_info.rs

+/// account derived from a signature
+/// as well as information that tells if it is valid for
+/// the current chain
+#[derive(Debug, Clone, Serialize, PartialEq, Eq)]


Do we really need all these derives? It's hard to tell from the code in this PR if that is the case. Thanks!

dvdplm · 2018-09-27T20:07:55Z

rpc/src/v1/types/account_info.rs

+	pub public_key: Public,
+	/// if the signature contains chain replay protection
+	/// and the chain_id encoded within the signature matches the current chain
+	/// this would be true, otherwise false.


Good docs on this type; would be great if you could fix the line lengths and start all sentences with a capital letter.

dvdplm · 2018-09-27T20:09:12Z

rpc/src/v1/types/account_info.rs

@@ -14,6 +14,8 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.



Would be great with some module docs here; I think these types are all return types for RPC calls? If so, perhaps "Return types for RPC calls" is enough.

5chdn · 2018-11-25T19:02:04Z

@dvdplm Can you have a look at this one again?

David can't review at the moment

5chdn · 2018-11-27T10:01:30Z

Happy to merge this if you could resolve the conflicts again

adds parity_verifySignature RPC method

moved verify_signature tests to live in the same file.

* closes #7009 adds parity_verifySignature RPC method * removed unneccesary into * adds support for chain replay protected signatures * corrected chain replay protection check * corrected possible overflow * added tests * use checked_sub to prevent possible overflow * use saturating_mul to prevent possible overflow * changed method signature to accept r,s,v components * added unit tests * more tests * refactored tests for better readability moved verify_signature tests to live in the same file. * removed unneccesary imports * fixed PR grumbles * fixed rsv notation * fixed rsv notation * renamed BasicAcount to RecoveredAccount, Support zero chain_id * fixed compile errors * fixed tests * doc comment

niklasad1 reviewed Sep 10, 2018

View reviewed changes

tomusdrw reviewed Sep 10, 2018

View reviewed changes

seunlanlege commented Sep 10, 2018

View reviewed changes

5chdn added this to the 2.1 milestone Sep 10, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Sep 10, 2018

niklasad1 reviewed Sep 10, 2018

View reviewed changes

5chdn modified the milestones: 2.1, 2.2 Sep 11, 2018

niklasad1 reviewed Sep 12, 2018

View reviewed changes

niklasad1 previously requested changes Sep 12, 2018

View reviewed changes

niklasad1 reviewed Sep 12, 2018

View reviewed changes

tomusdrw reviewed Sep 13, 2018

View reviewed changes

seunlanlege force-pushed the issue-7009 branch from ba9d6bc to 4d05109 Compare September 14, 2018 16:01

tomusdrw reviewed Sep 17, 2018

View reviewed changes

tomusdrw approved these changes Sep 17, 2018

View reviewed changes

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 17, 2018

seunlanlege force-pushed the issue-7009 branch from 2f2fa1e to 48ec65e Compare September 18, 2018 12:26

5chdn added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 19, 2018

dvdplm previously requested changes Sep 27, 2018

View reviewed changes

5chdn added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Sep 27, 2018

seunlanlege force-pushed the issue-7009 branch from 28cc5f0 to 9d1bc22 Compare November 12, 2018 12:22

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 25, 2018

seunlanlege added 20 commits November 27, 2018 11:05

closes #7009

37566fd

adds parity_verifySignature RPC method

removed unneccesary into

1606443

adds support for chain replay protected signatures

506b5ad

corrected chain replay protection check

77e1366

corrected possible overflow

821b552

added tests

f541ca2

use checked_sub to prevent possible overflow

cdfc2ef

use saturating_mul to prevent possible overflow

d3360d8

changed method signature to accept r,s,v components

70efe27

added unit tests

974287f

more tests

84a1aaa

refactored tests for better readability

70ece55

moved verify_signature tests to live in the same file.

removed unneccesary imports

72bd70f

fixed PR grumbles

f875a48

fixed rsv notation

3b6416b

fixed rsv notation

3f56ae8

renamed BasicAcount to RecoveredAccount, Support zero chain_id

b9d016d

fixed compile errors

5283f6b

fixed tests

804ec91

doc comment

0da1766

seunlanlege force-pushed the issue-7009 branch from 9d1bc22 to 0da1766 Compare November 27, 2018 10:46

5chdn merged commit 0e94ac0 into master Nov 28, 2018

5chdn deleted the issue-7009 branch November 28, 2018 11:18

		@@ -14,6 +14,8 @@
		// You should have received a copy of the GNU General Public License
		// along with Parity. If not, see <http://www.gnu.org/licenses/>.

adds parity_verifySignature RPC method #9507

adds parity_verifySignature RPC method #9507

Conversation

seunlanlege commented Sep 10, 2018 • edited by ordian

parity-cla-bot commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a 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

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

niklasad1 Sep 10, 2018 • edited

Choose a reason for hiding this comment

seunlanlege commented Sep 11, 2018

niklasad1 Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

5chdn commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seunlanlege Sep 28, 2018 • edited

Choose a reason for hiding this comment

niklasad1 Oct 29, 2018 • edited

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

Choose a reason for hiding this comment

5chdn commented Nov 25, 2018

5chdn commented Nov 27, 2018

seunlanlege commented Sep 10, 2018 •

edited by ordian

niklasad1 Sep 10, 2018 •

edited

niklasad1 Sep 12, 2018 •

edited

seunlanlege Sep 28, 2018 •

edited

niklasad1 Oct 29, 2018 •

edited