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

Constantinople SSTORE refund fix #5316

Merged
merged 2 commits into from
Oct 15, 2018
Merged

Constantinople SSTORE refund fix #5316

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2cba97
evm: Clean up refunds application
chfast Oct 15, 2018
7dc6ccd
evm: Allow refunds to be negative within an internal call
chfast Oct 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion libethereum/Executive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,8 @@ bool Executive::finalize()

// Refunds must be applied before the miner gets the fees.
assert(m_ext->sub.refunds >= 0);
m_gas += min((m_t.gas() - m_gas) / 2, m_ext->sub.refunds);
int64_t maxRefund = (static_cast<int64_t>(m_t.gas()) - static_cast<int64_t>(m_gas)) / 2;
m_gas += min(maxRefund, m_ext->sub.refunds);
}

if (m_t)
Expand Down
5 changes: 1 addition & 4 deletions libevm/ExtVMFace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,7 @@ evmc_storage_status setStorage(evmc_context* _context, evmc_address const* _addr
if (originalValue != 0)
{
if (currentValue == 0)
{
assert(env.sub.refunds >= schedule.sstoreRefundGas);
env.sub.refunds -= schedule.sstoreRefundGas;
}
env.sub.refunds -= schedule.sstoreRefundGas; // Can go negative.
if (newValue == 0)
env.sub.refunds += schedule.sstoreRefundGas;
}
Expand Down
2 changes: 1 addition & 1 deletion libevm/ExtVMFace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ struct SubState
{
std::set<Address> suicides; ///< Any accounts that have suicided.
LogEntries logs; ///< Any logs.
u256 refunds; ///< Refund counter for storage changes.
int64_t refunds = 0; ///< Refund counter for storage changes.
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be safer to use signed bigint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but if 63 bits are not enough I'd like to constructs a test for this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, you would need 3*10^13 millions gas in a transaction.

Copy link

Choose a reason for hiding this comment

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

Yeah. For this counter, geth uses u64, and parity uses i128 (which is basically signed u64). If we make every opcode give maximum amount of refund as we currently have, it would still take decades of execution for that to ever overflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for clarification @sorpaas.


SubState& operator+=(SubState const& _s)
{
Expand Down
5 changes: 1 addition & 4 deletions libevm/LegacyVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,7 @@ void LegacyVM::updateSSGasEIP1283(u256 const& _currentValue, u256 const& _newVal
if (originalValue != 0)
{
if (_currentValue == 0)
{
assert(m_ext->sub.refunds >= m_schedule->sstoreRefundGas);
m_ext->sub.refunds -= m_schedule->sstoreRefundGas;
}
m_ext->sub.refunds -= m_schedule->sstoreRefundGas; // Can go negative.
if (_newValue == 0)
m_ext->sub.refunds += m_schedule->sstoreRefundGas;
}
Expand Down