fix(validation): added da batch validation against state BD desc #1218

mtsitrin · 2024-11-11T12:21:12Z

refactored nextSequencer validation

PR Standards

Opening a pull request should be able to meet the following requirements

--

PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A

Close #XXX

<-- Briefly describe the content of this pull request -->

For Author:

Targeted PR against correct branch
included the correct type prefix in the PR title
Linked to Github issue with discussion and accepted design
Targets only one github issue
Wrote unit and integration tests
All CI checks have passed
Added relevant godoc comments

For Reviewer:

confirmed the correct type prefix in the PR title
Reviewers assigned
confirmed all author checklist items have been addressed

After reviewer approval:

In case targets main branch, PR should be squashed and merged.
In case PR targets a release branch, PR should be rebased.

refactored nextSequencer validation

block/slvalidator.go

mtsitrin · 2024-11-11T12:23:31Z

block/slvalidator.go

+	// last block of the batch
+	lastDABlock := daBlocks[len(daBlocks)-1]
+	expectedNextSeqHash := lastDABlock.Header.SequencerHash
+	if slBatch.NextSequencer != slBatch.Sequencer {


instead of using state proposer, doing slBatch.NextSequencer != slBatch.Sequencer to check for a change

skipped UpdateSequencerSetFromSL as we can assume sync with SL metadata

omritoptix

looks good though didn't figure out the extra validation of height vs end height on the retriever.

omritoptix · 2024-11-11T18:02:45Z

block/retriever.go

@@ -47,6 +47,12 @@ func (m *Manager) ApplyBatchFromSL(slBatch *settlement.Batch) error {
 			m.blockCache.Delete(block.Header.Height)
 		}
 	}
+
+	// validate the batch applied successfully and we are at the end height


if the batch isn't applied succesfully wouldn't we get an err from the applyBlockWithFraudHandling ?

we're looping over for i, block := range batch.Blocks, which data on the DA
it can be empty/missing

Not sure I got this @mtsitrin . Is it related to rollback on L1 truncating state info?

srene · 2024-11-12T13:18:33Z

block/slvalidator.go

@@ -47,7 +47,7 @@ func (v *SettlementValidator) ValidateStateUpdate(batch *settlement.ResultRetrie
 	for height := batch.StartHeight; height <= batch.EndHeight; height++ {
 		source, err := v.blockManager.Store.LoadBlockSource(height)
 		if err != nil {
-			v.logger.Error("load block source", "error", err)
+			// v.logger.Error("load block source", "error", err)


i dont think you need to remove this log. this will just happen in case no block is stored for the height, which should not happen, i think, because there will be always a block applied for the validated height.

I'll add the log back.
But I think it can happen
if u at height H, and the state info is [H-1, H+100],
u'll have 100 logs of it

im not sure this can happen. if you are at height h, and there is a new state update for h+100, then the node will sync first to height h+100, and then validate for the whole state info. upon reception of a new state update where endheight is higher than node height, it first calls the sync loop and from the sync loop it triggers validation once blocks are applied. so when a state info is validated the block should exist locally always, unless there is a bug.

yes u right
it actually handled by the validation added to the ApplyBatchFromSL

block/slvalidator.go

block/retriever.go

danwt · 2024-11-12T13:25:23Z

block/retriever.go

@@ -47,6 +47,12 @@ func (m *Manager) ApplyBatchFromSL(slBatch *settlement.Batch) error {
 			m.blockCache.Delete(block.Header.Height)
 		}
 	}
+
+	// validate the batch applied successfully and we are at the end height


Not sure I got this @mtsitrin . Is it related to rollback on L1 truncating state info?

block/slvalidator.go

danwt · 2024-11-12T13:38:22Z

block/slvalidator.go

@@ -84,8 +82,12 @@ func (v *SettlementValidator) ValidateStateUpdate(batch *settlement.ResultRetrie
 		if errors.Is(checkBatchResult.Error, da.ErrBlobNotIncluded) {
 			return types.NewErrStateUpdateBlobNotAvailableFraud(batch.StateIndex, string(batch.MetaData.DA.Client), batch.MetaData.DA.Height, hex.EncodeToString(batch.MetaData.DA.Commitment))
 		}
+
+		// FIXME: how to handle non-happy case? not returning error?
+		continue


Suggested change

continue

danwt · 2024-11-12T13:41:16Z

block/slvalidator.go

+	if numSLBlocks != numSlBDs || numDABlocks < numSlBDs {
+		return types.NewErrStateUpdateNumBlocksNotMatchingFraud(slBatch.EndHeight, numSLBlocks, numSLBlocks, numDABlocks)


why do we check numSLBlocks != numSlBDs . Isn't it guaranteed to be true by the hub?

block/slvalidator.go

added da batch validation against state BD desc

54cbcc8

refactored nextSequencer validation

mtsitrin requested a review from a team as a code owner November 11, 2024 12:21

mtsitrin requested review from omritoptix and srene November 11, 2024 12:21

github-actions bot added the dym-internal label Nov 11, 2024

mtsitrin commented Nov 11, 2024

View reviewed changes

block/slvalidator.go Outdated Show resolved Hide resolved

mtsitrin commented Nov 11, 2024

View reviewed changes

mtsitrin added 2 commits November 11, 2024 15:01

better validation when applying from SL

fda6a5c

fix UT

1ed5ee0

mtsitrin linked an issue Nov 11, 2024 that may be closed by this pull request

panics where there’s missing data in the DA #1217

Closed

omritoptix previously approved these changes Nov 11, 2024

View reviewed changes

mtsitrin requested a review from omritoptix November 12, 2024 10:29

srene reviewed Nov 12, 2024

View reviewed changes

pr comments

1fb40f5

mtsitrin dismissed omritoptix’s stale review via 1fb40f5 November 12, 2024 13:47

mtsitrin requested a review from srene November 12, 2024 13:55

srene approved these changes Nov 12, 2024

View reviewed changes

mtsitrin merged commit 3c33a48 into main Nov 12, 2024
4 checks passed

mtsitrin deleted the fix/DA_validation branch November 12, 2024 14:14

danwt reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(validation): added da batch validation against state BD desc #1218

fix(validation): added da batch validation against state BD desc #1218

mtsitrin commented Nov 11, 2024

mtsitrin Nov 11, 2024

omritoptix left a comment

omritoptix Nov 11, 2024

mtsitrin Nov 12, 2024

danwt Nov 12, 2024

srene Nov 12, 2024

mtsitrin Nov 12, 2024

srene Nov 12, 2024 •

edited

Loading

mtsitrin Nov 12, 2024

danwt Nov 12, 2024

danwt Nov 12, 2024

danwt Nov 12, 2024

		if numSLBlocks != numSlBDs \|\| numDABlocks < numSlBDs {
		return types.NewErrStateUpdateNumBlocksNotMatchingFraud(slBatch.EndHeight, numSLBlocks, numSLBlocks, numDABlocks)

fix(validation): added da batch validation against state BD desc #1218

fix(validation): added da batch validation against state BD desc #1218

Conversation

mtsitrin commented Nov 11, 2024

PR Standards

Opening a pull request should be able to meet the following requirements

Choose a reason for hiding this comment

omritoptix 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

Choose a reason for hiding this comment

srene Nov 12, 2024 • 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

srene Nov 12, 2024 •

edited

Loading