👍 pablomartin4btc approved a pull request: "doc: add release notes for #32425"
(https://github.com/bitcoin/bitcoin/pull/32727#pullrequestreview-2958946450)
> a4cfaa5eab...b9a2e8ee96: take suggestions
re-ACK b9a2e8ee96
(https://github.com/bitcoin/bitcoin/pull/32727#pullrequestreview-2958946450)
> a4cfaa5eab...b9a2e8ee96: take suggestions
re-ACK b9a2e8ee96
👍 hebasto approved a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2958947519)
re-ACK 6c2538d5bfeafe6f234e5382c151d173826d78f0, only rebased since my previous [review](https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2891203225).
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2958947519)
re-ACK 6c2538d5bfeafe6f234e5382c151d173826d78f0, only rebased since my previous [review](https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2891203225).
💬 vasild commented on pull request "doc: add release notes for #32425":
(https://github.com/bitcoin/bitcoin/pull/32727#discussion_r2167192118)
Done.
"for example `-proxy=127.0.0.1:5555=ipv6` would configure a proxy only for IPv6"
> it looks like it's talking about the past behavior
I guess I used "5: The second conditional" from https://www.perfect-english-grammar.com/would.html:
> If I had enough money, I would travel around the world.
> If we lived in Madrid, we would study Spanish.
(https://github.com/bitcoin/bitcoin/pull/32727#discussion_r2167192118)
Done.
"for example `-proxy=127.0.0.1:5555=ipv6` would configure a proxy only for IPv6"
> it looks like it's talking about the past behavior
I guess I used "5: The second conditional" from https://www.perfect-english-grammar.com/would.html:
> If I had enough money, I would travel around the world.
> If we lived in Madrid, we would study Spanish.
🤔 janb84 reviewed a pull request: "doc: add release notes for #32425"
(https://github.com/bitcoin/bitcoin/pull/32727#pullrequestreview-2959000718)
reACK b9a2e8ee965daffe2bc618f58b21f0ddebadeb23
Changes sinds last ACK;
- div textual changes
(still find it weird that a non-localhost ipv4 address for ipv6 proxy can be used, but agree to disagree on that )
(https://github.com/bitcoin/bitcoin/pull/32727#pullrequestreview-2959000718)
reACK b9a2e8ee965daffe2bc618f58b21f0ddebadeb23
Changes sinds last ACK;
- div textual changes
(still find it weird that a non-localhost ipv4 address for ipv6 proxy can be used, but agree to disagree on that )
💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2167253212)
I think the +1 offset would still be needed in a corner case when the input of the ceil function is already an integer. In this case, if +1 is removed, the timeout will not be triggered because the check `current_time > peer->m_headers_sync_timeout` ([here](https://github.com/bitcoin/bitcoin/blob/7d5a6d17398aae3cea6d63564d33e7e49da3c71c/src/net_processing.cpp#L5888)) would evaluate to `timeout > timeout` (false) instead of `timeout + 1 > timeout` (true).
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2167253212)
I think the +1 offset would still be needed in a corner case when the input of the ceil function is already an integer. In this case, if +1 is removed, the timeout will not be triggered because the check `current_time > peer->m_headers_sync_timeout` ([here](https://github.com/bitcoin/bitcoin/blob/7d5a6d17398aae3cea6d63564d33e7e49da3c71c/src/net_processing.cpp#L5888)) would evaluate to `timeout > timeout` (false) instead of `timeout + 1 > timeout` (true).
💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2167267966)
Aside from the +1 offset though, maybe it is good to apply your suggestion and use the ceil function so that `calculate_headers_timeout` returns a time that is >= the real timeout calculated by core, ensuring it doesn't return a lower value due to precision loss.
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2167267966)
Aside from the +1 offset though, maybe it is good to apply your suggestion and use the ceil function so that `calculate_headers_timeout` returns a time that is >= the real timeout calculated by core, ensuring it doesn't return a lower value due to precision loss.
👋 ishaanam's pull request is ready for review: "wallet, rpc: add anti-fee-sniping to `send` and `sendall`"
(https://github.com/bitcoin/bitcoin/pull/28944)
(https://github.com/bitcoin/bitcoin/pull/28944)
📝 hebasto opened a pull request: "cmake: Explicitly specify `Boost_ROOT` for Homebrew's package"
(https://github.com/bitcoin/bitcoin/pull/32814)
On macOS, this change ensures that the Boost package is located at its real path rather than via the symlink in the default prefix.
A backport to 29.x is required for https://github.com/bitcoin/bitcoin/pull/32804, as this change prevents contamination of include directories by broad locations such as `/usr/local/include` or `/opt/homebrew/include`, which take precedence over Qt’s `-iframework` flags.
Below is the relevant change in the configuration logs on my macOS 15.5 `x64`:
- master b
...
(https://github.com/bitcoin/bitcoin/pull/32814)
On macOS, this change ensures that the Boost package is located at its real path rather than via the symlink in the default prefix.
A backport to 29.x is required for https://github.com/bitcoin/bitcoin/pull/32804, as this change prevents contamination of include directories by broad locations such as `/usr/local/include` or `/opt/homebrew/include`, which take precedence over Qt’s `-iframework` flags.
Below is the relevant change in the configuration logs on my macOS 15.5 `x64`:
- master b
...
💬 hebasto commented on pull request "Fix build on macOS when `qt@6` is installed":
(https://github.com/bitcoin/bitcoin/pull/32804#issuecomment-3005666028)
> Please review #32805 first.
Also https://github.com/bitcoin/bitcoin/pull/32814.
(https://github.com/bitcoin/bitcoin/pull/32804#issuecomment-3005666028)
> Please review #32805 first.
Also https://github.com/bitcoin/bitcoin/pull/32814.
💬 stratospher commented on pull request "test: refactor out same-txid-diff-wtxid tx to reuse in other tests":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2167312793)
I like how the interface is simpler! but what about this comment though? - https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2074738962
transactions constructed using this method would be non-standard. so when we try to `sendrawtransaction` a high-S signature transaction, mempool accept fails and an error would be returned [here](https://github.com/bitcoin/bitcoin/blob/7d5a6d17398aae3cea6d63564d33e7e49da3c71c/src/node/transaction.cpp#L75).
```
test_framework.authproxy.JSONRPCExcep
...
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2167312793)
I like how the interface is simpler! but what about this comment though? - https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2074738962
transactions constructed using this method would be non-standard. so when we try to `sendrawtransaction` a high-S signature transaction, mempool accept fails and an error would be returned [here](https://github.com/bitcoin/bitcoin/blob/7d5a6d17398aae3cea6d63564d33e7e49da3c71c/src/node/transaction.cpp#L75).
```
test_framework.authproxy.JSONRPCExcep
...
🤔 hebasto reviewed a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2959223968)
My Guix build:
```
aarch64
dcedbdafa5857dbbb7748e27bbbfadc57a5dc70eff9b565c1d30061e64027ca4 guix-build-6c2538d5bfea/output/aarch64-linux-gnu/SHA256SUMS.part
ccf56c678924e933ef46f306d0be509251d1920f17529e11f007a5c0b74a5282 guix-build-6c2538d5bfea/output/aarch64-linux-gnu/bitcoin-6c2538d5bfea-aarch64-linux-gnu-debug.tar.gz
550fa33f9e19c06cf4651abd4e69d813db6702461b6b59b717a719821c8b6495 guix-build-6c2538d5bfea/output/aarch64-linux-gnu/bitcoin-6c2538d5bfea-aarch64-linux-gnu.tar.gz
aab708c3
...
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2959223968)
My Guix build:
```
aarch64
dcedbdafa5857dbbb7748e27bbbfadc57a5dc70eff9b565c1d30061e64027ca4 guix-build-6c2538d5bfea/output/aarch64-linux-gnu/SHA256SUMS.part
ccf56c678924e933ef46f306d0be509251d1920f17529e11f007a5c0b74a5282 guix-build-6c2538d5bfea/output/aarch64-linux-gnu/bitcoin-6c2538d5bfea-aarch64-linux-gnu-debug.tar.gz
550fa33f9e19c06cf4651abd4e69d813db6702461b6b59b717a719821c8b6495 guix-build-6c2538d5bfea/output/aarch64-linux-gnu/bitcoin-6c2538d5bfea-aarch64-linux-gnu.tar.gz
aab708c3
...
💬 glozow commented on pull request "descriptors: Allow `H` as a hardened indicator":
(https://github.com/bitcoin/bitcoin/pull/32788#issuecomment-3005754433)
Not merging for now - IIUC if the approach mentioned in https://groups.google.com/u/1/g/bitcoindev/c/IAYEx4zUhHA is taken, this would be closed?
(https://github.com/bitcoin/bitcoin/pull/32788#issuecomment-3005754433)
Not merging for now - IIUC if the approach mentioned in https://groups.google.com/u/1/g/bitcoindev/c/IAYEx4zUhHA is taken, this would be closed?
💬 jonatack commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2167367967)
@bigspider In commit 2c1deefedc16e, naively it looks like this diff would resolve the rebase for you.
```diff
bool tx_ok;
TxValidationState tx_state;
// If CheckInputScripts is called with a pointer to a checks vector, the resulting checks are appended to it. In that case
// they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning.
- if (fScriptChecks && !C
...
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2167367967)
@bigspider In commit 2c1deefedc16e, naively it looks like this diff would resolve the rebase for you.
```diff
bool tx_ok;
TxValidationState tx_state;
// If CheckInputScripts is called with a pointer to a checks vector, the resulting checks are appended to it. In that case
// they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning.
- if (fScriptChecks && !C
...
💬 theStack commented on pull request "doc: Add fetching single PRs from upstream to productivity.md":
(https://github.com/bitcoin/bitcoin/pull/32783#discussion_r2167374653)
```suggestion
# Fetch and checkout a single pr in a new branch with `git pr 12345`
```
(not a native english speaker, so maybe my word order gut-feeling is tricking me here and it's also fine as-is... the first "a" seems superflous though)
(https://github.com/bitcoin/bitcoin/pull/32783#discussion_r2167374653)
```suggestion
# Fetch and checkout a single pr in a new branch with `git pr 12345`
```
(not a native english speaker, so maybe my word order gut-feeling is tricking me here and it's also fine as-is... the first "a" seems superflous though)
🤔 maflcko reviewed a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2959123050)
left some nits only
review ACK f4e259b31f5785190c4912975173c5025dfe349c 🚿
<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:
...
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2959123050)
left some nits only
review ACK f4e259b31f5785190c4912975173c5025dfe349c 🚿
<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:
...
💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167386668)
f4e259b31f5785190c4912975173c5025dfe349c nit: Would be nice to use the full option name instead of `-t`
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167386668)
f4e259b31f5785190c4912975173c5025dfe349c nit: Would be nice to use the full option name instead of `-t`
💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167379833)
nit in the same commit: Could just use `print(f'\r...',flush=True,end='')
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167379833)
nit in the same commit: Could just use `print(f'\r...',flush=True,end='')
💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167311981)
nit in ee3d4e4f4940aa1468a65c4bf111ae3155aa227e: For new code, use snake_case: `archive_url` in those three lines.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167311981)
nit in ee3d4e4f4940aa1468a65c4bf111ae3155aa227e: For new code, use snake_case: `archive_url` in those three lines.
💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167330301)
nit in the same commit: Can just use `print(flush=True)`? https://docs.python.org/3/library/functions.html#print
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167330301)
nit in the same commit: Can just use `print(flush=True)`? https://docs.python.org/3/library/functions.html#print
💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167286122)
unrelated nit in 6f916050a9fa9e9fa1b226f1b2d681f8f3e21869: I don't like short option names in scripts, because I never know what they mean. Is this `-t` for `--tags`, ...?
Would be nice to use the correct long form here, while touching this line.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167286122)
unrelated nit in 6f916050a9fa9e9fa1b226f1b2d681f8f3e21869: I don't like short option names in scripts, because I never know what they mean. Is this `-t` for `--tags`, ...?
Would be nice to use the correct long form here, while touching this line.