Bitcoin Core Github
44 subscribers
119K links
Download Telegram
🚀 fanquake merged a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32589)
💬 willcl-ark commented on pull request "test: disable secp256 tests by default":
(https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3004247908)
I think if there is concern about configurations then I agree it would make more sense to drop the iterations to 4 or 8 as suggested by @hebasto and @real-or-random
💬 vasild commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3004262238)
> shouldn't this just be `CC=... CXX=...`

Yes, I think it should be. But is not because currently `depends/hosts/default.mk` contains:

```make
default_host_CC = $(host_toolchain)gcc
default_host_CXX = $(host_toolchain)g++
```

`gcc` and `g++` are hardcoded there. The following would make it possible to override the C and C++ compiler from the environment:

```diff
-default_host_CC = $(host_toolchain)gcc
-default_host_CXX = $(host_toolchain)g++
+default_host_CC = $(host_toolchain)
...
💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#discussion_r2166392824)
Thanks! Reworked.
💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#discussion_r2166395676)
Correct. I've just picked @sipa's [commit](https://github.com/bitcoin/bitcoin/pull/29167/commits/a51ebae75edcd33bcfd83eeae7e4ca4d6e8f74f7).
fanquake closed an issue: "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install"
(https://github.com/bitcoin/bitcoin/issues/32428)
🚀 fanquake merged a pull request: "build: add root dir to CMAKE_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798)
💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3004291231)
@hodlinator

Thank you for the review. Your feedback has been addressed.
📝 fanquake opened a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32810)
Backports:
* #32798
💬 fanquake commented on pull request "build: add root dir to CMAKE_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3004302416)
Backported to 29.x in #32810.
📝 fanquake opened a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32811)
Backports:
* #32765
* #32776
* #32777
🤔 maflcko reviewed a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2957517215)
lgtm. Nice test for both branches

review ACK 8d257ed5937a47e1a60b7fe085f2e984eb60a956 👋

<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=
tru
...
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166328136)

getheader -> getheaders [matches the method and plural usage elsewhere]
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166409317)
```suggestion
with node.assert_debug_log(expected_msgs=["initial getheaders (0) to peer=0"]):
peer1 = node.add_p2p_connection(P2PInterface())
peer1.wait_for_getheaders()
```

nit: it is a bit better to check the state (and wait for it directly) than to indirectly read the debug log in a loop until a specific log line is hit.
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166390209)
```suggestion
variable_timeout_sec = math.ceil(HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER_MS /1_000 *
seconds_since_best_header / POW_TARGET_SPACING_SEC) # ceil, to make this function usable with mocktime, which only has second precision.

return (current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE_SEC + variable_timeout_sec)
```

seems a bit odd to be rounding down in this function and then apply a random +1 offset below. So it is actually rounding up. It would be
...
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166420099)
also, could check the getheaders `block_hash=int(node.getbestblockhash(), 16)`
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166403740)
nit: it is probably a bit faster to `self.nodes[0].disconnect_p2ps()` when the goal is to disconnect all peers (and throw away their peer state)
🤔 dergoegge requested changes to a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2957730107)
Code review ACK 2ac8696b53e455dd27c8341828404a23b5cb68a9

I think the commit order needs to change, so that 3c30ee9107fd0e916f9784b091a4d02f3a73ce46 comes before the tests that make use of the change?

nit: I'd also suggest to squash b44d31455ad46ca3ed95690dfc0a913445b9c1c9 and 5fe08e01384c4a4c8525060d365fa38c312758f0 into one commit, so there is no intermediate state between the commits where the validation logs are rate limited.
💬 josibake commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3004472779)
> or is this issue MULTIPROCESS=1 capnp specific?

I don't think the issue is specific to multiprocess, but rather I'm running into the issue when trying to build multiprocess. I think the root cause is as @vasild mentions, i.e., hardcoding gcc/g++. Admittedly, it _is_ weird that I'm hitting this with multiprocess but its because I am specifically trying to build in an environment without gcc/g++.

I was surprised that `make -C depends -j$(nproc) CC=clang CXX=clang++ MULTIPROCESS=1` didn't w
...
🤔 hodlinator reviewed a pull request: "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)"
(https://github.com/bitcoin/bitcoin/pull/32564#pullrequestreview-2957841876)
re-ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf