Add util for finding trie root hash #6094

zhangchiqing · 2024-06-13T21:31:36Z

This PR adds a util command to find trie root hash in the wal file. It allows us to roll back executed height to any height.

Context: the current util allows us to roll back executed height to any height, which remove the execution results in the database. However, the EN is not able to start up, because the checkpoint and wal file has not been rollback to the corresponding height.

Rolling back the checkpoint and wal has been quite difficult, we've made tool to find the block for the block, however, it only allow us to roll back executed height to the block of the checkpoint. In order to be able to roll back to any height, we need to find the trie root hash in the wal files, and this is what this find trie root hash util is made for.

zhangchiqing · 2024-06-13T21:32:25Z

ledger/complete/wal/encoding.go

@@ -61,6 +61,7 @@ func Decode(data []byte) (operation WALOperation, rootHash ledger.RootHash, upda
 	switch operation {
 	case WALUpdate:
 		update, err = ledger.DecodeTrieUpdate(data[1:])
+		rootHash = update.RootHash


This fixes a bug, that the root hash is not returned when decoding a record in the wal file.

peterargue · 2024-06-14T20:07:35Z

cmd/util/cmd/find-trie-root/cmd.go

+	flagOutputDir         string
+)
+
+var Cmd = &cobra.Command{


Can you add some high level docs about what this command does and how it should be used?

Added some comments, and also documented some detailed use case here

zhangchiqing · 2024-06-14T22:16:15Z

ledger/complete/wal/encoding_test.go

@@ -44,7 +43,7 @@ func TestUpdate(t *testing.T) {
 		operation, stateCommitment, up, err := realWAL.Decode(data)
 		require.NoError(t, err)
 		assert.Equal(t, realWAL.WALUpdate, operation)
-		assert.Equal(t, stateCommitment, ledger.RootHash(hash.DummyHash))
+		assert.Equal(t, stateCommitment, ledger.RootHash(rootHash))


Hey @fxamacker , could you review my change here?

I'm not sure why the test case originally expected the decoded root hash to be zero. Would like to double check in case I missed anything.

@zhangchiqing I think currently wal.Decode() returns un-initialized rootHash (empty rootHash) if operation is WALUpdate.

rootHash is only populated in wal.Decode() for WALDelete operation.

The existing test expecting empty rootHash fails because you changed wal.Decode() in this PR. This can be a breaking change in other use cases as well.

Given this, instead of modifying wal.Decode() function, you can use returned update.RootHash from wal.Decode() for your use case in this PR.

OK, I see why it was decided this way.

There are two types of operation: WALUpdate and WALDelete.

WALDelete only has the root hash. WALUpdate can be decoded into update, which contains the root hash.

So the operation is WALDelete, than the root hash is returned, and the update is nil; if the operation is WALUpdate, then the decoded update will be returned. So the root hash is only for the case that the operation is WALDelete.

But I still think in the case the operation is WALUpdate, it should return the root hash. There isn't any code reading the root hash when the operation is WALUpdate. So fixing it would not cause any problem.

This fix would also make my logic simplier, because I don't need to check the returned update object, which might be nil if the operation is WALDelete. I could just check the root hash and the operation.

I took your suggestion and reverted my changes, and also added some commends on the Decode method to explain the behavior.

cmd/util/cmd/find-trie-root/cmd.go

+	from, to, err := prometheusWAL.Segments(dir)
+	if err != nil {
+		return 0, 0, fmt.Errorf("cannot get segments: %w", err)
+	}


fxamacker · 2024-06-14T22:06:50Z

cmd/util/cmd/find-trie-root/cmd.go

+	w, err := prometheusWAL.NewSize(lg, prometheus.DefaultRegisterer, dir, wal.SegmentSize, false)
+	if err != nil {
+		return 0, 0, fmt.Errorf("cannot create WAL: %w", err)
+	}


Do we need to create w here? We can use dir to replace w.Dir() as parameter NewSegmentsRangeReader().

fxamacker · 2024-06-14T22:35:55Z

ledger/complete/wal/encoding_test.go

@@ -44,7 +43,7 @@ func TestUpdate(t *testing.T) {
 		operation, stateCommitment, up, err := realWAL.Decode(data)
 		require.NoError(t, err)
 		assert.Equal(t, realWAL.WALUpdate, operation)
-		assert.Equal(t, stateCommitment, ledger.RootHash(hash.DummyHash))
+		assert.Equal(t, stateCommitment, ledger.RootHash(rootHash))


@zhangchiqing I think currently wal.Decode() returns un-initialized rootHash (empty rootHash) if operation is WALUpdate.

rootHash is only populated in wal.Decode() for WALDelete operation.

The existing test expecting empty rootHash fails because you changed wal.Decode() in this PR. This can be a breaking change in other use cases as well.

Given this, instead of modifying wal.Decode() function, you can use returned update.RootHash from wal.Decode() for your use case in this PR.

fxamacker

I left some more comments. Maybe add some docs for functions like copyWAL() to describe high level purpose of functions.

fxamacker · 2024-06-14T23:18:02Z

cmd/util/cmd/find-trie-root/cmd.go

+	return 0, 0, fmt.Errorf("finish reading all segment files from %d to %d, but not found", from, to)
+}
+
+func copyWAL(dir, outputDir string, segment int, expectedRoot ledger.RootHash) error {


To help me understand:

Is copyWAL() intended to create a new segment file with all records up to and including first update record with specified rootHash?

If outputDir has existing wal files, the new segment file would be created with higher index as filename suffix. Is this expected?

Is copyWAL() intended to create a new segment file with all records up to and including first update record with specified rootHash?

Yes

If outputDir has existing wal files, the new segment file would be created with higher index as filename suffix. Is this expected?

yeah, good point. the outputDir is supposed to be different from the dir, (I can add a check to prevent that), so the generated wal is always 00000000 in a separate output folder.

fxamacker · 2024-06-14T23:18:54Z

cmd/util/cmd/find-trie-root/cmd.go

+		bytes := wal.EncodeUpdate(update)
+		_, err = writer.Log(bytes)
+		if err != nil {
+			return fmt.Errorf("cannot write LedgerWAL record: %w", err)
+		}


update can be nil for delete operation record. Maybe check operation before encoding record.

fxamacker

Nice! Thanks for updating. I focused on reviewing code related to wal files.

fxamacker · 2024-06-17T20:44:51Z

ledger/complete/wal/encoding.go

+// Decode decodes the given data into a WAL operation, root hash and trie update.
+// It returns (WALDelete, rootHash, nil, nil) if the operation is WALDelete.
+// It returns (WALUpdate, hash.DummyHash, update, nil) if the operation is WALUpdate.
+// To read the root hash of the trie update, use update.RootHash.


fxamacker · 2024-06-17T20:57:34Z

cmd/util/cmd/find-trie-root/cmd.go

+	w, err := prometheusWAL.NewSize(log.Logger, nil, dir, wal.SegmentSize, false)
+	if err != nil {
+		return fmt.Errorf("cannot create WAL: %w", err)
+	}


It seems w isn't used except for w.Dir(), so we can remove w and use dir directly.

Suggested change

w, err := prometheusWAL.NewSize(log.Logger, nil, dir, wal.SegmentSize, false)

if err != nil {

return fmt.Errorf("cannot create WAL: %w", err)

}

zhangchiqing · 2024-06-18T00:27:42Z

I added some logic to replace the WAL file once it found the matching wal file with the --trim-as-latest-wal=true flag

fxamacker

Thanks for automating this aspect as suggested! 👍 I left some more comments and suggestions.

fxamacker · 2024-06-18T00:55:52Z

cmd/util/cmd/find-trie-root/cmd.go

+	// genereate a segment file to the temporary folder with the root hash as its last record
+	newSegmentFile, err := findRootHashAndCreateTrimmed(flagExecutionStateDir, segment, rootHash)
+	if err != nil {
+		log.Fatal().Err(err).Msg("cannot copy WAL")
+	}


Maybe we can create temp directory in this function and pass it to findRootHashAndCreateTrimmed() so that temp directory can be removed in defer.

fxamacker · 2024-06-18T01:06:49Z

cmd/util/cmd/find-trie-root/cmd.go

+		backupFile := prometheusWAL.SegmentName(backupDir, i)
+
+		log.Info().Msgf("backup segment file %s to %s, %v/%v", segmentFile, backupFile, i, last)
+		err := os.Rename(segmentFile, backupFile)


Do we want to use copyFile() here in case backup directory is in a different device?

Yeah, I thought about this. If I use copy file, it will take some time, I think we most likely would move within the same device. If a different device is used, then it fails, they can retry again with a different path on the same device.

I could also check if they are different device, and then use copy and remove the old files, but that would be too much code. I decided to keep it simple.

fxamacker · 2024-06-18T01:07:49Z

cmd/util/cmd/find-trie-root/cmd.go

+	return nil
+}
+
+func copyFile(src, dst string) (int64, error) {


Instead of copying file, do we want to move file instead?

fxamacker · 2024-06-18T01:18:33Z

cmd/util/cmd/find-trie-root/cmd.go

+		if os.IsNotExist(err) {
+			return true, nil
+		}
+		return false, nil


Should we return error here?

Suggested change

return false, nil

return false, err

zhangchiqing added 7 commits June 7, 2024 20:56

find trie root

4681643

add output-dir

7bbceca

use equals

4606afc

update log

c1024c2

fix root hash

d5ef271

fix outputer

1018f74

fix log

540a27d

zhangchiqing commented Jun 13, 2024

View reviewed changes

zhangchiqing marked this pull request as ready for review June 13, 2024 21:32

zhangchiqing requested review from ramtinms and AlexHentschel as code owners June 13, 2024 21:32

zhangchiqing requested review from fxamacker, janezpodhostnik and peterargue and removed request for ramtinms and AlexHentschel June 13, 2024 21:32

janezpodhostnik approved these changes Jun 14, 2024

View reviewed changes

peterargue approved these changes Jun 14, 2024

View reviewed changes

zhangchiqing added 2 commits June 14, 2024 14:41

add comment

7fddffe

fix test case

10d6aee

zhangchiqing commented Jun 14, 2024

View reviewed changes

fxamacker reviewed Jun 14, 2024

View reviewed changes

address review comments

d16b8ea

zhangchiqing requested a review from fxamacker June 15, 2024 00:18

revert decoding changes

fa6f02d

zhangchiqing force-pushed the leo/v0.33-find-trie-root branch from d840dcf to fa6f02d Compare June 17, 2024 15:32

fxamacker approved these changes Jun 17, 2024

View reviewed changes

zhangchiqing added 2 commits June 17, 2024 16:31

implement backup wal files

3153ec2

copy file

d3dfed9

fix copy file

3d2e8f9

zhangchiqing requested review from peterargue and fxamacker June 18, 2024 00:27

fxamacker approved these changes Jun 18, 2024

View reviewed changes

fxamacker reviewed Jun 18, 2024

View reviewed changes

zhangchiqing added 2 commits June 17, 2024 21:40

address review comments

00a37c3

fix logging

3bf99df

zhangchiqing force-pushed the leo/v0.33-find-trie-root branch from f9e9e0b to 3bf99df Compare June 18, 2024 17:44

zhangchiqing added 2 commits June 18, 2024 11:17

adjust folder creation

e60aa2d

log folder creation

daaaa5c

zhangchiqing merged commit 82de39d into v0.33 Jun 18, 2024
50 of 51 checks passed

zhangchiqing deleted the leo/v0.33-find-trie-root branch June 18, 2024 18:45

zhangchiqing mentioned this pull request Jun 18, 2024

[Util] find trie root hash #6119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add util for finding trie root hash #6094

Add util for finding trie root hash #6094

zhangchiqing commented Jun 13, 2024

zhangchiqing Jun 13, 2024

peterargue Jun 14, 2024

zhangchiqing Jun 14, 2024

zhangchiqing Jun 14, 2024 •

edited

Loading

fxamacker Jun 14, 2024

zhangchiqing Jun 15, 2024

zhangchiqing Jun 17, 2024

This comment was marked as resolved.

fxamacker Jun 14, 2024

fxamacker Jun 14, 2024

fxamacker left a comment

fxamacker Jun 14, 2024

zhangchiqing Jun 15, 2024

fxamacker Jun 14, 2024

fxamacker left a comment

fxamacker Jun 17, 2024

fxamacker Jun 17, 2024

zhangchiqing commented Jun 18, 2024

fxamacker left a comment

fxamacker Jun 18, 2024

fxamacker Jun 18, 2024

zhangchiqing Jun 18, 2024

fxamacker Jun 18, 2024

fxamacker Jun 18, 2024

Add util for finding trie root hash #6094

Add util for finding trie root hash #6094

Conversation

zhangchiqing commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing Jun 14, 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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxamacker 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

fxamacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing commented Jun 18, 2024

fxamacker 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

zhangchiqing Jun 14, 2024 •

edited

Loading