💬 pstratem commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#issuecomment-3055626490)
> Concept ACK, but I'm not convinced this implementation is safe as-is. If we want to maintain the current behaviour, it's not sufficient to update only when the tip changes. We also need to re-check when importing/reindexing completes, and schedule an update timer if max_tip_age is the final cause of not exiting IBD.
This made me revisit the function and consider what we're trying to achieve.
The function is only interesting when it can latch to the IBD finished state.
That's only poss
...
(https://github.com/bitcoin/bitcoin/pull/32885#issuecomment-3055626490)
> Concept ACK, but I'm not convinced this implementation is safe as-is. If we want to maintain the current behaviour, it's not sufficient to update only when the tip changes. We also need to re-check when importing/reindexing completes, and schedule an update timer if max_tip_age is the final cause of not exiting IBD.
This made me revisit the function and consider what we're trying to achieve.
The function is only interesting when it can latch to the IBD finished state.
That's only poss
...
💬 achow101 commented on issue "guix: Windows build is non-deterministic across build architectures":
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055693691)
#32597 being the cause of this non-determinism doesn't make any sense to me.
The function with the non-determinsm is `WalletModel::prepareTransaction` which is a GUI only function that was not modified by #32597. That PR didn't change anything remotely near that, so it being the root cause makes no sense.
addr2line indicates that the problematic line corresponds to https://github.com/bitcoin/bitcoin/blob/a40e9536588c366886de4f4b9d67b8665a509929/src/prevector.h#L174 After unwrapping all of the
...
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055693691)
#32597 being the cause of this non-determinism doesn't make any sense to me.
The function with the non-determinsm is `WalletModel::prepareTransaction` which is a GUI only function that was not modified by #32597. That PR didn't change anything remotely near that, so it being the root cause makes no sense.
addr2line indicates that the problematic line corresponds to https://github.com/bitcoin/bitcoin/blob/a40e9536588c366886de4f4b9d67b8665a509929/src/prevector.h#L174 After unwrapping all of the
...
📝 achow101 opened a pull request: "Resolve guix non-determinism with emplace_back instead of push_back"
(https://github.com/bitcoin/bitcoin/pull/32930)
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64 results in a single instruction difference which can be traced down to prevector.h:174. The ultimate caller of this is the copy constructor for a prevector that ends up being called by std::vector::push_back in walletmodel.cpp:183. By replacing the push_back with an emplace_back, somehow this non-determinism goes away.
Closes #32923
(https://github.com/bitcoin/bitcoin/pull/32930)
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64 results in a single instruction difference which can be traced down to prevector.h:174. The ultimate caller of this is the copy constructor for a prevector that ends up being called by std::vector::push_back in walletmodel.cpp:183. By replacing the push_back with an emplace_back, somehow this non-determinism goes away.
Closes #32923
💬 maflcko commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3055855671)
lgtm ACK 3add4b365c90c0943f18dd26d4a18c1e2ee5860e
Haven't tested a guix build, but the changes lgtm
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3055855671)
lgtm ACK 3add4b365c90c0943f18dd26d4a18c1e2ee5860e
Haven't tested a guix build, but the changes lgtm
💬 achow101 commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3055856542)
x86_64 guix build:
```
$ find guix-build-3add4b365c90/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
5d3ec518a66bcff88ae4032a352119945ecae1cad0337bdadc84315dd9405a2c guix-build-3add4b365c90/output/dist-archive/bitcoin-3add4b365c90.tar.gz
9a317eb639f20191d13df01c11cd71d6da0f2593c9b6048f882ef40e0a080cc1 guix-build-3add4b365c90/output/x86_64-w64-mingw32/SHA256SUMS.part
083fb802296373e8d0faa4fdda4ce072f0f95f9443e9ba0682cb5da6dc55d7b6 guix-build-3add4b365c90/output/x86
...
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3055856542)
x86_64 guix build:
```
$ find guix-build-3add4b365c90/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
5d3ec518a66bcff88ae4032a352119945ecae1cad0337bdadc84315dd9405a2c guix-build-3add4b365c90/output/dist-archive/bitcoin-3add4b365c90.tar.gz
9a317eb639f20191d13df01c11cd71d6da0f2593c9b6048f882ef40e0a080cc1 guix-build-3add4b365c90/output/x86_64-w64-mingw32/SHA256SUMS.part
083fb802296373e8d0faa4fdda4ce072f0f95f9443e9ba0682cb5da6dc55d7b6 guix-build-3add4b365c90/output/x86
...
💬 maflcko commented on issue "guix: Windows build is non-deterministic across build architectures":
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055873569)
> The function with the non-determinsm is `WalletModel::prepareTransaction` which is a GUI only function that was not modified by [#32597](https://github.com/bitcoin/bitcoin/pull/32597). That PR didn't change anything remotely near that, so it being the root cause makes no sense.
My guess would be that one of the O2 passes (or the linker) is confused by the `std::map` changes in the header. (`wallet.h` is included in that gui module)
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055873569)
> The function with the non-determinsm is `WalletModel::prepareTransaction` which is a GUI only function that was not modified by [#32597](https://github.com/bitcoin/bitcoin/pull/32597). That PR didn't change anything remotely near that, so it being the root cause makes no sense.
My guess would be that one of the O2 passes (or the linker) is confused by the `std::map` changes in the header. (`wallet.h` is included in that gui module)
💬 dstadulis commented on issue "guix: Windows build is non-deterministic across build architectures":
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055875782)
Nice catch!
tldr: seems there should be no *logical* difference between the two -- but CMIIW:
## Assessing the differences
Two changes exist between the two segments of machine code:
1. Registry Extension prefix of the `cmpq` instruction
2. The encoding of the operands are switched, in x86 assembly syntax convention.
Let's try to decode the logical effect of the changes!
A consequential, control-flow op code `cmpq` which sets "[condition codes](https://www.cs.cmu.edu/afs/cs/academic/class/
...
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055875782)
Nice catch!
tldr: seems there should be no *logical* difference between the two -- but CMIIW:
## Assessing the differences
Two changes exist between the two segments of machine code:
1. Registry Extension prefix of the `cmpq` instruction
2. The encoding of the operands are switched, in x86 assembly syntax convention.
Let's try to decode the logical effect of the changes!
A consequential, control-flow op code `cmpq` which sets "[condition codes](https://www.cs.cmu.edu/afs/cs/academic/class/
...
⚠️ achow101 opened an issue: "guix: Zip file non-determinism when building in WSL"
(https://github.com/bitcoin/bitcoin/issues/32931)
In debugging #32923, I found that while it is possible to do a guix build in WSL on a aarch64 machine, the resulting zip files did not match the build on x86_64. The binaries within the zip files matched, and it appears the non-determinism comes from zip including the file permissions. This is presumably because the WSL build occurs on a NTFS drive which handles file permissions differently from other unix file systems.
Diffoscope output:
```diff
--- x86_64-zipinfo 2025-07-09 23:52:39.650231781
...
(https://github.com/bitcoin/bitcoin/issues/32931)
In debugging #32923, I found that while it is possible to do a guix build in WSL on a aarch64 machine, the resulting zip files did not match the build on x86_64. The binaries within the zip files matched, and it appears the non-determinism comes from zip including the file permissions. This is presumably because the WSL build occurs on a NTFS drive which handles file permissions differently from other unix file systems.
Diffoscope output:
```diff
--- x86_64-zipinfo 2025-07-09 23:52:39.650231781
...
💬 Sjors commented on issue "guix: Windows build is non-deterministic across build architectures":
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055925920)
The most recent change in `prevector.h`, and the only one that hasn't been shipped in a release yet, was a40e9536588c366886de4f4b9d67b8665a509929. But that just removed the reverse iterator, which this doesn't use afaik.
You could try initiazing `std::vector<CRecipient> vecSend;` with the size of `recipients`, which might result in different code paths. Though even if that restored determinism it wouldn't fix the underlying issue.
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055925920)
The most recent change in `prevector.h`, and the only one that hasn't been shipped in a release yet, was a40e9536588c366886de4f4b9d67b8665a509929. But that just removed the reverse iterator, which this doesn't use afaik.
You could try initiazing `std::vector<CRecipient> vecSend;` with the size of `recipients`, which might result in different code paths. Though even if that restored determinism it wouldn't fix the underlying issue.
💬 achow101 commented on issue "guix: Windows build is non-deterministic across build architectures":
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055948842)
#32930 seems like it results in a different enough code path to workaround this issue.
Ultimately, this seems like an upstream compiler bug.
(https://github.com/bitcoin/bitcoin/issues/32923#issuecomment-3055948842)
#32930 seems like it results in a different enough code path to workaround this issue.
Ultimately, this seems like an upstream compiler bug.
💬 Sjors commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3055983620)
Concept ACK
I'll spin up a new aarch64 guix machine to test if this _actually_ makes determinism go away. Even if it doesn't the change itself is fine since it removes a line, but you'd have to change the commit message.
3add4b365c90c0943f18dd26d4a18c1e2ee5860e builds and passes the tests locally for me on Apple Silicon macOS 15.5. Not sure what's up with CI.
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3055983620)
Concept ACK
I'll spin up a new aarch64 guix machine to test if this _actually_ makes determinism go away. Even if it doesn't the change itself is fine since it removes a line, but you'd have to change the commit message.
3add4b365c90c0943f18dd26d4a18c1e2ee5860e builds and passes the tests locally for me on Apple Silicon macOS 15.5. Not sure what's up with CI.
💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2196815142)
I think the function from a functionality perspective is fine, but symbols in the signature could be renamed for clarity.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2196815142)
I think the function from a functionality perspective is fine, but symbols in the signature could be renamed for clarity.
👍 stratospher approved a pull request: "validation: invalid block handling followups"
(https://github.com/bitcoin/bitcoin/pull/32843#pullrequestreview-3004293734)
ACK e32e72039f02ea0b565e0ef5ab7fa40faeb65e4c.
(https://github.com/bitcoin/bitcoin/pull/32843#pullrequestreview-3004293734)
ACK e32e72039f02ea0b565e0ef5ab7fa40faeb65e4c.
💬 maflcko commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3056019241)
Congrats, you are triggering another compiler bug. See 9999dbc1bd171931f02266d7c1a5cfd39f49238e for more details.
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3056019241)
Congrats, you are triggering another compiler bug. See 9999dbc1bd171931f02266d7c1a5cfd39f49238e for more details.
💬 Sjors commented on issue "Guix build fails on M4 macOS host with Ubuntu in UTM":
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3056031910)
Going to setup a fresh UTM instance, this time using Apple Virtualization instead of Qemu. Let's see if that (a) works and (b) doesn't break again after a few months.
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3056031910)
Going to setup a fresh UTM instance, this time using Apple Virtualization instead of Qemu. Let's see if that (a) works and (b) doesn't break again after a few months.
💬 maflcko commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#discussion_r2196837764)
```suggestion
vecSend.emplace_back(CRecipient {DecodeDestination(rcp.address.toStdString()), rcp.amount, rcp.fSubtractFeeFromAmount});
```
I don't have an apple, but I guess this may be the patch to work around the xcode compiler bug. I don't know if that still works around the non-determinism.
As a completely different alternative, you could also try https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2141169742 to remove the `std::string` construction from the header file a
...
(https://github.com/bitcoin/bitcoin/pull/32930#discussion_r2196837764)
```suggestion
vecSend.emplace_back(CRecipient {DecodeDestination(rcp.address.toStdString()), rcp.amount, rcp.fSubtractFeeFromAmount});
```
I don't have an apple, but I guess this may be the patch to work around the xcode compiler bug. I don't know if that still works around the non-determinism.
As a completely different alternative, you could also try https://github.com/bitcoin/bitcoin/pull/32597#discussion_r2141169742 to remove the `std::string` construction from the header file a
...
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2196962104)
> I think there are a number of issues with this part of the test:
I also noticed those in my review, but also noticed you mentioned it already.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2196962104)
> I think there are a number of issues with this part of the test:
I also noticed those in my review, but also noticed you mentioned it already.
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2196975759)
yeah, i think the `std::hash` can be dropped here
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2196975759)
yeah, i think the `std::hash` can be dropped here
🚀 fanquake merged a pull request: "test: Turn rpcauth.py test into functional test"
(https://github.com/bitcoin/bitcoin/pull/32881)
(https://github.com/bitcoin/bitcoin/pull/32881)
📝 maflcko opened a pull request: "test: Add missing convert_to_json_for_cli"
(https://github.com/bitcoin/bitcoin/pull/32932)
(https://github.com/bitcoin/bitcoin/pull/32932)