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

💬 jonatack commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1476333698)
> code review & tested ACK, here are my benchmark results:

@martinus could you add the hash of the last commit to your ACK (like https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1458622474) so that it is counted by DrahtBot above in https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1400700700? (thanks!)

👍 alexsio27444 approved a pull request: "[24.x] Backports for 24.0.1"
(https://github.com/bitcoin/bitcoin/pull/26616)

💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1476359052)
Thank you for the review @hebasto,

Updated cc662d09386e50eb92a7ccbaa1edff62fd8cdd44 -> 3ceb8dde48fc12f4f16372f661a4cccf7104e194 ([splitSystemFs_5](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_5) -> [splitSystemFs_6](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_5..splitSystemFs_6)) to fix sorting in the Makefile and remove `fs.cpp`, which was still supposed to refer to `fs_helpers.cpp`
...

💬 kiminuo commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1476361661)
Closing the PR.

> If the goal of this change can be achieved externally, with 20 lines of Python, then that's where it should be done.

I would say it's rather a proof that one can _inefficiently_ achieve the goal.

An efficient solution would be to mirror mempool using tracepoints (that did not exist when the PR was created), if[^1] it is possible.

[^1]: I have not found the time to test it yet but **if** it is not possible then that would be imho a very nice PR to allow people to e
...

✅ kiminuo closed a pull request: "Add feerate histogram to getmempoolinfo"
(https://github.com/bitcoin/bitcoin/pull/21422)