feat[contracts]: Replace Lib_RingBuffer with a much simpler Lib_Buffer (#821)

* feat[contracts]: replace Lib_RingBuffer with a simpler Lib_Buffer

* chore: changeset

* test: add tests for Lib_Buffer

* lint: fix

* test: add extra coverage for Lib_Buffer

* Update packages/contracts/contracts/optimistic-ethereum/libraries/utils/Lib_Buffer.sol
Co-authored-by: ben-chain <ben@pseudonym.party>

* add some extra comments
Co-authored-by: ben-chain <ben@pseudonym.party>

.changeset/fair-donuts-lick.md

+---
+'@eth-optimism/contracts': patch
+---
+
+Replaces RingBuffer with a simpler Buffer library

packages/contracts/contracts/optimistic-ethereum/OVM/chain/OVM_ChainStorageContainer.sol

@@ -2,7 +2,7 @@
 pragma solidity >0.5.0 <0.8.0;
 
 /* Library Imports */
-import { Lib_RingBuffer } from "../../libraries/utils/Lib_RingBuffer.sol";
+import { Lib_Buffer } from "../../libraries/utils/Lib_Buffer.sol";
 import { Lib_AddressResolver } from "../../libraries/resolver/Lib_AddressResolver.sol";
 
 /* Interface Imports */
@@ -28,7 +28,7 @@ contract OVM_ChainStorageContainer is iOVM_ChainStorageContainer, Lib_AddressRes
      * Libraries *
      *************/
 
-    using Lib_RingBuffer for Lib_RingBuffer.RingBuffer;
+    using Lib_Buffer for Lib_Buffer.Buffer;
 
 
     /*************
@@ -36,7 +36,7 @@ contract OVM_ChainStorageContainer is iOVM_ChainStorageContainer, Lib_AddressRes
      *************/
 
     string public owner;
-    Lib_RingBuffer.RingBuffer internal buffer;
+    Lib_Buffer.Buffer internal buffer;
 
 
     /***************
@@ -189,17 +189,4 @@ contract OVM_ChainStorageContainer is iOVM_ChainStorageContainer, Lib_AddressRes
             _globalMetadata
         );
     }
-
-    /**
-     * @inheritdoc iOVM_ChainStorageContainer
-     */
-    function setNextOverwritableIndex(
-        uint256 _index
-    )
-        override
-        public
-        onlyOwner
-    {
-        buffer.nextOverwritableIndex = _index;
-    }
 }

packages/contracts/contracts/optimistic-ethereum/iOVM/chain/iOVM_ChainStorageContainer.sol

@@ -99,13 +99,4 @@ interface iOVM_ChainStorageContainer {
         bytes27 _globalMetadata
     )
         external;
-
-    /**
-     * Marks an index as overwritable, meaing the underlying buffer can start to write values over
-     * any objects before and including the given index.
-     */
-    function setNextOverwritableIndex(
-        uint256 _index
-    )
-        external;
 }

packages/contracts/contracts/optimistic-ethereum/libraries/utils/Lib_RingBuffer.sol → packages/contracts/contracts/optimistic-ethereum/libraries/utils/Lib_Buffer.sol

 // SPDX-License-Identifier: MIT
 pragma solidity >0.5.0 <0.8.0;
 
