Skip to content

Commit

Permalink
SafeCore now stores only recent state roots
Browse files Browse the repository at this point in the history
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 #504
  • Loading branch information
schemar committed Dec 13, 2018
1 parent 697156d commit 2816334
Show file tree
Hide file tree
Showing 8 changed files with 386 additions and 29 deletions.
33 changes: 18 additions & 15 deletions contracts/gateway/SafeCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pragma solidity ^0.5.0;

import "./WorkersInterface.sol";
import "../StateRootInterface.sol";
import "../lib/CircularBufferUint.sol";
import "../lib/IsMemberInterface.sol";
import "../lib/MerklePatriciaProof.sol";
import "../lib/Organized.sol";
Expand All @@ -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;


Expand All @@ -58,31 +59,32 @@ contract SafeCore is StateRootInterface, Organized {
*/
uint256 private remoteChainId;

/** Latest block height of block for which state root was committed. */
uint256 private latestStateRootBlockHeight;

/** Address of the core on the auxiliary chain. Can be zero. */
address public coCore;


/* Constructor */

/**
* @notice Contract constructor.
* @notice Contract constructor.
*
* @param _remoteChainId _remoteChainId is the chain id of the chain that
* this core tracks.
* @param _blockHeight Block height at which _stateRoot needs to store.
* @param _stateRoot State root hash of given _blockHeight.
* @param _membersManager Address of a members manager contract.
* @param _remoteChainId _remoteChainId is the chain id of the chain that
* this core tracks.
* @param _blockHeight Block height at which _stateRoot needs to store.
* @param _stateRoot State root hash of given _blockHeight.
* @param _maxStateRoots The max number of state roots to store in the
* circular buffer.
* @param _membersManager Address of a members manager contract.
*/
constructor(
uint256 _remoteChainId,
uint256 _blockHeight,
bytes32 _stateRoot,
uint256 _maxStateRoots,
IsMemberInterface _membersManager
)
Organized(_membersManager)
CircularBufferUint(_maxStateRoots)
public
{
require(
Expand All @@ -92,8 +94,8 @@ contract SafeCore is StateRootInterface, Organized {

remoteChainId = _remoteChainId;

latestStateRootBlockHeight = _blockHeight;
stateRoots[latestStateRootBlockHeight] = _stateRoot;
stateRoots[_blockHeight] = _stateRoot;
CircularBufferUint.store(_blockHeight);
}


Expand Down Expand Up @@ -152,7 +154,7 @@ contract SafeCore is StateRootInterface, Organized {
view
returns (uint256 height_)
{
height_ = latestStateRootBlockHeight;
height_ = CircularBufferUint.head();
}

/**
Expand Down Expand Up @@ -180,12 +182,13 @@ contract SafeCore is StateRootInterface, Organized {

// Input block height should be valid
require(
_blockHeight > latestStateRootBlockHeight,
_blockHeight > CircularBufferUint.head(),
"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);

Expand Down
107 changes: 107 additions & 0 deletions contracts/lib/CircularBufferUint.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
pragma solidity ^0.5.0;

// Copyright 2017 OpenST Ltd.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
// ----------------------------------------------------------------------------
//
// http://www.simpletoken.org/
//
// ----------------------------------------------------------------------------

contract CircularBufferUint {


/** Storage */

/**
* The circular buffer that stores the latest `items.length` items. Once
* `items.length` items were stored, items will be overwritten starting at
* zero.
*/
uint256[] private items;

/**
* The current index in the items array. The index increases up to
* `items.length - 1` and then resets to zero in an endless loop. This
* means that a new item will always overwrite the oldest item.
*/
uint256 private index;


/* Constructor */

/**
* @notice Create a new buffer with the size `_maxItems`.
*
* @param _maxItems Defines how many items this buffer stores before
* overwriting older items.
*/
constructor(uint256 _maxItems) public {
require(
_maxItems > 0,
"The max number of items to store in a circular buffer must be greater than 0."
);

items.length = _maxItems;
}


/* Internal functions */

/**
* @notice Store a new item in the circular buffer.
*
* @param _item The item to store in the circular buffer.
*
* @return overwrittenItem_ The item that was in the circular buffer's
* position where the new item is now stored. The
* overwritten item is no longer available in the
* circular buffer.
*/
function store(uint256 _item) internal returns(uint256 overwrittenItem_) {
nextIndex();

/*
* Retrieve the old item from the circular buffer before overwriting it
* with the new item.
*/
overwrittenItem_ = items[index];
items[index] = _item;
}

/**
* @notice Get the most recent item that was stored in the circular buffer.
*
* @return head_ The most recently stored item.
*/
function head() internal view returns(uint256 head_) {
head_ = items[index];
}


/* Private functions */

/**
* @notice Updates the index of the circular buffer to point to the next
* slot of where to store an item. Resets to zero if it gets to the
* end of the array that represents the circular.
*/
function nextIndex() private {
index++;
if (index == items.length) {
index = 0;
}
}
}
16 changes: 10 additions & 6 deletions contracts/test/core/MockSafeCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,28 @@ contract MockSafeCore is SafeCore {
/* Public functions */

/**
* @notice Contract constructor
* @notice Contract constructor.
*
* @param _remoteChainId _remoteChainId is the chain id of the chain that
* this core tracks.
* @param _blockHeight block height at which _stateRoot needs to store.
* @param _stateRoot state root hash of given _blockHeight.
* @param _membersManager Address of a members manager contract.
* @param _remoteChainId _remoteChainId is the chain id of the chain that
* this core tracks.
* @param _blockHeight Block height at which _stateRoot needs to store.
* @param _stateRoot State root hash of given _blockHeight.
* @param _maxStateRoots The max number of state roots to store in the
* circular buffer.
* @param _membersManager Address of a members manager contract.
*/
constructor(
uint256 _remoteChainId,
uint256 _blockHeight,
bytes32 _stateRoot,
uint256 _maxStateRoots,
IsMemberInterface _membersManager
)
SafeCore(
_remoteChainId,
_blockHeight,
_stateRoot,
_maxStateRoots,
_membersManager
)
public
Expand Down
64 changes: 64 additions & 0 deletions contracts/test/lib/TestCircularBufferUint.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
pragma solidity ^0.5.0;

// Copyright 2017 OpenST Ltd.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
// ----------------------------------------------------------------------------
//
// http://www.simpletoken.org/
//
// ----------------------------------------------------------------------------

import "../../lib/CircularBufferUint.sol";

contract TestCircularBufferUint is CircularBufferUint {


/* Constructor */

/**
* @notice Create a new test buffer with the size `_maxItems`.
*
* @param _maxItems Defines how many items this test buffer stores before
* overwriting older items.
*/
constructor(uint256 _maxItems) public CircularBufferUint(_maxItems) {}


/* Internal functions */

/**
* @notice Store a new item in the circular test buffer.
*
* @param _item The item to store in the circular test buffer.
*
* @return overwrittenItem_ The item that was in the circular test buffer's
* position where the new item is now stored. The
* overwritten item is no longer available in the
* circular test buffer.
*/
function storeExternal(uint256 _item) external returns(uint256 overwrittenItem_) {
overwrittenItem_ = CircularBufferUint.store(_item);
}

/**
* @notice Get the most recent item that was stored in the circular test
* buffer.
*
* @return head_ The most recently stored item.
*/
function headExternal() external view returns(uint256 head_) {
head_ = CircularBufferUint.head();
}
}
Loading

0 comments on commit 2816334

Please sign in to comment.