💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853679146)
why is this false and not an empty optional? As mentioned before, I don't really see the line
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853679146)
why is this false and not an empty optional? As mentioned before, I don't really see the line
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853676537)
as mentioned before, I find this tri-state-boolean quite confusing - if 3 states are indeed needed, maybe it's hiding a different structure such as a variant or enum or `optional<Error>`
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853676537)
as mentioned before, I find this tri-state-boolean quite confusing - if 3 states are indeed needed, maybe it's hiding a different structure such as a variant or enum or `optional<Error>`
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853672421)
will the `node0_cookie_file_path_tmp` file be kept when assertion fails on purpose? If not, you could wrap it in a try/finally (deleting regardless of the assertion's outcome)
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853672421)
will the `node0_cookie_file_path_tmp` file be kept when assertion fails on purpose? If not, you could wrap it in a try/finally (deleting regardless of the assertion's outcome)
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853701435)
Yes, I was confused by indentation, resolve the comment por favor
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853701435)
Yes, I was confused by indentation, resolve the comment por favor
💬 hebasto commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2493443094)
I'm a bit hesitant about this change.
> Conceptually there are many other nightly tasks, which rarely find issues and are not run by default, like the valgrind or s390x tasks. So putting the Wine unit tests in the same bucket should be fine.
I disagree. Conceptually, this is a different case because the cross-compiled `test_bitcoin.exe` is part of release bundle for Windows. And leaving the repo without a CI job to run it seems like a drawback.
> A follow-up could run them on real Windo
...
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2493443094)
I'm a bit hesitant about this change.
> Conceptually there are many other nightly tasks, which rarely find issues and are not run by default, like the valgrind or s390x tasks. So putting the Wine unit tests in the same bucket should be fine.
I disagree. Conceptually, this is a different case because the cross-compiled `test_bitcoin.exe` is part of release bundle for Windows. And leaving the repo without a CI job to run it seems like a drawback.
> A follow-up could run them on real Windo
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2493443688)
Updated a9b71eadb8eff5530500cdb7d7227b8575948df6 -> fc67047b7e1fb7031285f790ea3a7ea349474f31 ([kernelApi_3](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_3) -> [kernelApi_4](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_3..kernelApi_4))
* Made the `user_data` argument passed in with the callbacks `const` to better convey that the library doesn't do anything with it besides passing it back to the user whe
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2493443688)
Updated a9b71eadb8eff5530500cdb7d7227b8575948df6 -> fc67047b7e1fb7031285f790ea3a7ea349474f31 ([kernelApi_3](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_3) -> [kernelApi_4](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_3..kernelApi_4))
* Made the `user_data` argument passed in with the callbacks `const` to better convey that the library doesn't do anything with it besides passing it back to the user whe
...
👍 rkrux approved a pull request: "test: Rework wallet_migration.py to use previous releases"
(https://github.com/bitcoin/bitcoin/pull/31248#pullrequestreview-2454103454)
reACK 55347a5
Reviewed the range diff - only minor comment, commit message, renaming changes. Ran `cmake` again.
```
git range-diff a76ad56...55347a5
```
(https://github.com/bitcoin/bitcoin/pull/31248#pullrequestreview-2454103454)
reACK 55347a5
Reviewed the range diff - only minor comment, commit message, renaming changes. Ran `cmake` again.
```
git range-diff a76ad56...55347a5
```
⚠️ Sjors opened an issue: "ci: how to run native arm job on Apple silicon?"
(https://github.com/bitcoin/bitcoin/issues/31344)
I'm trying to run the `arm64` worker job (ARM, unit tests, no functional tests) on my M4 mac, because that should be lot faster than emulating it in Qemu on my x86_64 machine.
Unfortunately so far it fails:
```
[10:34:36.739] Test project /ci_container_base/ci/scratch/build-arm-linux-gnueabihf
[10:34:36.742] Start 2: util_rpcauth_test
[10:34:36.744] Start 4: univalue_object_test
[10:34:36.747] Start 6: secp256k1_tests
[10:34:36.751] Start 8: tes
...
(https://github.com/bitcoin/bitcoin/issues/31344)
I'm trying to run the `arm64` worker job (ARM, unit tests, no functional tests) on my M4 mac, because that should be lot faster than emulating it in Qemu on my x86_64 machine.
Unfortunately so far it fails:
```
[10:34:36.739] Test project /ci_container_base/ci/scratch/build-arm-linux-gnueabihf
[10:34:36.742] Start 2: util_rpcauth_test
[10:34:36.744] Start 4: univalue_object_test
[10:34:36.747] Start 6: secp256k1_tests
[10:34:36.751] Start 8: tes
...
💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2493503739)
> the cross-compiled `test_bitcoin.exe` is part of release bundle for Windows. And leaving the repo without a CI job to run it seems like a drawback.
How is this different from the whole macos-cross release, or even the real binary `bitcoind.exe`, which is never tested? Moreover, if you think that running the windows unit tests on Wine is useful, it would be good to give at least one example where it caught an issue that was found by none of the other tasks. (For example, they can't even dete
...
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2493503739)
> the cross-compiled `test_bitcoin.exe` is part of release bundle for Windows. And leaving the repo without a CI job to run it seems like a drawback.
How is this different from the whole macos-cross release, or even the real binary `bitcoind.exe`, which is never tested? Moreover, if you think that running the windows unit tests on Wine is useful, it would be good to give at least one example where it caught an issue that was found by none of the other tasks. (For example, they can't even dete
...
👍 laanwj approved a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2451750295)
Must admit that i'm not versed enough in the subtle specifics with regard to validity and spendability of scripts, to be sure all of this is correct. But where i've checked, the behavior matches the IsMine-based implementation. Overall changes LGTM.
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2451750295)
Must admit that i'm not versed enough in the subtle specifics with regard to validity and spendability of scripts, to be sure all of this is correct. But where i've checked, the behavior matches the IsMine-based implementation. Overall changes LGTM.
💬 laanwj commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1852300265)
comment: -there
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1852300265)
comment: -there
💬 laanwj commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1852336507)
Maybe add an assertion on `solutions.size() >= 2`,as an internal check against future bugs in Solve (and maybe also in other places before the solutions vector is indexed).
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1852336507)
Maybe add an assertion on `solutions.size() >= 2`,as an internal check against future bugs in Solve (and maybe also in other places before the solutions vector is indexed).
💬 laanwj commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1853733972)
Doing both inserting into and reading from spks here makes it somewhat harder to be confident that the behavior is independent from the specific order of processing mapScripts, but the comment above helps.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1853733972)
Doing both inserting into and reading from spks here makes it somewhat harder to be confident that the behavior is independent from the specific order of processing mapScripts, but the comment above helps.
💬 laanwj commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1853729309)
comment: in "spks"
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1853729309)
comment: in "spks"
💬 fanquake commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#discussion_r1853761691)
Bash is implied/available, so we are able to drop this.
(https://github.com/bitcoin/bitcoin/pull/31323#discussion_r1853761691)
Bash is implied/available, so we are able to drop this.
💬 maflcko commented on issue "ci: how to run native arm job on Apple silicon?":
(https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493529510)
My recommendation would be to try without the cirrus overhead for debugging. Just copy-pasting the one-line bash command may be easier and faster for troubleshooting. See https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally
As for the error itself: It is not possible to run 32-bit arm binaries on a CPU that only supports 64-bit mode.
I don't know how to fix this, but if nothing helps, you'll have to select qemu-arm in UTM. (Is there a reason why you haven't done this
...
(https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493529510)
My recommendation would be to try without the cirrus overhead for debugging. Just copy-pasting the one-line bash command may be easier and faster for troubleshooting. See https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally
As for the error itself: It is not possible to run 32-bit arm binaries on a CPU that only supports 64-bit mode.
I don't know how to fix this, but if nothing helps, you'll have to select qemu-arm in UTM. (Is there a reason why you haven't done this
...
💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2493534971)
> Sorry about the regression; go ahead and file a report at https://developercommunity.visualstudio.com/cpp/report and we'll see if we can get it serviced quickly.
https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-in-the-Bi/10797559
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2493534971)
> Sorry about the regression; go ahead and file a report at https://developercommunity.visualstudio.com/cpp/report and we'll see if we can get it serviced quickly.
https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-in-the-Bi/10797559
👍 hebasto approved a pull request: "ci: Skip broken Wine64 tests by default"
(https://github.com/bitcoin/bitcoin/pull/31284#pullrequestreview-2454213024)
ACK fa5e7064597fc51366f082e3d07a4591e576db38, to avoid false-positives.
(https://github.com/bitcoin/bitcoin/pull/31284#pullrequestreview-2454213024)
ACK fa5e7064597fc51366f082e3d07a4591e576db38, to avoid false-positives.
✅ Sjors closed a pull request: "mining: add early return to waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/31297)
(https://github.com/bitcoin/bitcoin/pull/31297)
💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2493553214)
I tested that `m_tip_block_cv.wait_for` indeed immediately checks the condition before waiting. And I could see that `notifications().m_tip_block` isn't set when you start a node that is already fully caught up.
I'll open a new PR to try and deal with that.
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2493553214)
I tested that `m_tip_block_cv.wait_for` indeed immediately checks the condition before waiting. And I could see that `notifications().m_tip_block` isn't set when you start a node that is already fully caught up.
I'll open a new PR to try and deal with that.