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.

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

Building on 32-bit systems fail #494

Closed
steev opened this issue Nov 15, 2023 · 10 comments · Fixed by #529
Closed

Building on 32-bit systems fail #494

steev opened this issue Nov 15, 2023 · 10 comments · Fixed by #529
Assignees

Comments

@steev
Copy link

steev commented Nov 15, 2023

We have kyber in our Kali Linux repositories as a dependency of Merlin and one thing that has come up in our CI is that kyber does not build on 32-bit systems.

You can see our build logs for armel, armhf, and i386 at each of the links; the basic gist is that while it compiles fine on 32-bit, the tests are failing.

--- FAIL: Test_Example_DKG (0.14s)
panic: dkg: cannot process own deal: Error while decoding field {Name:SecShare PkgPath: Type:*share.PriShare Tag: Offset:12 Index:[1] Anonymous:false}: Error while decoding field {Name:I PkgPath: Type:int Tag: Offset:0 Index:[0] Anonymous:false}: detected a 32bit machine, please use either int64 or int32 [recovered]
	panic: dkg: cannot process own deal: Error while decoding field {Name:SecShare PkgPath: Type:*share.PriShare Tag: Offset:12 Index:[1] Anonymous:false}: Error while decoding field {Name:I PkgPath: Type:int Tag: Offset:0 Index:[0] Anonymous:false}: detected a 32bit machine, please use either int64 or int32
goroutine 19 [running]:
testing.tRunner.func1.2({0x821f8e0, 0x88903b0})
	/usr/lib/go-1.21/src/testing/testing.go:1545 +0x2ab
testing.tRunner.func1()
	/usr/lib/go-1.21/src/testing/testing.go:1548 +0x43e
panic({0x821f8e0, 0x88903b0})
	/usr/lib/go-1.21/src/runtime/panic.go:914 +0x1ec
go.dedis.ch/kyber/share/dkg/pedersen.(*DistKeyGenerator).Deals(0x88983c0)
	/builds/kalilinux/packages/golang-go.dedis-kyber/debian/output/source_dir/_build/src/go.dedis.ch/kyber/share/dkg/pedersen/dkg.go:301 +0x346
go.dedis.ch/kyber/examples.Test_Example_DKG(0x88fe0f0)
	/builds/kalilinux/packages/golang-go.dedis-kyber/debian/output/source_dir/_build/src/go.dedis.ch/kyber/examples/dkg_test.go:79 +0x48e
testing.tRunner(0x88fe0f0, 0x8262280)
	/usr/lib/go-1.21/src/testing/testing.go:1595 +0x120
created by testing.(*T).Run in goroutine 1
	/usr/lib/go-1.21/src/testing/testing.go:1648 +0x3dd

This is only one example of the failures, and I'm not sure if I should open one for each individually or not.

@ineiti
Copy link
Member

ineiti commented Nov 15, 2023

This is related to #492. I added some comments over there where I suggest we could probably change all ints to int64 and it should work.

The actual failing part is here: https://github.com/search?q=repo%3Adedis%2Fprotobuf+32bit&type=code, because if you would use the DKG in a mixed 32/64 bit environment, it will fail. However, following Merlin -> opaque, and looking how it is used there, I don't think it's using the DKG part of kyber at all. So if you can skip the tests, I think you should be safe.

You don't need to open issues for each failure.

@pierluca now that we have an actual failing use-case for #492, shall we try to fix it by putting int64 everywhere?

@steev
Copy link
Author

steev commented Dec 11, 2023

I was trying to skip all of the tests, however, while skipping them, I ran into the genDistSecrets in an init function, and this was causing a segfault; I'm not familiar enough with go, so for now, we just skip all of the tests, and hopefully we can re-enable them once there is a release with the 32bit support :)

@ineiti
Copy link
Member

ineiti commented Dec 12, 2023

Thanks for the comment. Can you elaborate a bit more, please? You wrote that you skipped all the tests, but then ran into a call to genDistSecrets. But I looked for it and couldn't find it anywhere in the code. Is it a typo? Or is this something in your code?

