From e02f0ae8a8a8eea55b488e5a55bdc2a96f6471d1 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Tue, 26 Jul 2022 11:31:57 +0100 Subject: [PATCH 1/5] Make miner slightly more cautious about state --- helpers/test-helper.js | 26 ++- .../reputation-miner/ReputationMinerClient.js | 27 +++- .../ReputationMinerLongTransactionMined.js | 37 +++++ .../client-auto-functionality.js | 149 +++++++++++------- 4 files changed, 173 insertions(+), 66 deletions(-) create mode 100644 packages/reputation-miner/test/ReputationMinerLongTransactionMined.js diff --git a/helpers/test-helper.js b/helpers/test-helper.js index 80fd3291db..c8734c1910 100644 --- a/helpers/test-helper.js +++ b/helpers/test-helper.js @@ -10,6 +10,7 @@ const { UINT256_MAX, MIN_STAKE, MINING_CYCLE_DURATION, DEFAULT_STAKE, CHALLENGE_ const IColony = artifacts.require("IColony"); const IMetaColony = artifacts.require("IMetaColony"); +const IColonyNetwork = artifacts.require("IColonyNetwork"); const ITokenLocking = artifacts.require("ITokenLocking"); const Token = artifacts.require("Token"); const IReputationMiningCycle = artifacts.require("IReputationMiningCycle"); @@ -991,7 +992,12 @@ exports.getColonyEditable = async function getColonyEditable(colony, colonyNetwo exports.getWaitForNSubmissionsPromise = async function getWaitForNSubmissionsPromise(repCycleEthers, rootHash, nLeaves, jrh, n) { return new Promise(function (resolve, reject) { repCycleEthers.on("ReputationRootHashSubmitted", async (_miner, _hash, _nLeaves, _jrh, _entryIndex, event) => { - const nSubmissions = await repCycleEthers.getNSubmissionsForHash(rootHash, nLeaves, jrh); + let nSubmissions; + if (rootHash) { + nSubmissions = await repCycleEthers.getNSubmissionsForHash(rootHash, nLeaves, jrh); + } else { + nSubmissions = await repCycleEthers.getNSubmissionsForHash(_hash, _nLeaves, _jrh); + } if (nSubmissions.toNumber() >= n) { event.removeListener(); resolve(); @@ -1007,6 +1013,24 @@ exports.getWaitForNSubmissionsPromise = async function getWaitForNSubmissionsPro }); }; +exports.getMiningCycleCompletePromise = async function getMiningCycleCompletePromise(colonyNetworkEthers, oldHash, expectedHash) { + return new Promise(function (resolve, reject) { + colonyNetworkEthers.on("ReputationMiningCycleComplete", async (_hash, _nLeaves, event) => { + const colonyNetwork = await IColonyNetwork.at(colonyNetworkEthers.address); + const newHash = await colonyNetwork.getReputationRootHash(); + expect(newHash).to.not.equal(oldHash, "The old and new hashes are the same"); + expect(newHash).to.equal(expectedHash, "The network root hash doens't match the one submitted"); + event.removeListener(); + resolve(); + }); + + // After 30s, we throw a timeout error + setTimeout(() => { + reject(new Error("ERROR: timeout while waiting for confirming hash")); + }, 30000); + }); +}; + exports.encodeTxData = async function encodeTxData(colony, functionName, args) { const convertedArgs = []; args.forEach((arg) => { diff --git a/packages/reputation-miner/ReputationMinerClient.js b/packages/reputation-miner/ReputationMinerClient.js index d594d416d7..228fc12ba9 100644 --- a/packages/reputation-miner/ReputationMinerClient.js +++ b/packages/reputation-miner/ReputationMinerClient.js @@ -223,6 +223,7 @@ class ReputationMinerClient { */ async initialise(colonyNetworkAddress, startingBlock) { this.resolveBlockChecksFinished = undefined; + this.startingBlock = startingBlock; await this._miner.initialise(colonyNetworkAddress); let resumedSuccessfully = false; @@ -277,7 +278,7 @@ class ReputationMinerClient { await this._miner.loadState(latestConfirmedReputationHash); if (this._miner.nReputations.eq(0)) { this._adapter.log("Latest state not found - need to sync"); - await this._miner.sync(startingBlock, true); + await this._miner.sync(this.startingBlock, true); } // Initial call to process the existing log from the cycle we're currently in await this.processReputationLog(); @@ -388,6 +389,19 @@ class ReputationMinerClient { return; } + // First check is the confirmed cycle the one we expect? + // Note no blocktags in these calls - we care if we're up-to-date, not the historical state (here) + // If we're not, we back out and resync + const nLeaves = await this._miner.colonyNetwork.getReputationRootHashNNodes(); + const currentRootHash = await this._miner.colonyNetwork.getReputationRootHash(); + const localRootHash = await this._miner.reputationTree.getRootHash(); + if (!nLeaves.eq(this._miner.nReputations) || localRootHash !== currentRootHash) { + this._adapter.log(`Unexpected confirmed hash seen on colonyNetwork. Let's resync.`) + await this._miner.sync(this.startingBlock, true); + this.endDoBlockChecks(); + return; + } + await this._miner.updatePeriodLength(repCycle); await this.processReputationLog(); @@ -426,6 +440,8 @@ class ReputationMinerClient { const gasPrice = await updateGasEstimate("average", this.chainId, this._adapter); await this._miner.setGasPrice(gasPrice); await this.submitEntry(entryIndex); + this.endDoBlockChecks(); + return; } } } @@ -453,7 +469,10 @@ class ReputationMinerClient { if (oppSubmission.proposedNewRootHash === ethers.constants.AddressZero){ const responsePossible = await repCycle.getResponsePossible(disputeStages.INVALIDATE_HASH, entry.lastResponseTimestamp); - if (!responsePossible) { return; } + if (!responsePossible) { + this.endDoBlockChecks(); + return; + } // Then we don't have an opponent if (round.eq(0)) { // We can only advance if the window is closed @@ -645,9 +664,9 @@ class ReputationMinerClient { } - async processReputationLog() { + async processReputationLog(blockNumber) { this._adapter.log("📁 Processing reputation update log"); - await this._miner.addLogContentsToReputationTree(); + await this._miner.addLogContentsToReputationTree(blockNumber); this._adapter.log("💾 Writing new reputation state to database"); await this._miner.saveCurrentState(); this._adapter.log("💾 Caching justification tree to disk"); diff --git a/packages/reputation-miner/test/ReputationMinerLongTransactionMined.js b/packages/reputation-miner/test/ReputationMinerLongTransactionMined.js new file mode 100644 index 0000000000..dd3052196d --- /dev/null +++ b/packages/reputation-miner/test/ReputationMinerLongTransactionMined.js @@ -0,0 +1,37 @@ +const ethers = require("ethers"); +const ReputationMinerTestWrapper = require("./ReputationMinerTestWrapper"); + +class ReputationMinerLongTransactionMined extends ReputationMinerTestWrapper { + // Only difference between this and the 'real' client should be that submitRootHash + // doesn't resolve until we tell it to, via resolveSubmission() + + async submitRootHash(entryIndex) { + const hash = await this.getRootHash(); + const nLeaves = await this.getRootHashNLeaves(); + const jrh = await this.justificationTree.getRootHash(); + const repCycle = await this.getActiveRepCycle(); + + if (!entryIndex) { + entryIndex = await this.getEntryIndex(); // eslint-disable-line no-param-reassign + } + let gasEstimate = ethers.BigNumber.from(1000000); + try { + gasEstimate = await repCycle.estimate.submitRootHash(hash, nLeaves, jrh, entryIndex); + } catch (err) { // eslint-disable-line no-empty + + } + + // Submit that entry + this.p = new Promise((resolve) => { + this.result = repCycle.submitRootHash(hash, nLeaves, jrh, entryIndex, { gasLimit: gasEstimate, gasPrice: this.gasPrice }); + this.resolve = resolve; + }) + return this.p; + } + + async resolveSubmission() { + this.resolve(this.result); + } +} + +module.exports = ReputationMinerLongTransactionMined; diff --git a/test/reputation-system/reputation-mining-client/client-auto-functionality.js b/test/reputation-system/reputation-mining-client/client-auto-functionality.js index 05f4ea9efe..9f79d8ea5d 100644 --- a/test/reputation-system/reputation-mining-client/client-auto-functionality.js +++ b/test/reputation-system/reputation-mining-client/client-auto-functionality.js @@ -18,6 +18,7 @@ const { finishReputationMiningCycle, currentBlock, getWaitForNSubmissionsPromise, + getMiningCycleCompletePromise, TestAdapter, } = require("../../../helpers/test-helper"); const { @@ -30,6 +31,7 @@ const { const ReputationMinerClient = require("../../../packages/reputation-miner/ReputationMinerClient"); const ReputationMinerTestWrapper = require("../../../packages/reputation-miner/test/ReputationMinerTestWrapper"); const MaliciousReputationMinerExtraRep = require("../../../packages/reputation-miner/test/MaliciousReputationMinerExtraRep"); +const ReputationMinerLongTransactionMined = require("../../../packages/reputation-miner/test/ReputationMinerLongTransactionMined"); const { expect } = chai; chai.use(bnChai(web3.utils.BN)); @@ -104,7 +106,7 @@ process.env.SOLIDITY_COVERAGE afterEach(async function () { await reputationMinerClient.close(); const reputationMiningGotClean = await finishReputationMiningCycle(colonyNetwork, this); - if (!reputationMiningGotClean) await setupNewNetworkInstance(MINER1, MINER2); + if (!reputationMiningGotClean) await setupNewNetworkInstance(MINER1, MINER2, MINER3); }); describe("core functionality", function () { @@ -122,21 +124,7 @@ process.env.SOLIDITY_COVERAGE await forwardTime(MINING_CYCLE_DURATION * 0.9, this); await receive12Submissions; - const colonyNetworkEthers = await reputationMinerClient._miner.colonyNetwork; - const miningCycleComplete = new Promise(function (resolve, reject) { - colonyNetworkEthers.on("ReputationMiningCycleComplete", async (_hash, _nLeaves, event) => { - const newHash = await colonyNetwork.getReputationRootHash(); - expect(newHash).to.not.equal(oldHash, "The old and new hashes are the same"); - expect(newHash).to.equal(rootHash, "The network root hash doens't match the one submitted"); - event.removeListener(); - resolve(); - }); - - // After 30s, we throw a timeout error - setTimeout(() => { - reject(new Error("ERROR: timeout while waiting for confirming hash")); - }, 30000); - }); + const miningCycleComplete = getMiningCycleCompletePromise(reputationMinerClient._miner.colonyNetwork, oldHash, rootHash); // Forward time to the end of the mining cycle and since we are the only miner, check the client confirmed our hash correctly await forwardTime(MINING_CYCLE_DURATION * 0.1 + CHALLENGE_RESPONSE_WINDOW_DURATION + 1, this); @@ -169,21 +157,7 @@ process.env.SOLIDITY_COVERAGE await forwardTime(MINING_CYCLE_DURATION * 0.9, this); await receive12Submissions; - const colonyNetworkEthers = await reputationMinerClient._miner.colonyNetwork; - const miningCycleComplete = new Promise(function (resolve, reject) { - colonyNetworkEthers.on("ReputationMiningCycleComplete", async (_hash, _nLeaves, event) => { - const newHash = await colonyNetwork.getReputationRootHash(); - expect(newHash).to.not.equal(oldHash, "The old and new hashes are the same"); - expect(newHash).to.equal(rootHash, "The network root hash doens't match the one submitted"); - event.removeListener(); - resolve(); - }); - - // After 30s, we throw a timeout error - setTimeout(() => { - reject(new Error("ERROR: timeout while waiting for confirming hash")); - }, 30000); - }); + const miningCycleComplete = getMiningCycleCompletePromise(reputationMinerClient._miner.colonyNetwork, oldHash, rootHash); let oracleCheckInterval; @@ -253,21 +227,7 @@ process.env.SOLIDITY_COVERAGE await mineBlock(); await receive12Submissions; - const colonyNetworkEthers = await reputationMinerClient._miner.colonyNetwork; - const miningCycleComplete = new Promise(function (resolve, reject) { - colonyNetworkEthers.on("ReputationMiningCycleComplete", async (_hash, _nLeaves, event) => { - const newHash = await colonyNetwork.getReputationRootHash(); - expect(newHash).to.not.equal(oldHash, "The old and new hashes are the same"); - expect(newHash).to.equal(rootHash, "The network root hash doens't match the one submitted"); - event.removeListener(); - resolve(); - }); - - // After 30s, we throw a timeout error - setTimeout(() => { - reject(new Error("ERROR: timeout while waiting for confirming hash")); - }, 30000); - }); + const miningCycleComplete = getMiningCycleCompletePromise(reputationMinerClient._miner.colonyNetwork, oldHash, rootHash); // Forward time to the end of the mining cycle and since we are the only miner, check the client confirmed our hash correctly await forwardTime(MINING_CYCLE_DURATION * 0.6 + MINING_CYCLE_DURATION, this); @@ -293,21 +253,7 @@ process.env.SOLIDITY_COVERAGE await checkSuccessEthers(goodClient.submitRootHash()); await receive12Submissions; - const colonyNetworkEthers = await reputationMinerClient._miner.colonyNetwork; - const miningCycleComplete = new Promise(function (resolve, reject) { - colonyNetworkEthers.on("ReputationMiningCycleComplete", async (_hash, _nLeaves, event) => { - const newHash = await colonyNetwork.getReputationRootHash(); - expect(newHash).to.not.equal(oldHash, "The old and new hashes are the same"); - expect(newHash).to.equal(rootHash, "The network root hash doens't match the one submitted"); - event.removeListener(); - resolve(); - }); - - // After 30s, we throw a timeout error - setTimeout(() => { - reject(new Error("ERROR: timeout while waiting for confirming hash")); - }, 60000); - }); + const miningCycleComplete = getMiningCycleCompletePromise(reputationMinerClient._miner.colonyNetwork, oldHash, rootHash); // Forward time to the end of the mining cycle and since we are the only miner, check the client confirmed our hash correctly await forwardTime(MINING_CYCLE_DURATION / 2 + CHALLENGE_RESPONSE_WINDOW_DURATION + 1, this); @@ -769,6 +715,87 @@ process.env.SOLIDITY_COVERAGE await reputationMinerClient2.close(); }); + it(`should continue to mine successfully even if the submission hash takes a long time to be mined + (e.g. because it ran out of funds)`, async function () { + let repCycleEthers = await reputationMinerClient._miner.getActiveRepCycle(); + // Advance through a reputation cycle + let rootHash = await reputationMinerClient._miner.getRootHash(); + + let receive12Submissions = getWaitForNSubmissionsPromise(repCycleEthers, null, null, null, 12); + + const delayedReputationMinerClient = new ReputationMinerClient({ + loader, + realProviderPort, + minerAddress: MINER2, + useJsTree: true, + auto: true, + oracle: false, + }); + // That client is fine until we give it an awkward miner + delayedReputationMinerClient._miner = new ReputationMinerLongTransactionMined({ + minerAddress: MINER2, + loader, + realProviderPort, + useJsTree: true, + }); + + await delayedReputationMinerClient.initialise(colonyNetwork.address, startingBlockNumber); + + // Forward through most of the cycle duration and wait for the clients to submit all 12 allowed entries + await forwardTime(MINING_CYCLE_DURATION * 0.9, this); + await receive12Submissions; + + let oldHash = await colonyNetwork.getReputationRootHash(); + + let miningCycleComplete = getMiningCycleCompletePromise(reputationMinerClient._miner.colonyNetwork, oldHash, rootHash); + + // Forward time to the end of the mining cycle and since we are the only miner, check the client confirmed our hash correctly + await forwardTime(MINING_CYCLE_DURATION * 0.1 + CHALLENGE_RESPONSE_WINDOW_DURATION + 1, this); + await miningCycleComplete; + + // Advance through another - the client should still be waiting for the first transaction to return. + repCycleEthers = await reputationMinerClient._miner.getActiveRepCycle(); + + reputationMinerClient.blocksSinceCycleCompleted = 10; + await mineBlock(); + + receive12Submissions = getWaitForNSubmissionsPromise(repCycleEthers, null, null, null, 12); + + await forwardTime(MINING_CYCLE_DURATION * 0.9, this); + await receive12Submissions; + + rootHash = await reputationMinerClient._miner.getRootHash(); + oldHash = await colonyNetwork.getReputationRootHash(); + miningCycleComplete = getMiningCycleCompletePromise(reputationMinerClient._miner.colonyNetwork, oldHash, rootHash); + + await forwardTime(MINING_CYCLE_DURATION * 0.1 + CHALLENGE_RESPONSE_WINDOW_DURATION + 1, this); + await miningCycleComplete; + + // We now resolve the original + delayedReputationMinerClient._miner.resolveSubmission(); + + // And then we get both clients to process the newest cycle. The good miner will update normally. + // In the case of the delayedReputationMinerClient, we expect it to recognise something has gone + // wrong, and resync. + delayedReputationMinerClient.blocksSinceCycleCompleted = 10; + reputationMinerClient.blocksSinceCycleCompleted = 10; + await mineBlock(); + repCycleEthers = await reputationMinerClient._miner.getActiveRepCycle(); + + receive12Submissions = getWaitForNSubmissionsPromise(repCycleEthers, null, null, null, 12); + + await forwardTime(MINING_CYCLE_DURATION * 0.9, this); + await receive12Submissions; + + // check delayed miner and good miner have ended up in the same state. + const rootHash2 = await reputationMinerClient._miner.getRootHash(); + const rootHash3 = await delayedReputationMinerClient._miner.getRootHash(); + expect(rootHash2).to.equal(rootHash3); + + delayedReputationMinerClient._miner.resolveSubmission(); + await delayedReputationMinerClient.close(); + }); + function noEventSeen(contract, event) { return new Promise(function (resolve, reject) { contract.on(event, async () => { From ff446d99ec4e2f55e648df6462a947415ce55672 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Mon, 8 Aug 2022 15:26:44 +0100 Subject: [PATCH 2/5] Slight refactoring post-review --- .../reputation-miner/ReputationMinerClient.js | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/packages/reputation-miner/ReputationMinerClient.js b/packages/reputation-miner/ReputationMinerClient.js index 228fc12ba9..fa103e030a 100644 --- a/packages/reputation-miner/ReputationMinerClient.js +++ b/packages/reputation-miner/ReputationMinerClient.js @@ -391,13 +391,9 @@ class ReputationMinerClient { // First check is the confirmed cycle the one we expect? // Note no blocktags in these calls - we care if we're up-to-date, not the historical state (here) - // If we're not, we back out and resync - const nLeaves = await this._miner.colonyNetwork.getReputationRootHashNNodes(); - const currentRootHash = await this._miner.colonyNetwork.getReputationRootHash(); - const localRootHash = await this._miner.reputationTree.getRootHash(); - if (!nLeaves.eq(this._miner.nReputations) || localRootHash !== currentRootHash) { - this._adapter.log(`Unexpected confirmed hash seen on colonyNetwork. Let's resync.`) - await this._miner.sync(this.startingBlock, true); + // If we're not, we resync and stop here for this block. + const syncCheck = await this.ensureSynced(); + if (!syncCheck) { this.endDoBlockChecks(); return; } @@ -664,6 +660,18 @@ class ReputationMinerClient { } + async ensureSynced() { + const nLeaves = await this._miner.colonyNetwork.getReputationRootHashNNodes(); + const currentRootHash = await this._miner.colonyNetwork.getReputationRootHash(); + const localRootHash = await this._miner.reputationTree.getRootHash(); + if (!nLeaves.eq(this._miner.nReputations) || localRootHash !== currentRootHash) { + this._adapter.log(`Unexpected confirmed hash seen on colonyNetwork. Let's resync.`) + await this._miner.sync(this.startingBlock, true); + return false; + } + return true; + } + async processReputationLog(blockNumber) { this._adapter.log("📁 Processing reputation update log"); await this._miner.addLogContentsToReputationTree(blockNumber); From 6d7af66f3b914fc606e17a403300ef5b7c516a9a Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Wed, 10 Aug 2022 17:18:09 +0100 Subject: [PATCH 3/5] Last part of first review --- helpers/test-helper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpers/test-helper.js b/helpers/test-helper.js index c8734c1910..3f8b07a5a9 100644 --- a/helpers/test-helper.js +++ b/helpers/test-helper.js @@ -993,7 +993,7 @@ exports.getWaitForNSubmissionsPromise = async function getWaitForNSubmissionsPro return new Promise(function (resolve, reject) { repCycleEthers.on("ReputationRootHashSubmitted", async (_miner, _hash, _nLeaves, _jrh, _entryIndex, event) => { let nSubmissions; - if (rootHash) { + if (rootHash === _hash) { nSubmissions = await repCycleEthers.getNSubmissionsForHash(rootHash, nLeaves, jrh); } else { nSubmissions = await repCycleEthers.getNSubmissionsForHash(_hash, _nLeaves, _jrh); From d77bfcf8e3fe714e07976bbbd820b3a325f4b022 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Wed, 17 Aug 2022 15:40:46 +0100 Subject: [PATCH 4/5] Add explanation to getWaitForNSubmissionsPromise --- helpers/test-helper.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/helpers/test-helper.js b/helpers/test-helper.js index 3f8b07a5a9..cc3a392cfe 100644 --- a/helpers/test-helper.js +++ b/helpers/test-helper.js @@ -993,6 +993,9 @@ exports.getWaitForNSubmissionsPromise = async function getWaitForNSubmissionsPro return new Promise(function (resolve, reject) { repCycleEthers.on("ReputationRootHashSubmitted", async (_miner, _hash, _nLeaves, _jrh, _entryIndex, event) => { let nSubmissions; + // If we've passed in a hash, we check how many submissions that hash in particular has + // If not, and we're just waiting for N submissions from any hash, we lookup the number of + // submissions the hash in the emitted event that triggered this function has if (rootHash === _hash) { nSubmissions = await repCycleEthers.getNSubmissionsForHash(rootHash, nLeaves, jrh); } else { From 3891cf56aa3f3fe7464762fdbc82b66816114565 Mon Sep 17 00:00:00 2001 From: Daniel Kronovet Date: Tue, 23 Aug 2022 09:20:42 -0400 Subject: [PATCH 5/5] Update explanation for getWaitForNSubmissionsPromise --- helpers/test-helper.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helpers/test-helper.js b/helpers/test-helper.js index cc3a392cfe..6ff247b66c 100644 --- a/helpers/test-helper.js +++ b/helpers/test-helper.js @@ -993,10 +993,10 @@ exports.getWaitForNSubmissionsPromise = async function getWaitForNSubmissionsPro return new Promise(function (resolve, reject) { repCycleEthers.on("ReputationRootHashSubmitted", async (_miner, _hash, _nLeaves, _jrh, _entryIndex, event) => { let nSubmissions; - // If we've passed in a hash, we check how many submissions that hash in particular has - // If not, and we're just waiting for N submissions from any hash, we lookup the number of - // submissions the hash in the emitted event that triggered this function has - if (rootHash === _hash) { + // We want to see when our hash hits N submissions + // If we've passed in our hash, we check how many submissions that hash has + // If not, we're waiting for N submissions from any hash + if (rootHash) { nSubmissions = await repCycleEthers.getNSubmissionsForHash(rootHash, nLeaves, jrh); } else { nSubmissions = await repCycleEthers.getNSubmissionsForHash(_hash, _nLeaves, _jrh);