Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Merged by Bors] - Fix misleading error #4725

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Fix misleading error #4725

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec0b687
Improve logging when nipost_challenge.bin doesn't exist
poszu Jul 19, 2023
5bb8bb9
Fix reading CRC when file is too small
poszu Jul 19, 2023
a9f63e3
Change log level to Warning
poszu Jul 19, 2023
5e22282
Fix flaky UT
poszu Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion activation/activation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,9 +527,11 @@ func (b *Builder) PublishActivationTx(ctx context.Context) error {

challenge, err := b.loadChallenge()
if err != nil {
if !errors.Is(err, os.ErrNotExist) {
logger.With().Error("failed to load atx challenge", log.Err(err))
}
poszu marked this conversation as resolved.
Show resolved Hide resolved
logger.With().Info("building new atx challenge",
log.Stringer("current_epoch", b.currentEpoch()),
log.Err(err),
)
challenge, err = b.buildNIPostChallenge(ctx)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions activation/nipost_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ func read(path string) ([]byte, error) {
if err != nil {
return nil, fmt.Errorf("failed to get file info %s: %w", path, err)
}
if fInfo.Size() < crc64.Size {
return nil, fmt.Errorf("file %s is too small", path)
}

data := make([]byte, fInfo.Size()-crc64.Size)
checksum := crc64.New(crc64.MakeTable(crc64.ISO))
Expand Down
10 changes: 10 additions & 0 deletions activation/nipost_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,16 @@ func TestReadCRC(t *testing.T) {
data: []byte("spacemesh"),
crc: []byte{0xC7, 0x11, 0x73, 0xE0, 0x53, 0xD8, 0x75, 0x0F},
errMsg: "wrong checksum 0xC71173E053D8750F, computed 0xC61072DF52D7740E",
}, {
name: "file too short",
data: []byte("123"),
crc: []byte{},
errMsg: "too small",
}, {
name: "file empty",
data: []byte(""),
crc: []byte{0xC6, 0x10, 0x72, 0xDF, 0x52, 0xD7, 0x74, 0x0E},
errMsg: "wrong checksum 0xC61072DF52D7740E, computed 0x0",
},
Comment on lines +66 to 71
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this test. The CRC64 of an empty file should be 0x0000000000000000 and if the file only contains those bytes it will pass reading & CRC validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is confusing? It's exactly as you said. CRC of nothing is 0. The test file contains a wrong CRC and no data hence it should fail with "wrong checksum 0xC61072DF52D7740E, computed 0x0" message.

Copy link
Member

Choose a reason for hiding this comment

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

but isn't this just a repetition of the "badCRC" testcase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's an edge case when the input is empty.

Copy link
Member

Choose a reason for hiding this comment

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

Again back to my first comment: an empty input is not an error, it has a valid checksum. This test does not check if empty input is handled correctly (that would be "" and 0x0), it checks if a wrong checksum is detected (as does the "badCRC" testcase)

}

Expand Down