Remove `flattenNamespacedEDS` #338

evan-forbes · 2021-05-17T22:11:46Z

Description

This PR removes flattenNamespacedEDS and instead uses the nmt wrapper. It should probably wait to be merged until after #334, as that PR will simplify things even further by moving PutBlock. However, this is not strictly necessary.

In order to successfully close #336, this PR still needs to include a test to ensure that this actually fixes the issue plaguing the light client in #323.

Closes: #336

liamsi · 2021-05-17T23:13:19Z

This is cool!

this PR still needs to include a test to ensure that this actually fixes the issue plaguing the light client in #323.

The test I had in mind would basically create a block, call block.Hash() and PutBlock and validate that nothing in that block changed. Or maybe even simpler calling block.DataAvailabilityHeader.Hash() twice by resetting the hash field(s) and see if both hashes match.
Here is the very hacky way with which I've noticed that calling flattenNamespacedEDS twice modified the data: https://github.com/lazyledger/lazyledger-core/blob/437b8bf0d8f90f9eab7d034f95a4b207c93a316d/types/block.go#L343-L356

liamsi · 2021-05-19T09:55:43Z

And here is the most minimal test I could come up with, that also shows the problem:

type preprocessingApp struct {
	types.BaseApplication
}

func (app *preprocessingApp) PreprocessTxs(
	req types.RequestPreprocessTxs) types.ResponsePreprocessTxs {
	time.Sleep(time.Second * 2)
	randTxs := generateRandTxs(64, 256)
	randMsgs := generateRandNamespacedRawData(128, nmt.DefaultNamespaceIDLen, 256)
	randMessages := toMessageSlice(randMsgs)
	return types.ResponsePreprocessTxs{Txs: append(req.Txs, randTxs...), Messages: &tmproto.Messages{MessagesList: randMessages}}
}
func generateRandTxs(num int, size int) [][]byte {
	randMsgs := generateRandNamespacedRawData(num, nmt.DefaultNamespaceIDLen, size)
	for _, msg := range randMsgs {
		copy(msg[:nmt.DefaultNamespaceIDLen], TxNamespaceID)
	}
	return randMsgs
}

func toMessageSlice(msgs [][]byte) []*tmproto.Message {
	res := make([]*tmproto.Message, len(msgs))
	for i := 0; i < len(msgs); i++ {
		res[i] = &tmproto.Message{NamespaceId: msgs[i][:nmt.DefaultNamespaceIDLen], Data: msgs[i][nmt.DefaultNamespaceIDLen:]}
	}
	return res
}


func TestDataAvailabilityHeaderRewriteBug2(t *testing.T) {
	ipfsNode, err := coremock.NewMockNode()
	if err != nil {
		t.Error(err)
	}

	ipfsAPI, err := coreapi.NewCoreAPI(ipfsNode)
	if err != nil {
		t.Error(err)
	}
	txs := Txs{}
	l := len(txs)
	bzs := make([][]byte, l)
	for i := 0; i < l; i++ {
		bzs[i] = txs[i]
	}
	app := &preprocessingApp{}

	// See state.CreateProposalBlock to understand why we do this here:
	processedBlockTxs := app.PreprocessTxs(types.RequestPreprocessTxs{bzs})
	ppt := processedBlockTxs.GetTxs()

	pbmessages := processedBlockTxs.GetMessages()

	lp := len(ppt)
	processedTxs := make(Txs, lp)
	if lp > 0 {
		for i := 0; i < l; i++ {
			processedTxs[i] = ppt[i]
		}
	}

	messages := MessagesFromProto(pbmessages)
	lastID := makeBlockIDRandom()
	h := int64(3)

	voteSet, _, vals := randVoteSet(h-1, 1, tmproto.PrecommitType, 10, 1)
	commit, err := MakeCommit(lastID, h-1, 1, voteSet, vals, time.Now())
	assert.NoError(t, err)
	block := MakeBlock(1, processedTxs, nil, nil, messages, commit)
	block.Hash()	

	hash1 := block.DataAvailabilityHeader.Hash()

	ctx := context.TODO()
	err = block.PutBlock(ctx, ipfsAPI.Dag())
	if err != nil {
		t.Fatal(err)
	}

	block.fillDataAvailabilityHeader()
	hash2 := block.DataAvailabilityHeader.Hash()
	assert.Equal(t, hash1, hash2)

}

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

evan-forbes · 2021-05-20T19:34:07Z

