💬 ryanofsky commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1441943357)
The PR description seems to say that this fixes a bug and that there is alternative fix in #25008.
Is https://github.com/theuni/bitcoin/commit/9612f5f0f1e3295065dbd8a0e0f472f8f020bbd9 the alternative fix?
Neither of the two fixes seems to have an explanation of how they fix the problem. I also don't see a description of what the bug is other than "We're unable to build a `dll` for `libbitcoinkernel` right now because of unique problems arising out of the combination of `mingw-w64` + `winpt
...
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1441943357)
The PR description seems to say that this fixes a bug and that there is alternative fix in #25008.
Is https://github.com/theuni/bitcoin/commit/9612f5f0f1e3295065dbd8a0e0f472f8f020bbd9 the alternative fix?
Neither of the two fixes seems to have an explanation of how they fix the problem. I also don't see a description of what the bug is other than "We're unable to build a `dll` for `libbitcoinkernel` right now because of unique problems arising out of the combination of `mingw-w64` + `winpt
...
👍 TheCharlatan approved a pull request: "Convert ArgsManager::GetDataDir to a read-only function"
(https://github.com/bitcoin/bitcoin/pull/27073)
Re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387
(https://github.com/bitcoin/bitcoin/pull/27073)
Re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387
💬 fanquake commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1441982036)
> or why fanquake doesn't like it.
@ryanofsky I don't like it because it's clearly the wrong solution to what is ultimately a build/tooling problem. The code is fine, and works perfectly fine when compiled on all all other platforms. It doesn't work, when building a Windows DLL, and only when using `-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC` (`DEBUG=1`).
So this is clearly a build system/tooling problem, and the the idea that we should refactor perfectly working code, introduce new libs
...
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1441982036)
> or why fanquake doesn't like it.
@ryanofsky I don't like it because it's clearly the wrong solution to what is ultimately a build/tooling problem. The code is fine, and works perfectly fine when compiled on all all other platforms. It doesn't work, when building a Windows DLL, and only when using `-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC` (`DEBUG=1`).
So this is clearly a build system/tooling problem, and the the idea that we should refactor perfectly working code, introduce new libs
...
💬 theuni commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1115860697)
Our symbol visibility is not under control yet, meaning that the linker doesn't know what to do when it finds (for ex) an exported `pthread_mutex_unlock` in libbitcoinkernel.dll as well as libpthread. If you cherry-pick https://github.com/theuni/bitcoin/commit/1733d0199d68b2de2a72dfaa70802756540214a0 on top of this PR, linking should succeed.
This is why I've opted to allow for the dll to be built/linked here, but it's off by default (meaning that you have to do something like `--disable-sta
...
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1115860697)
Our symbol visibility is not under control yet, meaning that the linker doesn't know what to do when it finds (for ex) an exported `pthread_mutex_unlock` in libbitcoinkernel.dll as well as libpthread. If you cherry-pick https://github.com/theuni/bitcoin/commit/1733d0199d68b2de2a72dfaa70802756540214a0 on top of this PR, linking should succeed.
This is why I've opted to allow for the dll to be built/linked here, but it's off by default (meaning that you have to do something like `--disable-sta
...
💬 sipa commented on issue "miniscript_stable fuzz timeout":
(https://github.com/bitcoin/bitcoin/issues/27147#issuecomment-1441984460)
Reproduced it. It looks like it's just a giant input (53000 miniscript nodes) that will eventually lead to a too-large script, but the logic is only able to detect this once it's done. I think we can use a technique like the one we used in FromString parsing to detect too-large scripts early. Alternatively (and probably, independently) we can add a node count limit.
(https://github.com/bitcoin/bitcoin/issues/27147#issuecomment-1441984460)
Reproduced it. It looks like it's just a giant input (53000 miniscript nodes) that will eventually lead to a too-large script, but the logic is only able to detect this once it's done. I think we can use a technique like the one we used in FromString parsing to detect too-large scripts early. Alternatively (and probably, independently) we can add a node count limit.
💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1115871611)
Ok good feedback. Yeah these asserts are more like sanity checks. Testing the test! Although they also fail early on certain code regressions for example if the `-fastprune` size is changed in the future.
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1115871611)
Ok good feedback. Yeah these asserts are more like sanity checks. Testing the test! Although they also fail early on certain code regressions for example if the `-fastprune` size is changed in the future.
💬 ryanofsky commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1442018193)
@fanquake that all makes sense at a high level, but without knowing the details of what exactly is causing the bug, I don't actually know that that "the code is fine."
If the code works on all platforms and configurations except one, it does strongly suggest that there is no problem with the code, so I see where you are coming from. But there is some reason this fix works, so it could be fixing a problem that is masked on other platforms, and it could be making improvements that would make th
...
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1442018193)
@fanquake that all makes sense at a high level, but without knowing the details of what exactly is causing the bug, I don't actually know that that "the code is fine."
If the code works on all platforms and configurations except one, it does strongly suggest that there is no problem with the code, so I see where you are coming from. But there is some reason this fix works, so it could be fixing a problem that is masked on other platforms, and it could be making improvements that would make th
...
⚠️ pinheadmz opened an issue: "configure: error: cannot figure out how to use std::filesystem"
(https://github.com/bitcoin/bitcoin/issues/27148)
Got this error from `configure` when trying to build from tag `v24.0.1` on Ubuntu 18.
At first my gcc was version `gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)` so I tried installing clang 10: `clang version 10.0.0-4ubuntu1~18.04.2` and linked to `CXX`, etc... still got the same error.
```
$ uname -a
Linux party 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Descr
...
(https://github.com/bitcoin/bitcoin/issues/27148)
Got this error from `configure` when trying to build from tag `v24.0.1` on Ubuntu 18.
At first my gcc was version `gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)` so I tried installing clang 10: `clang version 10.0.0-4ubuntu1~18.04.2` and linked to `CXX`, etc... still got the same error.
```
$ uname -a
Linux party 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Descr
...
💬 fanquake commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442041863)
> ./configure --without-gui --with-incompatible-bdb
I see clang is being used in the output, but how are you passing clang through? `CC=clang=10 CXX=clang++`? What is the actual output in `config.log`?
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442041863)
> ./configure --without-gui --with-incompatible-bdb
I see clang is being used in the output, but how are you passing clang through? `CC=clang=10 CXX=clang++`? What is the actual output in `config.log`?
💬 pinheadmz commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442049297)
yeah:
```
$ env | grep clang
CC=/usr/bin/clang-10
CXX=/usr/bin/clang++-10
```
This is the relevant chunk from config.log:
```
configure:4823: checking whether /usr/bin/clang++-10 supports C++17 features with -std=c++17
configure:5619: /usr/bin/clang++-10 -std=c++17 -c -g -O2 conftest.cpp >&5
configure:5619: $? = 0
configure:5628: result: yes
configure:6565: checking whether std::filesystem can be used without link library
configure:6581: /usr/bin/clang++-10 -std=c++17 -o co
...
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442049297)
yeah:
```
$ env | grep clang
CC=/usr/bin/clang-10
CXX=/usr/bin/clang++-10
```
This is the relevant chunk from config.log:
```
configure:4823: checking whether /usr/bin/clang++-10 supports C++17 features with -std=c++17
configure:5619: /usr/bin/clang++-10 -std=c++17 -c -g -O2 conftest.cpp >&5
configure:5619: $? = 0
configure:5628: result: yes
configure:6565: checking whether std::filesystem can be used without link library
configure:6581: /usr/bin/clang++-10 -std=c++17 -o co
...
💬 pinheadmz commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442054213)
Errors in config.log look the same with gcc 7 too:
```
checking whether g++ supports C++17 features with -std=c++17... yes
checking whether std::filesystem can be used without link library... no
checking whether std::filesystem needs -lstdc++fs... no
checking whether std::filesystem needs -lc++fs... configure: error: in `/root/bitcoin':
configure: error: cannot figure out how to use std::filesystem
See `config.log' for more details
```
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442054213)
Errors in config.log look the same with gcc 7 too:
```
checking whether g++ supports C++17 features with -std=c++17... yes
checking whether std::filesystem can be used without link library... no
checking whether std::filesystem needs -lstdc++fs... no
checking whether std::filesystem needs -lc++fs... configure: error: in `/root/bitcoin':
configure: error: cannot figure out how to use std::filesystem
See `config.log' for more details
```
💬 hebasto commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442054926)
> yeah:
>
> ```
> $ env | grep clang
> CC=/usr/bin/clang-10
> CXX=/usr/bin/clang++-10
> ```
>
You still using old `libstdc++`.
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442054926)
> yeah:
>
> ```
> $ env | grep clang
> CC=/usr/bin/clang-10
> CXX=/usr/bin/clang++-10
> ```
>
You still using old `libstdc++`.
💬 theuni commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1115932874)
From libtool help:
```
-static do not do any dynamic linking of uninstalled libtool libraries
```
So, by default, use a static libbitcoinkernel for bitcoin-chainstate. This matches the default of our other executables linked to libs.
Reason being: otherwise test harnesses, c-i, benchmarks, etc have to play LD_PRELOAD or wrapper tricks.
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1115932874)
From libtool help:
```
-static do not do any dynamic linking of uninstalled libtool libraries
```
So, by default, use a static libbitcoinkernel for bitcoin-chainstate. This matches the default of our other executables linked to libs.
Reason being: otherwise test harnesses, c-i, benchmarks, etc have to play LD_PRELOAD or wrapper tricks.
💬 MarcoFalke commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442059015)
gcc 7 doesn't have filesystem. You can try `CC=clang-10 CXX="clang++-10 -stdlib=libc++"`
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442059015)
gcc 7 doesn't have filesystem. You can try `CC=clang-10 CXX="clang++-10 -stdlib=libc++"`
💬 fanquake commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442064307)
> This is the relevant chunk from config.log:
Installing `libstdc++-8-dev` will solve the problem.
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442064307)
> This is the relevant chunk from config.log:
Installing `libstdc++-8-dev` will solve the problem.
💬 hebasto commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1442064325)
This part in the PR description
> I've changed `trusted-keys` from just a list of key ids to a csv file with each row being the key id, and the earliest and latest commits the key is allowed to have signed.
looks outdated.
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1442064325)
This part in the PR description
> I've changed `trusted-keys` from just a list of key ids to a csv file with each row being the key id, and the earliest and latest commits the key is allowed to have signed.
looks outdated.
💬 pinheadmz commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442066649)
>
> Installing `libstdc++-8-dev` will solve the problem.
YES! Thanks. Add to docs ?
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442066649)
>
> Installing `libstdc++-8-dev` will solve the problem.
YES! Thanks. Add to docs ?
💬 theuni commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#issuecomment-1442071126)
@hebasto: I'm not sure how it looked before, but after this PR the exports look nice and clean:
```
$ objdump -p .libs/libbitcoinconsensus-0.dll
...
[Ordinal/Name Pointer] Table
[ 0] bitcoinconsensus_verify_script
[ 1] bitcoinconsensus_verify_script_with_amount
[ 2] bitcoinconsensus_version
The Function Table (interpreted .pdata section contents)
vma: BeginAddress EndAddress UnwindData
000000006db81000: 000000006d
...
(https://github.com/bitcoin/bitcoin/pull/27146#issuecomment-1442071126)
@hebasto: I'm not sure how it looked before, but after this PR the exports look nice and clean:
```
$ objdump -p .libs/libbitcoinconsensus-0.dll
...
[Ordinal/Name Pointer] Table
[ 0] bitcoinconsensus_verify_script
[ 1] bitcoinconsensus_verify_script_with_amount
[ 2] bitcoinconsensus_version
The Function Table (interpreted .pdata section contents)
vma: BeginAddress EndAddress UnwindData
000000006db81000: 000000006d
...
💬 hebasto commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442074169)
> Add to docs ?
Probably, it does not worth, considering that Ubuntu 18 is about to [reach](https://wiki.ubuntu.com/Releases) "End of Standard Support" soon.
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442074169)
> Add to docs ?
Probably, it does not worth, considering that Ubuntu 18 is about to [reach](https://wiki.ubuntu.com/Releases) "End of Standard Support" soon.
✅ pinheadmz closed an issue: "configure: error: cannot figure out how to use std::filesystem"
(https://github.com/bitcoin/bitcoin/issues/27148)
(https://github.com/bitcoin/bitcoin/issues/27148)
💬 MarcoFalke commented on issue "configure: error: cannot figure out how to use std::filesystem":
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442077762)
See also https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#dependencies:
* https://github.com/bitcoin/bitcoin/pull/24164
* https://github.com/bitcoin/bitcoin/pull/23060
(https://github.com/bitcoin/bitcoin/issues/27148#issuecomment-1442077762)
See also https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#dependencies:
* https://github.com/bitcoin/bitcoin/pull/24164
* https://github.com/bitcoin/bitcoin/pull/23060