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 OpenST#504
  • Loading branch information
schemar committed Dec 13, 2018
1 parent 697156d commit 7f415b6
Show file tree
Hide file tree
Showing 8 changed files with 338 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
81 changes: 81 additions & 0 deletions contracts/lib/CircularBufferUint.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
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 */

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 */

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;
}

function head() internal view returns(uint256 head_) {
head_ = items[index];
}


/* Private functions */

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
42 changes: 42 additions & 0 deletions contracts/test/lib/TestCircularBufferUint.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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 */

constructor(uint256 _maxItems) public CircularBufferUint(_maxItems) {}


/* Internal functions */

function storeExternal(uint256 _item) external returns(uint256 overwrittenItem_) {
overwrittenItem_ = CircularBufferUint.store(_item);
}

function headExternal() external view returns(uint256 head_) {
head_ = CircularBufferUint.head();
}
}
76 changes: 70 additions & 6 deletions test/gateway/safe_core.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,28 @@ contract('Core', function (accounts) {
});

describe('commitStateRoot', async () => {
// Before all.
before(async () => {
let core;
let worker;
let blockHeight;
let stateRoot;
let maxNumberOfStateRoots;

beforeEach(async () => {
blockHeight = 5;
contractsData = await coreUtils.deployCore(artifacts, accounts);
contractsData = await coreUtils.deployCore(
artifacts,
accounts,
blockHeight,
);
core = contractsData.core;
worker = contractsData.worker;
stateRoot = proof.account.stateRoot;
maxNumberOfStateRoots = contractsData.numberOfStateRoots;
});

it('should be able to commit state root and getStateRoot for given block height', async () => {
blockHeight++;

let response = await core.commitStateRoot.call(
blockHeight,
stateRoot,
Expand Down Expand Up @@ -151,27 +163,79 @@ contract('Core', function (accounts) {
assert.equal(latestStateRootBlockHeight.toNumber(), blockHeight);
});

it('should store only the given number of max store roots', async () => {
/*
* It should store the given state roots and they should be
* available for querying afterwards. After the max number of state
* roots has been exceeded, the old state roots should no longer be
* available.
*/

let iterations = maxNumberOfStateRoots * 2;
for (let i = 0; i < iterations; i++) {
blockHeight++;
await core.commitStateRoot(
blockHeight,
stateRoot,
{ from: worker },
);

// Check that the older state root has been deleted.
if (i > maxNumberOfStateRoots) {
let prunedBlockHeight = blockHeight - maxNumberOfStateRoots;
let storedStateRoot = await core.getStateRoot.call(
new BN(prunedBlockHeight),
);

assert.strictEqual(
storedStateRoot,
'0x0000000000000000000000000000000000000000000000000000000000000000',
'There should not be any state root stored at a ' +
'pruned height. It should have been reset by now.',
);

/*
* The state root that is one block younger than the pruned
* one should still be available.
*/
let existingBlockHeight = prunedBlockHeight + 1;
storedStateRoot = await core.getStateRoot.call(
new BN(existingBlockHeight),
);
assert.strictEqual(
storedStateRoot,
stateRoot,
'The stored state root should still exist.',
);

}
}
});

it('should not be able to commit state root of block height which is equal to latest block height', async () => {
await utils.expectThrow(
core.commitStateRoot(blockHeight, stateRoot, { from: worker }),
);
});

it('should not be able to commit state root of block height which is less than latest block height', async () => {
blockHeight--;
await utils.expectThrow(
core.commitStateRoot(3, stateRoot, { from: worker })
core.commitStateRoot(blockHeight, stateRoot, { from: worker })
);
});

it('should not be able to commit state root of block height if non worker commits root', async () => {
blockHeight++;
await utils.expectThrow(
core.commitStateRoot(6, stateRoot, { from: accounts[0] })
core.commitStateRoot(blockHeight, stateRoot, { from: accounts[0] })
);
});

it('should not be able to commit state root when state root is empty', async () => {
blockHeight++;
await utils.expectThrow(
core.commitStateRoot(6, '0x', { from: worker })
core.commitStateRoot(blockHeight, '0x', { from: worker })
);
});

Expand Down
Loading

0 comments on commit 7f415b6

Please sign in to comment.