💬 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.
💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707254)
I think this snippet of code could be documented in the following fashion
```
// If we receive many `SENDTXRCNCL` from the same peer, before to receive a
// single `VERACK`, re-register the peer id state with potentially different local
// and remote salts.
```
As far as I can see, at `SENDTXRCNCL` reception we do not check for duplicate `SENDTXRCNCL` message (we check if we have txreconciliation enabled, then that it’s happening before verack reception and then that transaction-relay
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707254)
I think this snippet of code could be documented in the following fashion
```
// If we receive many `SENDTXRCNCL` from the same peer, before to receive a
// single `VERACK`, re-register the peer id state with potentially different local
// and remote salts.
```
As far as I can see, at `SENDTXRCNCL` reception we do not check for duplicate `SENDTXRCNCL` message (we check if we have txreconciliation enabled, then that it’s happening before verack reception and then that transaction-relay
...
💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707917)
Logging that a given peer’s local reconciliation set max size has been reached can be useuful info for future debug.
```
diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp
index d4d6bdf1ea..b99bb41681 100644
--- a/src/node/txreconciliation.cpp
+++ b/src/node/txreconciliation.cpp
@@ -150,7 +150,11 @@ public:
if (!peer_state) return false;
// Transactions which don't make it to the set due to the limit are announced via fan-out.
- if (
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894707917)
Logging that a given peer’s local reconciliation set max size has been reached can be useuful info for future debug.
```
diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp
index d4d6bdf1ea..b99bb41681 100644
--- a/src/node/txreconciliation.cpp
+++ b/src/node/txreconciliation.cpp
@@ -150,7 +150,11 @@ public:
if (!peer_state) return false;
// Transactions which don't make it to the set due to the limit are announced via fan-out.
- if (
...
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894728411)
I simulated this to see if there was any difference between doing the decision-making before or after trickle.
I didn't do it for dependant transactions, just for a single one, trying to answer whether choosing fanout peers at scheduling time or at relay time made any difference. The answer is no, given the number of peers that know about the transaction at both times is almost the same.
The approach, simulation and results can be found here: https://srgi.notion.site/Select-fanout-candidates-
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1894728411)
I simulated this to see if there was any difference between doing the decision-making before or after trickle.
I didn't do it for dependant transactions, just for a single one, trying to answer whether choosing fanout peers at scheduling time or at relay time made any difference. The answer is no, given the number of peers that know about the transaction at both times is almost the same.
The approach, simulation and results can be found here: https://srgi.notion.site/Select-fanout-candidates-
...
💬 1440000bytes commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894742730)
```suggestion
if (coins_mem_usage >= WARN_FLUSH_COINS_SIZE) LogWarning("Flushing %d GiB UTXO set to disk, it may take several minutes.", coins_mem_usage >> 30);
```
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894742730)
```suggestion
if (coins_mem_usage >= WARN_FLUSH_COINS_SIZE) LogWarning("Flushing %d GiB UTXO set to disk, it may take several minutes.", coins_mem_usage >> 30);
```
💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2558426619)
The PR has been split into 3 to simplify review, please check those out first:
* refactor: cache block[undo] serialized size for consecutive calls - https://github.com/bitcoin/bitcoin/pull/31490
* optimization: bulk reads(27%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD - https://github.com/bitcoin/bitcoin/pull/31551
The 3 changes together achieve a 12.3% speedup in raw IBD (real nodes, multiple runs, 870k blocks, 1k dbcache, ssd)
<details>
<summary>Details</summary>
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2558426619)
The PR has been split into 3 to simplify review, please check those out first:
* refactor: cache block[undo] serialized size for consecutive calls - https://github.com/bitcoin/bitcoin/pull/31490
* optimization: bulk reads(27%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD - https://github.com/bitcoin/bitcoin/pull/31551
The 3 changes together achieve a 12.3% speedup in raw IBD (real nodes, multiple runs, 870k blocks, 1k dbcache, ssd)
<details>
<summary>Details</summary>
...
💬 l0rinc commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894920990)
The *large* part is important, we're not always displaying this, only when flushing is expected to take a long time
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894920990)
The *large* part is important, we're not always displaying this, only when flushing is expected to take a long time
💬 1440000bytes commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894921918)
It is grammatically incorrect to write "large N GiB".
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894921918)
It is grammatically incorrect to write "large N GiB".
💬 l0rinc commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894926926)
K, rephrased it to `Flushing large (1 GiB) UTXO set to disk, it may take several minutes`
(https://github.com/bitcoin/bitcoin/pull/31534#discussion_r1894926926)
K, rephrased it to `Flushing large (1 GiB) UTXO set to disk, it may take several minutes`