Refactor transaction replay protection implementation #5742
Conversation
gumb0
force-pushed
the
refactor-chainid
branch
3 times, most recently
from
e3c8950 to
b2560a0
Compare
Codecov Report
@@ Coverage Diff @@
## master #5742 +/- ##
==========================================
+ Coverage 63.71% 63.71% +<.01%
==========================================
Files 357 356 -1
Lines 30502 30517 +15
Branches 3392 3392
==========================================
+ Hits 19434 19444 +10
- Misses 9841 9845 +4
- Partials 1227 1228 +1 |
gumb0
force-pushed
the
refactor-chainid
branch
from
4871da4 to
d0cd4ae
Compare
libethcore/TransactionBase.cpp
Outdated
| BOOST_THROW_EXCEPTION(InvalidSignature()); | ||
|
|
||
| m_vrs = SignatureStruct{r, s, static_cast<byte>(v - (m_chainId * 2 + 35))}; | ||
| auto const recoveryID = | ||
| m_chainId.has_value() ? byte{v - (*m_chainId * 2 + 35)} : byte{v - 27}; |
There was a problem hiding this comment.
This silently truncates a uint256 to byte.
There was a problem hiding this comment.
I've added a check above to cap m_chainId at 64 bit. Given how chainID is calculated there, expression here inside byte{...} can only be either 0 or 1
libethcore/TransactionBase.cpp
Outdated
| auto const chainId = (v - 35) / 2; | ||
| if (chainId > std::numeric_limits<uint64_t>::max()) | ||
| BOOST_THROW_EXCEPTION(InvalidSignature()); | ||
| m_chainId = static_cast<byte>(chainId); |
There was a problem hiding this comment.
oh should be unit64_t, sorry
| int m_chainId = -4; ///< EIP155 value for calculating transaction hash https://github.com/ethereum/EIPs/issues/155 | ||
| /// EIP155 value for calculating transaction hash | ||
| /// https://github.com/ethereum/EIPs/blob/master/EIPS/eip-155.md | ||
| boost::optional<uint64_t> m_chainId; |
There was a problem hiding this comment.
Should we use std::optional now that we can build C++17 on Windows? Or should we just convert all boost::optional to std::optional in one go at some point in the future?
There was a problem hiding this comment.
We still build with C++11 standard at the moment (it should be a separate PR when we want to switch; I'm not sure whether AppVeyor supports it)
| @@ -41,19 +41,48 @@ BOOST_AUTO_TEST_CASE(TransactionGasRequired) | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(TransactionWithEmptyRecepient) | |||
| { | |||
| // recipient RLP is 0x80 (empty array) | |||
| // recepient RLP is 0x80 (empty array) | |||
There was a problem hiding this comment.
(Nit) The old spelling is the correct one? https://www.merriam-webster.com/dictionary/recipient
gumb0
force-pushed
the
refactor-chainid
branch
from
46ba824 to
4c6e198
Compare
|
@fubuloubu The test vectors here might be useful in case you plan to submit a separate proposal to fix the size of ChainID to 64 bit in EIP-155 signing rules (I wanted to create reference Transaction tests out of them, but it doesn't look possible with current test format) |
|
Thanks! It's looking clear that the restriction to |
|
Merge? |
|
Yeah I think it's safe to merge this regardless of the decisions around chain ID width |
I got rid of the magic value
-4which was used to represent transactions without replay protection (-4was used because inserting it into formulasCHAIN_ID * 2 + 35/CHAIN_ID * 2 + 35conveniently produced valid values27/28for transactions without replay protection)Instead of this I represent
chainIDwithoptional<uint64_t>For reference replay protection spec is in https://eips.ethereum.org/EIPS/eip-155
This is quite independent of the change to 64 bit in EVMC and CHAINID implementation #5740 (works and passes the tests without it), but I'd merge this after the Chain ID size decision is finalized.