💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2587121964)
re-ACK b448b014947093cd217dbde47c8fb9e6c2bc8ba3
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2587121964)
re-ACK b448b014947093cd217dbde47c8fb9e6c2bc8ba3
💬 laanwj commented on pull request "doc: Archive 28.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/31630#issuecomment-2587164948)
ACK, but probably want to fix https://github.com/bitcoin-core/bitcoincore.org/pull/1100#discussion_r1910510272 first.
(https://github.com/bitcoin/bitcoin/pull/31630#issuecomment-2587164948)
ACK, but probably want to fix https://github.com/bitcoin-core/bitcoincore.org/pull/1100#discussion_r1910510272 first.
🤔 stratospher reviewed a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2546740188)
ACK b2136da9
- Went through the logs in `rpc_invalidateblock.py` and checked how the children were marked invalid/`m_best_header` was set on master vs in this PR
- Went through logs of some functional tests like `feature_block.py` and verified that BLOCK_FAILED_CHILD wasn't being set in [`m_failed_blocks` loop](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/validation.cpp#L4395)
- Ran `invalidateblock` RPC on a full node.
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2546740188)
ACK b2136da9
- Went through the logs in `rpc_invalidateblock.py` and checked how the children were marked invalid/`m_best_header` was set on master vs in this PR
- Went through logs of some functional tests like `feature_block.py` and verified that BLOCK_FAILED_CHILD wasn't being set in [`m_failed_blocks` loop](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/validation.cpp#L4395)
- Ran `invalidateblock` RPC on a full node.
💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r1913267051)
e6ccd17275e06e6278473587257f80c2d9bd8834: maybe move the legacy wallet migration code to a separate file? And mark it read-only :-)
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r1913267051)
e6ccd17275e06e6278473587257f80c2d9bd8834: maybe move the legacy wallet migration code to a separate file? And mark it read-only :-)
👍 rkrux approved a pull request: "doc: Archive 28.1 release notes"
(https://github.com/bitcoin/bitcoin/pull/31630#pullrequestreview-2546789777)
crACK 42cbf1fcb057374eec1e5b6f89724e1f192142bd
(https://github.com/bitcoin/bitcoin/pull/31630#pullrequestreview-2546789777)
crACK 42cbf1fcb057374eec1e5b6f89724e1f192142bd
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2587261731)
`b8b042626e...f71f1a346c`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2587261731)
`b8b042626e...f71f1a346c`: rebase due to conflicts
💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-2587302557)
> and probably makes more work for merging BIP8.
Is there a PR open for such logic?
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-2587302557)
> and probably makes more work for merging BIP8.
Is there a PR open for such logic?
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913317790)
in 0e2887262e1a3a4a5ab0382f9e8713b135408a77
nit: this should now also be a `size_t`
```suggestion
size_t max_cache{std::min(total_cache / 8, MAX_FILTER_INDEX_CACHE)};
```
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913317790)
in 0e2887262e1a3a4a5ab0382f9e8713b135408a77
nit: this should now also be a `size_t`
```suggestion
size_t max_cache{std::min(total_cache / 8, MAX_FILTER_INDEX_CACHE)};
```
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913290613)
in 0e2887262e1a3a4a5ab0382f9e8713b135408a77
nit: optional header no longer necessary
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913290613)
in 0e2887262e1a3a4a5ab0382f9e8713b135408a77
nit: optional header no longer necessary
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913008528)
in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:
Right-shifting by more bits than the width of Output is UB, so we should probably add an extra check for that. I've also added some test cases, but interestingly they don't seem to trigger UBSan (without the `util/overflow.h` patch) on my machine:
<details>
<summary>git diff on e0a541702d</summary>
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 09ba278d3a..ae292d7761 100644
--- a/src/test/util_tests.cpp
+++
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913008528)
in e0a541702def3a4c6cce8e44b6ebe8d608edad4e:
Right-shifting by more bits than the width of Output is UB, so we should probably add an extra check for that. I've also added some test cases, but interestingly they don't seem to trigger UBSan (without the `util/overflow.h` patch) on my machine:
<details>
<summary>git diff on e0a541702d</summary>
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 09ba278d3a..ae292d7761 100644
--- a/src/test/util_tests.cpp
+++
...
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913095820)
in ccf8caa4430f57b8f148e96ac2a241221977bcab
nit: I've raised this before, but since we now have a `kernel::CacheSizes` and `node::CacheSizes`, I think `using` a `kernel::CacheSizes` in a `node` file is unnecessarily confusing, so I would prefer making this explicit (as in the `chainstate.h` file):
<details>
<summary>git diff on ccf8caa443</summary>
```diff
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 660d2c3289..d4d8828f73 100644
--- a/src/node/chainstate.cpp
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913095820)
in ccf8caa4430f57b8f148e96ac2a241221977bcab
nit: I've raised this before, but since we now have a `kernel::CacheSizes` and `node::CacheSizes`, I think `using` a `kernel::CacheSizes` in a `node` file is unnecessarily confusing, so I would prefer making this explicit (as in the `chainstate.h` file):
<details>
<summary>git diff on ccf8caa443</summary>
```diff
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 660d2c3289..d4d8828f73 100644
--- a/src/node/chainstate.cpp
...
🤔 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
...