⚠️ MarcoFalke opened an issue: "miniscript_stable fuzz timeout"
(https://github.com/bitcoin/bitcoin/issues/27147)
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56270
(https://github.com/bitcoin/bitcoin/issues/27147)
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56270
👍 furszy approved a pull request: "wallet: skip R-value signature grinding for external signers"
(https://github.com/bitcoin/bitcoin/pull/26032)
ACK 302f07ee
Looking quite straightforward now.
(https://github.com/bitcoin/bitcoin/pull/26032)
ACK 302f07ee
Looking quite straightforward now.
💬 furszy commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1115670527)
tiny nit:
could cache this result into a variable outside of the loop (after the `only_safe` variable)
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1115670527)
tiny nit:
could cache this result into a variable outside of the loop (after the `only_safe` variable)
💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1441833350)
All good about the none usage of the "-" prefix on the GUI 👍🏼 .
> The provided method executeConsoleOnlyCommand is almost identical to the suggested ExecuteCliCommand and is also ready for potential future new console only commands, the only difference is that it detects ( like IsCliOnlyCommand ) and executes commands in a single method.
I'm not fan of the current method mostly because of all the `!parsedCommand.empty()` and `parsedCommand.size() > something` and `parsedCommand.size() <
...
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1441833350)
All good about the none usage of the "-" prefix on the GUI 👍🏼 .
> The provided method executeConsoleOnlyCommand is almost identical to the suggested ExecuteCliCommand and is also ready for potential future new console only commands, the only difference is that it detects ( like IsCliOnlyCommand ) and executes commands in a single method.
I'm not fan of the current method mostly because of all the `!parsedCommand.empty()` and `parsedCommand.size() > something` and `parsedCommand.size() <
...
💬 hebasto commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#issuecomment-1441898542)
5da7c0b3e34626ca57d1f0773db61e7d8351d8c7
> Symbol visibility issues are not actually fixed yet because we have not yet
defined an api and exported symbols, but everything is now in place for that.
Do "symbol visibility issues" include https://github.com/bitcoin/bitcoin/issues/26698?
(https://github.com/bitcoin/bitcoin/pull/27146#issuecomment-1441898542)
5da7c0b3e34626ca57d1f0773db61e7d8351d8c7
> Symbol visibility issues are not actually fixed yet because we have not yet
defined an api and exported symbols, but everything is now in place for that.
Do "symbol visibility issues" include https://github.com/bitcoin/bitcoin/issues/26698?
💬 hebasto commented on pull request "contrib: Check symbols in the `bitcoinconsensus` shared library":
(https://github.com/bitcoin/bitcoin/pull/25020#issuecomment-1441900180)
> > Is this change (going to be) dependant on #24994, or not? If so, maybe mark as a draft for now, and add that to the PR description?
>
> Drafted.
Closing.
(https://github.com/bitcoin/bitcoin/pull/25020#issuecomment-1441900180)
> > Is this change (going to be) dependant on #24994, or not? If so, maybe mark as a draft for now, and add that to the PR description?
>
> Drafted.
Closing.
✅ hebasto closed a pull request: "contrib: Check symbols in the `bitcoinconsensus` shared library"
(https://github.com/bitcoin/bitcoin/pull/25020)
(https://github.com/bitcoin/bitcoin/pull/25020)
👍 hebasto approved a pull request: "Convert ArgsManager::GetDataDir to a read-only function"
(https://github.com/bitcoin/bitcoin/pull/27073)
re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387, only comments have been adjusted as requsted since my previous [review](https://github.com/bitcoin/bitcoin/pull/27073#pullrequestreview-1307435890).
(https://github.com/bitcoin/bitcoin/pull/27073)
re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387, only comments have been adjusted as requsted since my previous [review](https://github.com/bitcoin/bitcoin/pull/27073#pullrequestreview-1307435890).
💬 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++`.