types/block.go

+	// create the nmt wrapper to generate row and col commitments
+	squareSize := uint32(math.Sqrt(float64(len(shares))))
+	tree := wrapper.NewErasuredNamespacedMerkleTree(uint64(squareSize))
+


this is the major reason why we had to move the wrapper and constants into their own packages.

I do think that once we move DAHedear calculation to p2p/ipld we can take the wrapper back to where it exists now in master.

Having one more package for wrapper makes no sense for me in general

Same for consts actually

From experience I have in Golang: If I have an import cycle, it means I messed up proper logic segregation between packages architecturally, and simply extracting specific the piece of code causing the cycle is a simple hack, not a complete solution.

I think you are right about the import cycles and extraction into a separate package @Wondertan (in this case, not necessarily in general).

IMO, we can simply revisit this when we move the computation of the DAHeader into PutBlock.

evan-forbes · 2021-05-20T20:25:49Z

I'm still trying to debug why the e2e test is failing after merging the recent IPFS refactor with this PR. It was passing on both before merging. The good (?!) news is that at least this time it's failing locally as well as in the CI.

liamsi

This looks much better and consistently uses the same API in fillDAheader and PutBlock 👍🏼 great Change!

There is absolutely nothing related to the e2e tests really. So I assume they are still just a bit flaky and we still need to increase some timeouts. but this is orthogonal to this PR.

liamsi · 2021-05-20T20:56:38Z

types/block.go

-	copy(out, ParitySharesNamespaceID)
-	return out
-}
-


Wondertan · 2021-05-20T21:04:09Z

types/block.go

 	// TODO(ismail): for better efficiency and a larger number shares
 	// we should switch to the rsmt2d.LeopardFF16 codec:
-	extendedDataSquare, err := rsmt2d.ComputeExtendedDataSquare(shares, rsmt2d.NewRSGF8Codec(), rsmt2d.NewDefaultTree)
+	extendedDataSquare, err := rsmt2d.ComputeExtendedDataSquare(shares, rsmt2d.NewRSGF8Codec(), tree.Constructor)


This starts to remind me Java 😬
extendedAbstractFactoryForWrappedCodedDataSqaure...

Do you have any concrete suggestion how to make this more go-idiomatic then? Also, I don't think it's too java-like tbh.

Yep, renaming it to eds

yeah, I'm not the biggest fan of in object data processing either. 🤷‍♂️ We have two nested layers of abstraction here. I prefer everything in an explicit pipeline. We discussed this specific topic a few times already, and we decided to leave it as is. ref

if it works, it works

Yep, renaming it to eds

Yeah, I'm not opposed to short variable names if the code is obvious, a few lines long (the smaller the var's scope the shorter the var name could be). Here I'd say it is a matter of taste mostly. I think the name extendedDataSquare has little in common with extendedAbstractFactoryForWrappedCodedDataSqaure.

I prefer everything in an explicit pipeline.

Me too but I was also in favour of retiring rsmt2d and make just one concrete instantiation of it (with the only tree we want and everything we need) inside of this repo (one function creates the eds, another returns the row/col roots, done). For various reasons that were resolved for the most part in the meantime. Let's not get distracted by this right now.

Wondertan · 2021-05-20T21:06:21Z

types/block.go

+	// create the nmt wrapper to generate row and col commitments
+	squareSize := uint32(math.Sqrt(float64(len(shares))))
+	tree := wrapper.NewErasuredNamespacedMerkleTree(uint64(squareSize))
+


Having one more package for wrapper makes no sense for me in general

Wondertan · 2021-05-20T21:06:43Z

types/block.go

+	// create the nmt wrapper to generate row and col commitments
+	squareSize := uint32(math.Sqrt(float64(len(shares))))
+	tree := wrapper.NewErasuredNamespacedMerkleTree(uint64(squareSize))
+


Same for consts actually

Wondertan · 2021-05-20T21:10:22Z

types/block.go

+	// create the nmt wrapper to generate row and col commitments
+	squareSize := uint32(math.Sqrt(float64(len(shares))))
+	tree := wrapper.NewErasuredNamespacedMerkleTree(uint64(squareSize))
+


From experience I have in Golang: If I have an import cycle, it means I messed up proper logic segregation between packages architecturally, and simply extracting specific the piece of code causing the cycle is a simple hack, not a complete solution.

Wondertan · 2021-05-20T21:16:20Z

p2p/ipld/write_test.go

