Restore Unit Tests

[0xmovses]

Some of our unit tests are failing, these may have failed during war room efforts last week. We tested instead with load e2e tests, test implementation was updated to keep up with latest changes on Thursday, but subsequent changes likely broke the tests.

1. Inside the Nix Shell run:

cargo test \
            -p maptos-opt-executor \
            -p memseq \
            -p move-rocks \
            -p movement-types \
            -p movement-config \
            -p movement-da-util \
            -p movement-signer-test

Output:

test executor::execution::tests::test_revert_block_head_to ... ok
test gc_account_sequence_number::test::test_gc_removes_some_not_all ... ok
test gc_account_sequence_number::test::test_gc ... ok
test gc_account_sequence_number::test::test_inserts ... ok
test gc_account_sequence_number::test::test_removes ... ok
test tests::test_sign_transaction_with_hashi_corp_vault_executes ... FAILED
test tests::test_sign_transaction_with_hashi_corp_vault_includes_in_block ... FAILED
test executor::execution::tests::test_execute_block_state_get_api ... FAILED
test service::tests::test_pipe_mempool_while_server_running ... FAILED
test executor::execution::tests::test_execute_block ... FAILED
test background::transaction_pipe::tests::test_sequence_number_too_old ... FAILED
test background::transaction_pipe::tests::test_pipe_mempool ... FAILED
test background::transaction_pipe::tests::test_repeated_pipe_mempool_from_api ... FAILED
test background::transaction_pipe::tests::test_pipe_mempool_from_api ... FAILED
test background::transaction_pipe::tests::test_pipe_mempool_cancellation ... FAILED
test background::transaction_pipe::tests::test_cannot_submit_too_new ... FAILED
test background::transaction_pipe::tests::test_pipe_mempool_with_duplicate_transaction ... FAILED
test executor::execution::tests::test_execute_block_state_db ... ok

Separate tasks

• Fix the hashi_corp tests
  • test tests::test_sign_transaction_with_hashi_corp_vault_executes
  • test tests::test_sign_transaction_with_hashi_corp_vault_includes_in_block
• Fix the transaction_pipe_tests
• And fix the last two tests:
  • test executor::execution::tests::test_execute_block_state_get_api
  • test service::tests::test_pipe_mempool_while_server_running

[0xmovses]

It's been agreed that the implementation will change of the transaction pipe in such a way that these tests will be re-written or handled as part of the work in feature branch #1138

[andygolay]

As far as I can tell, the test test_cannot_submit_too_new is no longer relevant. There's not even a TOO_NEW_TOLERANCE const or an InvalidSeqNumber mempool status code anymore.

Devrel has also reported that they've never seen a sequence number too new error, only too old, depending on whether there's an intent to add the TOO_NEW_TOLERANCE back in or if something similar is being used somehow that I'm not noticing.

In Aptos transaction validation we have sequence number verified in the prologue at
https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/framework/aptos-framework/sources/transaction_validation.move#L162

I've tried several tests and haven't managed to get the sequence number too new error, eg this test

	#[tokio::test]
	async fn test_sequence_number_too_new() -> Result<(), anyhow::Error> {
		let (mempool_tx_exec_result_sender, mempool_commit_tx_receiver) =
			futures_mpsc::channel::<Vec<TxExecutionResult>>(EXECUTOR_CHANNEL_SIZE);

		let (mut executor, _tempdir) =
			Executor::try_test_default(GENESIS_KEYPAIR.0.clone(), mempool_tx_exec_result_sender)
				.await?;
		let (context, background) = executor.background(mempool_commit_tx_receiver)?;
		let transaction_pipe = background.into_transaction_pipe();

		// Run the transaction pipe
		let da_client = EmptyDaSequencerClient;
		let mempool_handle = tokio::spawn(transaction_pipe.run(da_client));

		let tx = create_signed_transaction(0, &context.config().chain);

		// Commit the first transaction to a block
		let block_id = HashValue::random();
		let block_metadata = Transaction::BlockMetadata(BlockMetadata::new(
			block_id,
			0,
			0,
			executor.signer.author(),
			vec![],
			vec![],
			chrono::Utc::now().timestamp_micros() as u64,
		));
		let txs = ExecutableTransactions::Unsharded(
			[block_metadata, Transaction::UserTransaction(tx)]
				.into_iter()
				.map(SignatureVerifiedTransaction::Valid)
				.collect(),
		);
		let block = ExecutableBlock::new(block_id.clone(), txs);
		executor.execute_block(block).await?;

		{
			let state_view = executor
				.db_reader()
				.latest_state_checkpoint_view()
				.expect("Failed to get latest state checkpoint view.");
			let account_address = account_config::aptos_test_root_address();
			let sequence_number =
				vm_validator::get_account_sequence_number(&state_view, account_address)?;
			assert_eq!(sequence_number, 1);
		}

		// Create a transaction using a sequence number that is too high
		let tx = create_signed_transaction(1000, &context.config().chain);

		// Send the transaction to mempool
		let (req_sender, callback) = oneshot::channel();
		context
			.mempool_client_sender()
			.send(MempoolClientRequest::SubmitTransaction(tx, req_sender))
			.await?;

		let status = callback.await??;

		assert_eq!(status.0.code, MempoolStatusCode::VmError);
		let vm_status = status.1.unwrap();
		assert_eq!(vm_status, DiscardedVMStatus::SEQUENCE_NUMBER_TOO_NEW);

		mempool_handle.abort();

		Ok(())
	}

panics with

thread 'background::transaction_pipe::tests::test_sequence_number_too_new' panicked at protocol-units/execution/maptos/opt-executor/src/background/transaction_pipe.rs:743:9:
assertion `left == right` failed
  left: Accepted
 right: VmError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It would be good to have a test for sequence number too new, I just haven't managed to trigger the error yet, if anyone has suggestions for how to do it.

[andygolay]

Update: @musitdev is working on fixing some tests based on new insights about the DA flow. I'll keep an aye on this issue as progress is made there. I'll check into the HashiCorp tests next as I'm guessing those are independent.