💬 maflcko commented on pull request "rpc: add address_type field in getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/30727#discussion_r1734175148)
If you want to return the descriptor type of the spkm, maybe it would be better to just return the full parent descriptor of the address instead? This should communicate the type, as well as other details that may be interesting. Though, I am not sure what your imagined use case is.
(https://github.com/bitcoin/bitcoin/pull/30727#discussion_r1734175148)
If you want to return the descriptor type of the spkm, maybe it would be better to just return the full parent descriptor of the address instead? This should communicate the type, as well as other details that may be interesting. Though, I am not sure what your imagined use case is.
💬 1440000bytes commented on pull request "rpc: add address_type field in getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/30727#issuecomment-2314661958)
Concept ACK
Including `p2sh-segwit` in `legacy` doesn't look correct.
```diff
-$ bitcoin-cli -regtest getnewaddress "PR30727" "p2sh-segwit"
2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq
```
```diff
$ bitcoin-cli -regtest getaddressinfo 2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq
{
"address": "2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq",
- "address_type": "legacy",
"scriptPubKey": "a9144e1f934cd4659c0f47ded79ac1be8fb65c978fe787",
"ismine": true,
"solvable": true,
"desc": "sh(wpkh([e4e21d4d/49h
...
(https://github.com/bitcoin/bitcoin/pull/30727#issuecomment-2314661958)
Concept ACK
Including `p2sh-segwit` in `legacy` doesn't look correct.
```diff
-$ bitcoin-cli -regtest getnewaddress "PR30727" "p2sh-segwit"
2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq
```
```diff
$ bitcoin-cli -regtest getaddressinfo 2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq
{
"address": "2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq",
- "address_type": "legacy",
"scriptPubKey": "a9144e1f934cd4659c0f47ded79ac1be8fb65c978fe787",
"ismine": true,
"solvable": true,
"desc": "sh(wpkh([e4e21d4d/49h
...
💬 fanquake commented on issue "b-msghand[4988] general protection fault":
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2314671319)
The main issue here is no core dump (in future please try and include it in the issue), and I don't think there's anything we can do with the gdb backtrace. I tested that using the 27.0 binary, and the associated debug symbols, works for inspecting a core dump. i.e:
```bash
# assuming you have bitcoind and bitcoind.dbg from
# https://bitcoincore.org/bin/bitcoin-core-27.0/bitcoin-27.0-x86_64-linux-gnu.tar.gz
# https://bitcoincore.org/bin/bitcoin-core-27.0/bitcoin-27.0-x86_64-linux-gnu-debug.t
...
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2314671319)
The main issue here is no core dump (in future please try and include it in the issue), and I don't think there's anything we can do with the gdb backtrace. I tested that using the 27.0 binary, and the associated debug symbols, works for inspecting a core dump. i.e:
```bash
# assuming you have bitcoind and bitcoind.dbg from
# https://bitcoincore.org/bin/bitcoin-core-27.0/bitcoin-27.0-x86_64-linux-gnu.tar.gz
# https://bitcoincore.org/bin/bitcoin-core-27.0/bitcoin-27.0-x86_64-linux-gnu-debug.t
...
🚀 fanquake merged a pull request: "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}"
(https://github.com/bitcoin/bitcoin/pull/30721)
(https://github.com/bitcoin/bitcoin/pull/30721)
✅ fanquake closed an issue: "fuzz: crypto_fschacha20poly1305 timeout"
(https://github.com/bitcoin/bitcoin/issues/30505)
(https://github.com/bitcoin/bitcoin/issues/30505)
🚀 fanquake merged a pull request: "fuzz: fix timeout in `crypto_fschacha20poly1305`"
(https://github.com/bitcoin/bitcoin/pull/30725)
(https://github.com/bitcoin/bitcoin/pull/30725)
🤔 glozow reviewed a pull request: "[26.x] Fix compilation with GCC 15"
(https://github.com/bitcoin/bitcoin/pull/30722#pullrequestreview-2265595091)
utACK 7d5764fb2969a3c1addec0d02a5ec38139eae791
Checked it's a clean backport and release notes look correct. Didn't test gcc build.
(https://github.com/bitcoin/bitcoin/pull/30722#pullrequestreview-2265595091)
utACK 7d5764fb2969a3c1addec0d02a5ec38139eae791
Checked it's a clean backport and release notes look correct. Didn't test gcc build.
💬 maflcko commented on issue "b-msghand[4988] general protection fault":
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2314717564)
Thanks for checking @fanquake . I was under the impression that even without a core dump one could call `addr2line` or the equivalent in gdb (assuming bitcoind and bitcoind.dbg are available), but maybe I may have been mistaken.
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2314717564)
Thanks for checking @fanquake . I was under the impression that even without a core dump one could call `addr2line` or the equivalent in gdb (assuming bitcoind and bitcoind.dbg are available), but maybe I may have been mistaken.
💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2314789718)
Is this rfm with three acks, or does it need more?
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2314789718)
Is this rfm with three acks, or does it need more?
🚀 fanquake merged a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071)
(https://github.com/bitcoin/bitcoin/pull/29071)
🚀 fanquake merged a pull request: "[26.x] Fix compilation with GCC 15"
(https://github.com/bitcoin/bitcoin/pull/30722)
(https://github.com/bitcoin/bitcoin/pull/30722)
👍 fanquake approved a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2265730991)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
Going to merge this now. Non-exhaustive list of things that need fixing / following up:
* Coverage builds seems to be broken: https://github.com/hebasto/bitcoin/issues/341
* It's not exactly clear what to do here (or exactly what's broken)
* We shouldn't just add more options (i.e something Clang specific) if the current options are broken / unused.
* Missing fuzz targets from bad rebases: https://github.com/bitcoin/bitcoin/pull/30712.
...
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2265730991)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
Going to merge this now. Non-exhaustive list of things that need fixing / following up:
* Coverage builds seems to be broken: https://github.com/hebasto/bitcoin/issues/341
* It's not exactly clear what to do here (or exactly what's broken)
* We shouldn't just add more options (i.e something Clang specific) if the current options are broken / unused.
* Missing fuzz targets from bad rebases: https://github.com/bitcoin/bitcoin/pull/30712.
...
🚀 fanquake merged a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454)
(https://github.com/bitcoin/bitcoin/pull/30454)
💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2314866592)
> Autotools removal (ideally today).
I understand that it should be done quickly. However, given that some things don't work out-of-the-box anymore, such as clang coverage, it would be good to give a few more days (maybe a week) to give everyone a bit time to adapt their scripts, which there may be many of.
This also has the benefit that any quick follow-ups and bugfixes can still be tested for autotools parity.
In any case, I am not going to block the autotools removal, if others agree
...
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2314866592)
> Autotools removal (ideally today).
I understand that it should be done quickly. However, given that some things don't work out-of-the-box anymore, such as clang coverage, it would be good to give a few more days (maybe a week) to give everyone a bit time to adapt their scripts, which there may be many of.
This also has the benefit that any quick follow-ups and bugfixes can still be tested for autotools parity.
In any case, I am not going to block the autotools removal, if others agree
...
👋 maflcko's pull request is ready for review: "fuzz: Add missing fuzz targets to cmake build"
(https://github.com/bitcoin/bitcoin/pull/30712)
(https://github.com/bitcoin/bitcoin/pull/30712)
💬 maflcko commented on pull request "fuzz: Add missing fuzz targets to cmake build":
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2314882755)
Rebased.
Should probably be merged before switching any fuzzing over to cmake.
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2314882755)
Rebased.
Should probably be merged before switching any fuzzing over to cmake.
💬 maflcko commented on pull request "fuzz: Add missing fuzz targets to cmake build":
(https://github.com/bitcoin/bitcoin/pull/30712#discussion_r1734378032)
dropped from this pull for now
(https://github.com/bitcoin/bitcoin/pull/30712#discussion_r1734378032)
dropped from this pull for now
💬 maflcko commented on pull request "fuzz: Add missing fuzz targets to cmake build":
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2314886755)
@hebasto I see your comments, but I am not sure if you approve this pull request, or if you want to open an alternative.
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2314886755)
@hebasto I see your comments, but I am not sure if you approve this pull request, or if you want to open an alternative.
👍 hebasto approved a pull request: "fuzz: Add missing fuzz targets to cmake build"
(https://github.com/bitcoin/bitcoin/pull/30712#pullrequestreview-2265776927)
ACK fa0e1e4f3c2ede723554d10b6596d89955441373
(https://github.com/bitcoin/bitcoin/pull/30712#pullrequestreview-2265776927)
ACK fa0e1e4f3c2ede723554d10b6596d89955441373
📝 fanquake opened a pull request: "build: fix version number post CMake"
(https://github.com/bitcoin/bitcoin/pull/30729)
CMake was merged after branching-off for `28.x`.
(https://github.com/bitcoin/bitcoin/pull/30729)
CMake was merged after branching-off for `28.x`.