Repository navigation
Conversation
A snapshot only has the private state hashes files when the channel has private data hashes. NewSnapshotReader returns a nil reader when the data file does not exist, but findAndWriteDifferences calls Next on both readers without checking. When one snapshot has private data hashes and the other has none, compare panics with a nil pointer dereference instead of listing the records that are missing. Treat a nil reader as a reader without records. The test helper now writes the private state files only when there are private records, as a real snapshot does. Related to hyperledger#3878 Signed-off-by: Hamed Yousefi <hdyousefi@gmail.com>
Signed-off-by: Hamed Yousefi <hdyousefi@gmail.com>
Signed-off-by: Hamed Yousefi <hdyousefi@gmail.com>
Signed-off-by: Hamed Yousefi <hdyousefi@gmail.com>
Signed-off-by: Hamed Yousefi <hdyousefi@gmail.com>
Signed-off-by: Hamed Yousefi <hdyousefi@gmail.com>
Signed-off-by: Hamed Yousefi <hdyousefi@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of change
Description
A peer snapshot only has
private_state_hashes.dataandprivate_state_hashes.metadatawhen the channel has private data hashes.ExportPubStateAndPvtStateHashescreates the writer for these files when it sees the first hashed record, so a channel without private data gets no such files. The same is true for the public state files when there is no public state.privacyenabledstate.NewSnapshotReaderreturns a nil reader when the data file does not exist. The ledger's own import (worldStateSnapshotReader) checks for that.ledgerutil comparedoes not.findAndWriteDifferencescallsNext()on both readers, so when one snapshot has private data hashes and the other has none, compare panics:The hashes in
_snapshot_signable_metadata.jsondiffer in this case (one side has none), so compare does go on to read the files. A difference like this is the kind of divergence compare is meant to report.This change reads records through a small helper that treats a nil reader as a reader without records. The records of the other snapshot are then reported as missing, in the same way as records that are missing from one side of a data file.
Additional details
createSnapshotalways wrote the private state files, even with no private records, so the tests never had a snapshot without them. It now writes them only when there are private records, like a real snapshot. The existing cases still pass, because in all of them without private records the private hashes are equal and the private comparison is skipped.Tests:
TestCompare:second-missing-pvt-dataandfirst-missing-pvt-data. One snapshot has a private data hash record and the other has no private state files. Compare reports the record as missing on the other side.compare.goreverted to main, both cases panic as above. With the fix they pass.Ran:
go test -race ./internal/ledgerutil/... ./cmd/ledgerutil/...passes.Related issues
The same panic was reported in #3878. That issue was closed without a code change, and the panic still happens on main.