Revert merge of PR #31826 #31908
Revert merge of PR #31826 #31908
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31908. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Up to reviewers to decide whether this or #31902 is more appropriate, but added to milestone so we do this before branch off. |
It was actually tested on the merged PR: see #31826 (comment) and #31826 (comment) but a later refactoring suggestion (#31826 (comment)) introduced the compilation issue and no re-testing was done before merge. I think if @laanwj and @giahuy98 are willing to re-test on #31902 we could consider merging the fix instead. |
i did actually test it extensively, but yes it was changed and i hadn't gotten around to reviewing or re-testing it yet. |
Ostensibly we do though, as the code is part of the aarch64 platform which we do have CI for. However, it appears that our ARM CI does not have |
Having the ARM64 RNG code in the first place makes sense, and we could test this on Graviton instances. But also agree that a workaround for an errata of specific SoC series is really on the edge of what is in scope for this project. Or user code in the first place, for that matter. Ideally a broken implementation of a feature is fixed in the kernel. Either by disabling the functionality on that specific SoC or another workaround like a microcode update or a hook for the instruction. |
|
ACK 3e9b12b |
I believe it was disabled for non-members of the repository because we were seeing too much spamming from that functionality. |
I don't think they have a 32-bit mode, so we'd have to drop that: bitcoin/ci/test/00_setup_env_arm.sh Line 9 in e606c57 So if something is added, it would be a new task? Moreover, there are also GHA runners with |
|
Just wanted to add that even though we discovered the issue on mobile devices (Samsung S25 and OnePlus 13), it looks like the same chip will be shipped in several laptop brands/models as well: https://www.google.com/search?q=laptop+snapdragon+x+elite My colleague @giahuy98 did test the original fix (#31826) on a real S25 device (it works), and @laanwj tested as well. Unfortunately, after that, there were further code changes (code refactoring) that did not get tested. |
They seem to have Aarch32 support! To verify this i spun up a
Not exactly the same chip (X Elite versus 8 Elite). But t's possible that they have the same problem. Would be good if someone with one of those laptops would test it. @achow101 suggested doing so in WSL might be easiest as they're nominally Windows devices, and we don't build Bitcoin Core for ARM64 Windows. To be clear my point is not that this isn't worth fixing. The question is where. I think it's ridiculous to have to implement a fix for a CPU issue in all end-user software. But it's not totally unheard of. These kind of model-specific workarounds happen a lot in gamedev GPU programming, historically a wasteland of out-of-spec hardware and drivers, but it's somewhat worrying to see it crossing to CPUs which you'd expect to have a higher standard. As said, ideally a fix would land in the kernel, as this would automatically fix userland. Linux support for these devices is still in the early stage, so it's not unimaginable. But testing and ACKing the original PR says enough in that i'm also not against to carrying the change, if necessary. It's not a big complication. |
Thanks for checking. I guess we'd still have to drop the 32-bit cross compilation (to compile the code here), if we wanted to do it in CI (or add a separate task)? Though, I wonder if CI is a good way to test such changes, because CI doesn't print the debug logs on success and part of the testing involves looking at debug logs. As a side note, for testing the happy-case, apart from m7g on aws, one can also use c4a on gcp, or the GHA arm runner (no idea what it is, but the docs are here: https://github.com/actions/partner-runner-images/blob/main/images/arm-ubuntu-24-image.md) |
Right, to make it usefully tested in CI we'd need a specific test case for it, that checks the instruction set is successfully detected and used. Otherwise all you're testing is "it doesn't hang". Which is, something at least. |
I think one could argue that the fix should be made both in the kernel and in Core. The design of From Nunchuk’s perspective, we reuse Core code directly and have thousands of users on these mobile devices. We have no certainty about when or whether a kernel fix will land on these devices. If there’s no fix in Core itself, we’d have to carry our own patch in a separate fork, which would be brittle and unfortunate. |
|
Some additional context: the PR that introduced RNDR/RNDRRS support for AArch64 (#26839) was merged about a year ago, and AFAIK it was only tested on Amazon Graviton 3 before merging—so it’s not like the feature itself has been extensively battle-tested across different SoCs. If someone feels this fix is too specific to a single chip and wants to revert it, the same reasoning could apply to the original PR itself. That might imply reverting the whole RNDR/RNDRRS feature, which doesn’t seem ideal given it works fine on other hardware and improves performance. Instead, I think it’s reasonable to fix it in Core and encourage a proper fix in the kernel, rather than remove the feature outright. |
|
If we accept the fact that we have assembly RNDR/RNDRRS code in our project, I don't think it's unreasonable that this also means taking on the responsibility of dealing with weird hardware incompatibility issues it causes, in general. If we don't want that, we can choose to not have this support at all and just rely on the kernel's RNG. But having merged something which doesn't compile on the platform it's support to fix things for is a red flag, and makes me feel that if we want to spend effort on this again, it should also come with CI that can detect regressions with it in the future, or other reasons to be confident it actually works and keeps working. |
|
I agree completely, @sipa. From a process perspective, it might be worth adopting a policy that any code changes not covered by CI must be verified on the relevant hardware—or at least a suitable emulator—before merging. In this particular case, the original changes were tested, but the subsequent refactor was not. Had it been tested, we would have caught the compile error. |
…513955891 29513955891 kernel: Add pure kernel bitcoin-chainstate 9c40433bd4a kernel: Add functions to get the block hash from a block 942df8f287f kernel: Add block index utility functions to C header 87102db87ac kernel: Add function to read block undo data from disk to C header 12b8c9442ad kernel: Add functions to read block from disk to C header d977db3feb2 kernel: Add function for copying block data to C header 8ae33627743 kernel: Add functions for the block validation state to C header 0565a0bbc01 kernel: Add validation interface to C header 837e5a0f536 kernel: Add interrupt function to C header a80b7bfe3de kernel: Add import blocks function to C header 54d1a1231ec kernel: Add chainstate load options for in-memory dbs in C header 659efa9969c kernel: Add options for reindexing in C header 2179127c079 kernel: Add block validation to C header 26143992693 kernel: Add chainstate loading when instantiating a ChainstateManager 82d2bebbe54 kernel: Add chainstate manager option for setting worker threads e875f520851 kernel: Add chainstate manager object to C header 4e486059178 kernel: Add notifications context option to C header a5eb699b978 kernel: Add chain params context option to C header 0818b8d2c07 kernel: Add kernel library context object 71c24c95b31 kernel: Add logging to kernel library C header 0cc810386f7 kernel: Introduce initial kernel C header API 82ba9257157 Merge bitcoin/bitcoin#31366: cmake: Check `-Wno-*` compiler options for `leveldb` target f236854a5bd Merge bitcoin/bitcoin#31731: doc: update translation generation cmake example eb51963d870 Merge bitcoin/bitcoin#31884: cmake: Make implicit `libbitcoinkernel` dependencies explicit 58f15d4b215 Merge bitcoin/bitcoin#31379: cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree e606c577cb2 Merge bitcoin/bitcoin#31899: cmake: Exclude generated sources from translation 758a93d6215 doc: update translation generation cmake example fd14995b6a8 Merge bitcoin/bitcoin#31908: Revert merge of PR #31826 3e9b12b3e0f Revert "Merge bitcoin/bitcoin#31826: random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop" 785649f3977 Merge bitcoin/bitcoin#29881: guix: use GCC 13 to build releases 139640079ff Merge bitcoin/bitcoin#31826: random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop dc3a7146337 Merge bitcoin/bitcoin#31794: wallet: abandon orphan coinbase txs, and their descendants, during startup 06757af2da5 Merge bitcoin/bitcoin#29156: tests: add functional test for miniscript decaying multisig ff4ddd3d2e3 Revert "cmake: Ensure generated sources are up to date for `translate` target" 03b3166aac5 cmake: Exclude generated sources from translation 43e287b3ff5 Merge bitcoin/bitcoin#31892: build: remove ENABLE_HARDENING condition from check-security 63d625f7610 Merge bitcoin/bitcoin#31893: test: remove scanning check on `wallet_importdescriptors` 3b42e05aa9e cmake: Make implicit `libbitcoinkernel` dependencies explicit 3fd64efb437 cmake: Avoid using `OBJECT` libraries 28dec6c5f8b Merge bitcoin/bitcoin#31268: cmake: add optional source files to bitcoin_crypto and crc32c directly 50afaf3a389 Merge bitcoin/bitcoin#31836: contrib: Add deterministic-fuzz-coverage 405dd0e647e test: remove scanning check on `wallet_importdescriptors` 113a7a363fa build: remove ENABLE_HARDENING cond from check-security 9da0820ec55 Merge bitcoin/bitcoin#31869: cmake: Add `libbitcoinkernel` target db36a92c02b Merge bitcoin/bitcoin#31879: doc: add release note for #27432 (utxo-to-sqlite tool) 95722d048a8 doc: add release note for #27432 (utxo-to-sqlite tool) e4dd5a351bd test: wallet, abandon coinbase txs and their descendants during startup 09b150bb8ad In `InitHardwareRand`, do trail test for `RNDRRS` by `VerifyRNDRRS` 43e71f74988 Merge bitcoin/bitcoin#27432: contrib: add tool to convert compact-serialized UTXO set to SQLite database e53310c47ab Merge bitcoin/bitcoin#30529: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior 254fd89d39f Merge bitcoin/bitcoin#31863: random: Initialize variables in hardware RNG functions 75f8396c907 Merge bitcoin/bitcoin#30746: test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests c4b46b45898 Merge bitcoin/bitcoin#31629: wallet: fix rescanning inconsistency d0dfd6d3f60 Merge bitcoin/bitcoin#31865: build: move `rpc/external_signer` to node library ce4dbfc3590 Merge bitcoin/bitcoin#31851: doc: build: Fix instructions for msvc gui builds 504d0c21e26 Merge bitcoin/bitcoin#31439: validation: In case of a continued reindex, only activate chain in the end 0b48f77e101 Merge bitcoin/bitcoin#31413: rpc: Remove deprecated dummy alias for listtransactions::label 21a0efaf8c9 Merge bitcoin/bitcoin#29858: test: Add test for rpcwhitelistdefault 8a00b755e98 Merge bitcoin/bitcoin#31634: doc: Improve dependencies documentation e58605e04f3 Merge bitcoin/bitcoin#31854: net: reduce CAddress usage to CService or CNetAddr 3a914ab96bd cmake: Rename `bitcoinkernel` component to `libbitcoinkernel` 06b9236f432 Merge bitcoin/bitcoin#31359: cmake: Add `CheckLinkerSupportsPIE` module 7ce09a59921 cmake: Add `libbitcoinkernel` target e501246e77c build: move rpc/external_signer to node library 73e2ec13737 Merge bitcoin/bitcoin#31844: cmake: add a component for each binary 99755e04ffa random: Initialize variables in hardware RNG functions 7bbd761e816 Merge bitcoin/bitcoin#31421: cmake: Improve compatibility with Python version managers 9491676438a Merge bitcoin/bitcoin#31157: Cleanups to port mapping module post UPnP drop 109bfe9573b Merge bitcoin/bitcoin#31857: depends: avoid an unset `CMAKE_OBJDUMP` 14d1d8e2120 Merge bitcoin/bitcoin#31758: test: deduplicates p2p_tx_download constants fa3e409c9a0 contrib: Add deterministic-fuzz-coverage 2549fc6fd1c Merge bitcoin/bitcoin#31768: test: check `scanning` field from `getwalletinfo` 9b033bebb18 cmake: rename Kernel component to bitcoinkernel for consistency 2e0c92558e9 cmake: add and use install_binary_component a85e8c0e615 doc: Add some general documentation about negated options 96d30ed4f96 Merge bitcoin/bitcoin#31495: wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases 490c8fa1782 doc: Add release notes summarizing negated option behavior changes. 458ef0a11b5 refactor: Avoid using IsArgSet() on -connect list option 752ab9c3c65 test: Add test to make sure -noconnect disables -dnsseed and -listen by default 3c2920ec98f refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options d05668922a2 refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options 3d1e8ca53a0 Normalize inconsistent -noexternalip behavior ecd590d4c1e Normalize inconsistent -noonlynet behavior 5544a19f863 Fix nonsensical bitcoin-cli -norpcwallet behavior 6e8e7f433fc Fix nonsensical -noasmap behavior b6ab3508064 Fix nonsensical -notest behavior 6768389917a Fix nonsensical -norpcwhitelist behavior e03409c70f7 Fix nonsensical -norpcbind and -norpcallowip behavior 40c4899bc20 Fix nonsensical -nobind and -nowhitebind behavior 5453e66fd91 Fix nonsensical -noseednode behavior c242fa5be35 Merge bitcoin/bitcoin#31858: chore: remove redundant word 4c62b37fcd2 chore: remove redundant word 251ea7367cf Merge bitcoin/bitcoin#31767: logging: Ensure -debug=0/none behaves consistently with -nodebug 2434aeab62b depends: avoid an unset CMAKE_OBJDUMP a5b0a441f85 Merge bitcoin/bitcoin#31855: chore: remove redundant word cd4bfaee103 net: reduce CAddress usage to CService or CNetAddr 033acdf03da chore: remove redundant word 55cf39e4c54 Merge bitcoin/bitcoin#31722: cmake: Copy `cov_tool_wrapper.sh.in` to the build tree c3fa043ae56 doc: build: Fix instructions for msvc gui builds 048ef98626b Merge bitcoin/bitcoin#31840: depends: add missing Darwin objcopy 713bf66b1f7 Merge bitcoin/bitcoin#31500: depends: Fix compiling `libevent` package on NetBSD ede388d03df Merge bitcoin/bitcoin#30911: build: simplify by flattening the dependency graph 534414ca9d4 Merge bitcoin/bitcoin#31678: ci: Skip read-write of default env vars 87ce116058f Merge bitcoin/bitcoin#31846: test: Remove stale gettime test fa3a4eafa11 test: Remove stale gettime test 42251e00e8b Merge bitcoin/bitcoin#30584: depends: Make default `host` and `build` comparable 0b6ed342b57 Merge bitcoin/bitcoin#31711: build: set build type and per-build-type flags as early as possible a44ccedcc2c Merge bitcoin/bitcoin#31818: guix: remove test-security/symbol-check scripts 0264c5d86c7 cmake: use per-target components for bitcoin-qt and bitcoin-gui fb0546b1c5e ci: don't try to install for a fuzz build c65233230f1 Merge bitcoin/bitcoin#31022: test: Add mockable steady clock, tests for PCP and NATPMP implementations 86528937e5c Merge bitcoin/bitcoin#31834: build: disable bitcoin-node if daemon is not built 7afeaa24693 test: `-debug=0` and `-debug=none` behave similarly to `-nodebug` a8fedb36a71 logging: Ensure -debug=0/none behaves consistently with -nodebug d39d521d86a test: `-nodebug` clears previously set debug options 3edaf0b4286 depends: add missing Darwin objcopy 2507ebdf1b2 Merge bitcoin/bitcoin#31837: test: add missing sync to p2p_tx_download.py 79f02d56ef7 Merge bitcoin/bitcoin#30623: test: Fuzz the human-readable part of bech32 as well ff3171f96d3 Merge bitcoin/bitcoin#31614: test: expect that files may disappear from /proc/PID/fd/ 56a9b847bba build: set build type and per-build-type flags as early as possible 8fe552fe6e0 test: add missing sync to p2p_tx_download.py af76664b12d test: Test migration of a solvable script with no privkeys 17f01b0795e test: Test migration of taproot output scripts 1eb9a2a39fd test: Test migration of miniscript in legacy wallets e8c3efc7d8f wallet migration: Determine Solvables with CanProvide fa1b7cd6e2c migration: Skip descriptors which do not parse 440ea1ab639 legacy spkm: use IsMine() to extract watched output scripts b777e84cd70 legacy spkm: Move CanProvide to LegacyDataSPKM b1ab927bbf2 tests: Test migration of additional P2WSH scripts 1d813e4bf52 Merge bitcoin/bitcoin#31819: doc: swap CPPFLAGS for APPEND_CPPFLAGS 2ffea09820e build: disable bitcoin-node if daemon is not built f8d3e0edf47 Merge bitcoin/bitcoin#30205: test: add mocked Sock that can read/write custom data and/or CNetMessages 6b165f5906f Merge bitcoin/bitcoin#31384: mining: bugfix: Fix duplicate coinbase tx weight reservation 6a46be75c43 Merge bitcoin/bitcoin#31793: ci: Use clang-20 for sanitizer tasks 76c090145e9 guix: remove test-security/symbol-check scripts 329b60f595e Merge bitcoin/bitcoin#31810: TxOrphanage: account for size of orphans and count announcements bc3f59ca530 Merge bitcoin/bitcoin#31820: build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in dead9086543 cmake: Improve compatibility with Python version managers e107bf78f9d [fuzz] TxOrphanage::SanityCheck accounting fb0ada982a7 Merge bitcoin/bitcoin#31811: test: test_inv_block, use mocktime instead of waiting f5b9a2f68c9 build: use CLIENT_NAME in libbitcoinkernel.pc.in ea687d20293 doc: swap CPPFLAGS for APPEND_CPPFLAGS 81eb6cc2c60 Merge bitcoin/bitcoin#31800: depends: Avoid using the `-ffile-prefix-map` compiler option 2f98d1e06ed Merge bitcoin/bitcoin#31814: ci: Bump fuzz task timeout 9cf746d6631 cmake: add optional source files to crc32c directly 9c7823c5b53 cmake: add optional source files to bitcoin_crypto directly faca7ac1321 ci: Bump fuzz task timeout 22dccea5532 [fuzz] txorphan byte accounting 982ce101781 add orphanage byte accounting to TxDownloadManagerImpl::CheckIsEmpty() c289217c014 [txorphanage] track the total number of announcements e5ea7daee01 [txorphanage] add per-peer weight accounting 672c69c688f [refactor] change per-peer workset to info map within orphanage 59cd0
PR #31826 was merged despite the code not compiling.
#31902 was opened to fix the code but since this code is only targeting a not officially supported platform, we don't have a CI in place to compile and run tests on this platform, neither apparently reviewers do (nor does the author?), don't take more risk right before 29 and revert the original broken PR.