👍 tdb3 approved a pull request: "coins: warn on shutdown for big UTXO set flushes"
(https://github.com/bitcoin/bitcoin/pull/31534#pullrequestreview-2518687009)
Tested ACK ec8fa17872c8cb7f214a451aaa23701ced8bd5fb
Great addition! This will prevent many users from believing bitcoind is "stuck/frozen".
Performed IBD to block ~100k (dbcache=6000, cache used < 1000MiB). Stopped bitcoind and (as expected) did not see the warning.
Restarted IBD and stopped at block ~300k (cache used > 1000MiB). Warning displayed as expected.
```
^C
2024-12-21T16:28:00Z Shutdown: In progress...
2024-12-21T16:28:00Z opencon thread exit
2024-12-21T16:28:00Z addcon
...
(https://github.com/bitcoin/bitcoin/pull/31534#pullrequestreview-2518687009)
Tested ACK ec8fa17872c8cb7f214a451aaa23701ced8bd5fb
Great addition! This will prevent many users from believing bitcoind is "stuck/frozen".
Performed IBD to block ~100k (dbcache=6000, cache used < 1000MiB). Stopped bitcoind and (as expected) did not see the warning.
Restarted IBD and stopped at block ~300k (cache used > 1000MiB). Warning displayed as expected.
```
^C
2024-12-21T16:28:00Z Shutdown: In progress...
2024-12-21T16:28:00Z opencon thread exit
2024-12-21T16:28:00Z addcon
...
📝 hebasto opened a pull request: "depends: Update capnproto to 1.1.0"
(https://github.com/bitcoin/bitcoin/pull/31552)
This change fixes compilation on NetBSD with GCC 14:
```
$ gmake -C depends capnp MULTIPROCESS=1 CC=/usr/pkg/gcc14/bin/gcc CXX=/usr/pkg/gcc14/bin/g++
gmake: Entering directory '/home/hebasto/dev/bitcoin/depends'
<snip>
In file included from /home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/capnp/1.0.2-ffdefffd9f5/src/kj/memory.h:24,
from /home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/capnp/1.0.2-ffdefffd9f5/src/kj/exception.h:
...
(https://github.com/bitcoin/bitcoin/pull/31552)
This change fixes compilation on NetBSD with GCC 14:
```
$ gmake -C depends capnp MULTIPROCESS=1 CC=/usr/pkg/gcc14/bin/gcc CXX=/usr/pkg/gcc14/bin/g++
gmake: Entering directory '/home/hebasto/dev/bitcoin/depends'
<snip>
In file included from /home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/capnp/1.0.2-ffdefffd9f5/src/kj/memory.h:24,
from /home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/capnp/1.0.2-ffdefffd9f5/src/kj/exception.h:
...
🤔 Prabhat1308 reviewed a pull request: "qa: Limit `-maxconnections` in tests"
(https://github.com/bitcoin/bitcoin/pull/31537#pullrequestreview-2518778032)
tack [d9d5bc2](https://github.com/bitcoin/bitcoin/pull/31537/commits/d9d5bc2e7466033d989432f53a112325fa3d6d4a)
OS - MacOS Sequoia 15.1.1
(https://github.com/bitcoin/bitcoin/pull/31537#pullrequestreview-2518778032)
tack [d9d5bc2](https://github.com/bitcoin/bitcoin/pull/31537/commits/d9d5bc2e7466033d989432f53a112325fa3d6d4a)
OS - MacOS Sequoia 15.1.1
💬 0xB10C commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430)
Inspired by discussions with @willcl-ark at and after the last CoreDev, I started working on a [NixOS](nixos.org) based configuration for a large host with multiple Cirrus CI runners with a focus on isolation and performance.
To isolate the runners, each runner lives in an ephemeral QEMU VM[^0] that's shut down and recreated after each run. While the runners are ephemeral and the QEMU VMs have a tmpfs as rootfs, the host maintains a shared cache. Since the cache is somewhat implicit on the c
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430)
Inspired by discussions with @willcl-ark at and after the last CoreDev, I started working on a [NixOS](nixos.org) based configuration for a large host with multiple Cirrus CI runners with a focus on isolation and performance.
To isolate the runners, each runner lives in an ephemeral QEMU VM[^0] that's shut down and recreated after each run. While the runners are ephemeral and the QEMU VMs have a tmpfs as rootfs, the host maintains a shared cache. Since the cache is somewhat implicit on the c
...
🤔 hodlinator reviewed a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2518616838)
Code review 7517126d9fd2c3e50dc95ce831bac6895b950e24
Some concern over implicit narrowing on 32-bit platforms (see inline comments).
*Rough* suggestions: https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes...hodlinator:bitcoin:pr/31483_suggestions
\+ Use of curly braces to catch narrowing.
\+ The above encourages asserts, leading to finding cases where truncation would eventually happen (increasing `DEFAULT_KERNEL_CACHE` by 10x triggers assert).
\- Not so happy about my s
...
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2518616838)
Code review 7517126d9fd2c3e50dc95ce831bac6895b950e24
Some concern over implicit narrowing on 32-bit platforms (see inline comments).
*Rough* suggestions: https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes...hodlinator:bitcoin:pr/31483_suggestions
\+ Use of curly braces to catch narrowing.
\+ The above encourages asserts, leading to finding cases where truncation would eventually happen (increasing `DEFAULT_KERNEL_CACHE` by 10x triggers assert).
\- Not so happy about my s
...
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894593777)
nit: The message of commit 69159326810822c804ab53a009ef57fb5f28fac9 belonging to this PR states:
> It always applied in the same way, no matter how the txindex is setup.
There is some nuance here, the comment was correct in 32cab91278651d07a11132b7636dc3d21144e616 (laanwj 2016 #8273), but was no longer valid after 8181db88f6e0ed96654951e18b1558cd8f78765b (jimpo 2017 #13033). Could add nuance to the commit message.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894593777)
nit: The message of commit 69159326810822c804ab53a009ef57fb5f28fac9 belonging to this PR states:
> It always applied in the same way, no matter how the txindex is setup.
There is some nuance here, the comment was correct in 32cab91278651d07a11132b7636dc3d21144e616 (laanwj 2016 #8273), but was no longer valid after 8181db88f6e0ed96654951e18b1558cd8f78765b (jimpo 2017 #13033). Could add nuance to the commit message.
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894643457)
No longer documents unit (MiB) due to commit 69159326810822c804ab53a009ef57fb5f28fac9.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894643457)
No longer documents unit (MiB) due to commit 69159326810822c804ab53a009ef57fb5f28fac9.
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894597050)
nit: Arguably clearer:
```suggestion
IndexCacheSizes index_sizes;
```
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894597050)
nit: Arguably clearer:
```suggestion
IndexCacheSizes index_sizes;
```
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894643162)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1890151750
My intuition was "why not keep using 64-bit types in case the platform is 32-bit", but 32-bit implies it will not be able to make use of larger than `size_t`, so `size_t` is preferable.
---
Added:
```
static_assert(sizeof(size_t) == 4);
```
and built on a 32-bit container (i686-centos).
---
What is worrying is that I also tested adding an extra zero here:
```C++
//! Suggested default amount of cache res
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894643162)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1890151750
My intuition was "why not keep using 64-bit types in case the platform is 32-bit", but 32-bit implies it will not be able to make use of larger than `size_t`, so `size_t` is preferable.
---
Added:
```
static_assert(sizeof(size_t) == 4);
```
and built on a 32-bit container (i686-centos).
---
What is worrying is that I also tested adding an extra zero here:
```C++
//! Suggested default amount of cache res
...
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894591427)
nit: Me and the compilers would prefer a contrived name over `std::tuple<...>`. It still allows for the structured bindings in *init.cpp*.
```suggestion
struct IndexAndKernelCacheSizes {
IndexCacheSizes index;
kernel::CacheSizes kernel;
};
IndexAndKernelCacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
```
<details>
<summary>Full diff</summary>
```diff
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index bd0c93d76f..c7197b6ef1 1006
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894591427)
nit: Me and the compilers would prefer a contrived name over `std::tuple<...>`. It still allows for the structured bindings in *init.cpp*.
```suggestion
struct IndexAndKernelCacheSizes {
IndexCacheSizes index;
kernel::CacheSizes kernel;
};
IndexAndKernelCacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
```
<details>
<summary>Full diff</summary>
```diff
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index bd0c93d76f..c7197b6ef1 1006
...
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894643334)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1890138649
Or make `total_cache` `size_t`, removing the need for an assert, and also helps signal the unit being bytes? (May require adding casts though).
---
We also have some implicit narrowing going on here, especially on 32-bit platforms, from `int64_t` to `size_t`. Adding curlies trigger an error.
```suggestion
block_tree_db = {std::min(total_cache / 8, MAX_BLOCK_DB_CACHE << 20)};
```
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894643334)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1890138649
Or make `total_cache` `size_t`, removing the need for an assert, and also helps signal the unit being bytes? (May require adding casts though).
---
We also have some implicit narrowing going on here, especially on 32-bit platforms, from `int64_t` to `size_t`. Adding curlies trigger an error.
```suggestion
block_tree_db = {std::min(total_cache / 8, MAX_BLOCK_DB_CACHE << 20)};
```
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894596535)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1890537379
Agree with the simplification mostly. Only way the old calculation was useful was if someone was to tweak `nMaxCoinsDBCache` to be lower than `(1 << 23)`/8MiB, which is very unlikely.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1894596535)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1890537379
Agree with the simplification mostly. Only way the old calculation was useful was if someone was to tweak `nMaxCoinsDBCache` to be lower than `(1 << 23)`/8MiB, which is very unlikely.
💬 A-Manning commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1894697011)
`signet_challenge` is consistent with `getblocktemplate`
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L660
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1894697011)
`signet_challenge` is consistent with `getblocktemplate`
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L660
💬 A-Manning commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1894697034)
`signet_challenge` is consistent with `getblocktemplate`
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L660
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1894697034)
`signet_challenge` is consistent with `getblocktemplate`
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L660
💬 luke-jr commented on pull request "rpc, cli: add getbalances#total, and use it for -getinfo":
(https://github.com/bitcoin/bitcoin/pull/31353#issuecomment-2558263107)
Seems like it would be better to just add them in `-getinfo`?
(https://github.com/bitcoin/bitcoin/pull/31353#issuecomment-2558263107)
Seems like it would be better to just add them in `-getinfo`?
💬 kallewoof commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2558266241)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2558266241)
Concept ACK
💬 luke-jr commented on issue "rfc: DATUM mining interface requirements":
(https://github.com/bitcoin/bitcoin/issues/31002#issuecomment-2558270819)
> No need to tell users to manually set -blockmaxsize and -blockmaxweight, because createNewBlock() can pass a custom coinbase weight reservation through its BlockCreateOptions argument.
Seems like it would be trivial to add this to GBT. The current approach in Knots isn't ideal (since it would override blockmaxsize/weight if the miner intentionally sets them lower).
> No need to use -blocknotify (which launches datum_gateway and presumable then calls getblocktemplate)
It only sends a s
...
(https://github.com/bitcoin/bitcoin/issues/31002#issuecomment-2558270819)
> No need to tell users to manually set -blockmaxsize and -blockmaxweight, because createNewBlock() can pass a custom coinbase weight reservation through its BlockCreateOptions argument.
Seems like it would be trivial to add this to GBT. The current approach in Knots isn't ideal (since it would override blockmaxsize/weight if the miner intentionally sets them lower).
> No need to use -blocknotify (which launches datum_gateway and presumable then calls getblocktemplate)
It only sends a s
...
🤔 ariard reviewed a pull request: "p2p: Fill reconciliation sets (Erlay) attempt 2"
(https://github.com/bitcoin/bitcoin/pull/30116#pullrequestreview-2519443536)
Reviewed up to commit 478137aa (“p2p: Add PeerManager method to count…”).
Just minor comments so far and I still have to answer back comments on #28765.
(https://github.com/bitcoin/bitcoin/pull/30116#pullrequestreview-2519443536)
Reviewed up to commit 478137aa (“p2p: Add PeerManager method to count…”).
Just minor comments so far and I still have to answer back comments on #28765.
💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894709738)
nit: s/Get the amount on/Get the amount of/g.
And can say inbound is given first.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894709738)
nit: s/Get the amount on/Get the amount of/g.
And can say inbound is given first.
💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894709326)
I think the variable naming could be enhanced, e.g `m_collision`, to dissociate from mempool conflicts. Here, it’s a collision from SipHash outputs that can arise from 2 distinct transactions, not even spending the same set of UTXOs.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894709326)
I think the variable naming could be enhanced, e.g `m_collision`, to dissociate from mempool conflicts. Here, it’s a collision from SipHash outputs that can arise from 2 distinct transactions, not even spending the same set of UTXOs.