feat!: remove square size from block data

[rach-id]

Description

Closes #1045

---

PR checklist

• Tests written/updated
• Changelog entry added in .changelog (we use
  unclog to manage our changelog)
• Updated relevant documentation (docs/ or spec/) and code comments

[evan-forbes]

wait, why are we removing the data hash from block data? Isn't this how we are passing the data root from the app back to core?

[rach-id]

I guess that changed, and we're using this:

celestia-core/state/execution.go

Lines 139 to 151 in d02553f

	// Celestia passes the data root back as the second to last transaction
	// and the big endian encoding of the square size as the last transaction.
	if len(rpp.Txs) < 2 {
	panic("state machine returned an invalid prepare proposal response: expected at least 2 transactions")
	}
	if len(rpp.Txs[len(rpp.Txs)-2]) != tmhash.Size {
	panic(fmt.Sprintf("state machine returned an invalid prepare proposal response: expected second to last transaction to be a hash, got %d bytes", len(rpp.Txs[len(rpp.Txs)-2])))
	}
	if len(rpp.Txs[len(rpp.Txs)-1]) != 8 {
	panic("state machine returned an invalid prepare proposal response: expected last transaction to be a uint64 (square size)")
	}

Note: the PR is to main and not to current releases branch

[evan-forbes]

this is what I was referring to @sweexordious

if we don't do this, then when we call data.Hash()

celestia-core/types/block.go

Lines 1025 to 1037 in d02553f

	// Hash returns the hash of the data
	func (data *Data) Hash() cmtbytes.HexBytes {
	if data == nil {
	return (Txs{}).Hash()
	}
	if data.hash == nil {
	data.hash = data.Txs.Hash() // NOTE: leaves of merkle tree are TxIDs
	}
	// this is the expected behavior where `data.hash` was set by celestia-app
	// in PrepareProposal
	return data.hash
	}

we will not use the celestia-app version of the hash

[evan-forbes]

this change is ignoring the hash we're passing back and instead we're using the standard tendermint one which is why the tests pass fine (that's actually the only reason we allow this to occur at all)

[evan-forbes]

it certainly doesn't have to be in this PR, but since this mechanism is so hidden it would probably be a good idea to include a test to ensure that we don't miss this in the future. Currently, those tests are only in the application, but we should be able to modify one of the test applications here to modify the data root

[evan-forbes]

#1053

[rach-id]

Good catch 🙏

what do you think about this? f84fa8a

We could either do this, or move those two lines that set the data hash and square size to the DataFromProto method. However, we will also need to change:

celestia-core/state/execution.go

Lines 154 to 156 in 842a0b5

	block.Data, _ = types.DataFromProto(&cmtproto.Data{
	Txs: rpp.Txs[:len(rpp.Txs)-2],
	})

to take the whole rpp.Txs slice with the square size and the data hash appended at the end.

What do you think?

[evan-forbes]

we call data.Hash() in other places, therefore we are unable to only set the value in the header

if we change one of the test applications to also modify the header, then we will be able to test this issue

[cmwaters]

Theoretically it is possible to remove the hash field from the data struct so long as the data root can be directly added to the header.

[rach-id]

Theoretically it is possible to remove the hash field from the data struct so long as the data root can be directly added to the header.

Agreed. But I checked comet and apparently they still have that in the types.Data struct. Also, in this PR I am leaving the square size there because it is being used here:

celestia-core/state/execution.go

Line 180 in d02553f

txs := append(block.Data.Txs.ToSliceOfBytes(), block.DataHash, squareSizeBytes)

[evan-forbes]

thanks for fixing 👍

[cmwaters]

approving my own changes like...

[evan-forbes]

one last hardcoded test I think

TestBlockchainMessageVectors

[cmwaters]

Yup I'm on it