Bitcoin Core Github
42 subscribers
126K links
Download Telegram
💬 pinheadmz commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816995291)
Do we have a 32-bit CI task already?
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1816997141)
> Do we have a 32-bit CI task already?

x86, not ARM.
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817042102)
This is a shot in the dark, but could this be due to memory fragmentation? The `PoolResource` allocates blocks of 262144 bytes, which seems quite large for such a system. Could you try this patch, or is there a way for me to try this? It reduces the allocated block size to 16K (and fixes a few tests that rely on the hardcoded size).

```patch
diff --git a/src/support/allocators/pool.h b/src/support/allocators/pool.h
index c8e70ebacff..9f93511995e 100644
--- a/src/support/allocators/pool.h

...
💬 ryanofsky commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#discussion_r1397847620)
I think it probably wasn't required to bump libmultiprocess at the same time as bumping capnproto, but good to do it anyway since to provide test coverage for newer code. Changes in the bump were:

* chaincodelabs/libmultiprocess#79
* chaincodelabs/libmultiprocess#83
* chaincodelabs/libmultiprocess#84
* chaincodelabs/libmultiprocess#85
* chaincodelabs/libmultiprocess#86

https://github.com/chaincodelabs/libmultiprocess/compare/1af83d15239ccfa7e47b8764029320953dd7fdf1...61d5a0e661f20a4928
...
📝 ryanofsky opened a pull request: "depends: bump libmultiprocess to fix capnproto deprecation warnings"
(https://github.com/bitcoin/bitcoin/pull/28907)
This incorporates PR chaincodelabs/libmultiprocess#88 and reverts the NO_WERROR CI workaround added in #28735

Upstream diff: https://github.com/chaincodelabs/libmultiprocess/compare/61d5a0e661f20a4928fbf868ec9c3c6f17883cc7...414542f81e0997354b45b8ade13ca144a3e35ff1
💬 TheCharlatan commented on pull request "refactor: Make CTxMemPoolEntry non-copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1817123452)
Thank you @ajtowns,

Updated b9e4b4cb275e998376b0e3e68b6c22c5b1877fbb -> b8b624b36ebeb167da2f9c6d12f563f6849ccd60 ([CTxMemPoolEntryNonCopy_0](https://github.com/TheCharlatan/bitcoin/tree/CTxMemPoolEntryNonCopy_0) -> [CTxMemPoolEntryNonCopy_1](https://github.com/TheCharlatan/bitcoin/tree/CTxMemPoolEntryNonCopy_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/CTxMemPoolEntryNonCopy_0..CTxMemPoolEntryNonCopy_1))
* Applied @ajtowns' [suggestion](https://github.com/bitcoin/bitcoin/pu
...
📝 brunoerg opened a pull request: "fuzz: compare scripts from `Expand` and `ExpandFromCache`"
(https://github.com/bitcoin/bitcoin/pull/28908)
When testing a descriptor, if it was able to `Expand`, `ExpandFromCache` should also work and scripts should be the same. This PR covers it in `TestDescriptor` for `descriptor_parse` fuzz.
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817172322)
@martinus

> This is a shot in the dark, but could this be due to memory fragmentation? The `PoolResource` allocates blocks of 262144 bytes, which seems quite large for such a system. Could you try this patch, or is there a way for me to try this? It reduces the allocated block size to 16K (and fixes a few tests that rely on the hardcoded size).

I've tested your [patch](https://github.com/hebasto/artefacts/blob/bisect-arm-OOM.1/bisect-arm-OOM/bitcoin-274a7b769afb-arm-linux-gnueabihf.tar.gz
...
💬 ryanofsky commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1817233427)
PR description lists a few reasons for bumping the capnproto version, but it doesn't actually mention the original motivation, which was to fix a C++20 compiler issue: https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1778595873, https://github.com/capnproto/capnproto/pull/1618. Just wanted to note this explicitly
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817427540)
It could be that the memory usage calculation has changed, and calculation for memory used in the CCoinsMap is now more correct than before. Previously it overestimated memory usage, and now it's more correct but for the same cache size this also leads to a bit higher memory usage
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817452601)
> It could be that the memory usage calculation has changed, and calculation for memory used in the CCoinsMap is now more correct than before. Previously it overestimated memory usage, and now it's more correct but for the same cache size this also leads to a bit higher memory usage

It depends on definition of "a bit higher" :) It was less than 0.9 GB of RAM. Now 2 GB of RAM plus 1 GB swap is not enough.
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817453279)
"a bit higher" should be about 2% or so...
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817525832)
I investigated a bit with [heaptrack](https://github.com/KDE/heaptrack) and it seems there are a bit higher spikes than I have thought. It's still not too bad, previously heaptrack says peack memory is ~600MB, and with the pool it's ~700 MB. That bigger peak seems to happen in `CCoinsViewCache::BatchWrite`.

Previously, the `mapCoins.erase(it)` freed a node, and a subsequent `cacheCoins[it->first]` could use that freed memory. With the pool that is currently not possible any more, because the
...
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817526549)
From the observing the RAM usage during runtime, I would say that RAM usage increases over time until OOM.
💬 hebasto commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817583116)
From [IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-11-17#984227;):
> 19:42 \<sipa\> and it's only with guix builds?

I also can confirm the regression for binaries that were natively compiled with GCC 11.4.0.
furszy closed a pull request: "wallet: group independent db writes on single batched db transaction"
(https://github.com/bitcoin/bitcoin/pull/25297)
💬 furszy commented on pull request "wallet: group independent db writes on single batched db transaction":
(https://github.com/bitcoin/bitcoin/pull/25297#issuecomment-1817587008)
Still relevant and quite useful but closing it until #28574 and its derivatives are merged.
👍 TheCharlatan approved a pull request: "Drop CAutoFile"
(https://github.com/bitcoin/bitcoin/pull/28904#pullrequestreview-1738457555)
ACK 4eb2a9ea4b6262bec0bc7c20cb3e684ea75caf42
💬 ishaanam commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1398253415)
In ebc39da8e5f72521fb4889bc1adc0a80136ce3da "wallet: Explicitly preserve transaction locktime in CreateTransaction":

The user-provided locktime and sequence values should also be set in the `sendall` `coin_control`. In the future if we want to take locktime into account when running `AvailbleCoins`, this would be very useful.
💬 martinus commented on issue "RAM usage regression in 26.x and master on ARM 32-bit":
(https://github.com/bitcoin/bitcoin/issues/28906#issuecomment-1817606023)
@hebasto could you try another patch? https://github.com/martinus/bitcoin/commit/04fb0b35cf18d5805d58595c27f027fae4428e62 This removes the placement new that didn't reuse the returned pointer, which technically has always been illegal