💬 hebasto commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2589491670)
> secp is the only C code we have. So arguably there's no need for `APPEND_CFLAGS` at all when `SECP256K1_APPEND_CFLAGS` can just be used directly, no?

Correct. It is solely about maintaining consistency with the other `APPEND_*` flags. I believe the following
```sh
cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DAPPEND_CFLAGS="-fprofile-update=prefer-atomic"
```
is better than
```sh
cmake -B build -DAPPEND_CXXFLAGS="-fprofile-update=prefer-atomic" -DSECP256K1_APPEND
...

👍 theStack approved a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2549426467)
re-ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424

👍 brunoerg approved a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2549447462)
code review ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390

💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914669735)
I was wondering if these are stored as part of the block index, or exposed via RPC. Otherwise changing their numeric values shouldn't matter.

💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914673933)
Indeed seems better to keep this. Perhaps mention the last mainnet checkpoint height, since `MinimumChainWork()` is far above that.

💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914677090)
Judging from the change to `ipc_test.cpp` below it seems that eventually the values might be sent over IPC. At that point we have to fix their integer values. Might as well do it now? cc @ryanofsky

💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914687073)
This is a nice improvement. I've been running my node with `-nocheckpoints=1` for years, so now it will just throw a warning rather than abort launch. (which I tested)

💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2589713954)
Concept ACK

If we go ahead with this, right after the v29 branch-off would seem like a good time.

I plan to expand the alternate mainnet chain in #31583 to 11111+ blocks and have ~p2p_dos_header_tree.py` use that instead. That would make it easier to revert the checkpoints even after testnet3 is completely dropped.

💬 hebasto commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2589737954)
> It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined. But I'm not sure who's supposed to define that? I suspect that adding it to our cppflags _might_ be the correct thing to do, but who knows what side-effects it would have.

Defining `_NETBSD_SOURCE` works, but I agree that it is not an optimal solution, especially when the `_XOPEN_SOURCE` is already defined.

> Also, it seems these were masked by the fact that we use `-Wno-
...

💬 kevkevinpal commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2589756582)
light code review ACK [2ed161c](https://github.com/bitcoin/bitcoin/pull/31646/commits/2ed161c5ce648cb66ec3d2941b02d68b6ca4c390)

I think it makes sense that our test suite should not attempt to make outbound connections this is a welcomed change

🤔 marcofleon reviewed a pull request: "fuzz: Abort when global PRNG is used before SeedRand::ZEROS"
(https://github.com/bitcoin/bitcoin/pull/31548#pullrequestreview-2549561974)
ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0

iiuc this PR adds a check to make sure that no randomness is used before `SeedRandomStateForTest(SeedRand::ZEROS)` occurs, vs what `check_globals` does which is just checking if seeding happened at any point in the fuzz test.

Tested by moving `SeedRandomStateForTest(SeedRand::ZEROS)` to after test setup in `utxo_total_supply` and saw the assertion failure.
```
/src/test/util/random.cpp:45 SeedRandomStateForTest: Assertion `!g_used_g_prng' fail
...

💬 Sjors commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2589771910)
Tested on my self-hosted Cirrus setup, seems fine: https://cirrus-ci.com/task/6297483418009600

👍 BrandonOdiwuor approved a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2549577230)
Code Review ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390

🤔 Mohammed-Alanazisa reviewed a pull request: "depends: Fix `CXXFLAGS` on NetBSD"
(https://github.com/bitcoin/bitcoin/pull/31502#pullrequestreview-2549579161)
gh pr checkout 31502gh

📝 Mohammed-Alanazisa opened a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/31652)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ hebasto closed a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/31652)

📝 hebasto locked a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/31652)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 Sjors commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1914754381)
This does make the debug level even noisier. But since it doesn't have the word "disconnect" it's probably fine.

💬 Sjors commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#issuecomment-2589826410)
utACK 286de9749612565fd8d42f08f66c8426faf8a1f7

💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1914779564)
I removed this because the same options were enabled by CMake's HARDERNING option when in Debug mode, so this became redundant. However, later I changed HARDENING+Debug to only use `_GLIBCXX_ASSERTIONS` instead of `-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC` because the latter produced: `undefined reference to mp::Connection::removeSyncCleanup(__gnu_debug::_Safe_iterator...`.

Maybe this should stay. I wonder why using `-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC` from here does not produce an
...

💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1914780635)
Interesting. Is that using libstdc++ or libc++?