EIP166 - replay protection via high-order bits of nonce #3620

gumb0 · 2017-03-06T14:45:03Z

Add check for CHAIN_ID in transaction validation for transactions in the block
Add the check for pending transactions
When checking against sender nonce, use only lower 64 bits of tx nonce
Creating nonce for eth.sendTransaction RPC call

gumb0 · 2017-03-06T15:20:14Z

Some tests from TransactionTests are failing now because metropolisForkBlock param is misspelled in libethashseal/genesis/*

chfast · 2017-03-07T12:34:43Z

Just a side question: is it possible to reject transactions with gas > 2^63-1 straight away when parsing?

gumb0 · 2017-03-07T12:42:54Z

@chfast I think it's possible, since there're currently some checks in TransactionBase contructor (like checking signature validity and chainId encoded in v value)

codecov-io · 2017-03-08T11:26:57Z

Codecov Report

Merging #3620 into develop will increase coverage by 0.06%.
The diff coverage is 89.65%.

@@             Coverage Diff             @@
##           develop    #3620      +/-   ##
===========================================
+ Coverage       65%   65.06%   +0.06%     
===========================================
  Files          306      307       +1     
  Lines        21337    21390      +53     
===========================================
+ Hits         13870    13918      +48     
- Misses        7467     7472       +5

Impacted Files	Coverage Δ
libethashseal/genesis/metropolisTest.cpp	`100% <ø> (ø)`	✅
libethashseal/genesis/transitionnetTest.cpp	`100% <ø> (ø)`	✅
libethashseal/genesis/homesteadTest.cpp	`100% <ø> (ø)`	✅
libethereum/Transaction.h	`81.81% <ø> (ø)`	✅
libethashseal/genesis/frontierTest.cpp	`100% <ø> (ø)`	✅
libethashseal/genesis/mainNetwork.cpp	`100% <ø> (ø)`	✅
libethcore/Transaction.h	`62.96% <ø> (ø)`	✅
libethashseal/genesis/ropsten.cpp	`100% <ø> (ø)`	✅
libethashseal/genesis/mainNetworkTest.cpp	`100% <ø> (ø)`	✅
libethashseal/genesis/eip150Test.cpp	`100% <ø> (ø)`	✅
... and 158 more

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 57b766c...ed90df0. Read the comment docs.

yann300 · 2017-03-15T13:24:01Z

libethereum/ClientBase.cpp

 		ts.nonce = max<u256>(postSeal().transactionsFrom(ts.from), m_tq.maxNonce(ts.from));
+		if (postSeal().info().number() >= bc().chainParams().u256Param("metropolisForkBlock"))
+		{


curving brace

yann300 · 2017-03-15T14:01:34Z

libethashseal/Ethash.cpp

-	// Unneeded as it's checked again in Executive. Keep it here since tests assume it's checked.
-	if (_ir & ImportRequirements::TransactionBasic && _t.baseGasRequired(evmSchedule(EnvInfo(_bi))) > _t.gas())
-		BOOST_THROW_EXCEPTION(OutOfGasIntrinsic());
+	if (_ir & ImportRequirements::TransactionBasic)


That function is called by the verifyBlock ,
but seems transactions that comes from onPeerTransaction get accepted and thus relayed even if the chainId is wrong.
Would it make sense to check for the replay protection also at the 'network' layer?

For unconfirmed transactions the check is in Executive.
(so they are checked when when we try to execute them and put into pending block)

We can't check that on network level, because the check depends on which block the transaction will be put into
(depends on the block number)

So currently we propagate all the transactions with incorrect chainID-in-nonce (which is fine, since they are currently valid)
We can add the check for chainID-in-nonce to TransactionQueue::import() later after the Metropolis fork, then we'll stop to propagate them.

chfast · 2017-03-16T11:43:48Z

test/libethashseal/CMakeLists.txt

@@ -0,0 +1,5 @@
+cmake_policy(SET CMP0015 NEW)


You don't know what this is about, do you? Remove it.

No idea, just copied from another dir 😆

chfast · 2017-03-16T11:44:14Z

test/libethashseal/CMakeLists.txt

@@ -0,0 +1,5 @@
+cmake_policy(SET CMP0015 NEW)
+
+aux_source_directory(. SRCS)


file(GLOB SRCS "*.cpp").

chfast · 2017-03-16T11:45:50Z

libethashseal/Ethash.cpp

+			_t.checkNonceChainId(nonceChainId);
+		}
+
+		// Unneeded as it's checked again in Executive. Keep it here since tests assume it's checked.


Can you add TODO: prefix for this comment.

chfast · 2017-03-16T11:47:31Z

libethcore/Transaction.cpp

+namespace
+{
+int const c_chainIdInNonceBits = 64;
+u256 const c_nonceLowMask = 0xffffffffffffffff;


I'm sure you can use primitive int type here, boost should have an overloading for it.

Sure it has, but it converts it to multiprecision internally anyway, so will we save anything?

We will save at least dynamic initialization of this global.

Buying it for additional initialization time for processing each transaction?
I'm not really convinced

So I checked :) https://gist.github.com/chfast/dc7cfb070947f703e57c25dc3cbb79f7.

Both versions are very similar, they call eval_bitwise_... template function (big unrolled code in the middle).

u256 c_nonceLowMask does not require dynamic initialization. See _ZN12_GLOBAL__N_1L10lowMask256E.

The version with uint64_t is a bit better because does not require global constant loading.

That was not to prove you ware wrong, I was really curious. I would slightly prefer native type in the constant, but you can decide. Let me know.

So it inlines the int value in code instead of using variable?

Ok, there's little difference, I'll change it, int looks slightly nicer in code for me, too

Global does not have to be created because it has local scope to linker namespace {}. Compiler usually produces code like mov -1 to reg0. The constant is included in the instruction itself.

… or CHAIN_ID

…ect unconfirmed transactions when we construct the pending block

…at it's equal to sender nonce

…oin nonce

gumb0 · 2017-03-17T11:48:31Z

Rebased & review comments addressed

chfast

Big thanks for the unit tests.

chfast · 2017-04-03T14:58:47Z

Is this ready to be merged?

gumb0 · 2017-04-03T15:06:14Z

@chfast It is, but it's probably not going to be included in Metropolis, so we're not merging it yet, waiting for the final decision on which EIPs are included.

pirapira · 2017-10-16T10:46:00Z

This EIP faded away.

gumb0 added the in progress label Mar 6, 2017

gumb0 added this to the Metropolis milestone Mar 6, 2017

gumb0 force-pushed the eip166 branch 3 times, most recently from 34c36ba to 0a3e302 Compare March 8, 2017 08:44

gumb0 force-pushed the eip166 branch from 1013e81 to 494aab8 Compare March 15, 2017 11:31

gumb0 added needs review and removed in progress labels Mar 15, 2017

gumb0 assigned chfast, yann300 and chriseth and unassigned chfast, yann300 and chriseth Mar 15, 2017

gumb0 requested review from chfast, yann300 and chriseth March 15, 2017 11:57

yann300 reviewed Mar 15, 2017

View reviewed changes

chfast reviewed Mar 16, 2017

View reviewed changes

gumb0 force-pushed the eip166 branch 2 times, most recently from 0a2cd31 to fa4f10c Compare March 16, 2017 14:13

gumb0 added 4 commits March 17, 2017 12:23

EIP166 - check that high-order bits of transaction nonce are either 0…

2279a42

… or CHAIN_ID

EIP166 - check CHAIN_ID encoded in nonce in Executive - this will rej…

30c1d6c

…ect unconfirmed transactions when we construct the pending block

EIP166 - Use only lower 64 bits of transaction nonce when checking th…

ba32dbd

…at it's equal to sender nonce

EIP166 - construct transaction with chainID in high-order bits of nonce

26475fc

gumb0 added 2 commits March 17, 2017 12:27

EIP166 - unit tests for Ethash::verifyTransaction() checking chainId …

e121a16

…oin nonce

Minor style fix & CMakeLists improvement

ed90df0

gumb0 force-pushed the eip166 branch from fa4f10c to ed90df0 Compare March 17, 2017 11:46

chfast approved these changes Mar 17, 2017

View reviewed changes

gumb0 removed the needs review label Apr 4, 2017

pirapira closed this Oct 16, 2017

gumb0 deleted the eip166 branch January 10, 2018 16:47

gumb0 restored the eip166 branch January 10, 2018 16:47

chfast deleted the eip166 branch April 9, 2018 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP166 - replay protection via high-order bits of nonce #3620

EIP166 - replay protection via high-order bits of nonce #3620

gumb0 commented Mar 6, 2017 •

edited

Loading

gumb0 commented Mar 6, 2017 •

edited

Loading

chfast commented Mar 7, 2017

gumb0 commented Mar 7, 2017

codecov-io commented Mar 8, 2017 •

edited

Loading

yann300 Mar 15, 2017

yann300 Mar 15, 2017

gumb0 Mar 15, 2017

gumb0 Mar 15, 2017

gumb0 Mar 15, 2017

chfast Mar 16, 2017

gumb0 Mar 16, 2017

chfast Mar 16, 2017

chfast Mar 16, 2017

chfast Mar 16, 2017

gumb0 Mar 16, 2017

chfast Mar 16, 2017

gumb0 Mar 16, 2017

chfast Mar 17, 2017

gumb0 Mar 17, 2017

chfast Mar 17, 2017

gumb0 commented Mar 17, 2017

chfast left a comment

chfast commented Apr 3, 2017

gumb0 commented Apr 3, 2017

pirapira commented Oct 16, 2017

		@@ -0,0 +1,5 @@
		cmake_policy(SET CMP0015 NEW)

		aux_source_directory(. SRCS)

EIP166 - replay protection via high-order bits of nonce #3620

EIP166 - replay protection via high-order bits of nonce #3620

Conversation

gumb0 commented Mar 6, 2017 • edited Loading

gumb0 commented Mar 6, 2017 • edited Loading

chfast commented Mar 7, 2017

gumb0 commented Mar 7, 2017

codecov-io commented Mar 8, 2017 • edited Loading

Codecov Report

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

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

gumb0 commented Mar 17, 2017

chfast left a comment

Choose a reason for hiding this comment

chfast commented Apr 3, 2017

gumb0 commented Apr 3, 2017

pirapira commented Oct 16, 2017

gumb0 commented Mar 6, 2017 •

edited

Loading

gumb0 commented Mar 6, 2017 •

edited

Loading

codecov-io commented Mar 8, 2017 •

edited

Loading