@steev
Copy link
Author

steev commented Dec 12, 2023

Hi, it's genDistSecret not Secrets, sorry about that!

And, what I attempted to do was t.Skip() the failing tests only, and ran into

panic: Error while decoding field {Name:SecShare PkgPath: Type:*share.PriShare Tag: Offset:12 Index:[1] Anonymous:false}: Error while decoding field {Name:I PkgPath: Type:int Tag: Offset:0 Index:[0] Anonymous:false}: detected a 32bit machine, please use either int64 or int32

goroutine 1 [running]:
go.dedis.ch/kyber/share/dkg/rabin.(*DistKeyGenerator).Deals(0xc63680)
        /<<PKGBUILDDIR>>/_build/src/go.dedis.ch/kyber/share/dkg/rabin/dkg.go:249 +0x218
go.dedis.ch/kyber/sign/dss.genDistSecret()
        /<<PKGBUILDDIR>>/_build/src/go.dedis.ch/kyber/sign/dss/dss_test.go:159 +0x1b4
go.dedis.ch/kyber/sign/dss.init.0()
        /<<PKGBUILDDIR>>/_build/src/go.dedis.ch/kyber/sign/dss/dss_test.go:37 +0x1a0
FAIL    go.dedis.ch/kyber/sign/dss      0.056s

but what i couldn't figure out was which test this was associated with as it was between TestMask and TestEdDSAMarchalling and both of those tests passed.

@ineiti
Copy link
Member

ineiti commented Dec 12, 2023

but what i couldn't figure out was which test this was associated with as it was between TestMask and TestEdDSAMarchalling and both of those tests passed.

That might be go test which doesn't cleanly separates the output between different tests. I think it tries to run things in parallel, and doesn't always correctly handle output coming from different portions of the system.

Instead of t.Skip, we could add a build constraint to the file: https://pkg.go.dev/cmd/go#hdr-Build_constraints

What architecture are you working on?

@steev
Copy link
Author

steev commented Dec 12, 2023

I work with amd64, arm64, armhf, armel and i386 - so 2 64-bit and 3 32-bit :) I think overall though, maybe the constraints are overkill, and this all goes away when 492 is taken care of, so maybe not focus on this at all?

@ineiti
Copy link
Member

ineiti commented Dec 12, 2023

If we can fix it with a build constraint it can be solved in v3 of kyber. For #492 we need to update the version to v4, which seems to be somewhat planned for next Spring / Summer. But this is not really confirmed yet :)

So, yes, #492 is the way to go. But it'll take time. Speak up if you're looking for a project...

@pierluca what do you think about adding some build constraints in the dkg directories? We either add explicitly working 64-bit architectures, or exclude 32-bit ones. I have a small preference for excluding, as I believe it's more rare to have 32-bit nowadays, than seeing a new 64-bit popping up where we'd need to add a new entry.

@pierluca
Copy link
Contributor

pierluca commented Jan 8, 2024

Sorry for the slow response.
Yes, excluding 32-bit architectures would be a reasonable build constraint, but then again, I'm not sure we can call this exactly backwards-compatible...
I would also favor (1) documenting this and (2) addressing it as part of #492 and a v4 release, considering this has been an issue for a long time.

@ineiti
Copy link
Member

ineiti commented Jan 9, 2024

Yes, excluding 32-bit architectures would be a reasonable build constraint, but then again, I'm not sure we can call this exactly backwards-compatible...

It currently panics on 32-bit architectures, so there is no compatibility to be kept :)

I would also favor (1) documenting this and (2) addressing it as part of #492 and a v4 release, considering this has been an issue for a long time.

I thought adding some comments in the package itself. Do you want also comments in the main README.md?

And yes, if there is a v4, this should definitely be addressed at that moment.

@steev do you have some cycles to make a first PR for this? It should contain:

Else I think later this month I can also do it.

@matteosz matteosz self-assigned this Jun 6, 2024
@matteosz matteosz linked a pull request Jun 9, 2024 that will close this issue
@pierluca
Copy link
Contributor

Fixed in #529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants