💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434948774)
Commit a710e08398 `net: move state dependent peer services flags` moves this code to `src/net_processing.cpp` and changes from `g_initial_block_download_completed` to `m_initial_sync_finished`. The variable `m_initial_sync_finished` is used for something else and has a different semantic, thus using `m_initial_sync_finished` here is wrong. Later another commit changes from `m_initial_sync_finished` to the proper check.

I think it would be better to reorder the commits, so that the proper chec
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434937507)
I like how the best block's time is cached in `PeerManagerImpl::m_best_block_time` but further caching the expression "is the best block time older than 288 blocks" seems excessive and unnecessary to me. It makes the code more complex and obviously gets stale one second after it is set.

Here you can use something like:
```cpp
if (ApproximateBestBlockAgeInNumberOfBlocks() < NODE_NETWORK_LIMITED_MIN_BLOCKS) {
return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);
}
...
int64_t Appr
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434938857)
Better expose this constant in `net_processing.h` instead of duplicating it here. This file already includes `net_processing.h`.

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434912424)
Maybe change the argument from `uint64_t` to `std::chrono::seconds`, so that the units of it are more clear. The comment warrants an update:

```cpp
/** Set the height of the best block and its time (seconds since epoch). */
virtual void SetBestBlock(int height, std::chrono::seconds time) = 0;
```

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1434954442)
Not in this PR, unless someone else gives me the code for it :-)

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1434955493)
Will consider new wording if I need to touch / rebase.

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434962398)
The conditions under which `m_connman.SetTryNewOutboundPeer(false);` is executed are not the same before and after this change. Maybe it is ok, but is an unrelated change to the aim of this PR:

* before `m_connman.SetTryNewOutboundPeer(false)` would have been executed if this was true:
```cpp
(m_chainman.m_blockman.LoadingBlocks() || !m_connman.GetNetworkActive() || !m_connman.GetUseAddrmanOutgoing() || !TipMayBeStale()) && m_connman.GetTryNewOutboundPeer()
```

* after `m_connman.SetTry
...

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1867587043)
Rebased after #28880. Cleaned up the commits a bit. Doesn't work yet, because Qt doesn't build..

Also rethinking the best approach here a bit, as we've now essentially got to accomodate 3 different builds for macOS:
* Building for mac, on mac, using Apple Clang & ld64 (which also doesn't like `-fuse-ld=lld`)
* Building for mac, on Linux, using our downloaded LLVM (Clang + lld + tools)
* Building for mac, in Guix, using Guix's Clang + our downloaded lld + tools??
* Or shift further into
...

💬 hebasto commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1434989597)
nit: As the word 'combining' has only one object, rather than two as it was before, it seems reasonable to drop it altogether.

💬 hebasto commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1434985886)
```suggestion
# variable is set to the full path of the tool, just like how AC_PATH_{TOOL,PROG}
```

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1434990706)
Given this doesn't yet work, and we might be reithinking the approach (which might render this text irrelevant, I'll just leave stuff like this for now.

👍 vasild approved a pull request: "build: Remove HAVE_CONSENSUS_LIB"
(https://github.com/bitcoin/bitcoin/pull/29123#pullrequestreview-1794384328)
ACK b37a171dbbcd9a7a8934099be1b508a0ea4f05a8

💬 hebasto commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#discussion_r1434996856)
b335710782c2545e6eeed67b5e1763c07eab26b0

Being a clang-specific attribute it should be properly gated to do not produce `warning: 'optnone' attribute directive ignored [-Wattributes]`.

However, such warnings are harmless for now.

💬 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`.