💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1550948362)
I guess a simpler way to phrase it is: what can go wrong if c14ae132c5a5204a9a755c84c6de05fb30459221 picks the incorrect chainstate for a block? What are all the different ways in which we can receive a block? What do we expect to happen in each of these cases? Tests would be a really nice way to illustrate that, but are apparently a pain to write for net_processing.
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1550948362)
I guess a simpler way to phrase it is: what can go wrong if c14ae132c5a5204a9a755c84c6de05fb30459221 picks the incorrect chainstate for a block? What are all the different ways in which we can receive a block? What do we expect to happen in each of these cases? Tests would be a really nice way to illustrate that, but are apparently a pain to write for net_processing.
💬 hebasto commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#issuecomment-1550949496)
> Also, it allows to drop build code, which means it won't waste review when rolling over into cmake (`cmake/module/CheckStdFilesystem.cmake`).
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27682#issuecomment-1550949496)
> Also, it allows to drop build code, which means it won't waste review when rolling over into cmake (`cmake/module/CheckStdFilesystem.cmake`).
Concept ACK.
💬 hebasto commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196123867)
The only left usage of `SAVED_LIBS` can de dropped as well:
```diff
--- a/build-aux/m4/l_filesystem.m4
+++ b/build-aux/m4/l_filesystem.m4
@@ -25,8 +25,7 @@ AC_DEFUN([CHECK_FILESYSTEM], [
AC_MSG_RESULT([yes])
],[
AC_MSG_RESULT([no])
- SAVED_LIBS="$LIBS"
- LIBS="$SAVED_LIBS -lstdc++fs"
+ LIBS="$LIBS -lstdc++fs"
AC_MSG_CHECKING([whether std::filesystem needs -lstdc++fs])
AC_LINK_IFELSE([AC_LANG_SOURCE([_CHECK_FILESYSTEM_testbody])],[
...
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196123867)
The only left usage of `SAVED_LIBS` can de dropped as well:
```diff
--- a/build-aux/m4/l_filesystem.m4
+++ b/build-aux/m4/l_filesystem.m4
@@ -25,8 +25,7 @@ AC_DEFUN([CHECK_FILESYSTEM], [
AC_MSG_RESULT([yes])
],[
AC_MSG_RESULT([no])
- SAVED_LIBS="$LIBS"
- LIBS="$SAVED_LIBS -lstdc++fs"
+ LIBS="$LIBS -lstdc++fs"
AC_MSG_CHECKING([whether std::filesystem needs -lstdc++fs])
AC_LINK_IFELSE([AC_LANG_SOURCE([_CHECK_FILESYSTEM_testbody])],[
...
📝 MarcoFalke converted_to_draft a pull request: "build: Bump minimum supported GCC to g++-9"
(https://github.com/bitcoin/bitcoin/pull/27662)
It is a bit frustrating to write valid C++ code only to realize that g++-8 fails to parse it later on.
The only non-EOL operating system still shipping with g++-8 is CentOS Stream 8. I think it is reasonable for users of affected Linux distributions to:
* Upgrade their operating system, or compiler to a supported version.
* Alternatively, stay with a previous release of Bitcoin Core as long as it is supported.
Fixes https://github.com/bitcoin/bitcoin/issues/27537
(https://github.com/bitcoin/bitcoin/pull/27662)
It is a bit frustrating to write valid C++ code only to realize that g++-8 fails to parse it later on.
The only non-EOL operating system still shipping with g++-8 is CentOS Stream 8. I think it is reasonable for users of affected Linux distributions to:
* Upgrade their operating system, or compiler to a supported version.
* Alternatively, stay with a previous release of Bitcoin Core as long as it is supported.
Fixes https://github.com/bitcoin/bitcoin/issues/27537
💬 Sjors commented on issue "Use muhash for assumeUTXO snapshot ":
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1550964299)
For the purpose of verifying a consensus change the ordering doesn't matter. The end user of the snapshot would benefit from a sha256 hash in order to not waste time verifying incorrect snapshots (assuming that sha256 is quicker than muhash here).
A file checksum doesn't need to be in the consensus code, though if we offer an automatic download it may need to be somewhere in the code. The right file checksum may depend on the specific distribution mechanism, e.g. if you use a torrent you'd us
...
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1550964299)
For the purpose of verifying a consensus change the ordering doesn't matter. The end user of the snapshot would benefit from a sha256 hash in order to not waste time verifying incorrect snapshots (assuming that sha256 is quicker than muhash here).
A file checksum doesn't need to be in the consensus code, though if we offer an automatic download it may need to be somewhere in the code. The right file checksum may depend on the specific distribution mechanism, e.g. if you use a torrent you'd us
...
💬 MarcoFalke commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196131293)
Ok, will do on the next push. Otherwise it will go away when the file is translated to cmake or when GCC is bumped to g++-9, whichever happens earlier.
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196131293)
Ok, will do on the next push. Otherwise it will go away when the file is translated to cmake or when GCC is bumped to g++-9, whichever happens earlier.
💬 hebasto commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#issuecomment-1550971166)
Just noting that our current Guix environment uses Clang 10.0.1.
(https://github.com/bitcoin/bitcoin/pull/27682#issuecomment-1550971166)
Just noting that our current Guix environment uses Clang 10.0.1.
💬 Sjors commented on issue "Mac osx 12.6.5 ":
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1550972453)
Also useful to know if this is an Intel mac or the newer M1 / M2.
(https://github.com/bitcoin/bitcoin/issues/27681#issuecomment-1550972453)
Also useful to know if this is an Intel mac or the newer M1 / M2.
🚀 hebasto merged a pull request: "test: Add missed header"
(https://github.com/bitcoin-core/gui/pull/729)
(https://github.com/bitcoin-core/gui/pull/729)
💬 fanquake commented on pull request "msvc: Rename `libbitcoinconsensus` to `libbitcoin_consensus` and other adjustments":
(https://github.com/bitcoin/bitcoin/pull/27615#issuecomment-1551007062)
Other than renaming things, what does this acheive? Unclear it's worth doing if everything done here is going to be thrown away when we switch to CMake.
(https://github.com/bitcoin/bitcoin/pull/27615#issuecomment-1551007062)
Other than renaming things, what does this acheive? Unclear it's worth doing if everything done here is going to be thrown away when we switch to CMake.
💬 MarcoFalke commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196171608)
Thanks, done.
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196171608)
Thanks, done.
💬 MarcoFalke commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551017016)
Any understanding why or when this happened?
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551017016)
Any understanding why or when this happened?
💬 fanquake commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551020550)
I think it was https://github.com/bitcoin/bitcoin/pull/26715.
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551020550)
I think it was https://github.com/bitcoin/bitcoin/pull/26715.
💬 MarcoFalke commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551028017)
But CI passed on the pull and merge commit (https://github.com/bitcoin/bitcoin/runs/13481551706), no?
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551028017)
But CI passed on the pull and merge commit (https://github.com/bitcoin/bitcoin/runs/13481551706), no?
💬 hebasto commented on pull request "msvc: Rename `libbitcoinconsensus` to `libbitcoin_consensus` and other adjustments":
(https://github.com/bitcoin/bitcoin/pull/27615#issuecomment-1551029540)
> Other than renaming things, what does this acheive?
It is [required](https://github.com/bitcoin/bitcoin/pull/24773#issuecomment-1541888040) for https://github.com/bitcoin/bitcoin/pull/24773.
> Unclear it's worth doing if everything done here is going to be thrown away when we switch to CMake.
Sure. Not a priority at all.
(https://github.com/bitcoin/bitcoin/pull/27615#issuecomment-1551029540)
> Other than renaming things, what does this acheive?
It is [required](https://github.com/bitcoin/bitcoin/pull/24773#issuecomment-1541888040) for https://github.com/bitcoin/bitcoin/pull/24773.
> Unclear it's worth doing if everything done here is going to be thrown away when we switch to CMake.
Sure. Not a priority at all.
💬 hebasto commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551033551)
The MSVC link issue was kind of intermittent. I guess, it was dependent on the actual order of processing of the source files.
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551033551)
The MSVC link issue was kind of intermittent. I guess, it was dependent on the actual order of processing of the source files.
💬 MarcoFalke commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551036780)
Ok, interesting. Maybe with iwyu this kind of bug will be fixed eventually :smiling_face_with_tear:
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551036780)
Ok, interesting. Maybe with iwyu this kind of bug will be fixed eventually :smiling_face_with_tear:
💬 fanquake commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551037870)
Yea. I guess the compiler is broken? Developers should not have to worry about putting files in the "right order" for it.
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551037870)
Yea. I guess the compiler is broken? Developers should not have to worry about putting files in the "right order" for it.
👍 hebasto approved a pull request: "build: Bump minimum supported Clang to clang-10"
(https://github.com/bitcoin/bitcoin/pull/27682#pullrequestreview-1430199164)
ACK fa199ee614a7ed99c6caf329093a3573ea5a664b
(https://github.com/bitcoin/bitcoin/pull/27682#pullrequestreview-1430199164)
ACK fa199ee614a7ed99c6caf329093a3573ea5a664b
💬 jamesob commented on issue "Use muhash for assumeUTXO snapshot ":
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1551051475)
See the proposal for why I didn't use muhash to begin with: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal#how-will-users-and-reviewers-efficiently-verify-hashes-of-a-given-utxo-set
That said, the original rationale (supporting snapshot chunks) doesn't seem as relevant anymore, so this might be worth revisiting...
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1551051475)
See the proposal for why I didn't use muhash to begin with: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal#how-will-users-and-reviewers-efficiently-verify-hashes-of-a-given-utxo-set
That said, the original rationale (supporting snapshot chunks) doesn't seem as relevant anymore, so this might be worth revisiting...