👍 BrandonOdiwuor approved a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235675454)
ACK 701530045553f2b9671a3fffea301bf4dc954514
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235675454)
ACK 701530045553f2b9671a3fffea301bf4dc954514
👍 vasild approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2235704287)
ACK 9bed303b9cc28e13b12af067b34e6058cbccf639
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2235704287)
ACK 9bed303b9cc28e13b12af067b34e6058cbccf639
👍 ryanofsky approved a pull request: "logging: use bitset for categories"
(https://github.com/bitcoin/bitcoin/pull/26697#pullrequestreview-2235717036)
Code review ACK d2067124aae098980de5c4a810753486491faea7. Since last review, just renamed array member to m_bits, switched from 32 to 64 bit ints and added comment about any and none methods being not atomic.
(https://github.com/bitcoin/bitcoin/pull/26697#pullrequestreview-2235717036)
Code review ACK d2067124aae098980de5c4a810753486491faea7. Since last review, just renamed array member to m_bits, switched from 32 to 64 bit ints and added comment about any and none methods being not atomic.
👍 danielabrozzoni approved a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235717662)
ACK 701530045553f2b9671a3fffea301bf4dc954514
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235717662)
ACK 701530045553f2b9671a3fffea301bf4dc954514
🤔 furszy reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2235734674)
Worked further on your [8d0b5c1](https://github.com/bitcoin-core/gui/pull/824/commits/8d0b5c1526c7abfe8048db15dd3df8c898a2fd35) to eliminate the need for the extra 'Yes, encrypted' button in the migration dialog, which I found quite unfriendly. Could replace it directly for https://github.com/furszy/bitcoin-core/commit/e8d147eeaacac7d434aad8dbbc5022aadf7d112a. It detects the wallet encryption status automatically regardless of its loaded/unloaded state.
Other than this, code is looking good.
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2235734674)
Worked further on your [8d0b5c1](https://github.com/bitcoin-core/gui/pull/824/commits/8d0b5c1526c7abfe8048db15dd3df8c898a2fd35) to eliminate the need for the extra 'Yes, encrypted' button in the migration dialog, which I found quite unfriendly. Could replace it directly for https://github.com/furszy/bitcoin-core/commit/e8d147eeaacac7d434aad8dbbc5022aadf7d112a. It detects the wallet encryption status automatically regardless of its loaded/unloaded state.
Other than this, code is looking good.
🤔 Johnnygatvol reviewed a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2235739639)
I need to know where my money is going
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2235739639)
I need to know where my money is going
👍 ryanofsky approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2235733223)
Code review ACK 9bed303b9cc28e13b12af067b34e6058cbccf639. I do think #26697 is a nicer alternative to this PR because it makes code more safe and readable. But this approach is also fine.
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2235733223)
Code review ACK 9bed303b9cc28e13b12af067b34e6058cbccf639. I do think #26697 is a nicer alternative to this PR because it makes code more safe and readable. But this approach is also fine.
💬 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.
(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!
(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
...
(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
...
(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
...
(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
(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)
(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
...
(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 🙂
(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.
(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.
(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?
(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
...
(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
...