💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822780992)
Should this be removed?
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822780992)
Should this be removed?
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823481424)
Why not use `FormatStringCheck<sizeof...(Args)>` in `printf` and `printfln` directly below as well?
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823481424)
Why not use `FormatStringCheck<sizeof...(Args)>` in `printf` and `printfln` directly below as well?
💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823488811)
Could also add
```C++
PassFmt<1>("%.f");
```
maybe grouped with the `<1>` calls above. Confirmed tinyformat allows for it, see how the success status returned by `parseWidthOrPrecision` is ignored:
https://github.com/bitcoin/bitcoin/blob/e53829d3952c6ed275507a66e77636aad67d106b/src/tinyformat.h#L778-L788
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823488811)
Could also add
```C++
PassFmt<1>("%.f");
```
maybe grouped with the `<1>` calls above. Confirmed tinyformat allows for it, see how the success status returned by `parseWidthOrPrecision` is ignored:
https://github.com/bitcoin/bitcoin/blob/e53829d3952c6ed275507a66e77636aad67d106b/src/tinyformat.h#L778-L788
💬 github12101 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2448576849)
> > In my main machine, is ryzen with 32gb ddr4 with 2 sticks of memory. The other machine is Xeon, with 32gb memory, in quad channel ddr3. So i dont think its memory, because the 2 computers are stable. I can run prime95 in both for 2 hours with no problem.
>
> it's true, I just ran disk diagnostics, no issues. this is a new 4tb ssd. going to run a memory diagnostic... but my system has been stable so I would be very surprised if it was that. I'm also running on ZFS. I keep hitting this, eve
...
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2448576849)
> > In my main machine, is ryzen with 32gb ddr4 with 2 sticks of memory. The other machine is Xeon, with 32gb memory, in quad channel ddr3. So i dont think its memory, because the 2 computers are stable. I can run prime95 in both for 2 hours with no problem.
>
> it's true, I just ran disk diagnostics, no issues. this is a new 4tb ssd. going to run a memory diagnostic... but my system has been stable so I would be very surprised if it was that. I'm also running on ZFS. I keep hitting this, eve
...
👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2404680812)
Code review ACK 3b5b2457f713014fd5073da34b76a5a8cd796e4a. Nice change that should make the API more safe and also makes tests more complete and easier to maintain.
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2404680812)
Code review ACK 3b5b2457f713014fd5073da34b76a5a8cd796e4a. Nice change that should make the API more safe and also makes tests more complete and easier to maintain.
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822513330)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
Would be better to leave this outside of the try loop so it is clearer this is not expected to throw std::logic_error, and if it it did, the test framework would catch it.
Same for CheckWriteCoins test below.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822513330)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
Would be better to leave this outside of the try loop so it is clearer this is not expected to throw std::logic_error, and if it it did, the test framework would catch it.
Same for CheckWriteCoins test below.
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822507538)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not very important, but this commit would be easier to understand if AddFlags function were not moving at the same time as it being modified. Specifically it is harder to see the new `Assume(flags & (DIRTY | FRESH))` change and related changes because of this.
Would suggest not moving AddFlags this commit, and instead moving it in the next commit "coins: Remove direct GetFlags access" g
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822507538)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not very important, but this commit would be easier to understand if AddFlags function were not moving at the same time as it being modified. Specifically it is harder to see the new `Assume(flags & (DIRTY | FRESH))` change and related changes because of this.
Would suggest not moving AddFlags this commit, and instead moving it in the next commit "coins: Remove direct GetFlags access" g
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822525401)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
For this commit and other commits, it would be helpful if there was an indication about whether they were supposed to change behavior or test coverage. Would suggest including "refactor" in the subject for commits which are not supposed to change behavior like "coins, refactor: Split up AddFlags to remove invalid states" or could just say behavior isn't changing in the commit description.
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822525401)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
For this commit and other commits, it would be helpful if there was an indication about whether they were supposed to change behavior or test coverage. Would suggest including "refactor" in the subject for commits which are not supposed to change behavior like "coins, refactor: Split up AddFlags to remove invalid states" or could just say behavior isn't changing in the commit description.
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823231744)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not directly related to this PR, but would it make sense for SetClean to set m_prev and m_next to null here?
It seems that would be better than keeping dangling or invalid pointers. This would also make it possible to add asserts in AddFlags to confirm that the linked list state is valid when it is called.
Not suggesting any change for this commit, just wondering if this idea makes s
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823231744)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Not directly related to this PR, but would it make sense for SetClean to set m_prev and m_next to null here?
It seems that would be better than keeping dangling or invalid pointers. This would also make it possible to add asserts in AddFlags to confirm that the linked list state is valid when it is called.
Not suggesting any change for this commit, just wondering if this idea makes s
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823243237)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
Not important, but usually test code and output is more readable with `CHECK(a); CHECK(b);` instead of `CHECK(a && b)`
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823243237)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
Not important, but usually test code and output is more readable with `CHECK(a); CHECK(b);` instead of `CHECK(a && b)`
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823185560)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Maybe wait for other opinions to see if this suggestion is worthwhile, but IMO this commit provides a good opportunity to remove the `Assume(&self.second == this);` footgun and make calling these functions more straightforward:
<details><summary>diff</summary>
<p>
```diff
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(cons
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823185560)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)
Maybe wait for other opinions to see if this suggestion is worthwhile, but IMO this commit provides a good opportunity to remove the `Assume(&self.second == this);` footgun and make calling these functions more straightforward:
<details><summary>diff</summary>
<p>
```diff
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(cons
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823264089)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
It seems like these methods are not called in this commit, but they could be called in `InsertCoinsMapEntry` below. Maybe call them there or drop them here?
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823264089)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)
It seems like these methods are not called in this commit, but they could be called in `InsertCoinsMapEntry` below. Maybe call them there or drop them here?
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823433791)
In commit "test: Group values and states in tests into CoinEntry wrappers" (553db5dec1334ed5e8e07e34306f16de9afc22bb)
Review note: It's not really easy to verify by looking at code that new and old test cases are equivalent, but an approach that worked for me was to add extra prints showing the test cases, and then diff the output from `test_bitcoin -t coins_tests` before and after this commit, and confirm there are were no changes. My change used to test this was:
<details><summary>diff</
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823433791)
In commit "test: Group values and states in tests into CoinEntry wrappers" (553db5dec1334ed5e8e07e34306f16de9afc22bb)
Review note: It's not really easy to verify by looking at code that new and old test cases are equivalent, but an approach that worked for me was to add extra prints showing the test cases, and then diff the output from `test_bitcoin -t coins_tests` before and after this commit, and confirm there are were no changes. My change used to test this was:
<details><summary>diff</
...
💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823241356)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
May should check `!IsFresh()` here so new check is equivalent to previous check. Alternately, could just mention in commit message that this commit reduces test coverage a little bit by not checking some flags that are not relevant to the tests.
Same comment applies to other tests below like `linked_list_random_deletion`
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823241356)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)
May should check `!IsFresh()` here so new check is equivalent to previous check. Alternately, could just mention in commit message that this commit reduces test coverage a little bit by not checking some flags that are not relevant to the tests.
Same comment applies to other tests below like `linked_list_random_deletion`
💬 m3dwards commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448635473)
The slowdown appears to be introduced with https://github.com/bitcoin/bitcoin/commit/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001
Not sure why yet.
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448635473)
The slowdown appears to be introduced with https://github.com/bitcoin/bitcoin/commit/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001
Not sure why yet.
💬 davidgumberg commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2448647985)
> I've built this branch on Fedora 41, linking against UCRT (see [this](https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446405138) workflow), but the issue persists.
+1: built this branch on Fedora 40 doing a similar workflow and the same crash occurred:
```bash
ucrt64-make -C depends/ HOST=x86_64-w64-mingw32 -j $(nproc) # rpm macro: /usr/lib/rpm/macros.d/macros.ucrt64
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
cmake --build build -j $(nproc)
``
...
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2448647985)
> I've built this branch on Fedora 41, linking against UCRT (see [this](https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446405138) workflow), but the issue persists.
+1: built this branch on Fedora 40 doing a similar workflow and the same crash occurred:
```bash
ucrt64-make -C depends/ HOST=x86_64-w64-mingw32 -j $(nproc) # rpm macro: /usr/lib/rpm/macros.d/macros.ucrt64
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
cmake --build build -j $(nproc)
``
...
💬 davidgumberg commented on pull request "[POC] ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2448653950)
Concept ACK
#31172 @ https://github.com/bitcoin/bitcoin/pull/31172/commits/7dd0ee89a092c6ec4e305fbdb0cf3afa9e41cab6 passes CI but trivially fails to start on any Windows system because Wine lacks some undocumented Windows behavior.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2448653950)
Concept ACK
#31172 @ https://github.com/bitcoin/bitcoin/pull/31172/commits/7dd0ee89a092c6ec4e305fbdb0cf3afa9e41cab6 passes CI but trivially fails to start on any Windows system because Wine lacks some undocumented Windows behavior.
🤔 hodlinator reviewed a pull request: "msvc: Update vcpkg manifest"
(https://github.com/bitcoin/bitcoin/pull/31186#pullrequestreview-2406377117)
Tested on Windows 11 Home:
```
> rmdir /S build
> cmake -B build --preset vs2022-static
> cmake --build build --config Release -j<X>
> build\src\qt\Release\bitcoin-qt.exe (started successfully)
```
(https://github.com/bitcoin/bitcoin/pull/31186#pullrequestreview-2406377117)
Tested on Windows 11 Home:
```
> rmdir /S build
> cmake -B build --preset vs2022-static
> cmake --build build --config Release -j<X>
> build\src\qt\Release\bitcoin-qt.exe (started successfully)
```
💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1823427758)
<details>
<summary>
`cat vcpkg.json | jq -S` results in a different order, seems more lexicographical to me?
</summary>
```json
{
"$schema": "https://raw.githubusercontent.com/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json",
"builtin-baseline": "9edb1b8e590cc086563301d735cae4b6e732d2d2",
"default-features": [
"wallet",
"zeromq",
"tests",
"qt5"
],
"dependencies": [
"boost-date-time",
"boost-multi-index",
"boost-signals2",
"libeve
...
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1823427758)
<details>
<summary>
`cat vcpkg.json | jq -S` results in a different order, seems more lexicographical to me?
</summary>
```json
{
"$schema": "https://raw.githubusercontent.com/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json",
"builtin-baseline": "9edb1b8e590cc086563301d735cae4b6e732d2d2",
"default-features": [
"wallet",
"zeromq",
"tests",
"qt5"
],
"dependencies": [
"boost-date-time",
"boost-multi-index",
"boost-signals2",
"libeve
...
💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1823436767)
`$comment` should appear ordered before `$schema`, unless the latter *must* appear first?
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1823436767)
`$comment` should appear ordered before `$schema`, unless the latter *must* appear first?