Tudor/aggressive header compression #1502

tudor-malene · 2023-09-05T17:19:25Z

Why this change is needed

add compression of the batches

What changes were made as part of this PR

no longer compress the actual batch headers
instead, create a data structure that allows the recreation of the headers (see readme)
remove some POBI checks from the validation

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

PR checks reviewed and performed

StefanIliev545 · 2023-09-06T13:23:43Z

go/common/headers.go

+	BatchTimeDeltas []*big.Int // todo - minimize assuming a default of 1 sec and then store only exceptions
+
+	ReOrgs         []*BatchHeader // sparse list of reorged headers - non null only for reorgs.
+	L1HeightDeltas []*big.Int     // Contains the L1 height on the indexes where it changes. Todo - can be optimised with deltas


This field name and the description are confusing.

go/enclave/components/batch_executor.go

StefanIliev545 · 2023-09-06T13:35:48Z

go/enclave/components/rollup_compression.go

+func (rc *RollupCompression) CreateExtRollup(r *core.Rollup) (*common.ExtRollup, error) {
+	header, err := rc.createRollupHeader(r.Batches)
+	if err != nil {
+		return nil, err


Would be good to wrap those errors in mroe descriptive ones; When running through errors in the logs it can be hard to find out during which bit of processing they happened.

I know!
I was thinking we should use a library everywhere that does the wrapping and recreates some sort of sane stacktraces. I hate to manually wrap everything, and write some random repetitive text just to know where something happened.

https://pkg.go.dev/github.com/pkg/errors - I think this one automatically creates stack traces

let's do this as part of a different work. And add everywhere.
We can use this library or another one

otherview · 2023-09-06T14:41:31Z

go/common/rpc/converters.go

@@ -208,7 +208,7 @@ func ToExtRollupMsg(rollup *common.ExtRollup) generated.ExtRollupMsg {
 		return generated.ExtRollupMsg{}
 	}

-	return generated.ExtRollupMsg{Header: ToRollupHeaderMsg(rollup.Header), BatchPayloads: rollup.BatchPayloads, BatchHeaders: rollup.BatchHeaders}
+	return generated.ExtRollupMsg{Header: ToRollupHeaderMsg(rollup.Header), BatchPayloads: rollup.BatchPayloads, BatchHeaders: rollup.CalldataRollupHeader}


Should BatchHeaders name be changed ?

I think it's ok. It is the compressed batch headers

Yeah, I think the name is fine! But it seems like the common.ExtRollup.BatchHeaders was changed to common.ExtRollup.CalldataRollupHeader so maybe the generated.ExtRollupMsg.BatchHeaders to generated.ExtRollupMsg.CalldataRollupHeader change was missed ?

go/enclave/components/batch_registry.go

otherview · 2023-09-06T14:44:29Z

go/enclave/components/rollup_compression.go

+4. The Signatures over the batches are not stored, since the rollup is itself signed.
+5. The cross chain messages are calculated.
+*/
+type RollupCompression struct {


Would be cool to have a unit tests for this struct with a few different data scenarios.

go/enclave/components/rollup_compression.go

otherview · 2023-09-06T14:55:51Z

go/enclave/components/rollup_compression.go

+		if can.Hash() != batch.Hash() {
+			// if the canonical batch of the same height is different from the current batch
+			// then add the entire header to a "reorgs" array
+			reorgs[i] = batch.Header
+			isReorg = true
+		} else {
+			reorgs[i] = nil
+		}
+		batchHashes[i] = batch.Hash()
+		batchHeaders[i] = batch.Header


What if the append() was used here for the reorgs and reorgs was a var regorgs []*common.BatchHeader instead of an initialized array? Would that simplify the reorgs array ? Then checking for len(reorgs) would be the same as isReorg ?

I'm still trying to find the best way to express this. It's very fiddly, and there is still an off-by-one error somewhere when there are reorgs

…er_compression # Conflicts: # integration/simulation/validate_chain.go

BedrockSquirrel

LGTM but a lot to take in! Couple of minor comments

BedrockSquirrel · 2023-09-12T09:11:05Z

go/enclave/components/rollup_compression.go

+	reorgs := make([]*common.BatchHeader, len(batches))
+
+	deltaTimes := make([]*big.Int, len(batches))
+	startTime := batches[0].Header.Time


Might be worth checking that the batches list isn't empty here? Avoid panic risk/give back a useful error if things get weird.

there is such a check when the "Rollup" is created.

BedrockSquirrel · 2023-09-12T09:19:50Z

go/enclave/components/rollup_compression.go

+	// get the first canonical batch
+	firstCanonBatchHeight := batches[0].Number()
+	firstCanonParentHash := batches[0].Header.ParentHash
+	for i, reorg := range reorgs {


Took me a while to figure out this loop, maybe worth a little comment to say it's looping to find the first batch with no reorg or something.

Also, weird edge case to think about, if we somehow had a rollup that was only reorgs, would it brick the network because the first canon data wouldn't align with other rollups?

otherview · 2023-09-12T09:55:47Z

go/enclave/components/rollup_compression.go

+	// get the first canonical batch
+	firstCanonBatchHeight := batches[0].Number()
+	firstCanonParentHash := batches[0].Header.ParentHash
+	for i, reorg := range reorgs {


Can more details be added in the coment ? - this logic is not straighforward

Oops, goland just reloaded and now saw that Matt made the same comment.

…er_compression

otherview · 2023-09-12T10:07:03Z

integration/simulation/simulation_full_network_test.go

-		AvgBlockDuration:      1 * time.Second,
-		SimulationTime:        75 * time.Second,
-		L1EfficiencyThreshold: 0.2,
-		// Very hard to have precision here as blocks are continually produced and not dependent on the simulation execution thread


Why removing these 2 Efficiencies ?

they have been obsolete for ages.

tudor-malene added 6 commits September 5, 2023 19:28

add compression

1b2d06c

add compression

d2365a4

reestablish batch broadcast

7cc6550

clarify and document

7257251

update the l1 proofs

123a9a6

rename

29d7855

StefanIliev545 reviewed Sep 6, 2023

View reviewed changes

go/enclave/components/batch_executor.go Outdated Show resolved Hide resolved

StefanIliev545 reviewed Sep 6, 2023

View reviewed changes

otherview reviewed Sep 6, 2023

View reviewed changes

tudor-malene added 6 commits September 8, 2023 10:57

fixes

fefdd62

fix tests

82845c0

Merge remote-tracking branch 'origin/main' into tudor/aggressive_head…

7c3b885

…er_compression # Conflicts: # integration/simulation/validate_chain.go

fix

eb2f1de

more fixes

07b870a

more fixes

3939223

tudor-malene marked this pull request as ready for review September 12, 2023 08:13

tudor-malene added 2 commits September 12, 2023 11:39

more fixes

3695298

more fixes

9ddc5a3

BedrockSquirrel reviewed Sep 12, 2023

View reviewed changes

otherview reviewed Sep 12, 2023

View reviewed changes

tudor-malene added 2 commits September 12, 2023 12:58

add comment

9d171d3

Merge remote-tracking branch 'origin/main' into tudor/aggressive_head…

07b8295

…er_compression

otherview reviewed Sep 12, 2023

View reviewed changes

otherview approved these changes Sep 12, 2023

View reviewed changes

fix proto name

cc485c4

tudor-malene merged commit 764f98d into main Sep 12, 2023
2 checks passed

tudor-malene deleted the tudor/aggressive_header_compression branch September 12, 2023 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tudor/aggressive header compression #1502

Tudor/aggressive header compression #1502

tudor-malene commented Sep 5, 2023 •

edited

Loading

StefanIliev545 Sep 6, 2023 •

edited

Loading

StefanIliev545 Sep 6, 2023

tudor-malene Sep 7, 2023

StefanIliev545 Sep 7, 2023

tudor-malene Sep 12, 2023

otherview Sep 6, 2023

tudor-malene Sep 7, 2023

otherview Sep 12, 2023

otherview Sep 6, 2023

otherview Sep 6, 2023

tudor-malene Sep 7, 2023

BedrockSquirrel left a comment

BedrockSquirrel Sep 12, 2023

tudor-malene Sep 12, 2023

BedrockSquirrel Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023 •

edited

Loading

otherview Sep 12, 2023

tudor-malene Sep 12, 2023

Tudor/aggressive header compression #1502

Tudor/aggressive header compression #1502

Conversation

tudor-malene commented Sep 5, 2023 • edited Loading

Why this change is needed

What changes were made as part of this PR

PR checks pre-merging

StefanIliev545 Sep 6, 2023 • 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

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

BedrockSquirrel left a 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

otherview Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor-malene commented Sep 5, 2023 •

edited

Loading

StefanIliev545 Sep 6, 2023 •

edited

Loading

otherview Sep 12, 2023 •

edited

Loading