+	return res
+}
+
+func TestDataAvailabilityHeaderRewriteBug(t *testing.T) {


From experience I have in Golang: If I have an import cycle, it means I messed up proper logic segregation between packages architecturally, and simply extracting specific the piece of code causing the cycle is a simple hack, not a complete solution.

same. It's a "code smell" for sure. I think I'll leave it the way it is for now, but we could totally move the contents of the wrapper package to the types package. Besides a massive refactor, I don't think we have other better options. I'm certainly not attached to the current structure by any means, and am open to suggestions.

Wondertan

So I added just a bunch of minor thoughts, but overall looks good!

evan-forbes added 2 commits May 17, 2021 15:42

break consts and the nmt wrapper into their own packages

d5b9197

remove flattenNamespacedEDS

d7f93ff

evan-forbes self-assigned this May 17, 2021

docs for linter

0a5999f

This comment has been minimized.

Sign in to view

evan-forbes and others added 6 commits May 19, 2021 07:42

add test to ensure that overwrite bug does not occur

9d8ab3e

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

linter

5092bd8

rename test

a3c2e5f

Merge branch 'master' into evan/remove-flattenNamespacedEDS

a7d7aa6

adapt new stuff to this PR

607f3c3

one of these days I'll remeber to run the linter before pushing

aa988d7

evan-forbes commented May 20, 2021

View reviewed changes

evan-forbes marked this pull request as ready for review May 20, 2021 20:25

evan-forbes requested review from liamsi and tac0turtle as code owners May 20, 2021 20:25

evan-forbes requested review from Wondertan and removed request for tac0turtle May 20, 2021 20:26

This comment has been minimized.

Sign in to view

liamsi approved these changes May 20, 2021

View reviewed changes

types/block.go

copy(out, ParitySharesNamespaceID)

return out

}

Copy link

Member

liamsi May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So good!

Wondertan reviewed May 20, 2021

View reviewed changes

Wondertan approved these changes May 20, 2021

View reviewed changes

evan-forbes merged commit 280cc82 into master May 20, 2021

evan-forbes deleted the evan/remove-flattenNamespacedEDS branch May 20, 2021 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `flattenNamespacedEDS` #338

Remove `flattenNamespacedEDS` #338

evan-forbes commented May 17, 2021 •

edited

Loading

This comment has been minimized.

liamsi commented May 17, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

liamsi commented May 19, 2021 •

edited

Loading

evan-forbes May 20, 2021 •

edited

Loading

Wondertan May 20, 2021

Wondertan May 20, 2021

Wondertan May 20, 2021

Wondertan May 20, 2021

liamsi May 20, 2021 •

edited

Loading

evan-forbes commented May 20, 2021

This comment has been minimized.

liamsi left a comment •

edited

Loading

liamsi May 20, 2021

Wondertan May 20, 2021

liamsi May 20, 2021 •

edited

Loading

Wondertan May 20, 2021

evan-forbes May 20, 2021 •

edited

Loading

liamsi May 20, 2021 •

edited

Loading

Wondertan May 20, 2021

Wondertan May 20, 2021

Wondertan May 20, 2021

Wondertan May 20, 2021

evan-forbes May 20, 2021

Wondertan left a comment •

edited

Loading

Remove flattenNamespacedEDS #338

Remove flattenNamespacedEDS #338

Conversation

evan-forbes commented May 17, 2021 • edited Loading

Description

This comment has been minimized.

liamsi commented May 17, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

liamsi commented May 19, 2021 • edited Loading

evan-forbes May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi May 20, 2021 • edited Loading

Choose a reason for hiding this comment

evan-forbes commented May 20, 2021

This comment has been minimized.

liamsi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes May 20, 2021 • edited Loading

Choose a reason for hiding this comment

liamsi May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan left a comment • edited Loading

Choose a reason for hiding this comment

Remove `flattenNamespacedEDS` #338

Remove `flattenNamespacedEDS` #338

evan-forbes commented May 17, 2021 •

edited

Loading

liamsi commented May 19, 2021 •

edited

Loading

evan-forbes May 20, 2021 •

edited

Loading

liamsi May 20, 2021 •

edited

Loading

liamsi left a comment •

edited

Loading

liamsi May 20, 2021 •

edited

Loading

evan-forbes May 20, 2021 •

edited

Loading

liamsi May 20, 2021 •

edited

Loading

Wondertan left a comment •

edited

Loading