Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2167155199)
The reviewers of the PR are only ACKing that the actual data from the collaborative run was used and that the data was encoded correctly by reproducing the file hash. This is the part I am referring to a little further below:

```
Files will not be merged to the repository
without at least two additional reviewers confirming that the process described
above was followed as expected and that the encoding step yielded the same
file hash.
```

The 5 participants is the number I am looking
...
👍 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
👍 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).
💬 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.
🤔 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 )
💬 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).
💬 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.
👋 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)
📝 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
...
💬 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.
💬 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
...
🤔 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
...
💬 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?
💬 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
...
💬 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)
🤔 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:
...
💬 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`
💬 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='')
💬 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.
💬 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