💬 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?

⚠️ dergoegge opened an issue: "Continuous benchmark tracking"
(https://github.com/bitcoin/bitcoin/issues/27284)
It would be beneficial to have continuous tracking of our benchmark tests, because regressions (or unexpected improvements) otherwise go undetected (at least for a while). Afaict currently, the only benefit of our benchmarking tests is to evaluate changes as they are being proposed but imo that only gives us ~50% of the benefit that benchmarks can provide.

I am imagining this to be a separate service (maybe integrated with @DrahtBot) that regularly runs the benchmarks in an environment config
...

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1142170248)
nit in b8e35a2: You can use `std::min<size_t>(total, InsecureRandRange(.))` instead. Not that it matters, but this allows to check for unintentional unsigned integer wrap around, I think.

💬 MarcoFalke commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1476301070)
I think @jamesob set something up at one point, but it had to be queried manually, as there were no notifications. Also, I am not sure if it is running at all. See https://codespeed.bitcoinperf.com/timeline/

💬 fanquake commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1476309129)
> [However, given that the functionality added is currently to just build a histogram based on the individual feerates, I'm a little less clear on its utility and why it is necessary to add and maintain this in Bitcoin Core given how easy it would be to do it externally](https://github.com/bitcoin/bitcoin/pull/21422#pullrequestreview-1336717945)

I agree. I don't think this is something that needs to exist in Core. The fact that you've had this PR open for a long time is unfortunate, but also
...