💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2236689505)
Oh, that makes sense, I thought that you were talking about changing the if-statement on line 393.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2236689505)
Oh, that makes sense, I thought that you were talking about changing the if-statement on line 393.
💬 enirox001 commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3127476258)
tACK 8810642 – Ran tests with/without --skipreorg; saw ~40 % speedup; no regressions.
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3127476258)
tACK 8810642 – Ran tests with/without --skipreorg; saw ~40 % speedup; no regressions.
💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2236729580)
User descriptor keys do not follow any derivation specification. They can import keys derived using any path.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2236729580)
User descriptor keys do not follow any derivation specification. They can import keys derived using any path.
💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3127506107)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3114802810
> For reference, the lint task still fails. Not sure if this is intentional or if you missed it.
Thanks! The lint task was failing in https://cirrus-ci.com/task/5047676548415488 because the subtree was modified. The subtree was modified to work around a bug fixed in https://github.com/bitcoin-core/libmultiprocess/pull/181 which is part of https://github.com/bitcoin/bitcoin/pull/32345 but not currently part of master.
...
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3127506107)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3114802810
> For reference, the lint task still fails. Not sure if this is intentional or if you missed it.
Thanks! The lint task was failing in https://cirrus-ci.com/task/5047676548415488 because the subtree was modified. The subtree was modified to work around a bug fixed in https://github.com/bitcoin-core/libmultiprocess/pull/181 which is part of https://github.com/bitcoin/bitcoin/pull/32345 but not currently part of master.
...
👋 fanquake's pull request is ready for review: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/33079)
(https://github.com/bitcoin/bitcoin/pull/33079)
🤔 maflcko reviewed a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3062416347)
there is a bug in the lint config. Also, left some nits.
looked at 72d82211fce78a18bce27a90726d30c85518955c~6 📊
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yu
...
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3062416347)
there is a bug in the lint config. Also, left some nits.
looked at 72d82211fce78a18bce27a90726d30c85518955c~6 📊
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yu
...
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236234660)
nit in 1f5eb93de45f6a530ba54d799f637c0f1b45bc1b: Could add a comment why this is needed? I guess due to GHA not supporting ipv6 in $current_year?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236234660)
nit in 1f5eb93de45f6a530ba54d799f637c0f1b45bc1b: Could add a comment why this is needed? I guess due to GHA not supporting ipv6 in $current_year?
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236357065)
nit in e9cf1c90052620b23745ca0d3fdccda2cf5fa0fc: Same, about `REPO_USE_CIRRUS_RUNNERS`
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236357065)
nit in e9cf1c90052620b23745ca0d3fdccda2cf5fa0fc: Same, about `REPO_USE_CIRRUS_RUNNERS`
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236522727)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Same (`REPO_USE_CIRRUS_RUNNERS`)
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236522727)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Same (`REPO_USE_CIRRUS_RUNNERS`)
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236350345)
nit in 1f5eb93de45f6a530ba54d799f637c0f1b45bc1b: Seems like there are several places that hard-code the main repo, which makes it harder for forks to use cirrus runners.
Maybe a global env next to `CIRRUS_CACHE_HOST` could be used to make that simpler?
Maybe `REPO_USE_CIRRUS_RUNNERS: 'bitcoin/bitcoin' # Use cirrus runners and cache for this repo, instead of falling back to the slow GHA runners`?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236350345)
nit in 1f5eb93de45f6a530ba54d799f637c0f1b45bc1b: Seems like there are several places that hard-code the main repo, which makes it harder for forks to use cirrus runners.
Maybe a global env next to `CIRRUS_CACHE_HOST` could be used to make that simpler?
Maybe `REPO_USE_CIRRUS_RUNNERS: 'bitcoin/bitcoin' # Use cirrus runners and cache for this repo, instead of falling back to the slow GHA runners`?
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236548912)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Unused? Maybe you wanted this to be the container name, or the job id instead?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236548912)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Unused? Maybe you wanted this to be the container name, or the job id instead?
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236658377)
nit in 67429ff9ae0ad7236d01fee7d25dceb965741b2f: Please use `-o xtrace`, so that non-Bash experts can also understand the intention, without having to <strike>ask an LLM</strike> read the manpage
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236658377)
nit in 67429ff9ae0ad7236d01fee7d25dceb965741b2f: Please use `-o xtrace`, so that non-Bash experts can also understand the intention, without having to <strike>ask an LLM</strike> read the manpage
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236595677)
nit in c7aa954564ceaf0800be461115a568d3c919228a: This seems somewhat redundant with the container name now. It would be less yaml code to use container name instead of job id here?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236595677)
nit in c7aa954564ceaf0800be461115a568d3c919228a: This seems somewhat redundant with the container name now. It would be less yaml code to use container name instead of job id here?
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236643414)
nit in a3789f2056a76e72fcafbae7396c35f62fb93f56: wrong (unused) suffix
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236643414)
nit in a3789f2056a76e72fcafbae7396c35f62fb93f56: wrong (unused) suffix
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236619074)
nit in 88816a4ed3f5d528ed7b997241df96fb1f51df44: `apt-get install` will fail with a stale Ubuntu IP list from a cached image. You'll have to call `apt-get update` before every install for each image layer.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236619074)
nit in 88816a4ed3f5d528ed7b997241df96fb1f51df44: `apt-get install` will fail with a stale Ubuntu IP list from a cached image. You'll have to call `apt-get update` before every install for each image layer.
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236533948)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Could just be a global env var, instead of repeating it thrice?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236533948)
nit in 35a9dd7352d84292fbc2dcf2e544b380be49b4c3: Could just be a global env var, instead of repeating it thrice?
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236668156)
Just as a note: This is fine to remove without replacement, because the cirrus cache now runs outside the container. Here, it ran inside it.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236668156)
Just as a note: This is fine to remove without replacement, because the cirrus cache now runs outside the container. Here, it ran inside it.
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236626008)
Also, I wonder if this can use `$PACKAGES`?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236626008)
Also, I wonder if this can use `$PACKAGES`?
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236739388)
693d3615cdb25c5f185c1e6097a6e647eca7153b: This commit isn't doing anything, because the MAKEJOBS are set globally in the env in GHA (same for the other "perf" commits below).
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236739388)
693d3615cdb25c5f185c1e6097a6e647eca7153b: This commit isn't doing anything, because the MAKEJOBS are set globally in the env in GHA (same for the other "perf" commits below).
💬 pinheadmz commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127567379)
Ok got a reliable crash on debian after upgrading to clang-20:
```
~/bitcoin$ rm -rf ./bld-cmake && \
cmake -B ./bld-cmake \
-DCMAKE_C_COMPILER='clang-20' \
-DCMAKE_CXX_COMPILER='clang++-20;-stdlib=libc++' \
-DBUILD_GUI=OFF \
-DBUILD_TESTS=OFF \
-DSANITIZERS=thread && \
cmake --build ./bld-cmake --parallel $( nproc ) && \
TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1" ./bld-cmake/test/functional/wallet_multiwallet.py
...
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127567379)
Ok got a reliable crash on debian after upgrading to clang-20:
```
~/bitcoin$ rm -rf ./bld-cmake && \
cmake -B ./bld-cmake \
-DCMAKE_C_COMPILER='clang-20' \
-DCMAKE_CXX_COMPILER='clang++-20;-stdlib=libc++' \
-DBUILD_GUI=OFF \
-DBUILD_TESTS=OFF \
-DSANITIZERS=thread && \
cmake --build ./bld-cmake --parallel $( nproc ) && \
TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1" ./bld-cmake/test/functional/wallet_multiwallet.py
...