feat: add feature flags (#351)

* Impelement feature flags module * Make flags serializable * Print spend_cond * Simplify flag value func * Flag for stricter spending conditions rules that will allow to sync with testnet * Updated readme * Update src/flags/index.js Co-Authored-By: Kosta Korenkov <[email protected]> * Typo
leapdao · Nov 12, 2019 · 224251f · 224251f
1 parent a7aff76
commit 224251f
Show file tree

Hide file tree

Showing 9 changed files with 185 additions and 17 deletions.
diff --git a/README.md b/README.md
@@ -79,6 +79,38 @@ Your local port `9999` forwards now to the remote host on address `127.0.0.1` an
 - `network` — plasma network name
 - `networkId` - network ID. Possible values: `1340` - Leap mainnet, `1341` - Leap testnet.
 - `peers` — array of peers
+- `flagHeights` — `flag → height` mapping to enable feature flags only on certain heights (see [feature flags section](#feature-flags))
+
+### Feature flags
+
+If you want to introduce breaking changes (it can break existing networks) in tx checks you should wrap these changes into condition with feature flag to avoid [resync problems](https://github.com/leapdao/leap-node/issues/334).
+
+```es6
+if (bridgeState.flags.tx_should_fail_on_zero_input) {
+  if (valueOf(input) === 0) {
+    throw new Error('WTF');
+  }
+}
+```
+
+#### How to add a new flag
+
+1. Add it into `FLAGS` array [here](src/flags/index.js#L3)
+2. That’s all, you can use it. It will be `true` by default
+
+#### How to configure feature flags
+
+1. Add `flagHeights` section into the network config
+2. Add activation height:
+```
+{
+  ...,
+  "flagHeights": {
+    "tx_should_fail_on_zero_input": 5000,
+  }
+}
+```
+3. Until this height flag will be `false`, after (inclusively) — `true`
 
 ### Config presets
 

diff --git a/src/bridgeState.js b/src/bridgeState.js
@@ -19,6 +19,7 @@ const singleOperatorABI = require('./abis/singleOperator');
 let operatorABI = require('./abis/operator');
 const proxyABI = require('./abis/proxy');
 const { NFT_COLOR_BASE, NST_COLOR_BASE } = require('./api/methods/constants');
+const flags = require('./flags');
 
 module.exports = class BridgeState {
   constructor(db, privKey, config, relayBuffer) {
@@ -65,6 +66,8 @@ module.exports = class BridgeState {
     this.epochLengths = [];
     this.minGasPrices = [];
 
+    this.flags = flags(this, config.flagHeights);
+
     this.onNewBlock = this.onNewBlock.bind(this);
     this.eventsBuffer = new TinyQueue([], (a, b) => {
       if (a.blockNumber === b.blockNumber) {
@@ -204,7 +207,7 @@ module.exports = class BridgeState {
     );
     this.exitEventSubscription.on('newEvents', this.handleExitingUtxos);
     this.exitEventSubscription.init();
-    
+
     logNode('Synced');
   }
 

diff --git a/src/flags/flags.test.js b/src/flags/flags.test.js
@@ -0,0 +1,76 @@
+const flagsFactory = require('./flagsFactory');
+
+const makeFlags = flagsFactory(['test_flag', 'test_flag_2']);
+
+describe('Feature flags', () => {
+  it('should be true by default', () => {
+    const bridgeState = { blockHeight: 0 };
+    const flags = makeFlags(bridgeState, {});
+
+    expect(flags.test_flag).toBe(true);
+  });
+
+  it('should be false if configured height not reached yet', () => {
+    const bridgeState = { blockHeight: 5 };
+    const flags = makeFlags(bridgeState, {
+      test_flag: 10,
+    });
+
+    expect(flags.test_flag).toBe(false);
+  });
+
+  it('should be true if configured height reached', () => {
+    const bridgeState = { blockHeight: 10 };
+    const flags = makeFlags(bridgeState, {
+      test_flag: 10,
+    });
+
+    expect(flags.test_flag).toBe(true);
+  });
+
+  it('should change value after height change', () => {
+    const bridgeState = { blockHeight: 0 };
+    const flags = makeFlags(bridgeState, {
+      test_flag: 10,
+    });
+
+    expect(flags.test_flag).toBe(false);
+    bridgeState.blockHeight = 10;
+    expect(flags.test_flag).toBe(true);
+  });
+
+  it('should throw on unknown flag', () => {
+    const bridgeState = { blockHeight: 0 };
+    const flags = makeFlags(bridgeState);
+    expect(() => {
+      const value = flags.unknown_flag; // eslint-disable-line no-unused-vars
+    }).toThrow('Unknown feature flag unknown_flag');
+  });
+
+  it('should throw on attempt to set a flag value', () => {
+    const bridgeState = { blockHeight: 0 };
+    const flags = makeFlags(bridgeState);
+    expect(() => {
+      flags.test_flag = true;
+    }).toThrow('Flags are read-only: test_flag');
+  });
+
+  it('should serialize flag values', () => {
+    const bridgeState = { blockHeight: 0 };
+    const flags = makeFlags(bridgeState, {
+      test_flag: 10,
+    });
+
+    bridgeState.blockHeight = 0;
+    expect(JSON.parse(JSON.stringify(flags))).toEqual({
+      test_flag: false,
+      test_flag_2: true,
+    });
+
+    bridgeState.blockHeight = 10;
+    expect(JSON.parse(JSON.stringify(flags))).toEqual({
+      test_flag: true,
+      test_flag_2: true,
+    });
+  });
+});
diff --git a/src/flags/flagsFactory.js b/src/flags/flagsFactory.js
@@ -0,0 +1,31 @@
+/* eslint-disable no-prototype-builtins */
+
+function flagValue(bridgeState, flagHeights, flag) {
+  const targetHeight = flagHeights[flag] || 0;
+  return bridgeState.blockHeight >= targetHeight;
+}
+
+module.exports = (flags = []) => (bridgeState, flagHeights = {}) => {
+  const proxy = new Proxy(flagHeights, {
+    get(target, key) {
+      if (key === 'toJSON') {
+        return () =>
+          flags.reduce((flagValues, flag) => {
+            flagValues[flag] = flagValue(bridgeState, target, flag);
+            return flagValues;
+          }, {});
+      }
+
+      if (!flags.includes(key)) {
+        throw new Error(`Unknown feature flag ${key}`);
+      }
+
+      return flagValue(bridgeState, target, key);
+    },
+    set(_, key) {
+      throw new Error(`Flags are read-only: ${key}`);
+    },
+  });
+
+  return proxy;
+};
diff --git a/src/flags/index.js b/src/flags/index.js
@@ -0,0 +1,9 @@
+const flagsFactory = require('./flagsFactory');
+
+// when adding a flag, add a link to the issue/PR describing the reason for the change
+const FLAGS = [
+  'spend_cond_stricter_rules', // https://github.com/leapdao/leap-node/pull/303
+  'spend_cond_new_bytecode' // https://github.com/leapdao/leap-node/pull/292
+];
+
+module.exports = flagsFactory(FLAGS);
diff --git a/src/tx/applyTx/checkSpendCond.js b/src/tx/applyTx/checkSpendCond.js
@@ -262,12 +262,20 @@ module.exports = async (state, tx, bridgeState, nodeConfig = {}) => {
     let allowance;
 
     if (!spendingIsOwner) {
-      if (input.signer) {
-        if (unspent.address !== input.signer) {
-          throw new Error(
-            `output owner ${unspent.address} unequal input signer: ${input.signer}`
-          );
+      if (input.signer && unspent.address !== input.signer) {
+        throw new Error(
+          `output owner ${unspent.address} unequal input signer: ${input.signer}`
+        );
+      }
+
+      // will throw if allowance is undefined
+      // stricter rule was introduced here
+      // https://github.com/leapdao/leap-node/blob/b534718807214318acaa6a9ff88b9cb8f1780ef1/src/tx/applyTx/checkSpendCond.js#L271
+      if (bridgeState.flags.spend_cond_stricter_rules) {
+        if (input.signer) {
+          allowance = {};
         }
+      } else {
         allowance = {};
       }
     } else {
@@ -281,10 +289,9 @@ module.exports = async (state, tx, bridgeState, nodeConfig = {}) => {
         tokenValueBuf,
         utils.toBuffer(unspent.data),
       ]);
-      bytecode =
-        bridgeState.networkId === 218508104 && bridgeState.blockHeight < 41000
-          ? ERC1948_BYTECODE_218508104
-          : ERC1948_BYTECODE;
+      bytecode = bridgeState.flags.spend_cond_new_bytecode
+        ? ERC1948_BYTECODE
+        : ERC1948_BYTECODE_218508104;
 
       if (allowance) {
         allowance = {
@@ -519,12 +526,15 @@ module.exports = async (state, tx, bridgeState, nodeConfig = {}) => {
       logOuts.push(new Output(amount, owner, deployed[originAddr]));
     }
   });
-  iterateBag(nftBag, (originAddr, owner) => {
-    if (!nftBag[originAddr][owner].touched) {
-      // throw instead of return, because of the cb function
-      throw new Error(`not touched ${nftBag[originAddr][owner].addr}`);
-    }
-  });
+
+  if (bridgeState.flags.spend_cond_stricter_rules) {
+    iterateBag(nftBag, (originAddr, owner) => {
+      if (!nftBag[originAddr][owner].touched) {
+        // throw instead of return, because of the cb function
+        throw new Error(`not touched ${nftBag[originAddr][owner].addr}`);
+      }
+    });
+  }
 
   const gasUsed = BigInt(evmResult.gasUsed);
   // XXX: Fixed gasPrice for now. We include it again in the tx format as the next breaking change.

diff --git a/src/tx/applyTx/checkSpendCond.test.js b/src/tx/applyTx/checkSpendCond.test.js
@@ -2,6 +2,7 @@ const { Tx, Input, Outpoint, Output } = require('leap-core');
 const utils = require('ethereumjs-util');
 const ethers = require('ethers'); // eslint-disable-line import/no-extraneous-dependencies
 const checkSpendCond = require('./checkSpendCond');
+const makeFlags = require('../../flags');
 const {
   NFT_COLOR_BASE,
   NST_COLOR_BASE,
@@ -34,6 +35,9 @@ const bridgeState = {
   logsCache: {},
   web3: new Web3(),
 };
+bridgeState.flags = makeFlags(bridgeState, {
+  spend_cond_new_bytecode: 1000,
+});
 
 const NFTCondition =
   '6080604052348015600f57600080fd5b5060043610602b5760e060020a6000350463d01a81e181146030575b600080fd5b605960048036036040811015604457600080fd5b50600160a060020a038135169060200135605b565b005b6040805160e060020a6323b872dd028152306004820152600160a060020a03841660248201526044810183905290517311111111111111111111111111111111111111119182916323b872dd9160648082019260009290919082900301818387803b15801560c857600080fd5b505af115801560db573d6000803e3d6000fd5b5050505050505056fea165627a7a723058206e658cc8b44fd3d32eef8c4222a0f8773e93bc6efa0fb08e4db77979dacc9e790029';

diff --git a/src/tx/index.js b/src/tx/index.js
@@ -25,7 +25,7 @@ module.exports = (bridgeState, nodeConfig) => async (
     await applyTx(state, tx, bridgeState, nodeConfig);
     accumulateTx(state, tx);
   } catch (err) {
-    logError(err.message);
+    logError(err.message || err);
     throw err;
   }
 

diff --git a/src/txHelpers/printTx.js b/src/txHelpers/printTx.js
@@ -49,6 +49,9 @@ module.exports = (state, tx) => {
         return `periodVote: slotId: ${tx.options.slotId},
          root: ${bufferToHex(tx.inputs[0].prevout.hash)}`;
       }
+      case Type.SPEND_COND: {
+        return 'SPEND_COND';
+      }
       default:
         return undefined;
     }