[wip] rng: adds support for x86 rdrand/rdseed instructions when building using MSVC #22158

EthanHeilman commented at 1:57 AM on June 5, 2021: contributor

The bitcoin-core RNG uses the x86 RDSeed and RDRand instructions as an additional source of entropy. However RDSeed and RDRand are not used as an additional source of entropy even if the CPU supports them if the bitcoind binary was built using the the Microsoft C++ compiler, MSVC. This results from the fact that:

The bitcoin-core code base uses the GCC macros such as __x86_64__ to detect the CPU architecture. Thus on platforms that do not support the GCC macros the compiler will make the incorrect assumption that the underlying system does not support RDRand and RDSeed.
The bitcoin-core code base uses the GCC keyword __asm__ for inline assembly. This keyword is not supported for MSVC.

This PR adds support for RDRand/RDSeed to bitcoind when built with MSVC by adding MSVC supported macros to correctly determine the target architecture and by adding MSVC supported keywords for the HDSeed and HDRand x86 instructions. This PR includes hwrand_tests to validate this behavior.

EthanHeilman force-pushed on Jun 5, 2021

in src/test/hwrand_tests.cpp:33 in 507e0710fa outdated

  28 | +
  29 | +        SeedHardwareSlow(hasher);
  30 | +        hasher.Finalize(output);
  31 | +        
  32 | +        #if defined(__x86_64__) || defined(__amd64__) || defined(__i386__) || (defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64)))
  33 | +            bool isNotEqual = (memcmp(output, emptyoutput, DIGEST_LEN) != 0);

sipa commented at 2:38 AM on June 5, 2021:

Won't this fail on systems without rdrand?

EthanHeilman commented at 2:52 AM on June 5, 2021:

Yep on older x86 CPUs that don't support rdseed/rdrand, good catch! Let me add a check for g_rdrand_supported and g_rdseed_supported

EthanHeilman force-pushed on Jun 5, 2021

DrahtBot added the label Tests on Jun 5, 2021

maflcko removed the label Tests on Jun 5, 2021

maflcko added the label Utils/log/libs on Jun 5, 2021

in src/random.cpp:144 in 5d407e4ef8 outdated

 139 | +    for (int i = 0; i < 10; ++i) {
 140 | +        uint8_t ok = _rdrand64_step(&r1);
 141 | +        if (ok) break;
 142 | +    }
 143 | +    return r1;
 144 | +#elif defined(_MSC_VER) && defined(_M_IX86)

laanwj commented at 1:34 PM on June 7, 2021:

Do we really care about x86 32 bit here? We don't build binaries for it anymore. I'm also not sure if there are actually existing x86 32-bit CPUs that support these instructions.

EthanHeilman commented at 5:23 PM on June 7, 2021:

I have 32-bit x86 support only because bitcoin currently supports 32-bit x86 rd instructions and I am just extending what already exists to the MSVC compiler. If it is decided that bitcoin-core will no longer target 32-bit x86 than it seems reasonable to remove.

EthanHeilman commented at 5:31 PM on June 7, 2021:

I can put together a PR which removes 32-bit rdrand and rdseed support if that is something you think would be mergeable.

in src/random.cpp:193 in 5d407e4ef8 outdated

 188 | @@ -170,6 +189,26 @@ static uint64_t GetRdSeed() noexcept
 189 |          __asm__ volatile ("pause");
 190 |      } while(true);
 191 |      return r1;
 192 | +#elif defined(_MSC_VER) && defined(_M_X64)
 193 | +    //TODO: Make this actually work

laanwj commented at 1:37 PM on June 7, 2021:

This doesn't seem like a good TODO to leave :smile:

EthanHeilman commented at 5:17 PM on June 7, 2021:

OMG! I must be going blind from staring at code too long. How did I forget to delete that comment? As penance, let me write some unittests specifically targeting GetRdSeed and GetRdRand.

EthanHeilman force-pushed on Jun 7, 2021

EthanHeilman renamed this:
~~rng: adds support for x86 rdrand/rdseed instructions when building using MSVC~~
[wip] rng: adds support for x86 rdrand/rdseed instructions when building using MSVC
on Jun 7, 2021

EthanHeilman marked this as a draft on Jun 7, 2021

EthanHeilman commented at 5:34 PM on June 7, 2021: contributor

I'm putting this PR into draft until I get a chance to write more unittests and fix the linter issue.

rng: adds support for x86 rdrand/rdseed instructions when using MSVC 7f0ff8c2fd

EthanHeilman force-pushed on Jun 8, 2021

fanquake commented at 3:42 PM on March 24, 2022: member

@EthanHeilman is this something you're interested in working on further? @sipsorcery you might be interested?

sipsorcery commented at 4:07 PM on March 24, 2022: member

Concept ACK from me if it results in improved randomness with msvc.

EthanHeilman commented at 5:56 PM on April 4, 2022: contributor

@fanquake If bitcoin-core wants this then yes I would be interested in improving it. I put this PR on the back burner because i got the sense bitcoin-core was moving away from msvc as a build platform.

fanquake commented at 8:14 AM on April 5, 2022: member

i got the sense bitcoin-core was moving away from msvc as a build platform.

I don't think we have any plans to drop support for MSVC right now.

DrahtBot commented at 8:14 PM on April 7, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	sipsorcery

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24773 (Enable HW-accelerated implementations of SHA256 for MSVC builds by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake commented at 2:50 PM on December 7, 2022: member

If bitcoin-core wants this then yes I would be interested in improving it. @EthanHeilman do you think you'll be following up here? Note that there is some overlap here with #24773, where a GetCPUID is also being added for MSVC.

hebasto commented at 3:12 PM on December 7, 2022: member

Also rebasing could be useful to leverage Cirrus CI native Windows task.

EthanHeilman commented at 3:11 PM on December 13, 2022: contributor

@fanquake My free time has disappeared until the end of 2022, but I will return to this in Jan 2023

fanquake commented at 2:56 PM on June 2, 2023: member

Closing for now. Feel free to ping for a reopen if/when you want to return to this.

fanquake closed this on Jun 2, 2023

bitcoin locked this on Jun 1, 2024