💬 ryanofsky commented on pull request "log: expand BCLog::LogFlags (categories) to 64 bits":
(https://github.com/bitcoin/bitcoin/pull/26619#discussion_r1715401167)
In commit "log: expand BCLog::LogFlags (categories) to 64 bits" (9bed303b9cc28e13b12af067b34e6058cbccf639)

Instead of hardcoding uint64_t throughout this PR, it might make sense to add `using CategoryMask = uint64_t;` and use `CategoryMask` as a type name describing purpose of the type rather than how many bits it presently contains.

👍 ryanofsky approved a pull request: "fix: increase consistency of rpcauth parsing"
(https://github.com/bitcoin/bitcoin/pull/30401#pullrequestreview-2235751865)
Code review ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586. Since last review commit message was just tweaked to clarify previous behavior.

I hope this will get reviewed and merged. It is now just a one line fix!

💬 andrewtoth commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2286433209)
> > I wonder if there is a clang-tidy warning that can be used here instead.

> I have installed Sonar locally, reproduced the warning first, it doesn't appear anymore afer the change.

I think what is being suggested here is to configure clang-tidy to produce this warning in our own CI instead of Sonar, which can then be shown to be fixed by this fix. I think that would be a more valuable change.

> It seems to me that the Coin class already has an explicit move constructor that moves the
...

💬 sipa commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2286446542)
> How can we be sure that this happens?

A class has an implicitly-declared move constructor whenever:
* There are no user-declared copy constructors.
* There are no user-declared copy assignment operators.
* There are no user-declared move constructors.
* There is no user-declared destructor

(from https://en.cppreference.com/w/cpp/language/move_constructor)

Personally, I find these rules too inscrutable to judge from a quick glance at a class, so if I want a class that is guaranteed to have a
...

⚠️ theStack opened an issue: "doc: deduplicate list of chain/network strings in RPC/parameter help texts"
(https://github.com/bitcoin/bitcoin/issues/30645)
### Motivation

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.

### Possible solution

For better maintainability, the list should be deduplicated, either with a constant string or a function creating that string like `ListChainTypes`. See the suggestions https://github.com/bitcoin/bitcoin/pull/306
...

💬 theStack commented on pull request "doc: add missing "testnet4" network string in RPC/init help texts":
(https://github.com/bitcoin/bitcoin/pull/30642#issuecomment-2286468371)
Thanks for the reviews! Deduplicating the chain list seems to be a good idea, I opened a good first issue for that: https://github.com/bitcoin/bitcoin/issues/30645

🚀 glozow merged a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642)

📝 epiccurious opened a pull request: "feat(build): improve dependency error reporting in autogen scripts"
(https://github.com/bitcoin/bitcoin/pull/30646)
Improve error reporting in `autogen.sh` (root directory) and `src/minisketch/autogen.sh` by redirecting the autoreconf dependency check's error message to stderr.

Add check for autoreconf dependency to `src/secp256k1/autogen.sh`.

## Impact

This PR modifies all three autogen build scripts:
1. `autogen.sh` (root directory)
2. `src/minisketch/autogen.sh`
3. `src/secp256k1/autogen.sh`

**`autogen.sh` (root directory) and `src/minisketch/autogen.sh`:**

- No change to the functiona
...

💬 martinsaposnic commented on issue "doc: deduplicate list of chain/network strings in RPC/parameter help texts":
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2286498559)
working on this 🙂

💬 epiccurious commented on pull request "feat(build): improve dependency error reporting in autogen scripts":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286499127)
Please let me know if it's preferred to squash the three commits.

💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2286505348)
Thanks @mzumsande - [all 11 CI runs with your patch succeed](https://github.com/0xB10C/bitcoin/actions/runs/10359982384/job/28712303493).

This should confirm it's indeed a lifetime issue.

💬 sipa commented on pull request "feat(build): improve dependency error reporting in autogen scripts":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286517592)
The changes to secp256k1 should be done in https://github.com/bitcoin-core/secp256k1, and the changes to minisketch should be done in https://github.com/sipa/minisketch.

I assume the autogen.sh will go away with the imminent cmake migration?

📝 epiccurious converted_to_draft a pull request: "feat(build): improve dependency error reporting in autogen scripts"
(https://github.com/bitcoin/bitcoin/pull/30646)
Improve error reporting in `autogen.sh` (root directory) and `src/minisketch/autogen.sh` by redirecting the autoreconf dependency check's error message to stderr.

Add check for autoreconf dependency to `src/secp256k1/autogen.sh`.

## Impact

This PR modifies all three autogen build scripts:
1. `autogen.sh` (root directory)
2. `src/minisketch/autogen.sh`
3. `src/secp256k1/autogen.sh`

**`autogen.sh` (root directory) and `src/minisketch/autogen.sh`:**

- No change to the functiona
...

💬 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,
....
};
```