-library Lib_RingBuffer {
-    using Lib_RingBuffer for RingBuffer;
+/**
+ * @title Lib_Buffer
+ * @dev This library implements a bytes32 storage array with some additional gas-optimized
+ * functionality. In particular, it encodes its length as a uint40, and tightly packs this with an
+ * overwritable "extra data" field so we can store more information with a single SSTORE.
+ */
+library Lib_Buffer {
+
+    /*************
+     * Libraries *
+     *************/
+
+    using Lib_Buffer for Buffer;
+
 
     /***********
      * Structs *
      ***********/
 
     struct Buffer {
-        uint256 length;
+        bytes32 context;
         mapping (uint256 => bytes32) buf;
     }
 
-    struct RingBuffer {
-        bytes32 contextA;
-        bytes32 contextB;
-        Buffer bufferA;
-        Buffer bufferB;
-        uint256 nextOverwritableIndex;
-    }
+    struct BufferContext {
+        // Stores the length of the array. Uint40 is way more elements than we'll ever reasonably
+        // need in an array and we get an extra 27 bytes of extra data to play with.
+        uint40 length;
 
-    struct RingBufferContext {
-        // contextA
-        uint40 globalIndex;
+        // Arbitrary extra data that can be modified whenever the length is updated. Useful for
+        // squeezing out some gas optimizations.
         bytes27 extraData;
-
-        // contextB
-        uint64 currBufferIndex;
-        uint40 prevResetIndex;
-        uint40 currResetIndex;
     }
 
 
-    /*************
-     * Constants *
-     *************/
-
-    uint256 constant MIN_CAPACITY = 16;
-
-
     /**********************
      * Internal Functions *
      **********************/
@@ -48,46 +44,21 @@ library Lib_RingBuffer {
      * Pushes a single element to the buffer.
      * @param _self Buffer to access.
      * @param _value Value to push to the buffer.
-     * @param _extraData Optional global extra data.
+     * @param _extraData Global extra data.
      */
     function push(
-        RingBuffer storage _self,
+        Buffer storage _self,
         bytes32 _value,
         bytes27 _extraData
     )
         internal
     {
-        RingBufferContext memory ctx = _self.getContext();
-        Buffer storage currBuffer = _self.getBuffer(ctx.currBufferIndex);
-
-        // Set a minimum capacity.
-        if (currBuffer.length == 0) {
-            currBuffer.length = MIN_CAPACITY;
-        }
-
-        // Check if we need to expand the buffer.
-        if (ctx.globalIndex - ctx.currResetIndex >= currBuffer.length) {
-            if (ctx.currResetIndex < _self.nextOverwritableIndex) {
-                // We're going to overwrite the inactive buffer.
-                // Bump the buffer index, reset the delete offset, and set our reset indices.
-                ctx.currBufferIndex++;
-                ctx.prevResetIndex = ctx.currResetIndex;
-                ctx.currResetIndex = ctx.globalIndex;
-
-                // Swap over to the next buffer.
-                currBuffer = _self.getBuffer(ctx.currBufferIndex);
-            } else {
-                // We're not overwriting yet, double the length of the current buffer.
-                currBuffer.length *= 2;
-            }
-        }
+        BufferContext memory ctx = _self.getContext();
 
-        // Index to write to is the difference of the global and reset indices.
-        uint256 writeHead = ctx.globalIndex - ctx.currResetIndex;
-        currBuffer.buf[writeHead] = _value;
+        _self.buf[ctx.length] = _value;
 
         // Bump the global index and insert our extra data, then save the context.
-        ctx.globalIndex++;
+        ctx.length++;
         ctx.extraData = _extraData;
         _self.setContext(ctx);
     }
@@ -98,12 +69,12 @@ library Lib_RingBuffer {
      * @param _value Value to push to the buffer.
      */
     function push(
-        RingBuffer storage _self,
+        Buffer storage _self,
         bytes32 _value
     )
         internal
     {
-        RingBufferContext memory ctx = _self.getContext();
+        BufferContext memory ctx = _self.getContext();
 
         _self.push(
             _value,
@@ -118,7 +89,7 @@ library Lib_RingBuffer {
      * @return Value of the element at the given index.
      */
     function get(
-        RingBuffer storage _self,
+        Buffer storage _self,
         uint256 _index
     )
         internal
@@ -127,47 +98,14 @@ library Lib_RingBuffer {
             bytes32
         )
     {
-        RingBufferContext memory ctx = _self.getContext();
+        BufferContext memory ctx = _self.getContext();
 
         require(
-            _index < ctx.globalIndex,
+            _index < ctx.length,
             "Index out of bounds."
         );
 
-        Buffer storage currBuffer = _self.getBuffer(ctx.currBufferIndex);
-        Buffer storage prevBuffer = _self.getBuffer(ctx.currBufferIndex + 1);
-
-        if (_index >= ctx.currResetIndex) {
-            // We're trying to load an element from the current buffer.
-            // Relative index is just the difference from the reset index.
-            uint256 relativeIndex = _index - ctx.currResetIndex;
-
-            // Shouldn't happen but why not check.
-            require(
-                relativeIndex < currBuffer.length,
-                "Index out of bounds."
-            );
-
-            return currBuffer.buf[relativeIndex];
-        } else {
-            // We're trying to load an element from the previous buffer.
-            // Relative index is the difference from the reset index in the other direction.
-            uint256 relativeIndex = ctx.currResetIndex - _index;
-
-            // Condition only fails in the case that we deleted and flipped buffers.
-            require(
-                ctx.currResetIndex > ctx.prevResetIndex,
-                "Index out of bounds."
-            );
-
-            // Make sure we're not trying to read beyond the array.
-            require(
-                relativeIndex <= prevBuffer.length,
-                "Index out of bounds."
-            );
-
-            return prevBuffer.buf[prevBuffer.length - relativeIndex];
-        }
+        return _self.buf[_index];
     }
 
     /**
@@ -177,29 +115,21 @@ library Lib_RingBuffer {
      * @param _extraData Optional global extra data.
      */
     function deleteElementsAfterInclusive(
-        RingBuffer storage _self,
+        Buffer storage _self,
         uint40 _index,
         bytes27 _extraData
     )
         internal
     {
-        RingBufferContext memory ctx = _self.getContext();
+        BufferContext memory ctx = _self.getContext();
 
         require(
-            _index < ctx.globalIndex && _index >= ctx.prevResetIndex,
+            _index < ctx.length,
             "Index out of bounds."
         );
 
-        if (_index < ctx.currResetIndex) {
-            // We're switching back to the previous buffer.
-            // Reduce the buffer index, set the current reset index back to match the previous one.
-            // We use the equality of these two values to prevent reading beyond this buffer.
-            ctx.currBufferIndex--;
-            ctx.currResetIndex = ctx.prevResetIndex;
-        }
-
-        // Set our global index and extra data, save the context.
-        ctx.globalIndex = _index;
+        // Set our length and extra data, save the context.
+        ctx.length = _index;
         ctx.extraData = _extraData;
         _self.setContext(ctx);
     }
@@ -210,12 +140,12 @@ library Lib_RingBuffer {
      * @param _index Index of the element to delete from (inclusive).
      */
     function deleteElementsAfterInclusive(
-        RingBuffer storage _self,
+        Buffer storage _self,
         uint40 _index
     )
         internal
     {
-        RingBufferContext memory ctx = _self.getContext();
+        BufferContext memory ctx = _self.getContext();
         _self.deleteElementsAfterInclusive(
             _index,
             ctx.extraData
@@ -228,7 +158,7 @@ library Lib_RingBuffer {
      * @return Current global index.
      */
     function getLength(
-        RingBuffer storage _self
+        Buffer storage _self
     )
         internal
         view
@@ -236,8 +166,8 @@ library Lib_RingBuffer {
             uint40
         )
     {
-        RingBufferContext memory ctx = _self.getContext();
-        return ctx.globalIndex;
+        BufferContext memory ctx = _self.getContext();
+        return ctx.length;
     }
 
     /**
@@ -246,12 +176,12 @@ library Lib_RingBuffer {
      * @param _extraData New global extra data.
      */
     function setExtraData(
-        RingBuffer storage _self,
+        Buffer storage _self,
         bytes27 _extraData
     )
         internal
     {
-        RingBufferContext memory ctx = _self.getContext();
+        BufferContext memory ctx = _self.getContext();
         ctx.extraData = _extraData;
         _self.setContext(ctx);
     }
@@ -262,7 +192,7 @@ library Lib_RingBuffer {
      * @return Current global extra data.
      */
     function getExtraData(
-        RingBuffer storage _self
+        Buffer storage _self
     )
         internal
         view
@@ -270,107 +200,59 @@ library Lib_RingBuffer {
             bytes27
         )
     {
-        RingBufferContext memory ctx = _self.getContext();
+        BufferContext memory ctx = _self.getContext();
         return ctx.extraData;
     }
 
     /**
-     * Sets the current ring buffer context.
+     * Sets the current buffer context.
      * @param _self Buffer to access.
-     * @param _ctx Current ring buffer context.
+     * @param _ctx Current buffer context.
      */
     function setContext(
-        RingBuffer storage _self,
-        RingBufferContext memory _ctx
+        Buffer storage _self,
+        BufferContext memory _ctx
     )
         internal
     {
-        bytes32 contextA;
-        bytes32 contextB;
-
-        uint40 globalIndex = _ctx.globalIndex;
+        bytes32 context;
+        uint40 length = _ctx.length;
         bytes27 extraData = _ctx.extraData;
         assembly {
-            contextA := globalIndex
-            contextA := or(contextA, extraData)
-        }
-
-        uint64 currBufferIndex = _ctx.currBufferIndex;
-        uint40 prevResetIndex = _ctx.prevResetIndex;
-        uint40 currResetIndex = _ctx.currResetIndex;
-        assembly {
-            contextB := currBufferIndex
-            contextB := or(contextB, shl(64, prevResetIndex))
-            contextB := or(contextB, shl(104, currResetIndex))
-        }
-
-        if (_self.contextA != contextA) {
-            _self.contextA = contextA;
+            context := length
+            context := or(context, extraData)
         }
 
-        if (_self.contextB != contextB) {
-            _self.contextB = contextB;
+        if (_self.context != context) {
+            _self.context = context;
         }
     }
 
     /**
-     * Retrieves the current ring buffer context.
+     * Retrieves the current buffer context.
      * @param _self Buffer to access.
-     * @return Current ring buffer context.
+     * @return Current buffer context.
      */
     function getContext(
-        RingBuffer storage _self
+        Buffer storage _self
     )
         internal
         view
         returns (
-            RingBufferContext memory
+            BufferContext memory
         )
     {
-        bytes32 contextA = _self.contextA;
-        bytes32 contextB = _self.contextB;
-
-        uint40 globalIndex;
+        bytes32 context = _self.context;
+        uint40 length;
         bytes27 extraData;
         assembly {
-            globalIndex := and(contextA, 0x000000000000000000000000000000000000000000000000000000FFFFFFFFFF)
-            extraData   := and(contextA, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF0000000000)
-        }
-
-        uint64 currBufferIndex;
-        uint40 prevResetIndex;
-        uint40 currResetIndex;
-        assembly {
-            currBufferIndex :=          and(contextB, 0x000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFF)
-            prevResetIndex  := shr(64,  and(contextB, 0x00000000000000000000000000000000000000FFFFFFFFFF0000000000000000))
-            currResetIndex  := shr(104, and(contextB, 0x0000000000000000000000000000FFFFFFFFFF00000000000000000000000000))
+            length    := and(context, 0x000000000000000000000000000000000000000000000000000000FFFFFFFFFF)
+            extraData := and(context, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF0000000000)
         }
 
-        return RingBufferContext({
-            globalIndex: globalIndex,
-            extraData: extraData,
-            currBufferIndex: currBufferIndex,
-            prevResetIndex: prevResetIndex,
-            currResetIndex: currResetIndex
+        return BufferContext({
+            length: length,
+            extraData: extraData
         });
     }
-
-    /**
-     * Retrieves the a buffer from the ring buffer by index.
-     * @param _self Buffer to access.
-     * @param _which Index of the sub buffer to access.
-     * @return Sub buffer for the index.
-     */
-    function getBuffer(
-        RingBuffer storage _self,
-        uint256 _which
-    )
-        internal
-        view
-        returns (
-            Buffer storage
-        )
-    {
-        return _which % 2 == 0 ? _self.bufferA : _self.bufferB;
-    }
 }

packages/contracts/contracts/test-libraries/utils/TestLib_RingBuffer.sol → packages/contracts/contracts/test-libraries/utils/TestLib_Buffer.sol

@@ -3,15 +3,15 @@ pragma solidity >0.5.0 <0.8.0;
 pragma experimental ABIEncoderV2;
 
 /* Library Imports */
-import { Lib_RingBuffer } from "../../optimistic-ethereum/libraries/utils/Lib_RingBuffer.sol";
+import { Lib_Buffer } from "../../optimistic-ethereum/libraries/utils/Lib_Buffer.sol";
 
 /**
- * @title TestLib_RingBuffer
+ * @title TestLib_Buffer
  */
-contract TestLib_RingBuffer {
-    using Lib_RingBuffer for Lib_RingBuffer.RingBuffer;
+contract TestLib_Buffer {
+    using Lib_Buffer for Lib_Buffer.Buffer;
 
-    Lib_RingBuffer.RingBuffer internal buf;
+    Lib_Buffer.Buffer internal buf;
 
     function push(
         bytes32 _value,
@@ -37,11 +37,21 @@ contract TestLib_RingBuffer {
         return buf.get(_index);
     }
 
+    function deleteElementsAfterInclusive(
+        uint40 _index
+    )
+        public
+    {
+        return buf.deleteElementsAfterInclusive(
+            _index
+        );
+    }
+
     function deleteElementsAfterInclusive(
         uint40 _index,
         bytes27 _extraData
     )
-        internal
+        public
     {
         return buf.deleteElementsAfterInclusive(
             _index,
@@ -50,7 +60,7 @@ contract TestLib_RingBuffer {
     }
 
     function getLength()
-        internal
+        public
         view
         returns (
             uint40
@@ -59,8 +69,18 @@ contract TestLib_RingBuffer {
         return buf.getLength();
     }
 
+    function setExtraData(
+        bytes27 _extraData
+    )
+        public
+    {
+        return buf.setExtraData(
+            _extraData
+        );
+    }
+
     function getExtraData()
-        internal
+        public
         view
         returns (
             bytes27

packages/contracts/test/contracts/libraries/utils/Lib_Buffer.spec.ts

+import { expect } from '../../../setup'
+
+import hre from 'hardhat'
+import { Contract, ethers } from 'ethers'
+
+describe('Lib_Buffer', () => {
+  let Lib_Buffer: Contract
+  beforeEach(async () => {
+    const Factory__Lib_Buffer = await hre.ethers.getContractFactory(
+      'TestLib_Buffer'
+    )
+    Lib_Buffer = await Factory__Lib_Buffer.deploy()
+  })
+
+  describe('push', () => {
+    for (const len of [1, 2, 4, 8, 32]) {
+      it(`it should be able to add ${len} element(s) to the array`, async () => {
+        for (let i = 0; i < len; i++) {
+          await expect(
+            Lib_Buffer.push(
+              ethers.utils.keccak256(`0x${i.toString(16).padStart(16, '0')}`),
+              `0x${'00'.repeat(27)}`
+            )
+          ).to.not.be.reverted
+        }
+      })
+    }
+  })
+
+  describe('get', () => {
+    for (const len of [1, 2, 4, 8, 32]) {
+      describe(`when the array has ${len} element(s)`, () => {
+        const values = []
+        beforeEach(async () => {
+          for (let i = 0; i < len; i++) {
+            const value = ethers.utils.keccak256(
+              `0x${i.toString(16).padStart(16, '0')}`
+            )
+            values.push(value)
+            await Lib_Buffer.push(value, `0x${'00'.repeat(27)}`)
+          }
+        })
+
+        for (let i = 0; i < len; i += Math.max(1, len / 4)) {
+          it(`should be able to get the ${i}th/st/rd/whatever value`, async () => {
+            expect(await Lib_Buffer.get(i)).to.equal(values[i])
+          })
+        }
+
+        it('should throw if attempting to access an element that does not exist', async () => {
+          await expect(Lib_Buffer.get(len + 1)).to.be.reverted
+        })
+      })
+    }
+  })
+
+  describe('getLength', () => {
+    it('should return zero by default', async () => {
+      expect(await Lib_Buffer.getLength()).to.equal(0)
+    })
+
+    for (const len of [1, 2, 4, 8, 32]) {
+      describe(`when the array has ${len} element(s)`, () => {
+        const values = []
+        beforeEach(async () => {
+          for (let i = 0; i < len; i++) {
+            const value = ethers.utils.keccak256(
+              `0x${i.toString(16).padStart(16, '0')}`
+            )
+            values.push(value)
+            await Lib_Buffer.push(value, `0x${'00'.repeat(27)}`)
+          }
+        })
+
+        it(`should return a value of ${len}`, async () => {
+          expect(await Lib_Buffer.getLength()).to.equal(len)
+        })
+      })
+    }
+  })
+
+  describe('getExtraData', () => {
+    it('should be bytes27(0) by default', async () => {
+      expect(await Lib_Buffer.getExtraData()).to.equal(`0x${'00'.repeat(27)}`)
+    })
+
+    it('should change if set by a call to push()', async () => {
+      const extraData = `0x${'11'.repeat(27)}`
+      await Lib_Buffer.push(ethers.utils.keccak256('0x00'), extraData)
+
+      expect(await Lib_Buffer.getExtraData()).to.equal(extraData)
+    })
+
+    it('should change if set multiple times', async () => {
+      await Lib_Buffer.push(
+        ethers.utils.keccak256('0x00'),
+        `0x${'11'.repeat(27)}`
+      )
+
+      const extraData = `0x${'22'.repeat(27)}`
+
+      await Lib_Buffer.push(ethers.utils.keccak256('0x00'), extraData)
+
+      expect(await Lib_Buffer.getExtraData()).to.equal(extraData)
+    })
+  })
+
+  describe('setExtraData', () => {
+    it('should modify the extra data', async () => {
+      const extraData = `0x${'11'.repeat(27)}`
+      await Lib_Buffer.setExtraData(extraData)
+
+      expect(await Lib_Buffer.getExtraData()).to.equal(extraData)
+    })
+
+    it('should be able to modify the extra data multiple times', async () => {
+      const extraData1 = `0x${'22'.repeat(27)}`
+      await Lib_Buffer.setExtraData(extraData1)
+      expect(await Lib_Buffer.getExtraData()).to.equal(extraData1)
+
+      const extraData2 = `0x${'11'.repeat(27)}`
+      await Lib_Buffer.setExtraData(extraData2)
+
+      expect(await Lib_Buffer.getExtraData()).to.equal(extraData2)
+    })
+  })
+
+  describe('deleteElementsAfterInclusive', () => {
+    it('should revert when the array is empty', async () => {
+      await expect(Lib_Buffer['deleteElementsAfterInclusive(uint40)'](0)).to.be
+        .reverted
+    })
+
+    for (const len of [1, 2, 4, 8, 32]) {
+      describe(`when the array has ${len} element(s)`, () => {
+        const values = []
+        beforeEach(async () => {
+          for (let i = 0; i < len; i++) {
+            const value = ethers.utils.keccak256(
+              `0x${i.toString(16).padStart(16, '0')}`
+            )
+            values.push(value)
+            await Lib_Buffer.push(value, `0x${'00'.repeat(27)}`)
+          }
+        })
+
+        for (let i = len - 1; i > 0; i -= Math.max(1, len / 4)) {
+          it(`should be able to delete everything after and including the ${i}th/st/rd/whatever element`, async () => {
+            await expect(Lib_Buffer['deleteElementsAfterInclusive(uint40)'](i))
+              .to.not.be.reverted
+
+            expect(await Lib_Buffer.getLength()).to.equal(i)
+            await expect(Lib_Buffer.get(i)).to.be.reverted
+          })
+        }
+
+        for (let i = len - 1; i > 0; i -= Math.max(1, len / 4)) {
+          it(`should be able to delete after and incl. ${i}th/st/rd/whatever element while changing extra data`, async () => {
+            const extraData = `0x${i.toString(16).padStart(54, '0')}`
+            await expect(
+              Lib_Buffer['deleteElementsAfterInclusive(uint40,bytes27)'](
+                i,
+                extraData
+              )
+            ).to.not.be.reverted
+
+            expect(await Lib_Buffer.getLength()).to.equal(i)
+            await expect(Lib_Buffer.get(i)).to.be.reverted
+            expect(await Lib_Buffer.getExtraData()).to.equal(extraData)
+          })
+        }
+      })
+    }
+  })
+})