GetRandBytes() Hangs on Samsung Galaxy S25 and OnePlus 13 #31817

issue giahuy98 openend this issue on February 7, 2025
  1. giahuy98 commented at 1:56 pm on February 7, 2025: none

    Is there an existing issue for this?

    • I have searched the existing issues

    Current behaviour

    We have been using Bitcoin Core natively in the Nunchuk Bitcoin wallet, available on both desktop and mobile. On Samsung Galaxy S25 and OnePlus 13 (both running Android 15 with the SM8750-AB Snapdragon 8 Elite 3nm processor), the app fails to initialize due to an issue in GetRandBytes().

    The problem occurs when GetRandBytes() calls GetRNDRRS(), resulting in an infinite loop without retrieving entropy from the hardware.

     0uint64_t GetRNDRRS() noexcept
     1{
     2    uint8_t ok;
     3    uint64_t r1;
     4    do {
     5        // https://developer.arm.com/documentation/ddi0601/2022-12/AArch64-Registers/RNDRRS--Reseeded-Random-Number
     6        __asm__ volatile("mrs %0, s3_3_c2_c4_1; cset %w1, ne;"
     7                         : "=r"(r1), "=r"(ok)::"cc");
     8        if (ok) break;
     9        __asm__ volatile("yield");
    10    } while (true);
    11    return r1;
    12}
    

    Build with: Android NDK 25.1.8937393 & 27.2.12479018

    Related PR: #26839

    Tested on commit: 57b47c47ef0bd36e1c32d709c62998c51dc76f34

    Expected behaviour

    GetRandBytes() should return entropy without hanging.

    Steps to reproduce

    • Build Bitcoin Core for Android on master or 57b47c47ef0bd36e1c32d709c62998c51dc76f34
    • Call GetRandBytes
    • Function gets stuck

    Relevant log output

    No response

    How did you obtain Bitcoin Core

    Compiled from source

    What version of Bitcoin Core are you using?

    master@57b47c47ef0bd36e1c32d709c62998c51dc76f34

    Operating system and version

    Android 15

    Machine specifications

    No response

  2. eval-exec commented at 11:57 am on February 8, 2025: contributor

    I have Samsung Galaxy S23 Ultra:

    Image

    Image

  3. laanwj commented at 10:25 pm on February 13, 2025: member

    Build with: Android NDK 25.1.8937393 & 27.2.12479018

    We’ve had some problems building on android (including NDK 25, see #29360), and are curious how you do the builds for that platform, for testing, can you list more specific build instructions please?

  4. hugohn commented at 10:37 pm on February 13, 2025: contributor
    Hey @laanwj ! Our Nunchuk Android app is open source at https://github.com/nunchuk-io/nunchuk-android. You can take a look at the build instructions included in the repo. Let us know if you have any questions!
  5. laanwj commented at 11:04 pm on February 13, 2025: member
    Thanks! i’d looked there but couldn’t find anything that compiles Bitcoin Core, or any C++ code for that matter.
  6. hugohn commented at 11:33 pm on February 13, 2025: contributor

    You need to first compile the SDK, which is a wrapper for libnunchuk (written in C++ and reuses Core code). @laanwj

    https://github.com/nunchuk-io/nunchuk-android-nativesdk

  7. achow101 closed this on Feb 19, 2025

  8. pull[bot] referenced this in commit 139640079f on Feb 19, 2025
  9. glozow commented at 6:44 pm on February 19, 2025: member
    Has not been fixed since #31908 reverted #31826, but also out of scope - see #31908 (comment). I believe this should stay closed.
  10. hugohn commented at 7:16 am on April 10, 2025: contributor

    This should be re-opened as per latest discussion in #31908 @glozow

    TL;DR: Either test and merge the updated fix (https://github.com/bitcoin/bitcoin/pull/31912), or revert the RNDR/RNDRRS feature entirely.

  11. maflcko commented at 8:39 am on April 10, 2025: member

    TL;DR: Either test and merge the updated fix (#31912), or revert the RNDR/RNDRRS feature entirely.

    31912 is currently a draft, so before further review and testing (and merge), it would be good to get something non-draft ready first.

  12. maflcko reopened this on Apr 10, 2025

  13. laanwj commented at 11:06 am on April 10, 2025: member

    Reflecting on this after a while, i’m still not convinced that a hardware feature not working to spec needs to be handled in user space. IMO this needs to be fixed (or worked around otherwise) in the kernel, as per #31908 (comment).

    Edit: If you can point ot another project (such as OpenSSL) merging code to handle this edge case i’d be more easily convinced we need this.

  14. glozow commented at 3:07 pm on April 10, 2025: member

    This should be re-opened as per latest discussion in #31908 @glozow

    I don’t see any recent discussion on that thread? I’m not trying to die on this hill or say this problem isn’t bad, but there are 414 issues open and imo we should only keep the ones that fall within the scope of what this project supports.

  15. maflcko added the label Android on Apr 10, 2025
  16. maflcko added the label Linux/Unix on Apr 10, 2025
  17. maflcko removed the label Linux/Unix on Apr 10, 2025
  18. maflcko added the label Utils/log/libs on Apr 10, 2025
  19. maflcko added the label Build system on Apr 10, 2025
  20. maflcko added the label Upstream on Apr 10, 2025
  21. hugohn commented at 0:11 am on April 11, 2025: contributor

    @laanwj Reposting my earlier comment here:

    “I think one could argue that the fix should be made both in the kernel and in Core.

    The design of GetRandBytes() and GetStrongRandBytes() is about being robust and sourcing entropy from multiple sources, so that even if one or some of those sources fail, Core can still obtain sufficiently good entropy. There could be multiple reasons hardware entropy fails to return lower down in the stack.”

    Since NOT getting entropy from RNDR/RNDRRS is not a dealbreaker, Core shouldn’t freeze if the RNDR/RNDRRS stack fails for any reason (hardware or otherwise), and there should exist a sensible timeout. The question is about the robustness of the feature design itself.

  22. sipa commented at 0:22 am on April 11, 2025: member
    @hugohn The reason it was reverted wasn’t because the patch didn’t work - it could have been fixed instead. It was reverted because merging it broke the build for more platforms than ones suffering just this issue, and this went undetected by all processes we have in place (both manual review and CI infrastructure). It’s one thing to accept a patch to improve compatibility with an unsupported platform if it doesn’t affect others, but this event clearly showed we weren’t equipped to properly assess that. My view is that we shouldn’t try to fix this again until/unless there is infrastructure in place that would help us detect build failures like this going forward.
  23. hugohn commented at 2:38 am on April 11, 2025: contributor

    @sipa What would it take to add the CI infrastructure needed? From @maflcko’s comments it sounds like it’s possible?

    In the meantime, given that RNDR/RNDRRS is only supported on a small subset of hardware, it might be prudent to temporarily revert the feature until CI support or hardware adoption is more robust. This would align with a conservative approach.

  24. maflcko commented at 5:41 am on April 11, 2025: member

    The discussion is a bit spread out, but when I looked in #31908 (comment), the feature wasn’t compiled into the release binaries. This raises two questions:

    • Why? My understanding is that the kernel headers in guix should provide the HWCAP2_RNG macro?
    • What is the target audience? Currently it only seems to be self-compiled executables.

    About the CI: It is possible, but testing compilation or the path on a “happy” device doesn’t help with the issue here on “sad” devices. See #31908 (comment)

  25. laanwj commented at 6:06 am on April 11, 2025: member

    Agree, let’s just drop the support for now, it’s rarely supported (broken on like half of the chips that support it in the first place), apparently not compiled into the release binary anyway, hard to test, and there are plenty of other sources of entropy.

    It’s not worth so much discussion, or CI work.

  26. laanwj referenced this in commit 7749d929a0 on Apr 11, 2025
  27. bitcoin deleted a comment on Apr 11, 2025
  28. achow101 closed this on Apr 16, 2025

  29. achow101 referenced this in commit dfa2813e31 on Apr 16, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-04-29 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me