💬 ryanofsky commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2108712060)
> Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, `std::variant<CTransactionRef, BlockData>`, where block data holds the block hash and block number.

The drawback of a separate data field is that there is no longer a guarantee that data accompanying the reason is always present when the reason is set, and always absent when it is not set. So it is possible for code to only partially initialize the RemovalRea
...

💬 paplorinc commented on pull request "refactor: reserve memory allocation for transaction outputs":
(https://github.com/bitcoin/bitcoin/pull/30093#discussion_r1599029438)
Hey @theuni, excellent review, thanks!
1) yes, it's always empty, leaving it out is indeed more readable - and if people modify it in the future, they should just pay attention to this
2) nice, haven't noticed it, added the +1 (added you as co-author)
3) To be consistent with `txNew.vin`, I've changed it to `emplace_back` instead, the performance should be comparable - do you agree?
4) wanted to be focused only on the issue discovered during code review, but this is much better, thanks for t
...

✅ ryanofsky closed an issue: "Slow memory leak in v22.0?"
(https://github.com/bitcoin/bitcoin/issues/24542)

🚀 ryanofsky merged a pull request: "rpc: move UniValue in blockToJSON"
(https://github.com/bitcoin/bitcoin/pull/30094)

💬 achow101 commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2108732705)
> For frequent pruning flushes there's no need to empty the cache and kill connect block speed. However, simply using `Sync` in place of `Flush` actually slows down a pruned full IBD with a high `dbcache` value. This is because as the cache grows, sync takes longer since every coin in the cache is scanned to check if it's dirty. For frequent prune flushes and a large cache this constant scanning starts to really slow IBD down, and just emptying the cache on every prune becomes faster.
>
> To
...

🚀 achow101 merged a pull request: "validation: don't clear cache on periodic flush: >2x block connection speed"
(https://github.com/bitcoin/bitcoin/pull/28233)

💬 theuni commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108751842)

> > I would be great to check all UniValue::push_back and UniValue::pushKV invocations in the codebase.
>
> I can check the remainder of the codebase for other occurrences too.

I was already working on this and... wow. You've opened a big can of worms here @willcl-ark :). But that's a good thing because it should reduce memusage and increase performance I'd assume.

See here for my first pass. It's a beast. https://github.com/theuni/bitcoin/commits/less-univalue-copies/

The first co
...

💬 Rob1Ham commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2108752829)
tACK https://github.com/bitcoin/bitcoin/commit/06c2c713c52b60231efc3e00d2c5eb0bf9e345f9

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2108754056)
@achow101 That works, but it misses the point. If you delete the dirty entries on a prune flush, then those entries need to be re-read from disk when they are (likely soon) spent.

💬 achow101 commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2108756050)
> @achow101 That works, but it misses the point. If you delete the dirty entries on a prune flush, then those entries need to be re-read from disk when they are (likely soon) spent.

Sorry, I meant clear the Dirty and spent ones.

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1599061492)
Commit 48dc2ff8e7819e810d2436219268f220b39c14f

in fc0a50e92aaaf3b049fcfcd0873dadc7150cfecf I made this one `info` (default) level [here](https://github.com/bitcoin/bitcoin/pull/25203/commits/fc0a50e92aaaf3b049fcfcd0873dadc7150cfecf#diff-8945a03581b372d674f34dcb6a63e537343e376ebcd9d8c7c3d94d4366cd9b9dR379). It seems important, when the user turns `i2p` logging on, to see if the session was successfully created. I look for this logging when launching a node or restarting the I2P router.

💬 theuni commented on pull request "refactor: reserve memory allocation for transaction outputs":
(https://github.com/bitcoin/bitcoin/pull/30093#discussion_r1599061542)
This will just be forwarded to the copy ctor. `push_back(std::move(txout))` would be more correct (and clear).

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1599062889)
same here as https://github.com/bitcoin/bitcoin/pull/29833/files#r1599061492

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1599063683)
same comment here as https://github.com/bitcoin/bitcoin/pull/29833/files#r1599061492

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1599069512)
(Missing space here?)

```suggestion
LogPrintLevel(BCLog::I2P, BCLog::Level::Error, "Couldn't accept %s: %s\n", disconnect ? " (will close the session)" : "", errmsg);
```

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2108786764)
If the third commit is re-changing lines that are already changed in the first commit, would suggest combining them.

💬 theStack commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1599046868)
nit: seems like the second parameter (`std::error_code{}`) isn't needed (here and in all other instances below)
```suggestion
throw std::ios_base::failure("Invalid UTXO set snapshot magic bytes. Please check if this is indeed a snapshot file or if you are using an outdated snapshot format.");
```

💬 theStack commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1599077443)
nit: could use our fancy `Join` helper here (available in util/string.h)
```suggestion
std::string heights_formatted = Join(available_heights, ", ", [&](const auto& i) { return ToString(i); });
```

🤔 jonatack reviewed a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2053826139)
(reposting using the "review changes" button to perhaps appease the bot :)

ACK modulo comments above. If the third commit is re-changing lines that are already changed in the first commit, would suggest combining them.

👍 TheCharlatan approved a pull request: "depends: set AR & RANLIB for CMake"
(https://github.com/bitcoin/bitcoin/pull/30078#pullrequestreview-2053850771)
ACK 019ad7327c397094d7648b55503bf5373b108a57

Guix build (aarch64):
```
bdbb759d06e9766c5aa29a9ee1cea6f021d50618e1abe4732ae0120c4b444829 guix-build-019ad7327c39/output/aarch64-linux-gnu/SHA256SUMS.part
82aa7b4af365dde09b7fe435bd20432aa59168cb448f7a8cb898a8acb6178453 guix-build-019ad7327c39/output/aarch64-linux-gnu/bitcoin-019ad7327c39-aarch64-linux-gnu-debug.tar.gz
93c539e8c42ff767e46fc3f501cf9d543b954202372c930c177ff22eea6037f5 guix-build-019ad7327c39/output/aarch64-linux-gnu/bitcoin-0
...