💬 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)

💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445244468)
In 4e746e223a016c00abcaece6002fbbf37ff19f73 "[mempool] evict everything paying 0 fees in TrimToSize()"

This seems like it should belong in a separate commit? At least the motivation for this is not entirely clear from the code comments nor the commit message.

💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445293662)
In 299680a5c736783fbb03fd448064ed35ef1a42a0 "[policy] add v3 policy rules"

nit: Could use `GetVirtualTransactionSize` instead of calculated the sigops adjusted size manually.

💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445328322)
In fd55f70257456e804cadd7af3e70f783de373008 "[functional test] v3 transaction submission"

nit: use `self.check_mempool`, here and elsewhere.

💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445273282)
In 299680a5c736783fbb03fd448064ed35ef1a42a0 "[policy] add v3 policy rules"

nit: This could be at the top of the loop as well instead of retrieving it for every input.

💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445305284)
In 6de06e116f0dbe632be0e4babd45d3f59c117171 "[policy/validation] allow v3 transactions with certain restrictions"

This seems unnecessary since if the tx was v3, then we wouldn't be able to reach this error condition.