Repository: celestiaorg/celestia-node
Pull request #1952 (2023-03-22 08:17:30)

fix(core): Only assume empty square if reported square size is 1 *and* there are no txs
A bug was caught on blockspacerace network that caused a data root mismatch between bridge nodes and core nodes because celestia-node incorrectly assumes an empty square if there are no transactions which is not the case as squares can contain only tail-padding even if no transactions land in the block itself. 

We also will now: 
* rely on using `data.SquareSize` from the block data for extending shares instead of calculating square size from `len(shares)`, and
* panic on data root mismatch

**Future considerations:**

* we need a fuzzing suite against our block extension mechanism that can generate all kinds of blocks (our integration testing doesn't test for edge cases like this.
* we need to audit the rest of the codebase to ensure we perform the same calculations over square size or assume empty square where we shouldn't be.

codecov-commenter: ## [Codecov](https://codecov.io/gh/celestiaorg/celestia-node/pull/1952?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg) Report
> Merging [#1952](https://codecov.io/gh/celestiaorg/celestia-node/pull/1952?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg) (d92e482) into [main](https://codecov.io/gh/celestiaorg/celestia-node/commit/b4fe04de727a1556520846eb91e19947e41617de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg) (b4fe04d) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main    #1952      +/-   ##
==========================================
+ Coverage   57.72%   57.75%   +0.02%     
==========================================
  Files         245      245              
  Lines       16186    16184       -2     
==========================================
+ Hits         9344     9347       +3     
+ Misses       5891     5888       -3     
+ Partials      951      949       -2     
```


| [Impacted Files](https://codecov.io/gh/celestiaorg/celestia-node/pull/1952?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg) | Coverage Δ | |
|---|---|---|
| [core/eds.go](https://codecov.io/gh/celestiaorg/celestia-node/pull/1952?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg#diff-Y29yZS9lZHMuZ28=) | `63.15% <100.00%> (-1.85%)` | :arrow_down: |
| [header/header.go](https://codecov.io/gh/celestiaorg/celestia-node/pull/1952?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg#diff-aGVhZGVyL2hlYWRlci5nbw==) | `63.30% <100.00%> (-0.34%)` | :arrow_down: |

... and [3 files with indirect coverage changes](https://codecov.io/gh/celestiaorg/celestia-node/pull/1952/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg)

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celestiaorg)


Patch:
diff --git a/core/eds.go b/core/eds.go
index 5ffa52ac..e37fa5d9 100644
--- a/core/eds.go
+++ b/core/eds.go
@@ -11,7 +11,6 @@ import (
 	appshares "github.com/celestiaorg/celestia-app/pkg/shares"
 	"github.com/celestiaorg/rsmt2d"
 
-	"github.com/celestiaorg/celestia-node/libs/utils"
 	"github.com/celestiaorg/celestia-node/share"
 	"github.com/celestiaorg/celestia-node/share/eds"
 )
@@ -20,15 +19,15 @@ import (
 // ExtendedDataSquare (EDS). If there are no transactions in the block,
 // nil is returned in place of the eds.
 func extendBlock(data types.Data) (*rsmt2d.ExtendedDataSquare, error) {
-	if len(data.Txs) == 0 {
+	if len(data.Txs) == 0 && data.SquareSize == uint64(1) {
 		return nil, nil
 	}
 	shares, err := appshares.Split(data, true)
 	if err != nil {
 		return nil, err
 	}
-	size := utils.SquareSize(len(shares))
-	return da.ExtendShares(size, appshares.ToBytes(shares))
+
+	return da.ExtendShares(data.SquareSize, appshares.ToBytes(shares))
 }
 
 // storeEDS will only store extended block if it is not empty and doesn't already exist.
diff --git a/header/header.go b/header/header.go
index 3e371e1c..28da7fc0 100644
--- a/header/header.go
+++ b/header/header.go
@@ -141,8 +141,8 @@ func (eh *ExtendedHeader) Validate() error {
 
 	// ensure data root from raw header matches computed root
 	if !bytes.Equal(eh.DAH.Hash(), eh.DataHash) {
-		return fmt.Errorf("mismatch between data hash commitment from core header and computed data root: "+
-			"data hash: %X, computed root: %X", eh.DataHash, eh.DAH.Hash())
+		panic(fmt.Sprintf("mismatch between data hash commitment from core header and computed data root "+
+			"at height %d: data hash: %X, computed root: %X", eh.Height(), eh.DataHash, eh.DAH.Hash()))
 	}
 
 	return eh.DAH.ValidateBasic()


Test patch:
diff --git a/core/eds_test.go b/core/eds_test.go
new file mode 100644
index 00000000..d67f5005
--- /dev/null
+++ b/core/eds_test.go
@@ -0,0 +1,43 @@
+package core
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"github.com/tendermint/tendermint/types"
+
+	"github.com/celestiaorg/celestia-app/pkg/da"
+
+	"github.com/celestiaorg/celestia-node/share"
+)
+
+// TestTrulyEmptySquare ensures that a truly empty square (square size 1 and no
+// txs) will be recognized as empty and return nil from `extendBlock` so that
+// we do not redundantly store empty EDSes.
+func TestTrulyEmptySquare(t *testing.T) {
+	data := types.Data{
+		Txs:        []types.Tx{},
+		Blobs:      []types.Blob{},
+		SquareSize: 1,
+	}
+
+	eds, err := extendBlock(data)
+	require.NoError(t, err)
+	assert.Nil(t, eds)
+}
+
+// TestNonEmptySquareWithZeroTxs tests that a non-empty square with no
+// transactions or blobs computes the correct data root (not the minimum DAH).
+func TestNonEmptySquareWithZeroTxs(t *testing.T) {
+	data := types.Data{
+		Txs:        []types.Tx{},
+		Blobs:      []types.Blob{},
+		SquareSize: 16,
+	}
+
+	eds, err := extendBlock(data)
+	require.NoError(t, err)
+	dah := da.NewDataAvailabilityHeader(eds)
+	assert.NotEqual(t, share.EmptyRoot().Hash(), dah.Hash())
+}
diff --git a/core/header_test.go b/core/header_test.go
index 4f70963c..524d9979 100644
--- a/core/header_test.go
+++ b/core/header_test.go
@@ -44,6 +44,8 @@ func TestMismatchedDataHash_ComputedRoot(t *testing.T) {
 
 	header.DataHash = rand.Bytes(32)
 
-	err := header.Validate()
-	assert.ErrorContains(t, err, "mismatch between data hash")
+	panicFn := func() {
+		header.Validate() //nolint:errcheck
+	}
+	assert.Panics(t, panicFn)
 }