💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142036850)
In most logging systems I've used elsewhere, (i) INFO is the default loglevel, and (ii) the level is configurable via flag/envvar. I guess what I'll do here is use `Warning` and leave a TODO.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142037129)
Good point! Thanks.

💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142037513)
Indeed this code is pretty ancient (and could use a comment): https://github.com/bitcoin/bitcoin/blame/f3ae51dcced8a16175426051ce888130cc2493af/src/main.cpp#L2056-L2063

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476147317)
@fanquake that was 65840 bytes, this one is 320 bytes, so I assume it's a different issue?

I was going to add more details if anyone couldn't reproduce it, but here we go:

Found it while testing a PR, but reproduced on master @ 40e1c4d4024b8ad35f2511b2e10bf80c5531dfde.

```
Ubuntu clang version 15.0.6
Target: x86_64-pc-linux-gnu

Ubuntu 22.10 (GNU/Linux 6.0.6-060006-generic x86_64)
```

@MarcoFalke I'll see if I can get that too work.

💬 MarcoFalke commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476150784)
This leak may be fixed in bdb5.3, but I haven't re-checked this.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476162524)
Thanks for quick feedback, all. I *think* the latest update should make everyone happy.

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476189108)
Narrowed it down to `BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)`.

💬 fanquake commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476197268)
> I was going to add more details if anyone couldn't reproduce it, but here we go

In future, save us all wasting our time, and provide the (trivial to do so) details initially.

💬 virtu commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1142083883)
In reply to the original comment: oversight on my part; will push a fix.

In reply to adding event timestamps for all tracepoints: in omitting the actual event timestamps as tracepoint parameters, I followed the precedent set by other tracepoints, where, as laid out by @0xB10C, event timestamps are generated inside tracing scripts.

💬 Sjors commented on issue "Memory leak in wallet_tests with BDB using address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-1476207042)
That test can be further narrowed down:

```
BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
{
WalletContext context;
context.args = &m_args;
context.chain = m_node.chain.get();
auto wallet = TestLoadWallet(context);
// TestUnloadWallet(std::move(wallet));
// wallet = TestLoadWallet(context);
TestUnloadWallet(std::move(wallet));
}
```

Uncommenting the extra unload & load will trigger the sanitizer.

💬 dergoegge commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142103379)
Wouldn't we also want to add the same logging to `ProcessHeadersMessage`? Thinking of BIP 130 block announcements.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142125339)
My thinking was that (i) legacy/low-bandwidth nodes are uninteresting for these logs in the sense that they are either in IBD or [...whatever prompts a node to be low-bandwidth-cmpctblock using]. And the log message in AcceptBlockHeader() obviously covers those cases too, and the lack of the cmpctblock message tells you that it's LB/legacy.

And it gets a little hairy because if you want to stick a log in ProcessHeadersMessage, you have to exclude the compactblock path because PHM() is called
...

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476248917)
Updated ff6c0abeea00deffcaeaf3b02092110d6895b1c0 -> e580afee614b93236cb0e37457a5a7c28f4a0b73 ([pr26749.11](https://github.com/hebasto/bitcoin/commits/pr26749.11) -> [pr26749.12](https://github.com/hebasto/bitcoin/commits/pr26749.12), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.11..pr26749.12)).

Addressed @martinus's performance nits:
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141442527
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1141443409


...

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1142133225)
> The commit that fixes the use-after-free should come with a commit message to say that it fixed the bug

Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476248917).

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1142133588)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476248917).

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1142133823)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1476248917).

💬 hebasto commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1476260377)
Concept ACK.

💬 jamesob commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142145002)
Hm yeah, the trade-off between DoSable log placement and accurate log placement is too complex for me in this case. I'm going to leave the PR as-is and then someone can do a follow-up if they want to do The Good Thinking about logging the non-HB case specifically.

💬 hebasto commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1142160746)
3665c1bdf7b9d47ccab39916c13a8cd9dfbf0eea

There is no `src/util/fs.cpp` file at this commit. It is renamed in the last commit only.

💬 hebasto commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1142161665)
Sort files after renaming?