💬 martinus commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1867604026)
Oh no! You are using C++20 so I finally need to learn its features... Code review ACK and ran tests & benchmarks, the performance difference for `std::byte` before and after is a factor 14 for me (143.39 ns/op down to 10.03 ns/op).

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#discussion_r1434998519)
The Qt build already produces heaps of other irrelevant warning output, so I'm not sure this makes any real difference? Properly gateing this would also just increase the patch size, and make rebasing them harder?

This same thing also already occurs for other qt patches, i.e fast_fixed_dtoa_no_optimize, where clang already warns about ignoring the pragma usage.

💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1867610348)
What behavior changes we might expect due to replacing `std::stable_sort` with `std::sort`?

How do we ensure that no subtle bugs are introduced?

💬 Sjors commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1867621855)
> * Building for mac, in Guix, using Guix's Clang + our downloaded lld + tools??
>
> * Or shift further into using all Guix linker + tools, or, try drop Guix Clang?

Would you say that (temporarily) using Apple's Clang instead of Guix would still make this PR an overall improvement for macOS reproducibility? If so, and if makes things easier, that could make sense.

💬 hebasto commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867645491)
@ryanofsky
> my question is what is the harm of exporting these symbols?

I can add nothing to the best practises:
- https://gcc.gnu.org/wiki/Visibility
- https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fvisibility:
> symbol visibility should be viewed as part of the API interface contract

💬 Sjors commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1867671534)
Did some testing with f5d2014e96240aab9540aaba0a792710aa7725e7.

Using `contrib/devtools/utxo_snapshot.sh` on signet I get the same `txoutset_hash`. The resulting is also identical to what I generated in earlier, see https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1739652329. I was then able to load the loadshot and sync the chain (to the tip in about a minute, wonderful) and complete the background sync.

I also generated a mainnet snapshot which was also identical to what I foun
...

💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867691764)
I tested it with HWI and Ledger Nano S Plus.

--------

- I created a wallet:
`./src/bitcoin-cli -named createwallet wallet_name=ledger3 disable_private_keys=true descriptors=true external_signer=true`

- Got a new address:
```sh
$ ./src/bitcoin-cli -rpcwallet=ledger3 getnewaddress
bc1qfshzjl770c6qz560cade6ynswj2a3scqzm4a4m
```
- Ran `walletdisplayaddress`:
```sh
$ ./src/bitcoin-cli -rpcwallet=ledger3 walletdisplayaddress "bc1qfshzjl770c6qz560cade6ynswj2a3scqzm4a4m"
```
- Checke
...

💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1867704340)
For the code here, there shouldn't be anything new to learn. It it just a different syntax, for something that can be written with `enable_if` as well. See https://www.foonathan.net/2016/09/cpp14-concepts/#conclusion

> Emulation of the `requires` clause is possible using almost the same syntax with `std::enable_if`.

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867721948)
re: https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1866961865

> The new keys would need to be stored in a new database field

This is not actually true. Thinking about this more, I wonder if a more natural way of representing standalone keys in the wallet is just to treat them as a new [descriptor type](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md#reference), like `void(KEY)`, where the void descriptor would hold the key, but instead of [expanding](https://g
...

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1435094066)
Done

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1435094504)
Done as suggested

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1435096415)
Sure, thanks!. Done as suggested.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1435097429)
Dropped per comment https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434937507. Thanks

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1435097994)
Sure. Have reordered the commits.

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867747340)
Mmm, on the bright side, both the master and this PR have a really unclear message when the device is not connected :-)

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867748967)
@achow101 any thoughts on having HWI catch this cryptic `0x6982` status code and throw a more clear error (that we can then just pass on to the user)?

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867754041)
@ryanofsky interesting idea, I especially like the backup implication, because descriptors can be used for paper backups.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1435131324)
This is the only change I did not implement. I think we should discuss further where to place this constants before moving them to the header. It seems that with every new unit test, we will be forced to move some/most constants from `net_processing.cpp` to the header.

💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867779809)
> @achow101 any thoughts on having HWI catch this cryptic 0x6982 status code and throw a more clear error (that we can then just pass on to the user)?

Not sure, but this status code is specific from Ledger?

💬 theuni commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1867794427)
Adding one more bit to complicate and maybe explain why I described the boundary as a firewall... We build `libbitcoinconsensus.so` with guix using `-static-libstdc++` (side-note, we should perhaps move this into the build-system rather than guix).

So the idea here is that the entire c++ implementation should be compiled into the shared lib with no unresolved c++ stdlib symbols, and exporting only C symbols. That way the end-user can use any c++ impl (if any) they want without fear of collisi
...