💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491148886)
Just tried that - if I store the constants as `string_view`, e.g. `constexpr std::string_view VERSION{"version"};`, then I get this greeting:

```
test/util/net.cpp:34:9: error: no matching function for call to 'Make'
34 | NetMsg::Make(NetMsgType::VERSION,
| ^~~~~~~~~~~~
./netmessagemaker.h:14:23: note: candidate function template not viable: no known conversion from 'const std::string_view' (aka 'const basic_string_view<char>') to 'std::string' (aka 'basic_string<
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1946293497)
Bumping macOS to 14 on the CI does not help. I also can't reproduce this failure on my own Intel macOS machines, not on 13.6.4 and not on 14.2.1. A `Sock` mock is probably the most robust solution, but it'd be nice to find another workaround.

👍 ryanofsky approved a pull request: "log: Nuke error(...)"
(https://github.com/bitcoin/bitcoin/pull/29236#pullrequestreview-1883061202)
Code review ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96

This is a nice improvement, and it actually doesn't change many lines of code, so I don't think we should be worried about conflicts if we want to make more improvements later, like changing log levels, or adding categories, or switching to util::Result.

One thing I think we probably should revisit after this is the interaction with `-logsourcelocations`, since it seems like a lot (maybe most?) of the new LogError calls are manually
...

💬 maflcko commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1946313480)
```
test/sharedlock_tests.cpp:18:17: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
18 | workers.push_back(std::thread(task));
| ^~~~~~~~~~~~~~~~~~~~~~ ~
| emplace_back(

👍 ryanofsky approved a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1883074045)
Code review ACK f1684bb88a878eb3f54e945db39ea69b14256eef

"This RPC may take a long time to complete" seems like much more helpful and accurate documentation than "This call may not work as expected and may be changed in future releases" at this point.

💬 TheCharlatan commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1946371110)
Rebased c34dcea019ea3b639b9f387df18777bc9a2d1534 -> 7882748c1c260dd77c22ac3a85bad18768c08f0d ([setEntryRefs_4](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_4) -> [setEntryRefs_5](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/setEntryRefs_4..setEntryRefs_5))

* Fixed conflict with #29424

👍 willcl-ark approved a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1883175987)
ACK f1684bb88a878eb3f54e945db39ea69b14256eef

Agree that this should no longer be marked as experimental in the next release even though some migrations do appear to take a(n unexpectedly) long time.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1946417576)
Thank you for the review @vasild,

Updated c02fd0379c425f486f1b80a81962c1aa68b8a852 -> 4e270f3aa70f0bb4e7053d84d5cdd1a1fdcf85c2 ([noGlobalSignals_15](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_15) -> [noGlobalSignals_16](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_15..noGlobalSignals_16))

* Addressed @vasild's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1
...

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1491224522)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484708844

> The next paragraph refers to this deleted comment ("using the nStatus flag mentioned above") and should be updated too.

Thanks, fixed

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1491225507)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1483671675

> comment can be removed, it's no longer faked.

Good catch, dropped this comment.

🤔 ryanofsky reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1883156605)
Thanks for the review!

Updated c9ca4ca4d970f9d42d0254775c04d114bd2152c3 -> 8ab4e07a1a54c9f4818a557fd1182bbb59e1cf9b ([`pr/nofake.7`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.7) -> [`pr/nofake.8`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.7..pr/nofake.8)) with suggestions

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1491224965)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1483625151

> Why check these values indirectly instead of directly (field `txcount`)?

Thanks, checking these values directly now

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1491224648)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484705186

> Is it necessary to keep a placeholder / would just removing it break something? I'm asking beacuse as far as I can see, the `enum BlockStatus` is only used in `IsValid` / `RaiseValidity`, but not for `nStatus`, which is a `uint32_t`.

This is mostly just for documentation. I think it's useful to keep a note that the field was previously used and may be saved to disk. Probably though it would not break anything if the
...

💬 theuni commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#discussion_r1491272508)
Agreed, but would you be ok with fixing this up in a follow-up? I think it'd make sense to address while dealing with this https://github.com/bitcoin/bitcoin/pull/29404#pullrequestreview-1870581585

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1491287980)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484705186

> Is it necessary to keep a placeholder / would just removing it break something? I'm asking beacuse as far as I can see, the `enum BlockStatus` is only used in `IsValid` / `RaiseValidity`, but not for `nStatus`, which is a `uint32_t`.

If this asking whether dropping the placeholder would change the underlying enum type, I think it wouldn't because it it's declared 32-bit unsigned explicitly (`enum BlockStatus : uint3
...

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#issuecomment-1946512678)
Pushed a commit to clarify the docs while touching the code here.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1491319580)
In commit "kernel: Remove dependency on CScheduler" (4e270f3aa70f0bb4e7053d84d5cdd1a1fdcf85c2)

The comment isn't accurate if you "change scheduler" to "task runner". The comment is trying to say "We can't assume CScheduler only runs in one thread, but must ensure all callbacks happen in-order, so we end up creating our own queue inside SingleThreadedSchedulerClient."

Now that SingleThreadedSchedulerClient is replaced by `std::unique_ptr<util::TaskRunnerInterface>` the comment doesn't reall
...

💬 Kixunil commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1946567935)
Hey, I had a use case: create a chain of transactions in WASM (browser) in a multiparty protocol where the child transaction must be verified before signing their parents. The inputs of the parent are controlled by the verifying user so there's no "you need a full node anyway" argument. Thus `libbitcoinkernel` is not the solution. Now in my case the template is simple enough to not need `libbitcoinconsensus` - it's fine to just verify the signatures. But being able to just run the same code as C
...

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1491329059)
good point; added

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1491329325)
mistakenly left at some point; removed