celestia-app icon indicating copy to clipboard operation
celestia-app copied to clipboard

Published 20 hours ago • verified publisher icon celestiaorg

Improve `TestPrepareProposalConsistency`

Open evan-forbes opened this issue 2 years ago • 7 comments

The TestPrepareProposalConsistency test uses randomly sized transactions to test if PrepareProposal will ever create a proposal block that ProcessProposal rejects. While other tests are responsible for ensuring that a transaction that gets through CheckTx, gets through PrepareProposal, and then ProcessProposal, this test can still be improved upon by being more exact with how many transactions are expected to be included in the block.

https://github.com/celestiaorg/celestia-app/blob/b59e876cd15ea9f17608d1c2ca865a86151b7e5f/app/test/fuzz_abci_test.go#L144

evan-forbes avatar Sep 18 '23 03:09 evan-forbes

Hey @evan-forbes, can I handle this issue?

ThanhNhann avatar Nov 29 '23 16:11 ThanhNhann

Hi @evan-forbes, I see the code from branch main now have some differs from your code reference above, do we still need to improve it now?

ThanhNhann avatar Dec 07 '23 15:12 ThanhNhann

Thanks for your interest @ThanhNhann ! It looks like this assertion still exists on main

https://github.com/celestiaorg/celestia-app/blob/c5e178642f9854d774ea2cdd112f85d495f20032/app/test/fuzz_abci_test.go#L154

and I think this issue wants a more exact assertion for the # of transactions (blob and non-blob) that get included in the process proposal block. Perhaps it can assert that the # of transactions in the prepare proposal response == the # of transactions in the process proposal response (?)

rootulp avatar Dec 07 '23 16:12 rootulp

Greetings! @evan-forbes @rootulp Looks like this is still open. Can I work on it?

ex9-fyi avatar Feb 02 '24 23:02 ex9-fyi

Looking

ex9-fyi avatar Feb 03 '24 17:02 ex9-fyi

Closing this as won't do because it's not immediately clear what this issue is asking for. See https://github.com/celestiaorg/celestia-app/pull/3087#issuecomment-1932966792

cc: @evan-forbes please re-open with acceptance criteria if we still want this.

rootulp avatar Feb 28 '24 15:02 rootulp

this test can still be improved upon by being more exact with how many transactions are expected to be included in the block.

I think this is the core piece, we aren't actually checking how many transactions are being included, only that one blobTx did. This means that the rest of the blob txs could be filtered out. Ideally we would make this more restrictive. This might involve writing a different or doing some analysis for what conservative % of txs should be expected to get through.

https://github.com/celestiaorg/celestia-app/blob/949a2d579619512e9e56eea10538fe847f12306d/app/test/fuzz_abci_test.go#L149-L155

evan-forbes avatar Feb 28 '24 15:02 evan-forbes