💬 martinus commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143303982)
The standard doesn't seem to say anything else, so the real allocation capacity seems to be implementation defined: https://eel.is/c++draft/vector#capacity-4

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477759517)
> Like https://github.com/bitcoin/bitcoin/commit/06f5f3931910dcd2056167b1d440d327f3a510b0?

I was referring to the `bugprone-use-after-move` workaround.

💬 josibake commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477762451)
> > > Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.
> >
> >
> > ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).
> > Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend
...

💬 furszy commented on pull request "RPC: Fix fund transaction crash when at 0-value, 0-fee":
(https://github.com/bitcoin/bitcoin/pull/27271#issuecomment-1477783857)
k sure. I didn't suggest that because the error message will make people select the input/s manually etc. Which, I'm not so sure that it's better than a crash (at least the crash prevent them from making mistakes).

But.. I'm not that strong over that point. So, all good, either way works for me. The coin selection fixes are already up anyway, matter of continue moving forward until all the problems are corrected, so this can be enabled.

💬 jamesob commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1477800508)
Cool! Seeing a **~8% speedup** over a modern region of the chain with **lower memory usage**.

---


| bench name | command
...

💬 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-1477801767)
Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .

See also https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1409699173

📝 MarcoFalke opened a pull request: "Refactor: Remove unused FlatFilePos::SetNull"
(https://github.com/bitcoin/bitcoin/pull/27289)
This is unused outside of tests and the default constructor. With C++11, it can be replaced by C++11 member initializers in the default constructor.

Beside removing unused code, this also makes it less fragile in light of uninitialized memory. (See also https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1477801767)

If new code needs to set this to null, it can use `std::optional`, or in the worst case re-introduce this method.

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477809450)
Updated a87002550c1461a4cfbe666d5034597276e32639 -> 95ad70ab652ddde7de65f633c36c1378b26a313a ([pr26749.15](https://github.com/hebasto/bitcoin/commits/pr26749.15) -> [pr26749.16](https://github.com/hebasto/bitcoin/commits/pr26749.16), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.15..pr26749.16)):

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

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

💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1477825604)
> Yikes. Work on adding [`cppcoreguidelines-pro-type-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html)` to clang-tidy maybe?

Well, it fires too many false warnings for our code base (for example, for `CTxOut::CTxOut()` constructor).

💬 ajtowns commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1477834856)
> Completely unsure on increasing the scope of what we disconnect for. Are we seeing these messages in the wild?

I've added logging for `TX_NOT_STANDARD` in `MaybePunishNodeForTx`. After 20 hours or so running that on a listening node, I've had a couple of instances of rejections for `dust` (via `IsStandardTx` for outputs matching `IsDust`), but so far that's it.

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