Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: add extra tests to ensure we remove txs correctly #1345

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Feb 6, 2023

Addresses: #1102

this test could be as simple adding an additional check to an existing test that creates square not the max square size, and ensure that each transaction is getting included in the block.

Test_finalizeLayout already includes tests where a non max square size square is created and none of the transactions are dropped so I'm a little confused about this. I've added tests to include that we are dropping transactions if the blobs exceed the square size.

@MSevey MSevey requested review from a team and rootulp and removed request for a team February 6, 2023 22:08
@evan-forbes
Copy link
Member

Test_finalizeLayout already includes tests where a non max square size square is created and none of the transactions are dropped so I'm a little confused about this.

hmm good point, I guess that is tested!

@cmwaters cmwaters merged commit 580d186 into main Feb 7, 2023
@cmwaters cmwaters deleted the cal/extra-write-square-tests branch February 7, 2023 00:12
@cmwaters
Copy link
Contributor Author

cmwaters commented Feb 7, 2023

Shall I close #1102?

@rootulp
Copy link
Collaborator

rootulp commented Feb 7, 2023

I think the test described in #1102 tests something slightly different. Since the tests in this PR specify a square size, I don't think they are exactly applicable. These tests are contrived (but still useful) because for example in this scenario:

		{
			// only two blob txs should make it in the square
			squareSize:      4,
			nonreserveStart: 4,
			blobTxs: generateBlobTxsWithNIDs(
				t,
				[][]byte{ns1, ns2, ns3},
				[][]int{{2000}, {2000}, {6000}},
			),
			expectedIndexes: [][]uint32{{4}, {8}},
		},

this would keep doubling the square size so that all three blob txs would fit. I think the test for #1102 may want to test PrepareProposal as a whole.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants