👍 vasild approved a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1379215224)
ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1379215224)
ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
📝 fanquake opened a pull request: "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode"
(https://github.com/bitcoin/bitcoin/pull/27447)
It was deprecated in LLVM 15, turned into compile-time error in LLVM 16:
```bash
In file included from /usr/lib/llvm-16/bin/../include/c++/v1/cassert:19:
/usr/lib/llvm-16/bin/../include/c++/v1/__assert:22:5: error: "Defining _LIBCPP_DEBUG is not supported anymore.
Please use _LIBCPP_ENABLE_DEBUG_MODE instead."
^
1 error generated.
```
and has been removed entirely in LLVM 17 (main): https://github.com/llvm/llvm-project/commit/ff573a42cd1f1d05508f165dc3e645a0ec17edb5.
[Building l
...
(https://github.com/bitcoin/bitcoin/pull/27447)
It was deprecated in LLVM 15, turned into compile-time error in LLVM 16:
```bash
In file included from /usr/lib/llvm-16/bin/../include/c++/v1/cassert:19:
/usr/lib/llvm-16/bin/../include/c++/v1/__assert:22:5: error: "Defining _LIBCPP_DEBUG is not supported anymore.
Please use _LIBCPP_ENABLE_DEBUG_MODE instead."
^
1 error generated.
```
and has been removed entirely in LLVM 17 (main): https://github.com/llvm/llvm-project/commit/ff573a42cd1f1d05508f165dc3e645a0ec17edb5.
[Building l
...
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1162906791)
Sure. Having a proxy class is nice for any future IPC mechanism. All the protobuf/protocan/etc messages are simpler to craft in this way.
Based on how close we are to v25 branch-off, I'm thinking on whether should do it now or later :/. Otherwise I would push it right away.
Probably, could tackle it on a quick follow-up with https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156018577 and few other cleanups as well.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1162906791)
Sure. Having a proxy class is nice for any future IPC mechanism. All the protobuf/protocan/etc messages are simpler to craft in this way.
Based on how close we are to v25 branch-off, I'm thinking on whether should do it now or later :/. Otherwise I would push it right away.
Probably, could tackle it on a quick follow-up with https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156018577 and few other cleanups as well.
💬 MarcoFalke commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1503475232)
I don't see how this should be implemented. The Check suites don't link back to the pull request. Example:
* https://api.github.com/repos/bitcoin/bitcoin/check-suites/12155872750
And the pulls or status API doesn't link to the check suite. Example:
* https://api.github.com/repos/bitcoin/bitcoin/commits/fa4a46de0b3c1a5895e95dba7e95278932fbfc2c/status
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1503475232)
I don't see how this should be implemented. The Check suites don't link back to the pull request. Example:
* https://api.github.com/repos/bitcoin/bitcoin/check-suites/12155872750
And the pulls or status API doesn't link to the check suite. Example:
* https://api.github.com/repos/bitcoin/bitcoin/commits/fa4a46de0b3c1a5895e95dba7e95278932fbfc2c/status
💬 MarcoFalke commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1162926241)
```suggestion
linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_ASSERTIONS=1
```
Any reason to not enable assertions in the headers?
> ... most of the code in libc++ is in the headers, so the user-selected value for _LIBCPP_ENABLE_ASSERTIONS (if any) will usually be respected. https://releases.llvm.org/16.0.0/projects/libcxx/docs/UsingLibcxx.html#assertions-mode
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1162926241)
```suggestion
linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_ASSERTIONS=1
```
Any reason to not enable assertions in the headers?
> ... most of the code in libc++ is in the headers, so the user-selected value for _LIBCPP_ENABLE_ASSERTIONS (if any) will usually be respected. https://releases.llvm.org/16.0.0/projects/libcxx/docs/UsingLibcxx.html#assertions-mode
💬 RandyMcMillan commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503527508)
Concept ACK
Having more control over custom signet parameters/conditions seems really useful IMO.
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503527508)
Concept ACK
Having more control over custom signet parameters/conditions seems really useful IMO.
💬 MarcoFalke commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#issuecomment-1503551017)
> I'm wondering if as a followup, we could enable a DEBUG build of libc++
in our MSAN CI job?
sgtm
(https://github.com/bitcoin/bitcoin/pull/27447#issuecomment-1503551017)
> I'm wondering if as a followup, we could enable a DEBUG build of libc++
in our MSAN CI job?
sgtm
🚀 fanquake merged a pull request: "fuzz: Add HeadersSyncState target"
(https://github.com/bitcoin/bitcoin/pull/26662)
(https://github.com/bitcoin/bitcoin/pull/26662)
📝 fanquake opened a pull request: "ci: build libc++ in DEBUG mode in MSAN jobs "
(https://github.com/bitcoin/bitcoin/pull/27448)
Based on #27447.
See https://releases.llvm.org/16.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html:
> Libc++ provides a debug mode that enables special debugging checks meant to detect incorrect usage of the standard library. These checks are disabled by default, but they can be enabled by vendors when building the library by using LIBCXX_ENABLE_DEBUG_MODE.
(https://github.com/bitcoin/bitcoin/pull/27448)
Based on #27447.
See https://releases.llvm.org/16.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html:
> Libc++ provides a debug mode that enables special debugging checks meant to detect incorrect usage of the standard library. These checks are disabled by default, but they can be enabled by vendors when building the library by using LIBCXX_ENABLE_DEBUG_MODE.
💬 achow101 commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163046096)
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 "wallet: introduce generic recursive tx state updating function"
The comments should be updated to avoid mentioning specific transaction states.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163046096)
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 "wallet: introduce generic recursive tx state updating function"
The comments should be updated to avoid mentioning specific transaction states.
💬 achow101 commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163045689)
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 "wallet: introduce generic recursive tx state updating function"
This could also be used by `AbandonTransaction`.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163045689)
In c3fe92175b6ac709e4fdfd4766253c75d7ffd184 "wallet: introduce generic recursive tx state updating function"
This could also be used by `AbandonTransaction`.
💬 fanquake commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163049972)
> Any reason to not enable assertions in the headers?
Given that this shouldn't conflict with also turning them on at compile time, i.e also as part of #27448, I don't think so.
Should we also do `_GLIBCXX_ASSERTIONS`?
> [Undefined by default. When defined, enables extra error checking in the form of precondition assertions, such as bounds checking in strings and null pointer checks when dereferencing smart pointers.](https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html)
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163049972)
> Any reason to not enable assertions in the headers?
Given that this shouldn't conflict with also turning them on at compile time, i.e also as part of #27448, I don't think so.
Should we also do `_GLIBCXX_ASSERTIONS`?
> [Undefined by default. When defined, enables extra error checking in the form of precondition assertions, such as bounds checking in strings and null pointer checks when dereferencing smart pointers.](https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html)
💬 MarcoFalke commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163056807)
_GLIBCXX_ASSERTIONS is enabled by _GLIBCXX_DEBUG
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163056807)
_GLIBCXX_ASSERTIONS is enabled by _GLIBCXX_DEBUG
💬 theStack commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1163054934)
nit: Is there any specific reason for this change? Without it, everything seems to work still fine on my side (but obviously, it shouldn't hurt to construct the message box with a parent...)
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1163054934)
nit: Is there any specific reason for this change? Without it, everything seems to work still fine on my side (but obviously, it shouldn't hurt to construct the message box with a parent...)
👍 theStack approved a pull request: "wallet, gui: bugfix, getAvailableBalance skips selected coins"
(https://github.com/bitcoin/bitcoin/pull/26699#pullrequestreview-1379656461)
ACK 68eed5df8656bed1be6526b014e58d3123102b03
Reviewed the changeset (with only light review for the qt unit tests, as I'm not too familiar in that area), verified manually that the [GUI coin selection issue](https://github.com/bitcoin/bitcoin/pull/26699#pullrequestreview-1253771673) is fixed, tested the tests by reverting the fix commits dc1cc1c35995dc09085b3d9270c445b7923fdb51 and cd98b717398f7b13ace91ea9efac9ce1e60b4d62 each and checked that the corresponding unit tests fail. LGTM.
(https://github.com/bitcoin/bitcoin/pull/26699#pullrequestreview-1379656461)
ACK 68eed5df8656bed1be6526b014e58d3123102b03
Reviewed the changeset (with only light review for the qt unit tests, as I'm not too familiar in that area), verified manually that the [GUI coin selection issue](https://github.com/bitcoin/bitcoin/pull/26699#pullrequestreview-1253771673) is fixed, tested the tests by reverting the fix commits dc1cc1c35995dc09085b3d9270c445b7923fdb51 and cd98b717398f7b13ace91ea9efac9ce1e60b4d62 each and checked that the corresponding unit tests fail. LGTM.
💬 fanquake commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163065163)
Yea I should have read another 2 lines down
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163065163)
Yea I should have read another 2 lines down
💬 fanquake commented on pull request "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode":
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163067961)
Added now.
(https://github.com/bitcoin/bitcoin/pull/27447#discussion_r1163067961)
Added now.
💬 1440000bytes commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503775491)
Concept ACK
However I am not sure if other reviewers really matter here and only 2 main signet gatekeepers can approve this. To address the concern in https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1502605806 maybe this works only for custom signet.
How?
Maybe if bitcoin.conf has `signetchallenge=xxx` in it?
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503775491)
Concept ACK
However I am not sure if other reviewers really matter here and only 2 main signet gatekeepers can approve this. To address the concern in https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1502605806 maybe this works only for custom signet.
How?
Maybe if bitcoin.conf has `signetchallenge=xxx` in it?
💬 benthecarman commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503777153)
> Configurable consensus parameters seems like a great way to make the network inconsistent in general (in particular, if you get the parameter wrong, you'll end up marking valid blocks invalid, with various consequences).
I don't know if that is a really fair criticism for signet, `-signetchallenge` is a consensus param as well.
> If you're trying to generate many blocks, then regtest is already better as it doesn't require a baseline proof of work, and simply having multiple regtest conf
...
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503777153)
> Configurable consensus parameters seems like a great way to make the network inconsistent in general (in particular, if you get the parameter wrong, you'll end up marking valid blocks invalid, with various consequences).
I don't know if that is a really fair criticism for signet, `-signetchallenge` is a consensus param as well.
> If you're trying to generate many blocks, then regtest is already better as it doesn't require a baseline proof of work, and simply having multiple regtest conf
...
💬 benthecarman commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503780795)
> Maybe if bitcoin.conf has signetchallenge=xxx in it?
Yes, this currently it will throw an error if you don't have signet challenge set.
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1503780795)
> Maybe if bitcoin.conf has signetchallenge=xxx in it?
Yes, this currently it will throw an error if you don't have signet challenge set.