💬 epiccurious commented on pull request "feat(build): improve dependency error reporting in autogen scripts":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286523041)
@sipa understood about secp256k1 and minisketch.

Setting as draft to remove those changes from this PR.

💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2286529365)
> Could replace it directly for [furszy/bitcoin-core@e8d147e](https://github.com/furszy/bitcoin-core/commit/e8d147eeaacac7d434aad8dbbc5022aadf7d112a). It detects the wallet encryption status automatically regardless of its loaded/unloaded state.

Cherry picked.

Also fixed test failure.

💬 LarryRuane commented on pull request "log: expand BCLog::LogFlags (categories) to 64 bits":
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-2286539254)
@ryanofsky
> Instead of hardcoding uint64_t throughout this PR, it might make sense to add using CategoryMask = uint64_t

I took this suggestion, thanks. As a test, I defined `CategoryMask` to be `uint32_t`, but the compile failed because of the `ULL` suffix on all those constants (width exceeds 32 bits). So I'm trying to work around that by defining a `mask_bit` constant. (Should it have a name like `MaskBit`? It's only used within the `enum` definition.) I think it makes the code easy to r
...

👍 vasild approved a pull request: "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds"
(https://github.com/bitcoin/bitcoin/pull/30465#pullrequestreview-2235917868)
ACK 5ff8074361 the changes look ok.

I just wonder, why the change to `depends/funcs.mk` is not in `hebasto/cmake-staging`, but is in this PR?

👍 vasild approved a pull request: "depends: Amend handling flags environment variables"
(https://github.com/bitcoin/bitcoin/pull/30477#pullrequestreview-2235931261)
ACK d438b501f859befa4f758efcff463c0d647bc033

💬 ryanofsky commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2286604343)
I guess another thing I'd like to know is if this is the initial C API, and the implementation is around 3000 lines, and it doesn't handle "transactions, block headers, coins cache, utxo set, meta data, and the mempool", how much bigger do you think it will get if it does cover most of the things you would like it to cover? Like is this 20%, 30%, or 50% of the expected size?

I like the idea of reviewing and merging this PR, and establishing a way to interoperate with rust libraries and extern
...

👍 vasild approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2236014102)
ACK f64b861c0940ca90869303a9754ebe5bf40bb0c7

I was fine with `(1ULL << 23)` as well. Another way is to use the `0b` prefix (then no need for the `ULL` suffix):

```cpp
enum LogFlags : CategoryMask {
NONE = 0b00000000000000000000000000000000,
NET = 0b00000000000000000000000000000001,
TOR = 0b00000000000000000000000000000010,
MEMPOOL = 0b00000000000000000000000000000100,
HTTP = 0b00000000000000000000000000001000,
....
};
```

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1715587191)
I think I would probably add `base_blob::FromUserHex()` function wrapping `base_blob::FromHex()`. `FromHex()` would require strict formatting with fixed width strings and no extraneous characters. But when provided with user input, `FromUserHex()` would allow `0x` prefixes, and not require numbers to be padded to a fixed number of digits. Either way, numbers that are too big to fit in the blob or contained unrecognized characters would be errors.

There is no need for either of these functions
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1715612905)
> [8523d19](https://github.com/bitcoin/bitcoin/commit/8523d19bb3f8c982c6e01a4635c98cd9ac386dea): I wonder if this causes a problem when handling a block template without a witness commitment.
>
> If so, I might just have stratum v2 templates always set a coinbase witness.

This just sets a serialization flag called `allow_witness` and `fAllowWitness` in [`SerializeTransaction`](https://github.com/bitcoin/bitcoin/blob/1a41e63575986887ae34993b4433ec711ae0ffbc/src/primitives/transaction.h#L256
...

📝 TheBlueMatt opened a pull request: "Move maximum timewarp attack threshold back to 600s from 7200s"
(https://github.com/bitcoin/bitcoin/pull/30647)
In 6bfa26048dbafb91e9ca63ea8d3960271e798098 the testnet4 timewarp attack fix block time variation was increased from the Great Consensus Cleanup value of 600s to 7200s on the thesis that this allows miners to always create blocks with the current time. Sadly, doing so does allow for some nonzero inflation, even if not a huge amount.

While it could be that some hardware ignores the timestamp provided to it over Stratum and forces the block header timestamp to the current time, I'm not aware of
...

💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2286681264)
I got about 20 hours to find running libfuzzer with value profile + length 50, starting from every corpus entry that's 50 or less (seeing if throwing out longer corpus entries is better than truncating, for this kind of thing)

💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2286681759)
None of my other configs have found it yet...

👍 stickies-v approved a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2235986033)
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f

Even though not having span comparison operators can make things a bit more verbose, these changes are needed to start using `std::span` over our own implementation. This approach seems like the least invasive one (cleaning up wherever possible), and I can't see any behaviour change.

💬 stickies-v commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1715559304)
nit: missing `#include <algorithm>`

💬 stickies-v commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1715613138)
nit: I think we can use `std::array::operator==` here? (edit: or a switch statement would probably be even better)

```suggestion
if (message == mainnet_msg) {
```

<details>
<summary>git diff on fad0cf6f26</summary>

```diff
diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp
index 7d06d24be5..a760926ec8 100644
--- a/src/kernel/chainparams.cpp
+++ b/src/kernel/chainparams.cpp
@@ -689,15 +689,15 @@ std::optional<ChainType> GetNetworkForMagic(const MessageStartC
...

📝 martinsaposnic opened a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648)
As mentioned in issue https://github.com/bitcoin/bitcoin/issues/30645:

Many command line parameter and RPC help texts currently contain the list of chain/network names hardcoded ("main, test, testnet4, signet, regtest"), which is error-prone as it can easily happen to miss an instance if the list ever changes again.

This PR deduplicates the list of possible chain/network strings in RPC/parameter help texts, and it creates a macro `LIST_CHAIN_NAMES` in src/chainparamsbase.h. In the future,
...

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2286722016)
I introduced `using NoiseHash = std::array<uint8_t, HASHLEN>;` for improved readability.

Although all tests passed, I noticed while testing #29432 that SRI hangs up after the handshake. Turns out I broke the `HKDF2` implementation, which the test didn't catch because there's no hardcoded test vector. That's fixed now.

💬 martinsaposnic commented on issue "doc: deduplicate list of chain/network strings in RPC/parameter help texts":
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2286723952)
new PR with possible solution: https://github.com/bitcoin/bitcoin/pull/30648

👍 ryanofsky approved a pull request: "refactor: Add consteval ArrayFromBytes()"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2236080683)
Did not review in detail yet but light code review ACK d6d7a0b5221935518d2797aec7abc5c9632cbf68. All changes seem like what I would expect.

I think I would suggest changing title of PR to "refactor: Replace ParseHex with consteval ArrayFromHex" so it mentions ParseHex and it is more obvious how this affects existing code.

💬 ryanofsky commented on pull request "refactor: Add consteval ArrayFromBytes()":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715636099)
In commit "refactor: Add consteval ArrayFromHex()" (00dc16bc29124dfbaf9d47c5411e4ebcf95e30b5)

This warning seems unnecessarily scary and maybe not true. I don't think the function itself would use any stack space with NRVO, and callers could be calling this to initialize an array on the heap not the stack. Would suggest deleting this comment, because it seems like a warning about std::array in general, and I don't think there is anything that really distinguishes this function from other func
...