SafeCore now stores only recent state roots #546

schemar · 2018-12-13T08:45:06Z

The safe core now only tracks a fixed number of most recent state roots.
The number of state roots to track can be set on construction.
The safe core has a ring buffer that tracks the last n state roots and
from n + 1 starts overwriting the oldest state root whenever a new
state root is committed.

I created a separate contract for the circular buffer in order to
decouple the concept of a circular buffer and the concept of a state
root storage. There were three options of how to add the circular buffer
logic to the state root:

Deploy the buffer as a separate contract (on core construction) and
call to that contract for storage.
Use dependency injection and pass the address of the buffer when
constructing the core.
Extend the buffer with the core to make the logic available.
Make the buffer a library.

Option 4. was quickly ruled out, as the storage part of the buffer is
essential to its logic and inner workings.

Options 1. and 2. are somewhat similar. They very well decouple the
contracts and make testing easy. From an architectural point of view,
option 2. is the easy choice. However, in solidity it does cost actual
money to split contracts and make external calls. So that's the
drawback.

Option 3. puts everything into a single contract and makes all calls to
the buffer internal, which is good from an economical point of view. The
drawback is of cours that the contracts are very tightly coupled and
testing is a pain, as a wrapper contract must be written for the buffer
to "externalize" its methods.

After a discussion with @benjaminbollen and @pgev, I was convinced to
choose the economically more viable approach over the better
architectural approach. Core now extends the buffer and makes internal
calls to it. I decided to leave the contract name in front of all the
method names to make it clear to the reader of the contract when the
buffer is used.

I added tests for the circular buffer and also added a test specific to
the state roots to the safe core.

Fixes #504

@benjaminbollen

The safe core now only tracks a fixed number of most recent state roots. The number of state roots to track can be set on construction. The safe core has a ring buffer that tracks the last `n` state roots and from `n + 1` starts overwriting the oldest state root whenever a new state root is committed. I created a separate contract for the circular buffer in order to decouple the concept of a circular buffer and the concept of a state root storage. There were three options of how to add the circular buffer logic to the state root: 1. Deploy the buffer as a separate contract (on core construction) and call to that contract for storage. 2. Use dependency injection and pass the address of the buffer when constructing the core. 3. Extend the buffer with the core to make the logic available. 4. Make the buffer a library. Option 4. was quickly ruled out, as the storage part of the buffer is essential to its logic and inner workings. Options 1. and 2. are somewhat similar. They very well decouple the contracts and make testing easy. From an architectural point of view, option 2. is the easy choice. However, in solidity it does cost actual money to split contracts and make external calls. So that's the drawback. Option 3. puts everything into a single contract and makes all calls to the buffer internal, which is good from an economical point of view. The drawback is of cours that the contracts are very tightly coupled and testing is a pain, as a wrapper contract must be written for the buffer to "externalize" its methods. After a discussion with @benjaminbollen and @pgev, I was convinced to choose the economically more viable approach over the better architectural approach. Core now extends the buffer and makes `internal` calls to it. I decided to leave the contract name in front of all the method names to make it clear to the reader of the contract when the buffer is used. I added tests for the circular buffer and also added a test specific to the state roots to the safe core. Fixes OpenST#504

0xsarvesh

Very Clean and elegant design 💯 🥇
Look really good to me 🔥 ⚡️

test/lib/circular_buffer_uint/constructor.js

gulshanvasnani

Great work 👍 💯
Few comments. Please see inline.

contracts/lib/CircularBufferUint.sol

test/gateway/gateway_base/prove_gateway.js

contracts/test/lib/TestCircularBufferUint.sol

gulshanvasnani · 2018-12-17T06:34:34Z

contracts/gateway/SafeCore.sol

            "Given block height is lower or equal to highest committed state root block height."
        );

        stateRoots[_blockHeight] = _stateRoot;
-        latestStateRootBlockHeight = _blockHeight;
+        uint256 oldestStoredBlockHeight = CircularBufferUint.store(_blockHeight);
+        delete stateRoots[oldestStoredBlockHeight];

        emit StateRootAvailable(_blockHeight, _stateRoot);


I think to emit an event for block height which is overridden.It will ensure from JS layer we wont call getStateRoot for the block height which is deleted.
What do you think ?
@sarvesh-ost @deepesh-kn

Could be helpful. But at the same time, the JS layer should switch to a newer state root as it becomes available. Or is that not always feasible?

Yes,it is feasible.

test/lib/circular_buffer_uint/constructor.js

deepesh-kn

🤝 🙌 Yes, logic is nice.
Just a few inline comments.

deepesh-kn · 2018-12-17T14:10:06Z

contracts/gateway/SafeCore.sol

