Tests for invalid transaction handling #18

sveitser · 2023-11-30T15:24:16Z

mock the HotShot server to make sure the transactions we want are included in an espresso block
check the message count in transaction streamer and block validator
check the count of transactions in the l2 block

nomaxg · 2023-12-05T15:52:16Z

system_tests/espresso_test.go

+
+func espresso_block_txs_generators(t *testing.T, l2Info *BlockchainTestInfo) map[int][][]byte {
+	return map[int][][]byte{
+		5:  onlyMalformedTxs(t),


I think that this might be cleaner with some sort mock Txn Interface:

type MockTxn struct { Type MockTxnType // Valid or invalid Amount uint64 ...etc }

Then you can build test cases with arrays of MockTxnTemplates and create a single function that parses them into the correct txn bytes

Great point. But currently mocking txs is still not easy. There is no any readily available functions to mock the malformed and invalid txs and I don't have any good idea about how to implement this interface yet. I think it might be better to optimize here after we know the test system well.

Cleaner way to mock Espresso transactions for tests #22

nomaxg · 2023-12-05T15:53:57Z

system_tests/espresso_test.go

+		func(req *http.Request) (*http.Response, error) {
+			log.Info("GET", "url", req.URL)
+			block := uint64(httpmock.MustGetSubmatchAsUint(req, 1))
+			// todo


Do we still need these todos? If not, we should remove them, but if we do, let's comment them with a GitHub issue

These todos are here just because we don't realize the validation of espresso. After we realize that, we need to make the proof or header correctly or the test will fail.

Now todos are removed and a comment is left.

nomaxg · 2023-12-05T15:59:02Z

system_tests/espresso_test.go

+		"POST",
+		"http://127.0.0.1:50000/submit/submit",
+		func(req *http.Request) (*http.Response, error) {
+			log.Info("POST", "url", req.URL, "body", req.Body)


Is this log necessary?

The submit/submit handler is removed now. There should not be any submission during the test.

nomaxg · 2023-12-05T16:01:58Z

system_tests/espresso_test.go

+		})
+
+	generators := espresso_block_txs_generators(t, l2Info)
+	dummyProof := func(n int) []byte {


We could even consider hardcoding this entirely, unless you think there is a benefit to encoding the block number in each proof

nomaxg · 2023-12-05T16:02:51Z

system_tests/espresso_test.go

+			log.Info("GET", "url", req.URL)
+			if !ok {
+				r := espresso.NamespaceResponse{
+					Proof:        (*json.RawMessage)(&proof), // todo


What are these todos?

nomaxg · 2023-12-05T16:03:44Z

system_tests/espresso_test.go

+				}
+				return httpmock.NewJsonResponse(200, r)
+			}
+			for _, rawTx := range data {


I think that generators (or whatever we end up using to generate txns) can do this wrapping - we can even hardcode the VM ID

system_tests/espresso_test.go

sveitser · 2023-12-06T11:36:08Z

@ImJeremyHe I have trouble running the new test locally.

I made this change for debugging

modified   system_tests/espresso_test.go
@@ -248,6 +248,7 @@ func TestEspresso(t *testing.T) {
 	err = waitFor(t, ctx, func() bool {
 		cnt := node.ConsensusNode.BlockValidator.Validated(t)
 		expected := arbutil.MessageIndex(expectedMsgCnt)
+		log.Info("Got expected messages", "count", cnt, "need", expected)
 		return cnt >= expected
 	})
 	Require(t, err)

At the end I just get

NFO [12-06|12:34:06.788] Got expected messages                    count=2 need=4
INFO [12-06|12:34:07.789] Got expected messages                    count=2 need=4
INFO [12-06|12:34:08.349] GET                                      url=http://127.0.0.1:50000/availability/header/101
INFO [12-06|12:34:08.350] Fetching tx, block: 101, namespace: 100
ERROR[12-06|12:34:08.350] request failed                           err=nil                              url=http://127.0.0.1:50000/availability/block/101/namespace/100 status=404 response=0
ERROR[12-06|12:34:08.350] Error fetching transactions              err="request failed with status 404"
INFO [12-06|12:34:08.789] Got expected messages                    count=2 need=4
    espresso_test.go:254:  [] context deadline exceeded
ERROR[12-06|12:34:09.774] Failed to write trie to disk             err="pebble: closed"
ERROR[12-06|12:34:09.774] failed writing latest valid block state to DB err="pebble: closed"
INFO [12-06|12:34:13.350] GET                                      url=http://127.0.0.1:50000/availability/header/101
INFO [12-06|12:34:13.350] Fetching tx, block: 101, namespace: 100

log.txt

Any ideas?

sveitser · 2023-12-06T11:37:53Z

I'm running go test -v -run TestEspresso ./system_tests/

sveitser · 2023-12-06T11:38:37Z

ERROR[12-06|12:36:31.065] Error during validation                  err="validation failed: expected {0x7854bc708e7a4e54b7fd6b0a69a5a558c50d132c27404927cbb76bd06f3eb353 0x0000000000000000000000000000000000000000000000000000000000000000 1 2} got {0xd57c5b5a1264a69882f2e5316eb61ba5f62754765d266cb41c42651f2afb0e94 0x0000000000000000000000000000000000000000000000000000000000000000 1 2}"
common_test.go:859: error occurred: validation failed: expected {0x7854bc708e7a4e54b7fd6b0a69a5a558c50d132c27404927cbb76bd06f3eb353 0x0000000000000000000000000000000000000000000000000000000000000000 1 2} got {0xd57c5b5a1264a69882f2e5316eb61ba5f62754765d266cb41c42651f2afb0e94 0x0000000000000000000000000000000000000000000000000000000000000000 1 2}

ImJeremyHe · 2023-12-06T11:46:01Z

@sveitser Have you tried to rebuild the replay wasm?

sveitser · 2023-12-06T11:48:18Z

@ImJeremyHe That was it thanks!

sveitser · 2023-12-06T11:51:24Z

At least we know now that if validation fails the test also fails. I think we should add a timeout though so we don't wait too long. The test takes about 15 seconds on the CI and 10 seconds locally. So I think setting something like a minute should be safe. The waitFor looks like it has a 30 seconds timeout but that doesn't seem to be the case.

sveitser · 2023-12-06T12:03:21Z

@ImJeremyHe Nevermind. I works as intended now 😕

sveitser · 2023-12-06T12:05:10Z

LGTM. But I can't approve the PR because I opened it.

chore: merge from main to develop

Fixed array tests

ImJeremyHe force-pushed the jh/tests branch 11 times, most recently from 0b311b7 to fc8a1f5 Compare December 5, 2023 08:49

ImJeremyHe marked this pull request as ready for review December 5, 2023 08:51

ImJeremyHe requested review from ImJeremyHe, nomaxg, philippecamacho and jbearer as code owners December 5, 2023 08:51

ImJeremyHe force-pushed the jh/tests branch 3 times, most recently from 89816cc to 735bbb5 Compare December 5, 2023 09:15

nomaxg reviewed Dec 5, 2023

View reviewed changes

system_tests/espresso_test.go Show resolved Hide resolved

ImJeremyHe force-pushed the jh/tests branch from 735bbb5 to 8ba8d68 Compare December 6, 2023 01:06

Add espresso test

f311393

ImJeremyHe force-pushed the jh/tests branch from 8ba8d68 to f311393 Compare December 6, 2023 02:31

sveitser commented Dec 6, 2023

View reviewed changes

system_tests/espresso_test.go Show resolved Hide resolved

ImJeremyHe approved these changes Dec 6, 2023

View reviewed changes

sveitser mentioned this pull request Dec 6, 2023

Cleaner way to mock Espresso transactions for tests #22

Closed

ImJeremyHe merged commit 618eb65 into integration Dec 6, 2023
5 checks passed

ImJeremyHe deleted the jh/tests branch December 6, 2023 12:40

zacshowa pushed a commit that referenced this pull request Nov 26, 2024

Merge pull request #18 from OffchainLabs/main

b22c926

chore: merge from main to develop

zacshowa pushed a commit that referenced this pull request Nov 26, 2024

Merge pull request #18 from OffchainLabs/fixed-array-tests

60d71ca

Fixed array tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for invalid transaction handling #18

Tests for invalid transaction handling #18

sveitser commented Nov 30, 2023 •

edited by ImJeremyHe

Loading

nomaxg Dec 5, 2023 •

edited

Loading

ImJeremyHe Dec 6, 2023 •

edited

Loading

sveitser Dec 6, 2023 •

edited

Loading

nomaxg Dec 5, 2023

ImJeremyHe Dec 6, 2023

ImJeremyHe Dec 6, 2023

nomaxg Dec 5, 2023

ImJeremyHe Dec 6, 2023 •

edited

Loading

nomaxg Dec 5, 2023

nomaxg Dec 5, 2023

nomaxg Dec 5, 2023 •

edited

Loading

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023 •

edited

Loading

ImJeremyHe commented Dec 6, 2023 •

edited

Loading

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

Tests for invalid transaction handling #18

Tests for invalid transaction handling #18

Conversation

sveitser commented Nov 30, 2023 • edited by ImJeremyHe Loading

nomaxg Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

ImJeremyHe Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

sveitser Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

nomaxg Dec 5, 2023

Choose a reason for hiding this comment

ImJeremyHe Dec 6, 2023

Choose a reason for hiding this comment

ImJeremyHe Dec 6, 2023

Choose a reason for hiding this comment

nomaxg Dec 5, 2023

Choose a reason for hiding this comment

ImJeremyHe Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

nomaxg Dec 5, 2023

Choose a reason for hiding this comment

nomaxg Dec 5, 2023

Choose a reason for hiding this comment

nomaxg Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023 • edited Loading

ImJeremyHe commented Dec 6, 2023 • edited Loading

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

sveitser commented Dec 6, 2023

sveitser commented Nov 30, 2023 •

edited by ImJeremyHe

Loading

nomaxg Dec 5, 2023 •

edited

Loading

ImJeremyHe Dec 6, 2023 •

edited

Loading

sveitser Dec 6, 2023 •

edited

Loading

ImJeremyHe Dec 6, 2023 •

edited

Loading

nomaxg Dec 5, 2023 •

edited

Loading

sveitser commented Dec 6, 2023 •

edited

Loading

ImJeremyHe commented Dec 6, 2023 •

edited

Loading