🤔 stickies-v reviewed a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2546242450)
Did a full re-review on 0e2887262e1a3a4a5ab0382f9e8713b135408a77. Thank you for incorporating my `util/overflow.h` and `util/byte_units.h` suggestions, I really like the current approach.
Nits can be ignored, but I think my 2 `util/overflow.h` UB comments would be best to fix before merge. I probably won't be having any further comments after this.
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2546242450)
Did a full re-review on 0e2887262e1a3a4a5ab0382f9e8713b135408a77. Thank you for incorporating my `util/overflow.h` and `util/byte_units.h` suggestions, I really like the current approach.
Nits can be ignored, but I think my 2 `util/overflow.h` UB comments would be best to fix before merge. I probably won't be having any further comments after this.
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912972952)
in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:
nit: would prefer replacing `i` with `input` (I'm sorry for not doing this in my initial suggestion)
<details>
<summary>git diff on e0a541702d</summary>
```diff
diff --git a/src/util/overflow.h b/src/util/overflow.h
index a0698f5c69..c35e44690a 100644
--- a/src/util/overflow.h
+++ b/src/util/overflow.h
@@ -50,38 +50,38 @@ template <class T>
/**
* @brief Left bit shift with overflow checking.
- * @param i The input value to be lef
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912972952)
in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:
nit: would prefer replacing `i` with `input` (I'm sorry for not doing this in my initial suggestion)
<details>
<summary>git diff on e0a541702d</summary>
```diff
diff --git a/src/util/overflow.h b/src/util/overflow.h
index a0698f5c69..c35e44690a 100644
--- a/src/util/overflow.h
+++ b/src/util/overflow.h
@@ -50,38 +50,38 @@ template <class T>
/**
* @brief Left bit shift with overflow checking.
- * @param i The input value to be lef
...
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913307791)
in 0e2887262e
nit: I would default-initialize `IndexCacheSizes` members to 0, avoiding potential issues of accessing uninitialized members (and removing the need for this line):
<details>
<summary>git diff on 0e2887262e</summary>
```diff
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index 0e0305cd19..f4fbb7ead9 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -33,7 +33,6 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
IndexC
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913307791)
in 0e2887262e
nit: I would default-initialize `IndexCacheSizes` members to 0, avoiding potential issues of accessing uninitialized members (and removing the need for this line):
<details>
<summary>git diff on 0e2887262e</summary>
```diff
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index 0e0305cd19..f4fbb7ead9 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -33,7 +33,6 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
IndexC
...
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913070129)
in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:
Left shifting a 0-input should probably always return a 0-output, so suggested update (+ tests, one depending on my other comment re right-shift UB):
<details>
<summary>git diff on e0a541702d</summary>
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 09ba278d3a..07b98be446 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1897,6 +1897,7 @@ void TestCheckedLeftShift()
// Overflow
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913070129)
in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:
Left shifting a 0-input should probably always return a 0-output, so suggested update (+ tests, one depending on my other comment re right-shift UB):
<details>
<summary>git diff on e0a541702d</summary>
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 09ba278d3a..07b98be446 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1897,6 +1897,7 @@ void TestCheckedLeftShift()
// Overflow
...
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913090648)
in ccf8caa4430f57b8f148e96ac2a241221977bcab
note/nit: this can overflow on 32 bit machines. It is fixed in later commits, and I think fixing it in this commit would be overly verbose, but I'm mentioning it in case there are further code changes that could make this problematic again.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913090648)
in ccf8caa4430f57b8f148e96ac2a241221977bcab
note/nit: this can overflow on 32 bit machines. It is fixed in later commits, and I think fixing it in this commit would be overly verbose, but I'm mentioning it in case there are further code changes that could make this problematic again.
📝 fanquake opened a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/31648)
Backports:
* https://github.com/bitcoin/bitcoin/pull/31623
* https://github.com/bitcoin/bitcoin/pull/31627
(https://github.com/bitcoin/bitcoin/pull/31648)
Backports:
* https://github.com/bitcoin/bitcoin/pull/31623
* https://github.com/bitcoin/bitcoin/pull/31627
📝 marcofleon opened a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649)
The headers presync logic (only downloading headers that lead to a chain with sufficient work) should be enough to prevent memory DoS using low-work headers. Therefore, we no longer have any use for checkpoints.
All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.
Some previous discussion can be found in https://github.com/bitcoin/bitcoin/pull/25725. The conclusion at the time was that more testing of the presync logic was needed. Now
...
(https://github.com/bitcoin/bitcoin/pull/31649)
The headers presync logic (only downloading headers that lead to a chain with sufficient work) should be enough to prevent memory DoS using low-work headers. Therefore, we no longer have any use for checkpoints.
All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.
Some previous discussion can be found in https://github.com/bitcoin/bitcoin/pull/25725. The conclusion at the time was that more testing of the presync logic was needed. Now
...
💬 pablomartin4btc commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2587470767)
btw ACK on dependencies removal.
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2587470767)
btw ACK on dependencies removal.
💬 sipa commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587500105)
FWIW, the reason for the existence of the batch size behavior (as opposed to just writing everything at once) is that it causes a memory usage spike at flush time. If that spike exceeds the memory the process can allocate it causes a crash, at a particularly bad time (may require a replay to fix, which may be slower than just reprocessing the blocks).
Given that changing this appears to improve performance it's worth considering of course, but it is essentially a trade-off between speed and m
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587500105)
FWIW, the reason for the existence of the batch size behavior (as opposed to just writing everything at once) is that it causes a memory usage spike at flush time. If that spike exceeds the memory the process can allocate it causes a crash, at a particularly bad time (may require a replay to fix, which may be slower than just reprocessing the blocks).
Given that changing this appears to improve performance it's worth considering of course, but it is essentially a trade-off between speed and m
...
💬 shivaenigma commented on issue "Faster way to get block with prevouts in JSON-RPC":
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2587568570)
I am also interested in this. Even this _getrawblock_ response take 2-5 seconds depending on block size, which is very high. Few low hanging fruits that can be implemented:
- Support binding to unix domain sockets
- Support Gzip compression
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2587568570)
I am also interested in this. Even this _getrawblock_ response take 2-5 seconds depending on block size, which is very high. Few low hanging fruits that can be implemented:
- Support binding to unix domain sockets
- Support Gzip compression
💬 pinheadmz commented on issue "Faster way to get block with prevouts in JSON-RPC":
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2587573080)
> Support binding to unix domain sockets
This is a common request, but requires replacing libevent, which I'm working on: https://github.com/bitcoin/bitcoin/issues/31194
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2587573080)
> Support binding to unix domain sockets
This is a common request, but requires replacing libevent, which I'm working on: https://github.com/bitcoin/bitcoin/issues/31194
💬 sipa commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587635968)
If we go through with this, perhaps it's worth keeping the `-checkpoints` option around, just to trigger a startup warning if provided (as opposed to an error if someone still has it in the config).
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587635968)
If we go through with this, perhaps it's worth keeping the `-checkpoints` option around, just to trigger a startup warning if provided (as opposed to an error if someone still has it in the config).
💬 davidgumberg commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#issuecomment-2587639604)
lgtm ACK https://github.com/bitcoin/bitcoin/commit/8888ee4403fa62972c49e18752695d15fd32c0b0
(https://github.com/bitcoin/bitcoin/pull/31428#issuecomment-2587639604)
lgtm ACK https://github.com/bitcoin/bitcoin/commit/8888ee4403fa62972c49e18752695d15fd32c0b0
💬 achow101 commented on pull request "doc: Archive 28.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/31630#issuecomment-2587656651)
> ACK, but probably want to fix [bitcoin-core/bitcoincore.org#1100 (comment)](https://github.com/bitcoin-core/bitcoincore.org/pull/1100#discussion_r1910510272) first.
Done
(https://github.com/bitcoin/bitcoin/pull/31630#issuecomment-2587656651)
> ACK, but probably want to fix [bitcoin-core/bitcoincore.org#1100 (comment)](https://github.com/bitcoin-core/bitcoincore.org/pull/1100#discussion_r1910510272) first.
Done
💬 petamas commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2587664804)
@hebasto: Did you get a reply in the Developer Community issue? I've run into the same ICE at my workplace, but I don't have access to the linked issue. ("We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist")
Thanks!
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2587664804)
@hebasto: Did you get a reply in the Developer Community issue? I've run into the same ICE at my workplace, but I don't have access to the linked issue. ("We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist")
Thanks!
💬 dergoegge commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587672306)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587672306)
Concept ACK
💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913521985)
In commit "util: Add integer left shift helpers" (e0a541702def3a4c6cce8e44b6ebe8d608edad4e)
It seems wrong that calling `SaturatingLeftShift` on a negative input can wrap around to 0. The point of saturating operations should be that they don't wrap around, but effectively perform operations with full precision and just clamp the result values. In c++ [left shift](https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators) `i << j` is defined as <em>i * 2<sup>j</su
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913521985)
In commit "util: Add integer left shift helpers" (e0a541702def3a4c6cce8e44b6ebe8d608edad4e)
It seems wrong that calling `SaturatingLeftShift` on a negative input can wrap around to 0. The point of saturating operations should be that they don't wrap around, but effectively perform operations with full precision and just clamp the result values. In c++ [left shift](https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators) `i << j` is defined as <em>i * 2<sup>j</su
...
💬 fanquake commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2587704437)
> The root cause of the issue is that the -Wl,-z,separate-code linker option is incompatible with NetBSD's dynamic linker.
That's pretty surprising, because `-z,separate-code` has been enabled by default, in the NetBSD linker, for the upcoming 11.0 release. https://www.netbsd.org/changes/changes-11.0.html#x86:
> Enable the -z separate-code security feature by default in [ld(1)](https://man.netbsd.org/ld.1).
It seems kinda unlikely they'd do that if the option was incompatible with the
...
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2587704437)
> The root cause of the issue is that the -Wl,-z,separate-code linker option is incompatible with NetBSD's dynamic linker.
That's pretty surprising, because `-z,separate-code` has been enabled by default, in the NetBSD linker, for the upcoming 11.0 release. https://www.netbsd.org/changes/changes-11.0.html#x86:
> Enable the -z separate-code security feature by default in [ld(1)](https://man.netbsd.org/ld.1).
It seems kinda unlikely they'd do that if the option was incompatible with the
...
🤔 Sjors reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2547082375)
Some questions in the form of feedback...
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2547082375)
Some questions in the form of feedback...
💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913418467)
88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b nit: `wallet =` would be more consistent
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913418467)
88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b nit: `wallet =` would be more consistent