@@ -38,7 +39,7 @@ import "../lib/SafeMath.sol";
 *         by the workers that are registered as part of the `Organized`
 *         interface.
 */
-contract SafeCore is StateRootInterface, Organized {
+contract SafeCore is StateRootInterface, Organized, CircularBufferUint {
    using SafeMath for uint256;


As per style guide use the following

/* Usings */

All section are declared as /** abc */.
eg /** Events */, we need to change all to /* abc */

Done and done 👍

contracts/gateway/SafeCore.sol

contracts/lib/CircularBufferUint.sol

contracts/test/core/MockSafeCore.sol

contracts/test/lib/TestCircularBufferUint.sol

schemar

All updated, please check 👍

schemar · 2018-12-17T16:28:58Z

contracts/gateway/SafeCore.sol

@@ -38,7 +39,7 @@ import "../lib/SafeMath.sol";
 *         by the workers that are registered as part of the `Organized`
 *         interface.
 */
-contract SafeCore is StateRootInterface, Organized {
+contract SafeCore is StateRootInterface, Organized, CircularBufferUint {
    using SafeMath for uint256;


Done and done 👍

contracts/gateway/SafeCore.sol

schemar · 2018-12-17T16:31:08Z

contracts/gateway/SafeCore.sol

            "Given block height is lower or equal to highest committed state root block height."
        );

        stateRoots[_blockHeight] = _stateRoot;
-        latestStateRootBlockHeight = _blockHeight;
+        uint256 oldestStoredBlockHeight = CircularBufferUint.store(_blockHeight);
+        delete stateRoots[oldestStoredBlockHeight];

        emit StateRootAvailable(_blockHeight, _stateRoot);


Could be helpful. But at the same time, the JS layer should switch to a newer state root as it becomes available. Or is that not always feasible?

contracts/lib/CircularBufferUint.sol

contracts/test/core/MockSafeCore.sol

contracts/test/lib/TestCircularBufferUint.sol

test/lib/circular_buffer_uint/constructor.js

gulshanvasnani · 2018-12-18T07:23:55Z

LGTM 👍
Thank you for the changes.

deepesh-kn

LGTM 🎉

schemar self-assigned this Dec 13, 2018

schemar requested review from deepesh-kn, 0xsarvesh and gulshanvasnani December 13, 2018 08:45

schemar force-pushed the core_storage branch from 2816334 to 8d9aef2 Compare December 13, 2018 11:45

schemar added 2 commits December 14, 2018 14:11

Merge branch 'develop' into core_storage

53496cc

Added documentation to the circular buffer test

015414b

schemar force-pushed the core_storage branch from 9177015 to 015414b Compare December 14, 2018 13:16

0xsarvesh approved these changes Dec 17, 2018

View reviewed changes

test/lib/circular_buffer_uint/constructor.js Show resolved Hide resolved

test/lib/circular_buffer_uint/constructor.js Show resolved Hide resolved

gulshanvasnani reviewed Dec 17, 2018

View reviewed changes

deepesh-kn reviewed Dec 17, 2018

View reviewed changes

schemar added 2 commits December 17, 2018 17:26

Merge branch 'develop' into core_storage

407116f

Updated code based on PR feedback

4246972

schemar force-pushed the core_storage branch from c164c7d to 4246972 Compare December 17, 2018 16:42

schemar commented Dec 17, 2018

View reviewed changes

deepesh-kn approved these changes Dec 18, 2018

View reviewed changes

deepesh-kn merged commit c6b7d09 into OpenST:develop Dec 18, 2018

schemar deleted the core_storage branch December 18, 2018 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SafeCore now stores only recent state roots #546

SafeCore now stores only recent state roots #546

schemar commented Dec 13, 2018 •

edited

Loading

0xsarvesh left a comment

gulshanvasnani left a comment

gulshanvasnani Dec 17, 2018

schemar Dec 17, 2018

gulshanvasnani Dec 18, 2018

deepesh-kn left a comment

deepesh-kn Dec 17, 2018

deepesh-kn Dec 17, 2018

schemar Dec 17, 2018

schemar left a comment

schemar Dec 17, 2018

schemar Dec 17, 2018

gulshanvasnani commented Dec 18, 2018 •

edited

Loading

deepesh-kn left a comment

SafeCore now stores only recent state roots #546

SafeCore now stores only recent state roots #546

Conversation

schemar commented Dec 13, 2018 • edited Loading

0xsarvesh left a comment

Choose a reason for hiding this comment

gulshanvasnani left a 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

deepesh-kn left a 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

schemar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gulshanvasnani commented Dec 18, 2018 • edited Loading

deepesh-kn left a comment

Choose a reason for hiding this comment

schemar commented Dec 13, 2018 •

edited

Loading

gulshanvasnani commented Dec 18, 2018 •

edited

Loading