🤔 pablomartin4btc reviewed a pull request: "test: wallet migration, add coverage for tx extra data"
(https://github.com/bitcoin/bitcoin/pull/29204#pullrequestreview-1809733645)
ACK 016cc807f77a9128d430a0df1edd133521628a33

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1881626022)
> It looks safe to me to replace all uses of `std::string` with `std::string_view` in this PR.

Addressed most, except for `exclusively_command` which involves dynamic changes with different string values on each cycle, making it unsuitable for a `std::string_view`. Therefore, `std::string` is retained for `exclusively_command`. Thanks!

💬 aureleoules commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1881627296)
> I see, that is a bit unfortunate. I also found some more things to change: https://github.com/TheCharlatan/bitcoin/commit/63fcfbecd133551032857187e5e639d36a2f7b06, but not sure if non-tidy changes should be made here tbh.

Thanks for the commit, I added it. Did you find them manually or with clang-tidy?
I'm not sure why the CI didn't catch them...

👍 stickies-v approved a pull request: "logging: Simplify API for level based logging"
(https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1808983581)
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c

After e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c, is there a reason not to remove `LogPrintfCategory` completely since it's now only used in tests?

> For the severity level logging, I'd prefer to have a single macro like Log(category, level) with a consistent, simple interface, rather than five macros.

In most other codebases I've worked on, having a per-level logging function was pretty standard. I think the approach suggested here is more dev
...

💬 stickies-v commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1444750684)
nit: it seems like we can easily avoid using a macro here, wouldn't that be better?
```suggestion
template <typename... Args>
static inline void LogInfo(Args&&... args) { LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, std::forward<Args>(args)...); }
```

💬 stickies-v commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445019995)
nit: for consistency to avoid confusion
```suggestion
Note that `LogPrint(BCLog::CATEGORY, fmt, params...)` is a deprecated
alias for `LogDebug(fmt, params...)`.
```

💬 stickies-v commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1444540725)
nit: would it make sense to just move this to `BCLog`, out of `::Logger`?

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

```diff
diff --git a/src/init/common.cpp b/src/init/common.cpp
index 6560258ef5..0832845921 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -31,7 +31,7 @@ void AddLoggingArgs(ArgsManager& argsman)
ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for
...

💬 mzumsande commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#discussion_r1445146016)
`count` -> `contains`

💬 aureleoules commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#discussion_r1445153343)
Thanks, fixed.

🤔 brunoerg reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-1809876740)
WIP review!

Tested with an empty addrman and `-nodnsseed -blocksonly -debug=net`. I could check the new flow, an example:

1. `2024-01-08T19:08:02Z [net] Added hardcoded seed: 192.146.137.44:8333`
2. `2024-01-08T19:08:18Z [net:debug] trying v1 connection 192.146.137.44 lastseen=0.0hrs`
3. `2024-01-08T19:08:19Z New addr-fetch v1 peer connected: version: 70016, blocks=824898, peer=2`
4. `2024-01-08T19:08:19Z [net] sending getaddr (0 bytes) peer=2`
5. `2024-01-08T19:08:19Z [net] Received
...

💬 jonatack commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1881690072)
> forcing usage of a single `Log` function

That doesn't make sense if the goal, like the title of this PR, is to simplify the API. One macro is simpler, and less code and test coverage, than five.

I plan to review here soon. My work on severity-based logging in https://github.com/bitcoin/bitcoin/pull/25203 was set aside to first fix current bugs, and add missing tests and documentation in https://github.com/bitcoin/bitcoin/pull/27231 that has been open for ten months.

💬 ajtowns commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445230410)
Until we can use [std::source_location](https://en.cppreference.com/w/cpp/utility/source_location) (see https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1852024695) we need to use macros in order to pass `__func__`, `__FILE__` and `__LINE__` information through.

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1881694073)
I'm going to open an alternate pull that does what you suggest and see what reviewers (if any) prefer.

💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1881695275)
Force pushed to fix clean commits.

🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1800970305)
Addressed @sipa’s comments, and completely restructured the PR. Added two more tests, and counting of the number of attempts used in CoinGrinder to measure the optimizations.

Ready for review

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445276896)
Thanks, I replaced all the `std::vector::at` with `std::vector::operator[]`.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1439793432)
I have reduced the description here to the interface and will add the remaining explanations to the corresponding spots of the function.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1439882229)
I have completely rewritten my description. I hope it’s clearer now.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445277299)
I’ve completely rewritten CoinGrinder to implement this suggestion.

👋 murchandamus's pull request is ready for review: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877)