test: add differential test

[exp7l]

Overview

This implements a solution to #2519.

What is the problem?

The fuzz test that tests against the celestia-app's ABCI is not exact in asserting the number of transactions returned by PrepareProposal.

What is the solution? What other solutions did I consider?

A solution is to reuse square.Build to make a square out of the unfiltered and unordered transactions provided by the fuzz test. Then compare the number of transactions in this square with that returned by PrepareProposal. This basically reimplements some of PrepareProposal to differentially test it.

The other potential solution is somehow calculate how many transactions will end up in the block. It seems to me it'd be complicated to extract a formula. I was looking here https://github.com/celestiaorg/go-square/blob/cd8e5d1065842d958f11d85e8a2ad73cbce252b0/square/square.go#L27-L35.

Checklist

• Rebase later to include the commits for go-square repo migration
  • Figure I'd let you take a quick look at the approach then figure out rebase on the main's HEAD
  • My fork is based off of 9e1c8a7 on main https://github.com/celestiaorg/celestia-app/commits/main/
• New and updated code has appropriate documentation
• New and updated code has new and/or updated testing
• Required CI checks are passing
• Visual proof for any user facing features like CLI or documentation updates
• Linked issues closed with keywords

[exp7l]

Hi @rootulp, I'm curious what you think about the approach here.

(I still need to rebase onto main's HEAD later, but like for you to see if the approach makes sense before I figure out the rebasing)

[rootulp]

Note: I updated the PR title to conform to conventional commits b/c the PR title will be used as the commit message if this PR gets merged

[rootulp]

I haven't looked deeply into the alternative approach this PR is proposing but it looks like the assertion still exists in this PR:

celestia-app/app/test/fuzz_abci_test.go

Line 144 in b59e876

require.GreaterOrEqual(t, len(resp.BlockData.Txs), sendTxCount+1)

so I'm wondering if this PR would actually close #2519. TBH it's tough to say because #2519 doesn't have acceptance criteria so maybe we should work on clarifying the desirable outcome of that issue before we proceed with implementing a fix. cc: @evan-forbes

[exp7l]

Any updates? @rootulp @evan-forbes

[exp7l]

Thanks for assigning. @rootulp

To clarify myself above, I meant to ask: does my approach here provide "exactness with how many transactions are expected to be included in the block." (quote from issue)? If not, can you help me understand what is missing and what is the desirable outcome?

Perhaps @evan-forbes can help like you say.

Thanks

[exp7l]

@rootulp Hi!

So I'm just wondering if you folks like to onboard people to contribute.
And is this draft going in the right direction or a different approach is needed?

I am quite curious about Celestia core dev work, and like to contribute as an outside contributor. It can be this issue or some other issue, if you folks are interested in bringing people on.

[rootulp]

Hi @exp7l sorry for our delay. We are interested in on-boarding new contributors but I don't think #2519 is easy to work on because it's not clear (at least to me) what that issue is asking for. I apologize because you already started working on it but are there other issues here that seem interesting to you?

[evan-forbes]

this is great work @exp7l !! its a good thing to test, but we already have tests that reconstruct the block data from the square and compare these things. per this comment I think that what we want is to compare the input to the square with the resulting txs from PrepareProposal

[evan-forbes]

apologies for the delay @exp7l this slipped through my notifications

[rootulp]

Going to close this b/c it doesn't appear to be actively worked on.