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

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

- addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1142170248)

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

💬 jonatack commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1476316638)
I proposed to @LarryRuane last week to check in with @jamesob about picking up https://bitcoinperf.com/ and checking with @0xB10C about potential cross-fertilization with tracepoints and their dashboards. Also https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474955390.

💬 dergoegge commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1142194524)
I think in terms of DoS risk it is pretty similar as the compact block location. Basically: "Only log for BIP 130 announcements that told us about new headers meeting our DoS threshold".

From the logs you shared here: https://twitter.com/jamesob/status/1637191706691903488 it looks like logging for headers announcements would have been useful. Looking at `PeerManagerImpl::NewPoWValidBlock`, we only announce via compact block once per height, so a competing height will be announced via headers.
...

💬 jonatack commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1476324797)
See also https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474761651 by @martinus for one nice way, with an example, to create and share detailed benchmark results.