This PR contains a plethora of changes made to SafeVars internal functionality. Huge thanks to @fcecin , @itamarcps and @Pedrozo ! This wouldn't have been possible (or even bearable to begin with) if it weren't for your insights and unending patience :sweat_smile:
General overview
All SafeVars now work internally with an optimist approach
Before the change, the tmp value was always changing, and the real (original) value was kept intact - this meant a commit would always copy from tmp to ori, which was unoptimal
After the change, the real value is always operated upon, and the tmp value is kept as a copy (and a supplementary undo stack, depending on the var) of the real value - this means a commit will simply discard the tmp value, which is closer to how Ethereum's EVM does it internally
Tests for SafeVars were all redone and simplified to adapt to the optimist approach
Each SafeVar now has the real value as its own type, and the tmp value as one of the following:
SafeAddress, SafeInt, SafeUint, SafeString, SafeArray, SafeVector: std::unique_ptr<UnderlyingType> (any non-const operation should do a full copy of the original value before it is changed, assuming there's no copy already made beforehand, indicated by nullptr)
SafeBool: bool (same idea as the other simple types, but not a pointer because a raw bool takes only 1 byte instead of 8 for a pointer, plus it's just a bool anyway, two possible values, using a pointer would be kinda overkill...)
SafeUnorderedMap: std::unordered_map<Key, std::unique_ptr<UnderlyingType>, SafeHash>> (any non-const operation should copy only the affected key or range of keys, using nullptr to indicate when the key didn't exist at all in the real value before the operation - if the key itself doesn't exist on tmp on the other hand, it means the same key on the real value was unchanged)
SafeTuple: std::tuple<std::unique_ptr<UnderlyingType>...> (same idea as SafeUnorderedMap)
SafeBase::check() was removed, which means each non-const function of any given SafeVar is independently handling the tmp value in an isolated manner (this was made on purpose to ensure correct logic but could be optimized further if needed)
SafeBase::revert() is not a const function anymore (since both commit() and revert() alter member variables anyway)
(Almost) all functions, including constructors, now operate specifically on the current/real value only, with a few specific ones handling both the real and the tmp value (e.g. swap() for SafeString and SafeTuple swaps both values and marks both values as used - actually in hindsight I'm not sure whether this is correct now, but would appreciate a second opinion)
Any internal value that's not the real one is always init at construction as nullptr or an "empty/default" equivalent
(Almost) all functions that return iterators return specifically const_iterators to ensure "safety" is guaranteed (see the exceptions at the end)
Undo stacks
SafeArray and SafeVector have an extra functionality due to their inherent characteristics: they use undo stacks for a finer backup process
A SafeVar that uses an undo stack should implement the following:
An enum containing each partial non-const operation that can be done on the real value (a partial operation only alters a portion of the real value, e.g. vec[0] = "xyz";, while a full operation changes the value as a whole, e.g. vec.clear();)
An UndoOp structure that contains a specific partial operation done on the real value - SafeArray implements it as a std::tuple<Op, std::size_t, T> (operation enum, at which index or quantity, and the old value), and SafeVector implements it as a std::tuple<Op, std::size_t, std::size_t, std::vector<T>> (operation enum, at which index, optionally at which quantity, and one or more old values)
The undo stack itself - a std::unique_ptr<std::stack<UndoOp, std::vector<UndoOp>>> meant to track only the partial operations made in the real value
A processUndoStack() function called on revert that takes each partial operation done in the past and applies its "polar opposite" directly to the real value (e.g. the opposite of insert(pos) is erase(pos) and vice-versa)
If a full operation happens midway, a full copy of the current value is made (altered or not), and the undo stack no longer keeps track of any changes after that (but still retains the previous ones so revert can function the way it should)
Commit and revert logic
When you commit a SafeVar, it simply discards the tmp value entirely (e.g. this->copy_ = nullptr;)
When you revert a SafeVar, the logic varies according to the var:
Pointer copies: simply copy the value back from tmp if it exists
SafeArray and SafeVector: copy the value back from tmp if it exists, then apply the undo stack on top of the real value if it has any stored operations
SafeUnorderedMap and SafeTuple: iterate through each existing key on tmp, and either delete or insert/(re-)assign the original value depending on whether the key on tmp is nullptr or not
SafeBool is an oddball: commit copies the real value to tmp, and revert copies the tmp value back to the real one
Tests
Tests were massively rewritten, simplified and adapted to not only be aware of the optimist approach, but also cover a bigger surface (most tests were either not checking all possible constructors and/or overloads, or were kinda messy logic-wise with unoptimized checks, unnecessary extra and/or more verbose vars)
The general SafeVar behaviour has changed overall, so code that depended on the previous behaviour may need to be changed
e.g. if a piece of code depended specifically on a SafeVar being initialized and committed/reverted to "fixate" the value, now it doesn't need to anymore thanks to the optimist approach
See the diff for the "SafeAddress constructor" test as an example, it's subtle but much better now, the point is if a dev has a contract with code that depended on the previous behaviour, it might be broken at best and undefined behaviour at worst
Tests for SafeInt and SafeUint were merged into one file each (Boost types were separate from native C++ types for some reason, now they're all together and tested all at once)
What is missing
SafeIterators - this could probably be a separate PR depending on the (lack of) urgency
Due to the point above, SafeUnorderedMap::find(), begin() and end()currently DO NOT MAKE ANY COPIES of the original value, but weren't removed nor changed to return const_iterator because they are necessary for some contract templates to work - for now those functions are considered "unsafe" until SafeIterators are properly implemented (this will be specified in the docs later after the PR is merged)
Some functions/wrappers/overloads from the STD interface were either removed or not implemented at all due to "lack of importance" or "conflicts related to the new optimist approach" - that means SafeVars are not 1:1 to their STD counterparts (e.g. some swap() and operator= functions were removed, and functions/overloads that deal with ranges and/or some functions/overloads from C++23 forward are technically missing because we're unsure if they're actually necessary in our specific context)
Tests are better now but they do not cover 100% of possibilities - they're definitely covering more than the previous ones, but it's always good to have a second opinion
Probably a benchmark? Changing to an optimist approach should in theory reduce a bit of overhead since we're not doing as many copies as before, would be good to stress test this somehow
This PR contains a plethora of changes made to SafeVars internal functionality. Huge thanks to @fcecin , @itamarcps and @Pedrozo ! This wouldn't have been possible (or even bearable to begin with) if it weren't for your insights and unending patience :sweat_smile:
General overview
Detailed changelog
Hang on to your seats, we're gonna go deep.
General structures
std::unique_ptr<UnderlyingType>
(any non-const operation should do a full copy of the original value before it is changed, assuming there's no copy already made beforehand, indicated bynullptr
)bool
(same idea as the other simple types, but not a pointer because a raw bool takes only 1 byte instead of 8 for a pointer, plus it's just a bool anyway, two possible values, using a pointer would be kinda overkill...)std::unordered_map<Key, std::unique_ptr<UnderlyingType>, SafeHash>>
(any non-const operation should copy only the affected key or range of keys, usingnullptr
to indicate when the key didn't exist at all in the real value before the operation - if the key itself doesn't exist on tmp on the other hand, it means the same key on the real value was unchanged)std::tuple<std::unique_ptr<UnderlyingType>...>
(same idea as SafeUnorderedMap)SafeBase::check()
was removed, which means each non-const function of any given SafeVar is independently handling the tmp value in an isolated manner (this was made on purpose to ensure correct logic but could be optimized further if needed)SafeBase::revert()
is not aconst
function anymore (since bothcommit()
andrevert()
alter member variables anyway)swap()
for SafeString and SafeTuple swaps both values and marks both values as used - actually in hindsight I'm not sure whether this is correct now, but would appreciate a second opinion)nullptr
or an "empty/default" equivalentconst_iterator
s to ensure "safety" is guaranteed (see the exceptions at the end)Undo stacks
vec[0] = "xyz";
, while a full operation changes the value as a whole, e.g.vec.clear();
)UndoOp
structure that contains a specific partial operation done on the real value - SafeArray implements it as astd::tuple<Op, std::size_t, T>
(operation enum, at which index or quantity, and the old value), and SafeVector implements it as astd::tuple<Op, std::size_t, std::size_t, std::vector<T>>
(operation enum, at which index, optionally at which quantity, and one or more old values)std::unique_ptr<std::stack<UndoOp, std::vector<UndoOp>>>
meant to track only the partial operations made in the real valueprocessUndoStack()
function called on revert that takes each partial operation done in the past and applies its "polar opposite" directly to the real value (e.g. the opposite ofinsert(pos)
iserase(pos)
and vice-versa)Commit and revert logic
this->copy_ = nullptr;
)nullptr
or notTests
What is missing
SafeUnorderedMap::find()
,begin()
andend()
currently DO NOT MAKE ANY COPIES of the original value, but weren't removed nor changed to returnconst_iterator
because they are necessary for some contract templates to work - for now those functions are considered "unsafe" until SafeIterators are properly implemented (this will be specified in the docs later after the PR is merged)swap()
andoperator=
functions were removed, and functions/overloads that deal with ranges and/or some functions/overloads from C++23 forward are technically missing because we're unsure if they're actually necessary in our specific context)