💬 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
...
💬 maflcko commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127574724)
It looks like a false positive, so I guess if you really want to fix it, you'll have to do it upstream.
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3127574724)
It looks like a false positive, so I guess if you really want to fix it, you'll have to do it upstream.
💬 darosior commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3127602406)
I ran two identically compiled nodes on two identical VMs on the same host, one on top of fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c (this PR modified to enable caching for block validation too) and one on top of 2cad7226c2d02ec67bbac7ec909030f8bae593f8 (the commit it is based on).
Over 4 days of blocks (from height 906875 (`00000000000000000000264374e6d9b26be206793712e94266a62ddb0e9551e9`) to height 907450 (`00000000000000000001d85ba0ff861fd2499bd72fd436e2c37e11e0c28caf74`)), the average input
...
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3127602406)
I ran two identically compiled nodes on two identical VMs on the same host, one on top of fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c (this PR modified to enable caching for block validation too) and one on top of 2cad7226c2d02ec67bbac7ec909030f8bae593f8 (the commit it is based on).
Over 4 days of blocks (from height 906875 (`00000000000000000000264374e6d9b26be206793712e94266a62ddb0e9551e9`) to height 907450 (`00000000000000000001d85ba0ff861fd2499bd72fd436e2c37e11e0c28caf74`)), the average input
...