Merge pull request #7339 from LiskHQ/7299-invalid-transactions-should…

…-not-make-state-changes

Invalid transactions should not make state changes - Closes #7299

commander/package.json

@@ -94,7 +94,7 @@
 		"@liskhq/lisk-chain": "^0.4.0-alpha.0",
 		"@liskhq/lisk-codec": "^0.3.0-alpha.0",
 		"@liskhq/lisk-cryptography": "^4.0.0-alpha.0",
-		"@liskhq/lisk-db": "^0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27",
+		"@liskhq/lisk-db": "0.3.0-debug.20-b876469836935925369a887fe03e50a67c26c38f",
 		"@liskhq/lisk-passphrase": "^4.0.0-alpha.0",
 		"@liskhq/lisk-transactions": "^6.0.0-alpha.0",
 		"@liskhq/lisk-utils": "^0.3.0-alpha.0",

elements/lisk-chain/package.json

@@ -37,7 +37,7 @@
 	"dependencies": {
 		"@liskhq/lisk-codec": "^0.3.0-alpha.0",
 		"@liskhq/lisk-cryptography": "^4.0.0-alpha.0",
-		"@liskhq/lisk-db": "^0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27",
+		"@liskhq/lisk-db": "0.3.0-debug.20-b876469836935925369a887fe03e50a67c26c38f",
 		"@liskhq/lisk-tree": "^0.3.0-alpha.0",
 		"@liskhq/lisk-utils": "^0.3.0-alpha.0",
 		"@liskhq/lisk-validator": "^0.7.0-alpha.0",

elements/lisk-elements/package.json

@@ -39,7 +39,7 @@
 		"@liskhq/lisk-chain": "^0.4.0-alpha.0",
 		"@liskhq/lisk-codec": "^0.3.0-alpha.0",
 		"@liskhq/lisk-cryptography": "^4.0.0-alpha.0",
-		"@liskhq/lisk-db": "^0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27",
+		"@liskhq/lisk-db": "0.3.0-debug.20-b876469836935925369a887fe03e50a67c26c38f",
 		"@liskhq/lisk-p2p": "^0.8.0-alpha.0",
 		"@liskhq/lisk-passphrase": "^4.0.0-alpha.0",
 		"@liskhq/lisk-transaction-pool": "^0.6.0-alpha.0",

framework/package.json

@@ -45,7 +45,7 @@
 		"@liskhq/lisk-chain": "^0.4.0-alpha.0",
 		"@liskhq/lisk-codec": "^0.3.0-alpha.0",
 		"@liskhq/lisk-cryptography": "^4.0.0-alpha.0",
-		"@liskhq/lisk-db": "^0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27",
+		"@liskhq/lisk-db": "0.3.0-debug.20-b876469836935925369a887fe03e50a67c26c38f",
 		"@liskhq/lisk-p2p": "^0.8.0-alpha.0",
 		"@liskhq/lisk-transaction-pool": "^0.6.0-alpha.0",
 		"@liskhq/lisk-transactions": "^6.0.0-alpha.0",

framework/src/state_machine/event_queue.ts

@@ -21,7 +21,6 @@ interface RevertibleEvent {
 
 export class EventQueue {
 	private readonly _events: RevertibleEvent[];
-	private _snapshotIndex = -1;
 
 	public constructor() {
 		this._events = [];
@@ -59,23 +58,22 @@ export class EventQueue {
 		});
 	}
 
-	public createSnapshot(): void {
-		this._snapshotIndex = this._events.length;
+	public createSnapshot(): number {
+		return this._events.length;
 	}
 
-	public restoreSnapshot(): void {
-		const newEvents = this._events.splice(this._snapshotIndex);
+	public restoreSnapshot(snapshotID: number): void {
+		const newEvents = this._events.splice(snapshotID);
 		const nonRevertableEvents = newEvents
 			.filter(eventData => eventData.noRevert)
 			.map((eventData, i) => ({
 				event: new Event({
 					...eventData.event.toObject(),
-					index: this._snapshotIndex + i,
+					index: snapshotID + i,
 				}),
 				noRevert: false,
 			}));
 		this._events.push(...nonRevertableEvents);
-		this._snapshotIndex = -1;
 	}
 
 	public getEvents(): Event[] {

framework/src/state_machine/prefixed_state_read_writer.ts

@@ -21,8 +21,8 @@ export interface StateDBReadWriter {
 	set(key: Buffer, value: Buffer): Promise<void>;
 	del(key: Buffer): Promise<void>;
 	range(options?: IterateOptions): Promise<{ key: Buffer; value: Buffer }[]>;
-	snapshot(): void;
-	restoreSnapshot(): void;
+	snapshot(): number;
+	restoreSnapshot(snapshotID: number): void;
 }
 
 interface KeyValue {
@@ -117,12 +117,12 @@ export class PrefixedStateReadWriter {
 		}));
 	}
 
-	public createSnapshot(): void {
-		this._readWriter.snapshot();
+	public createSnapshot(): number {
+		return this._readWriter.snapshot();
 	}
 
-	public restoreSnapshot(): void {
-		this._readWriter.restoreSnapshot();
+	public restoreSnapshot(snapshotID: number): void {
+		this._readWriter.restoreSnapshot(snapshotID);
 	}
 
 	private _getKey(key: Buffer): Buffer {

framework/src/state_machine/state_machine.ts

@@ -143,13 +143,16 @@ export class StateMachine {
 	public async executeTransaction(ctx: TransactionContext): Promise<TransactionExecutionResult> {
 		let status = TransactionExecutionResult.OK;
 		const transactionContext = ctx.createTransactionExecuteContext();
+		const eventQueueSnapshotID = ctx.eventQueue.createSnapshot();
+		const stateStoreSnapshotID = ctx.stateStore.createSnapshot();
 		for (const mod of this._systemModules) {
 			if (mod.beforeCommandExecute) {
 				try {
 					await mod.beforeCommandExecute(transactionContext);
 				} catch (error) {
-					status = TransactionExecutionResult.INVALID;
-					return status;
+					ctx.eventQueue.restoreSnapshot(eventQueueSnapshotID);
+					ctx.stateStore.restoreSnapshot(stateStoreSnapshotID);
+					return TransactionExecutionResult.INVALID;
 				}
 			}
 		}
@@ -158,15 +161,16 @@ export class StateMachine {
 				try {
 					await mod.beforeCommandExecute(transactionContext);
 				} catch (error) {
-					status = TransactionExecutionResult.INVALID;
-					return status;
+					ctx.eventQueue.restoreSnapshot(eventQueueSnapshotID);
+					ctx.stateStore.restoreSnapshot(stateStoreSnapshotID);
+					return TransactionExecutionResult.INVALID;
 				}
 			}
 		}
 		const command = this._getCommand(ctx.transaction.moduleID, ctx.transaction.commandID);
 		// Execute command
-		ctx.eventQueue.createSnapshot();
-		ctx.stateStore.createSnapshot();
+		const commandEventQueueSnapshotID = ctx.eventQueue.createSnapshot();
+		const commandStateStoreSnapshotID = ctx.stateStore.createSnapshot();
 		const commandContext = ctx.createCommandExecuteContext(command.schema);
 		try {
 			await command.execute(commandContext);
@@ -177,8 +181,8 @@ export class StateMachine {
 				[ctx.transaction.id],
 			);
 		} catch (error) {
-			ctx.eventQueue.restoreSnapshot();
-			ctx.stateStore.restoreSnapshot();
+			ctx.eventQueue.restoreSnapshot(commandEventQueueSnapshotID);
+			ctx.stateStore.restoreSnapshot(commandStateStoreSnapshotID);
 			ctx.eventQueue.add(
 				ctx.transaction.moduleID,
 				EVENT_STANDARD_TYPE_ID,
@@ -194,8 +198,9 @@ export class StateMachine {
 				try {
 					await mod.afterCommandExecute(transactionContext);
 				} catch (error) {
-					status = TransactionExecutionResult.INVALID;
-					return status;
+					ctx.eventQueue.restoreSnapshot(eventQueueSnapshotID);
+					ctx.stateStore.restoreSnapshot(stateStoreSnapshotID);
+					return TransactionExecutionResult.INVALID;
 				}
 			}
 		}
@@ -204,7 +209,9 @@ export class StateMachine {
 				try {
 					await mod.afterCommandExecute(transactionContext);
 				} catch (error) {
-					status = TransactionExecutionResult.INVALID;
+					ctx.eventQueue.restoreSnapshot(eventQueueSnapshotID);
+					ctx.stateStore.restoreSnapshot(stateStoreSnapshotID);
+					return TransactionExecutionResult.INVALID;
 				}
 			}
 		}

framework/src/testing/in_memory_prefixed_state.ts

@@ -61,7 +61,9 @@ export class InMemoryPrefixedStateDB {
 	}
 
 	// eslint-disable-next-line @typescript-eslint/no-empty-function
-	public snapshot(): void {}
+	public snapshot(): number {
+		return 0;
+	}
 
 	// eslint-disable-next-line @typescript-eslint/no-empty-function
 	public restoreSnapshot(): void {}

framework/test/unit/state_machine/event_queue.spec.ts

@@ -108,11 +108,11 @@ describe('EventQueue', () => {
 		events.map(e => eventQueue.add(e.moduleID, e.typeID, e.data, e.topics));
 		expect(eventQueue.getEvents()).toHaveLength(events.length);
 
-		eventQueue.createSnapshot();
+		const snapshotID = eventQueue.createSnapshot();
 		eventQueue.add(utils.intToBuffer(3, 4), Buffer.from([0, 0, 0, 1]), utils.getRandomBytes(100), [
 			utils.getRandomBytes(32),
 		]);
-		eventQueue.restoreSnapshot();
+		eventQueue.restoreSnapshot(snapshotID);
 
 		expect(eventQueue.getEvents()).toHaveLength(events.length);
 		eventQueue.getEvents().forEach((e, i) => {
@@ -128,7 +128,7 @@ describe('EventQueue', () => {
 		events.map(e => eventQueue.add(e.moduleID, e.typeID, e.data, e.topics));
 		expect(eventQueue.getEvents()).toHaveLength(events.length);
 
-		eventQueue.createSnapshot();
+		const snapshotID = eventQueue.createSnapshot();
 		eventQueue.add(
 			utils.intToBuffer(3, 4),
 			Buffer.from([0, 0, 0, 1]),
@@ -150,7 +150,7 @@ describe('EventQueue', () => {
 			[utils.getRandomBytes(32)],
 			false,
 		);
-		eventQueue.restoreSnapshot();
+		eventQueue.restoreSnapshot(snapshotID);
 
 		expect(eventQueue.getEvents()).toHaveLength(events.length + 1);
 		const queuedEvents = eventQueue.getEvents();

framework/test/unit/state_machine/state_machine.spec.ts

@@ -206,6 +206,64 @@ describe('state_machine', () => {
 			expect(events).toHaveLength(1);
 			expect(events[0].toObject().topics[0]).toEqual(transaction.id);
 		});
+
+		it('should rollback state if afterCommandExecute fails', async () => {
+			const events = [
+				{
+					moduleID: utils.intToBuffer(3, 4),
+					typeID: Buffer.from([0, 0, 0, 0]),
+					data: utils.getRandomBytes(20),
+					topics: [utils.getRandomBytes(32), utils.getRandomBytes(20)],
+				},
+				{
+					moduleID: utils.intToBuffer(4, 4),
+					typeID: Buffer.from([0, 0, 0, 0]),
+					data: utils.getRandomBytes(20),
+					topics: [utils.getRandomBytes(32), utils.getRandomBytes(20)],
+				},
+				{
+					moduleID: utils.intToBuffer(2, 4),
+					typeID: Buffer.from([0, 0, 0, 0]),
+					data: utils.getRandomBytes(20),
+					topics: [utils.getRandomBytes(32)],
+				},
+			];
+			for (const e of events) {
+				eventQueue.add(e.moduleID, e.typeID, e.data, e.topics);
+			}
+
+			mod.beforeCommandExecute.mockImplementation(() => {
+				eventQueue.add(
+					utils.intToBuffer(3, 4),
+					Buffer.from([0, 0, 0, 1]),
+					utils.getRandomBytes(100),
+					[utils.getRandomBytes(32)],
+				);
+			});
+
+			systemMod.afterCommandExecute.mockImplementation(() => {
+				throw new Error('afterCommandExecute failed');
+			});
+			mod.afterCommandExecute.mockImplementation(() => {
+				throw new Error('afterCommandExecute failed');
+			});
+
+			const ctx = new TransactionContext({
+				eventQueue,
+				logger,
+				stateStore,
+				header,
+				assets,
+				networkIdentifier,
+				transaction,
+				currentValidators: [],
+				impliesMaxPrevote: true,
+				maxHeightCertified: 0,
+				certificateThreshold: BigInt(0),
+			});
+			await stateMachine.executeTransaction(ctx);
+			expect(ctx.eventQueue.getEvents()).toHaveLength(events.length);
+		});
 	});
 
 	describe('verifyAssets', () => {

sdk/package.json

@@ -33,7 +33,7 @@
 		"@liskhq/lisk-chain": "^0.4.0-alpha.0",
 		"@liskhq/lisk-codec": "^0.3.0-alpha.0",
 		"@liskhq/lisk-cryptography": "^4.0.0-alpha.0",
-		"@liskhq/lisk-db": "^0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27",
+		"@liskhq/lisk-db": "0.3.0-debug.20-b876469836935925369a887fe03e50a67c26c38f",
 		"@liskhq/lisk-p2p": "^0.8.0-alpha.0",
 		"@liskhq/lisk-passphrase": "^4.0.0-alpha.0",
 		"@liskhq/lisk-transaction-pool": "^0.6.0-alpha.0",

yarn.lock

@@ -2500,10 +2500,10 @@
   dependencies:
     "@types/node" "11.11.2"
 
-"@liskhq/lisk-db@^0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27":
-  version "0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27"
-  resolved "https://npm.lisk.com/@liskhq%2flisk-db/-/lisk-db-0.3.0-debug.20-b44082d07ab158736d00f7fd1706ca72ca2c9d27.tgz#596e6e645ea2e5f4763661f776f4b82086316705"
-  integrity sha512-5tXeUCOjwYDVJGCdXj3gKBFC+EhIZschM111EWyAzHjIdQU6FQ10pEwNRadoW/b9ixDzCOhBKBhklwNSykADYQ==
+"@liskhq/[email protected]b876469836935925369a887fe03e50a67c26c38f":
+  version "0.3.0-debug.20-b876469836935925369a887fe03e50a67c26c38f"
+  resolved "https://npm.lisk.com/@liskhq%2flisk-db/-/lisk-db-0.3.0-debug.20-b876469836935925369a887fe03e50a67c26c38f.tgz#e2fc899d796c22422027361a0666ef7564f4a2ae"
+  integrity sha512-1I35XKfjUTZAcARrcTUSnYjFMliDEPVOFro6Jo+KsrBEMjgpoUz2qZQqEfuFp1aVTsmkGjYw66jWfGHKZOAb1g==
   dependencies:
     "@mapbox/node-pre-gyp" "^1.0.9"
     "@types/node" "^16"