👋 pinheadmz's pull request is ready for review: "system: allow GUI to initialize default data dir"
(https://github.com/bitcoin/bitcoin/pull/27273)

💬 pinheadmz commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1477880541)
@desirepl PR https://github.com/bitcoin/bitcoin/pull/27273 is ready for review, are you able to try it out / review? See if it fixes your use-case and leave a comment if you can.

I did not address anything directly related to `bitcoin-cli`, I wasn't able to reproduce any issues and I'm not exactly sure what your cli issue was. Can you provide steps and unexpected behavior for that?

💬 fanquake commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477881142)
> Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .

💬 fanquake commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477882049)
> Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .

Last I checked, turning that on was essentially unusable, due to false-positives. Will take another look.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1477882937)
FWIW I plan to leave this as-is.

💬 fanquake commented on pull request "Refactor: Remove unused FlatFilePos::SetNull":
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1477884296)
Concept ACK

💬 MarcoFalke commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477885991)
It should only hit on classes that have a `SetNull` method, no? In those cases the `SetNull` method can be removed, or the initializers duplicated from the method?

💬 Sjors commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477915405)
One solution is to ignore the problem but fix the address sanitizer test suite.

This branch does that: https://github.com/bitcoin/bitcoin/compare/master...Sjors:bitcoin:2023/03/legacy-leak

It sets `options.use_shared_memory = true` for wallet unit tests, and it adds `-dbpriv=0` to the default config of functional tests.

The second commit also makes the wallet migration tool `use_shared_memory`. This seems safe, since the operation only runs briefly, so the concern about other processes
...

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477921746)
Thanks for the benchmark @jamesob! There was no cache flush in the benchmark, that's why the memory usage was lower. When flushes would happen with e.g. lower dbcache size or longer range of blocks memory usage should be about equal, but then there should be an even larger performance benefit for this PR because it can cache more data with the same memory.

💬 dergoegge commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1477933351)
Code review ACK 2c3a90f663a61ee147d785167a2902494d81d34d

💬 dergoegge commented on pull request "Refactor: Remove unused FlatFilePos::SetNull":
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1477936145)
Code review ACK fa67b8181c3ecf94395ecc50fd8acd436f1f8c3a

💬 jamesob commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477939852)
> When flushes would happen with e.g. lower dbcache size or longer range of blocks memory usage should be about equal, but then there should be an even larger performance benefit for this PR because it can cache more data with the same memory.

Yup - I started another run with dbcache=1000.

💬 martinus commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477952301)
re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a

💬 dergoegge commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1143495360)
Done!

💬 MarcoFalke commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1477980568)
My preference would be to suppress libdb4.8, but I guess that is not possible because the sanitizer doesn't see the lib in static builds?

💬 hebasto commented on pull request "Refactor: Remove unused FlatFilePos::SetNull":
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1478004598)
Concept ACK.

💬 hebasto commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1478008931)
> Last I checked, turning that on was essentially unusable, due to false-positives.

[Indeed](https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1477825604).

👍 theStack approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
re-ACK 60c3f4918190900e5f79341abcc0878214656257

👍 josibake approved a pull request: "Log new headers"
(https://github.com/bitcoin/bitcoin/pull/27278)
ACK https://github.com/bitcoin/bitcoin/pull/27278/commits/2c3a90f663a61ee147d785167a2902494d81d34d

There were some great suggestions for how this could be improved, but I think it's smart to address them in a follow-up, considering this is useful as-is and addresses a real need.

💬 josibake commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1143555402)
leaving this style nit in case someone ends up re-touching this in a follow-up, because this makes my eyes burn :sob:

```suggestion
const auto msg = strprintf(
"Saw new header hash=%s height=%d", hash.ToString(), pindex->nHeight
);
```