💬 stringintech commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2631293264)
Just noticed a previous related reviewed comment: https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508035777
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2631293264)
Just noticed a previous related reviewed comment: https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508035777
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670577096)
This is intended to work on CI and it does so well. Should I reintroduce [INTERNET_TRAFFIC_EXPECTED](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2333792206) to deal with local runs by making it possible to turn these reports into non-fatal errors?
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670577096)
This is intended to work on CI and it does so well. Should I reintroduce [INTERNET_TRAFFIC_EXPECTED](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2333792206) to deal with local runs by making it possible to turn these reports into non-fatal errors?
💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3670582500)
Sorry for the conflict - happy to re-review after the rebase. Tried it locally, seems trivial.
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3670582500)
Sorry for the conflict - happy to re-review after the rebase. Tried it locally, seems trivial.
💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670590566)
> This is intended to work on CI and it does so well.
Being able to run the CI locally is fully supported and a required use case (there are CI jobs which are not run in the main repo).
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670590566)
> This is intended to work on CI and it does so well.
Being able to run the CI locally is fully supported and a required use case (there are CI jobs which are not run in the main repo).
🤔 furszy reviewed a pull request: "rpc, doc: clarify the response of listtransactions RPC"
(https://github.com/bitcoin/bitcoin/pull/32737#pullrequestreview-3593328432)
ACK 1ed8e7616527c69dbaa9904cda59e3b73c29fa5d
(https://github.com/bitcoin/bitcoin/pull/32737#pullrequestreview-3593328432)
ACK 1ed8e7616527c69dbaa9904cda59e3b73c29fa5d
💬 maflcko commented on pull request "scripted-diff: [doc] Unify stale copyright headers":
(https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670620730)
> Nit: if you need to touch again, please consider:
Thanks, addressed those that are in scope for the issues listed in the pull request description. The others seem unrelated and can be addressed in a different pull request either now or later, if there is need to.
(https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670620730)
> Nit: if you need to touch again, please consider:
Thanks, addressed those that are in scope for the issues listed in the pull request description. The others seem unrelated and can be addressed in a different pull request either now or later, if there is need to.
🤔 rkrux reviewed a pull request: "doc: Use multipath descriptors in descriptors.md and linked test"
(https://github.com/bitcoin/bitcoin/pull/34100#pullrequestreview-3593332537)
Concept ACK 912d837326145c304662ca29a01ccd1d240946e8
Multi-path convention is indeed convenient and preferred.
(https://github.com/bitcoin/bitcoin/pull/34100#pullrequestreview-3593332537)
Concept ACK 912d837326145c304662ca29a01ccd1d240946e8
Multi-path convention is indeed convenient and preferred.
💬 rkrux commented on pull request "doc: Use multipath descriptors in descriptors.md and linked test":
(https://github.com/bitcoin/bitcoin/pull/34100#discussion_r2631352329)
Nit, feel free to ignore but I think reading `/0` instead of just `0` would be more relatable given the context.
```diff
diff --git a/doc/descriptors.md b/doc/descriptors.md
index 6c600af5ba..1b79c99fe6 100644
--- a/doc/descriptors.md
+++ b/doc/descriptors.md
@@ -67,8 +67,8 @@ Output descriptors currently support:
- `wsh(sortedmulti(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3Zu
...
(https://github.com/bitcoin/bitcoin/pull/34100#discussion_r2631352329)
Nit, feel free to ignore but I think reading `/0` instead of just `0` would be more relatable given the context.
```diff
diff --git a/doc/descriptors.md b/doc/descriptors.md
index 6c600af5ba..1b79c99fe6 100644
--- a/doc/descriptors.md
+++ b/doc/descriptors.md
@@ -67,8 +67,8 @@ Output descriptors currently support:
- `wsh(sortedmulti(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3Zu
...
💬 rkrux commented on pull request "doc: Use multipath descriptors in descriptors.md and linked test":
(https://github.com/bitcoin/bitcoin/pull/34100#discussion_r2631357406)
This test fails now, at least in my system:
```zsh
2025-12-18T14:35:06.064047Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py", line 95, in run_test
psbt = multisig.walletcreatefundedpsbt(inputs=[],
...
(https://github.com/bitcoin/bitcoin/pull/34100#discussion_r2631357406)
This test fails now, at least in my system:
```zsh
2025-12-18T14:35:06.064047Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py", line 95, in run_test
psbt = multisig.walletcreatefundedpsbt(inputs=[],
...
👍 rkrux approved a pull request: "test: address self-announcement"
(https://github.com/bitcoin/bitcoin/pull/34039#pullrequestreview-3593348031)
ACK 1841bf9
Thanks for the co-author attribution.
(https://github.com/bitcoin/bitcoin/pull/34039#pullrequestreview-3593348031)
ACK 1841bf9
Thanks for the co-author attribution.
📝 hebasto opened a pull request: "cmake: Add missed `Boost::headers`"
(https://github.com/bitcoin/bitcoin/pull/34103)
On Fedora 43:
```
$ gmake -C depends NO_LIBEVENT=1 NO_QT=1 NO_ZMQ=1 NO_WALLET=1 NO_USDT=1 NO_IPC=1
$ cmake -B build -DBoost_ROOT=depends/x86_64-pc-linux-gnu --preset dev-mode
<snip>
[363/1092] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc.dir/capnp/mining.cpp.o
FAILED: [code=1] src/ipc/CMakeFiles/bitcoin_ipc.dir/capnp/mining.cpp.o
/usr/bin/ccache /usr/lib64/ccache/c++ -DKJ_USE_FIBERS -I/home/hebasto/dev/bitcoin/build/src -I/home/hebasto/dev/bitcoin/src -I/home/hebasto/dev/bitcoin/bu
...
(https://github.com/bitcoin/bitcoin/pull/34103)
On Fedora 43:
```
$ gmake -C depends NO_LIBEVENT=1 NO_QT=1 NO_ZMQ=1 NO_WALLET=1 NO_USDT=1 NO_IPC=1
$ cmake -B build -DBoost_ROOT=depends/x86_64-pc-linux-gnu --preset dev-mode
<snip>
[363/1092] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc.dir/capnp/mining.cpp.o
FAILED: [code=1] src/ipc/CMakeFiles/bitcoin_ipc.dir/capnp/mining.cpp.o
/usr/bin/ccache /usr/lib64/ccache/c++ -DKJ_USE_FIBERS -I/home/hebasto/dev/bitcoin/build/src -I/home/hebasto/dev/bitcoin/src -I/home/hebasto/dev/bitcoin/bu
...
💬 hebasto commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670657739)
> > Is it a good time to consider bumping of the Boost minimum supported version?
>
> I think there are users out there on Ubuntu:22.04 and Debian Bookworm, so breaking them seems a bit too early?
FWIW, they could workaround using depends. For example, see https://github.com/bitcoin/bitcoin/pull/34103.
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670657739)
> > Is it a good time to consider bumping of the Boost minimum supported version?
>
> I think there are users out there on Ubuntu:22.04 and Debian Bookworm, so breaking them seems a bit too early?
FWIW, they could workaround using depends. For example, see https://github.com/bitcoin/bitcoin/pull/34103.
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670670860)
I tend to agree with @fanquake. Running the CI locally should be easy and supported. We don't want to end up in a place where the CI is basically just a prayer toward Microsoft/GHA to please run the scripts and to please run them correctly.
I think the open questions are:
* Why does the GHA CI *not* catch the issue seen in local runs?
* Which test is responsible for the issues in local runs, and what is the fix?
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670670860)
I tend to agree with @fanquake. Running the CI locally should be easy and supported. We don't want to end up in a place where the CI is basically just a prayer toward Microsoft/GHA to please run the scripts and to please run them correctly.
I think the open questions are:
* Why does the GHA CI *not* catch the issue seen in local runs?
* Which test is responsible for the issues in local runs, and what is the fix?
💬 l0rinc commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670680873)
regardless of the workaround, I don't mind reverting to make it work with old boost again - we can enable the new clang-tidy rule without them as well.
I have reverted the above changes in https://github.com/bitcoin/bitcoin/pull/34095 - can someone please validate that it fixed the build with older boost?
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670680873)
regardless of the workaround, I don't mind reverting to make it work with old boost again - we can enable the new clang-tidy rule without them as well.
I have reverted the above changes in https://github.com/bitcoin/bitcoin/pull/34095 - can someone please validate that it fixed the build with older boost?
💬 hebasto commented on pull request "depends: capnp 1.3.0":
(https://github.com/bitcoin/bitcoin/pull/34102#issuecomment-3670687077)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/34102#issuecomment-3670687077)
Concept ACK.
💬 l0rinc commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670692392)
Reverted the boost changes (already merged and new ones), clang-tidy should still pass.
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670692392)
Reverted the boost changes (already merged and new ones), clang-tidy should still pass.
💬 l0rinc commented on pull request "scripted-diff: [doc] Unify stale copyright headers":
(https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670703429)
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
(https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670703429)
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
⚠️ fanquake opened an issue: "p2p: `seed.bitcoin.sipa.be` is down"
(https://github.com/bitcoin/bitcoin/issues/34104)
```bash
./check-dnsseeds.py
* mainnet
FAIL seed.bitcoin.sipa.be
OK dnsseed.bluematt.me (31 results)
OK dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us (35 results)
OK seed.bitcoin.jonasschnelli.ch (24 results)
<snip>
```
(https://github.com/bitcoin/bitcoin/issues/34104)
```bash
./check-dnsseeds.py
* mainnet
FAIL seed.bitcoin.sipa.be
OK dnsseed.bluematt.me (31 results)
OK dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us (35 results)
OK seed.bitcoin.jonasschnelli.ch (24 results)
<snip>
```
💬 fanquake commented on issue "p2p: `seed.bitcoin.sipa.be` is down":
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670729702)
cc @sipa
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670729702)
cc @sipa
👋 fanquake's pull request is ready for review: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095)
(https://github.com/bitcoin/bitcoin/pull/34095)
📝 l0rinc opened a pull request: "kernel: revert accidentally removed copyright header"
(https://github.com/bitcoin/bitcoin/pull/34105)
See:
https://github.com/bitcoin/bitcoin/commit/7990463b1059ba5fc4ebe37fd1105a9e168ae20d?diff=split#diff-04e685224f1ac5bfd91d47d8d7528a2e44f94fab5535d4b6b5af79b5a13aeb93L1-L12
The author mentioned it was likely a bad rebase.
Found while reviewing https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670396535
(https://github.com/bitcoin/bitcoin/pull/34105)
See:
https://github.com/bitcoin/bitcoin/commit/7990463b1059ba5fc4ebe37fd1105a9e168ae20d?diff=split#diff-04e685224f1ac5bfd91d47d8d7528a2e44f94fab5535d4b6b5af79b5a13aeb93L1-L12
The author mentioned it was likely a bad rebase.
Found while reviewing https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670396535