✅ fanquake closed an issue: "build: `cmake --install` fails if only select targets are built"
(https://github.com/bitcoin/bitcoin/issues/31745)
(https://github.com/bitcoin/bitcoin/issues/31745)
🚀 fanquake merged a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844)
(https://github.com/bitcoin/bitcoin/pull/31844)
💬 s373nZ commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2659328035)
re-ACK https://github.com/bitcoin/bitcoin/commit/9b033bebb18dfd609c02736292f37cc6589fcc8d
Consider the prefix name feedback a nit.
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2659328035)
re-ACK https://github.com/bitcoin/bitcoin/commit/9b033bebb18dfd609c02736292f37cc6589fcc8d
Consider the prefix name feedback a nit.
💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659333795)
> Uploading the macOS binaries here, so I can test the download flow (which in the past behaved different from the scp flow):
>
> * [bitcoin-096525e92cc2-arm64-apple-darwin.zip](https://github.com/user-attachments/files/18781660/bitcoin-096525e92cc2-arm64-apple-darwin.zip)
> * [bitcoin-096525e92cc2-x86_64-apple-darwin.zip](https://github.com/user-attachments/files/18781669/bitcoin-096525e92cc2-x86_64-apple-darwin.zip)
Tested on macOS 15.3 (Intel):
```
% shasum -a 256 bitcoin-096525e92cc
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659333795)
> Uploading the macOS binaries here, so I can test the download flow (which in the past behaved different from the scp flow):
>
> * [bitcoin-096525e92cc2-arm64-apple-darwin.zip](https://github.com/user-attachments/files/18781660/bitcoin-096525e92cc2-arm64-apple-darwin.zip)
> * [bitcoin-096525e92cc2-x86_64-apple-darwin.zip](https://github.com/user-attachments/files/18781669/bitcoin-096525e92cc2-x86_64-apple-darwin.zip)
Tested on macOS 15.3 (Intel):
```
% shasum -a 256 bitcoin-096525e92cc
...
💬 fanquake commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2659376971)
> If it's not too much trouble, could you recall which distros those were?
It's still the case with Ubuntu, i.e 24.04:
```bash
# apt install llvm
# ls /usr/bin/llvm-* | grep objdump
/usr/bin/llvm-objdump
/usr/bin/llvm-objdump-18
# ls /usr/bin/llvm-* | grep install-name
/usr/bin/llvm-install-name-tool-18
```
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2659376971)
> If it's not too much trouble, could you recall which distros those were?
It's still the case with Ubuntu, i.e 24.04:
```bash
# apt install llvm
# ls /usr/bin/llvm-* | grep objdump
/usr/bin/llvm-objdump
/usr/bin/llvm-objdump-18
# ls /usr/bin/llvm-* | grep install-name
/usr/bin/llvm-install-name-tool-18
```
💬 maflcko commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2659392932)
Looks like this is still up for grabs, waiting on a pull request for 29.x.
> > Can't they just unset it in the UI settings?
>
> No, because the checkbox for upnp no longer exists.
Also not with a previous release as a workaround?
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2659392932)
Looks like this is still up for grabs, waiting on a pull request for 29.x.
> > Can't they just unset it in the UI settings?
>
> No, because the checkbox for upnp no longer exists.
Also not with a previous release as a workaround?
💬 maflcko commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2659397191)
review ACK 301017c6217378ca868e17c7cb8ffe3447abb11d 🎸
<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+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 301017c62173
...
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2659397191)
review ACK 301017c6217378ca868e17c7cb8ffe3447abb11d 🎸
<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+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 301017c62173
...
💬 Sjors commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659427920)
I had `export PATH=/opt/homebrew/bin/:$PATH` in my `~/.zshrc` probably to use gnu versions of various tools. But that's not the right way.
Once I removed that, and restore `brew link python3`, the path is as follows:
```
which -a python3
/Users/sjors/.pyenv/shims/python3
/opt/homebrew/bin/python3
/usr/bin/python3
```
And cmake finds 3.10.14.
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659427920)
I had `export PATH=/opt/homebrew/bin/:$PATH` in my `~/.zshrc` probably to use gnu versions of various tools. But that's not the right way.
Once I removed that, and restore `brew link python3`, the path is as follows:
```
which -a python3
/Users/sjors/.pyenv/shims/python3
/opt/homebrew/bin/python3
/usr/bin/python3
```
And cmake finds 3.10.14.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956203848)
Hm, one of the CI failures is a timeout for this (the other is a wallet thing). Perhaps it takes a bit too long?
We could also change these to batches of 200 * 10 + 101 * 10 and just do the `wait_until` once to make the test faster. Or, we use `-maxorphantxs` to reduce the limit.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956203848)
Hm, one of the CI failures is a timeout for this (the other is a wallet thing). Perhaps it takes a bit too long?
We could also change these to batches of 200 * 10 + 101 * 10 and just do the `wait_until` once to make the test faster. Or, we use `-maxorphantxs` to reduce the limit.
💬 Sjors commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2659432408)
I suspect "unset" will store upnp=0 in the settings which would still be an error.
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2659432408)
I suspect "unset" will store upnp=0 in the settings which would still be an error.
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956206594)
Too bad :(
Try the smaller batches and see how we do? Lowering maxorphantxs is better than no test at all but clearly not optimal.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956206594)
Too bad :(
Try the smaller batches and see how we do? Lowering maxorphantxs is better than no test at all but clearly not optimal.
💬 instagibbs commented on pull request "test: Rename send_message to send_without_ping":
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2659437755)
concept ACK, many intermittent test failures end up being this
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2659437755)
concept ACK, many intermittent test failures end up being this
🤔 murchandamus reviewed a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2617937191)
LGTM, ACK 3adf43df82a7659c1734ce552917daf8b429bb24
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2617937191)
LGTM, ACK 3adf43df82a7659c1734ce552917daf8b429bb24
🤔 rkrux reviewed a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2617954596)
Concept ACK
Initial pass, will review in detail soon.
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2617954596)
Concept ACK
Initial pass, will review in detail soon.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1956236842)
```diff
- for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
- SyncTransaction(ptx, TxStateInactive{});
+ for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
+ // Coinbase transactions are not only inactive but also abandoned,
+ // meaning they should never be relayed standalone via the p2p protocol.
+ SyncTransaction(ptx, TxStateInactive{ /*abandoned=*/ ptx->IsCoinBase()});
```
Build and tests all good with this.
I considered this with
...
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1956236842)
```diff
- for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
- SyncTransaction(ptx, TxStateInactive{});
+ for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
+ // Coinbase transactions are not only inactive but also abandoned,
+ // meaning they should never be relayed standalone via the p2p protocol.
+ SyncTransaction(ptx, TxStateInactive{ /*abandoned=*/ ptx->IsCoinBase()});
```
Build and tests all good with this.
I considered this with
...
💬 Sjors commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1956245111)
It seems neither of these are available on Apple Silicon (`HWCAP2_RNG` isn't defined), so not much to test here.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1956245111)
It seems neither of these are available on Apple Silicon (`HWCAP2_RNG` isn't defined), so not much to test here.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2659582312)
Rebased 5aeaa3f49d10562b8936ef36b1c25a6466dbe03e -> a604321c3e4bd50b52fa28e8567f6b068b2d2fb3 ([kernelApi_23](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_23) -> [kernelApi_24](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_24), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_23..kernelApi_24))
* Fixed conflict with #31844
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2659582312)
Rebased 5aeaa3f49d10562b8936ef36b1c25a6466dbe03e -> a604321c3e4bd50b52fa28e8567f6b068b2d2fb3 ([kernelApi_23](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_23) -> [kernelApi_24](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_24), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_23..kernelApi_24))
* Fixed conflict with #31844
💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2659604837)
Rebased d2ceb2e0735a2c8343f8316b55fac55323aba62c -> 45bfd97ec7c9991f41673d79b01277bfd940e64e ([`pr/libexec.3`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.3) -> [`pr/libexec.4`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.3-rebase..pr/libexec.4)) due to conflict with #31844
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2659604837)
Rebased d2ceb2e0735a2c8343f8316b55fac55323aba62c -> 45bfd97ec7c9991f41673d79b01277bfd940e64e ([`pr/libexec.3`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.3) -> [`pr/libexec.4`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.3-rebase..pr/libexec.4)) due to conflict with #31844
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2659615530)
Rebased 9194e461518e6b843c1a707ba1b10ab7a000e096 -> 59e1d4abb34453805a09a62c51ead7347787b2e3 ([`pr/mine.16`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.16) -> [`pr/mine.17`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.16-rebase..pr/mine.17)) due to conflict with #31844
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2659615530)
Rebased 9194e461518e6b843c1a707ba1b10ab7a000e096 -> 59e1d4abb34453805a09a62c51ead7347787b2e3 ([`pr/mine.16`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.16) -> [`pr/mine.17`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.16-rebase..pr/mine.17)) due to conflict with #31844
💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2659617238)
Will review again soon, but a quick question: would it make sense to provide the ability for a user (who doesn't necessarily have easy access to the source code) to extract the asmap data from a running node? For example, to be able to run `asmap.py diff` on it. It could be an RPC command that just spits out the data in textual format, or a REST endpoint that directly provides it in binary.
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2659617238)
Will review again soon, but a quick question: would it make sense to provide the ability for a user (who doesn't necessarily have easy access to the source code) to extract the asmap data from a running node? For example, to be able to run `asmap.py diff` on it. It could be an RPC command that just spits out the data in textual format, or a REST endpoint that directly provides it in binary.