💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426803120)
nit in 9f71029192282baa4f7be6b5124bb7bc2491ef84: Could mark commit as "refactor", if it is one?

💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426843022)
nit in db5ed8682f99c64a97e598bda69147307b447820: comma before etc?

💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426847187)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Clarify "production system *with enough free storage space*". IIRC ~1TB per 100 connection-years? So if you have 100 connections and run for a year, 1TB of your storage will be eaten by transaction relay `net` debug messages, or so.

💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426850159)
same :)

💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426815783)
Also, could add a test in this commit about `[info]` that will change in the next commit?

💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426849181)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace "disk" with "storage" or "persistent storage", so that non-spinning storage doesn't feel excluded?

💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426853766)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace `bitcoind` with "the program" or "Bitcoin Core", so that `bitcoin-node` or `bitcoin-qt` do not feel excluded?

💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426859372)
Or replace `UniValue` with https://github.com/nlohmann/json

💬 sipa commented on pull request "fuzz: Improve fuzzing stability for minisketch harness":
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1856042019)
This fixes it I think:

```diff
--- a/src/test/fuzz/minisketch.cpp
+++ b/src/test/fuzz/minisketch.cpp
@@ -65,7 +65,9 @@ FUZZ_TARGET(minisketch)
sketch_br.Deserialize(sketch_b.Serialize());

Minisketch sketch_diff{std::move(fuzzed_data_provider.ConsumeBool() ? sketch_a : sketch_ar)};
- sketch_diff.Merge(fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br);
+ auto& sketch_merge = fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br;
+ if (sketch_diff.GetIm
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1856045015)
I renamed `-stratumv2` to `-sv2` and `stratumv2port` to `-sv2port`. Added `-sv2interval` (default 30 seconds) and `-sv2feedelta` (default 1000 sat) to control how often templates are updated.

💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426869879)
> In commit "ArgsManager: return path by value from GetBlocksDirPath()" (https://github.com/bitcoin/bitcoin/commit/856c88776f8486446602476a1c9e133ac0cff510)

Wrong commit? :)

> documention

Thanks, copy-pasted this and removed my one-line comment.

💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1856053272)
> To verify that nothing has been forgotten in the last commit which renames `fs::path::u8string()` to `fs::path::utf8string()` I added `std::u8string u8string() const = delete;`

This should not be needed, because the two functions return different and incompatible types, so any oversight should result in a compile failure as-is.

💬 sipa commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1856056462)
@vasild FWIW, Bitcoin Core used to use json-spirit before the introduction of UniValue. The motivating argument was being able to access the string representation of numeric value (because when amounts are passed as JSON "numbers", we don't want to roundtrip through `double` before converting to `CAmount`).

💬 hebasto commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856059883)
> I still don't really understand this fix

This fix makes Homebrew skip upgrading the `aws-sam-cli` package that we do not use.

💬 maflcko commented on issue "bitcoin-tx replaceable value should be optional, but isn't":
(https://github.com/bitcoin/bitcoin/issues/28638#issuecomment-1856061889)
See https://github.com/bitcoin/bitcoin/pull/29022

👍 instagibbs approved a pull request: "Make bitcoin-tx replaceable value optional"
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1782087346)
untested ACK https://github.com/bitcoin/bitcoin/pull/29022/commits/98afe7866185ed4157ffc581763e11dc02fcbae0

Test coverage looks good, thanks!

👍 ryanofsky approved a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1782116404)
Code review ACK 66667130416b86208e01a0eb5541a15ea805ac26. Just documentation change since last review.

To clarify and clean things up more, we could also document the `fs::path::utf8string()` method and `fs::u8path` function as being inverses of each other, and maybe rename `fs::u8path` to `fs::utf8path` for consistency. I don't think either of these things are necessary though, just thoughts for improvement.

💬 theuni commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856117820)
> > This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.
>
> I know a [WIP] branch for this "exists" in some form. cc @theuni

It's not a WIP so much as a POC: https://github.com/theuni/bitcoin/commits/replace-boost-signals/

That implements the subset of signals that we rely on as a drop-in replacement. It passes tests and would serve as a good guide, but I don't think that's how we'd want to do it.

I could look at pic
...

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426925744)
this block is worth encapsulating and adding unit tests to ensure the populated sets `ws.m_num_in_package_ancestors` and `ws.m_ancestors_of_in_package_ancestors` match expected

💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1856138054)
Big concept ACK on the interface. This is a nice simplification.