💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171554198)
Fixed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171516957)
Done.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171514903)
Yep, that's a good point. Will do, thanks!

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171552243)
Reworded.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171549876)
Good point, added.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171551677)
Good point, done.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171552108)
Thanks, done.

💬 jonatack commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1171560928)
> Seems like a lot of these includes are unnecessary and can be removed... With the following diff it still compiles

A way to verify is to add the file to `ci/test/06_script_b.sh` and look at the `lint` CI output after repushing.

💬 jonatack commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1171566094)
>> the tests added in this PR can be done without ... touching the source code at all

> Yes, that works. Here it is:

Nice!

Virtualization/dynamic dispatch has performance and overhead costs in addition to increased class/code size and complexity, and there are places in this codebase where it seems to have been avoided, and still is, for that reason. For an example, see the discussion in #25349.

If virtualization is still considered to be worth the overhead here, on its own merits ab
...

💬 jonatack commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#issuecomment-1515003669)
Concept ACK on adding testing modulo the comments above and https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1171566094. Built and ran a green make check on each commit. Needs rebase.

💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1515021149)
They're both experimental RPCs, so the fact that dropping support for them as a positional param (1) is a backwards incompatibility doesn't seem terribly important? That we can do positional+named params easily now also seems like it doesn't make things very inconvenient?

Could add an `RPCArgOptions{.also_positional=true}` to mark the `options` members that are intentionally duplicating positional arguments.

🚀 fanquake merged a pull request: "ci: build libc++ in DEBUG mode in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27448)

👋 fanquake's pull request is ready for review: "ci: standardize custom libc++ usage in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27485)

💬 fanquake commented on pull request "ci: standardize custom libc++ usage in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27485#issuecomment-1515100837)
Rebased past #27448, and taking out of draft.

📝 fanquake opened a pull request: "[WIP] ci: Use DEBUG=1 in depends for MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27495)
Followup to #27448, which was deffered, as it produces #27448 and another similar issue in sqlite, see comment here: https://github.com/bitcoin/bitcoin/pull/27448#issuecomment-1514902450.

💬 fanquake commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171669217)
Followed up in #27495.

💬 fanquake commented on issue "test: use-of-uninitialized-value in sqlite3Strlen30":
(https://github.com/bitcoin/bitcoin/issues/27222#issuecomment-1515129711)
See #27495, which should reproduce this, and a similar issue on aarch64:
```bash
2023-04-19T14:57:40.859244Z [test] [validation.cpp:2834] [ConnectTip] [bench] - Load block from disk: 0.33ms [0.00s (infms/blk)]
2023-04-19T14:57:40.859773Z [test] [validationinterface.cpp:25Uninitialized bytes in __interceptor_strcmp at offset 0 inside [0xe01000000e0c, 1)
==37748==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0xaaaab8a82008 in sqlite3BtreeOpen /home/fedora/ci_scratch/depends/wo
...

💬 ryanofsky commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1171681447)
In commit "refactor: Replace string chain name constants with ChainTypes" (5589a4b0dd5b25017299f666127b7537f5f7a4c1)

It seems messy and potentially error prone for these functions convert from enum values to strings and back to enums again. Would suggest avoiding unneeded conversions with something like:

```c++
std::variant<ChainType, std::string> ArgsManager::GetChainArg() const
{
...
const auto chain_arg = GetArg("-chain");
const bool fRegTest = get_net("-regtest");

...

👍 ryanofsky approved a pull request: "refactor: Move chain constants to the util library"
(https://github.com/bitcoin/bitcoin/pull/27491#pullrequestreview-1392475293)
Code review ACK bbe05915adad45560907b5a9b58fc55052838c08. Left a few suggestions, but this looks good in its current form

💬 ryanofsky commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1171598387)
In commit "refactor: Create chaintype files" (bd6493bf37554b1fc5848d81bce2bc5487a952dd)

Might be better to move the `throw` after the `switch` statement and drop the `default:` case so the compiler would warn about unhandled enum values

Same suggestion applies to `CreateChainParams` and `CreateBaseChainParams` in the next commit