Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

EIP658: Status code in receipt #4275

Merged
merged 4 commits into from
Jul 20, 2017
Merged

EIP658: Status code in receipt #4275

merged 4 commits into from
Jul 20, 2017

Conversation

pirapira
Copy link
Member

@codecov-io
Copy link

codecov-io commented Jul 19, 2017

Codecov Report

Merging #4275 into develop will increase coverage by <.01%.
The diff coverage is 65.9%.

@@             Coverage Diff             @@
##           develop    #4275      +/-   ##
===========================================
+ Coverage    67.48%   67.49%   +<.01%     
===========================================
  Files          306      306              
  Lines        23450    23472      +22     
===========================================
+ Hits         15826    15843      +17     
- Misses        7624     7629       +5
Impacted Files Coverage Δ
libethereum/TransactionReceipt.h 20% <ø> (-7.28%) ⬇️
libethereum/Executive.h 66.66% <ø> (ø) ⬆️
libethcore/Exceptions.h 76.74% <0%> (-1.83%) ⬇️
libweb3jsonrpc/JsonHelper.cpp 4.73% <0%> (-0.04%) ⬇️
libethereum/State.cpp 75.44% <100%> (-0.08%) ⬇️
libethereum/Executive.cpp 61.53% <100%> (ø) ⬆️
libethereum/Block.cpp 85.46% <50%> (-0.22%) ⬇️
libethereum/TransactionReceipt.cpp 62.5% <68.75%> (-5.07%) ⬇️
libdevcore/RLP.h 80.59% <0%> (-0.61%) ⬇️
libp2p/Common.cpp 52.8% <0%> (+0.79%) ⬆️
... and 2 more

@pirapira pirapira requested review from gumb0 and chfast and removed request for gumb0 July 19, 2017 15:37
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

We need also to modify Block::stateRootBeforeTx, so that it returns 0 when receipt doesn't have state root - this is what RPC methods that need intermediate state rely on.

@@ -52,7 +58,12 @@ class TransactionReceipt
bytes rlp() const { RLPStream s; streamRLP(s); return s.out(); }

private:
h256 m_stateRoot;
bool m_hasStatusCode = false;
union
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest trying to use boost::variant for this, it kind of encapsulates union + type variable

@gumb0
Copy link
Member

gumb0 commented Jul 19, 2017

Also toJson for receipt in libweb3jsonrpc/JsonHelper.cpp needs a fix

@gumb0
Copy link
Member

gumb0 commented Jul 19, 2017

More exactly, dev::eth::createIntermediateState in State.cpp assumes that intermediate state root returned from Block::stateRootBeforeTx is either valid hash or 0 (then replays transactions) - we could also handle exception there instead of modifying Block::stateRootBeforeTx

@pirapira pirapira force-pushed the status_code_in_receipt branch from 183ce75 to 0ddd07a Compare July 19, 2017 17:47
@pirapira
Copy link
Member Author

I started using boost::variant and changed the stateRoot() usage.

_s.appendList(3);

_s << m_gasUsed << m_bloom;
struct appenderVisitor : boost::static_visitor<void>
Copy link
Member

Choose a reason for hiding this comment

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

Wow, couldn't we just do something like

    if (m_statusCodeOrStateRoot.which() == 0)
        _s  << boost::get<uint8_t>(m_statusCodeOrStateRoot);
    else
        _s  << boost::get<h256>(m_statusCodeOrStateRoot);

{
try
{
return boost::get<h256>(m_statusCodeOrStateRoot);
Copy link
Member

Choose a reason for hiding this comment

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

In stateRoot you use different approach than in hasStatusCode & stateRoot, choose one for consistency (the one without visitor is easier).

I would actually prefer just

    if (m_statusCodeOrStateRoot.which() == 1)
      return boost::get<h256>(m_statusCodeOrStateRoot);
    else
      BOOST_THROW_EXCEPTION(TransactionReceiptVersionError());

@pirapira
Copy link
Member Author

I made everything into if (hasStatusCode()) ... else ....

@@ -22,54 +22,88 @@
#include "TransactionReceipt.h"
#include <libethcore/Exceptions.h>

#include <boost/variant/static_visitor.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

This one is not needed now

{
return (_i > 0 ? receipt(_i - 1).stateRoot() : m_previousBlock.stateRoot());
}
catch(TransactionReceiptVersionError const&)
Copy link
Member

Choose a reason for hiding this comment

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

space after catch

if (r[0].size() == 32)
m_statusCodeOrStateRoot = (h256)r[0];
else if (r[0].size() == 1)
m_statusCodeOrStateRoot = (uint8_t)r[0];
Copy link
Member

Choose a reason for hiding this comment

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

Probable add else BOOST_THROW_EXCEPTION(InvalidTransactionReceiptFormat());

@@ -185,7 +185,14 @@ Json::Value toJson(dev::eth::TransactionSkeleton const& _t)
Json::Value toJson(dev::eth::TransactionReceipt const& _t)
{
Json::Value res;
res["stateRoot"] = toJS(_t.stateRoot());
try
Copy link
Member

Choose a reason for hiding this comment

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

Maybe somewhat simpler would be to use hasStatusCode() instead of try/catch

/// @returns true if the receipt has a status code. Otherwise the receipt has a state root (pre-EIP658).
bool hasStatusCode() const;
/// @returns the state root.
/// @throw TransactionReceiptVersionError when m_hasStatusCode is true.
Copy link
Member

Choose a reason for hiding this comment

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

Fix comment, there's no m_hasStatusCode anymore

@pirapira pirapira force-pushed the status_code_in_receipt branch from ee46ab2 to eca4fbf Compare July 20, 2017 12:26
@pirapira pirapira force-pushed the status_code_in_receipt branch from eca4fbf to c9d14fc Compare July 20, 2017 12:29
@pirapira pirapira merged commit b352d83 into develop Jul 20, 2017
@pirapira pirapira deleted the status_code_in_receipt branch July 20, 2017 15:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants