💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2543648184)
I meant you could could have this in the intermediate commit:

```C++
NetGroupManager netgroup{std::vector(ASMAP_DATA.begin(), ASMAP_DATA.end())};
```
But then once we reach the commit adding embedding support change to:
```C++
auto netgroup{NetGroupManager::WithEmbeddedAsmap(ASMAP_DATA)};
```
...ending up going directly from `std::array` -> `std::span`.

---

If you prefer the current way I would still avoid `auto` and change:

```diff
- auto asmap_vec{std::vector(ASMAP_DATA.beg
...

🤔 fjahr reviewed a pull request: "net: Decouple `CConnman::GetAddresses` from `CNode`"
(https://github.com/bitcoin/bitcoin/pull/33900#pullrequestreview-3484304377)
I'm neutral on this change. I don't think the decoupling is very effective because conceptually the function is still tied to a "requesting peer"/user/attacker looking at the comments around that function. And pushing the access of the `m_network_key` to the call site doesn't really make the API nicer in my view. It would be different if the key was aready available at the call sites alone but that doesn't seem to be the case.

💬 fjahr commented on pull request "net: Decouple `CConnman::GetAddresses` from `CNode`":
(https://github.com/bitcoin/bitcoin/pull/33900#discussion_r2543218680)
I think the comment is slightly less helpful with the current version and the renaming of the variable otherwise.

```suggestion
* @param[in] network_key Network key of the requesting peer. Used to key the cache to prevent privacy leaks.
```

💬 fjahr commented on pull request "net: Decouple `CConnman::GetAddresses` from `CNode`":
(https://github.com/bitcoin/bitcoin/pull/33900#discussion_r2543220778)
Alternatively the variable could be named `requestor_key` or something similar to transport this information.

💬 achow101 commented on pull request "wallet: refactor ProcessDescriptorImport":
(https://github.com/bitcoin/bitcoin/pull/33874#discussion_r2543714661)
This is dangerous, we should never return `success: true` if the descriptor hasn't been fully processed yet.

💬 achow101 commented on pull request "wallet: refactor ProcessDescriptorImport":
(https://github.com/bitcoin/bitcoin/pull/33874#issuecomment-3554828252)
Concept meh

`importdescriptors` was modeled after the old `importmulti`, which IIRC part of the point was that even if some things couldn't be imported that everything else that can be imported was.

But I do see how exiting early and doing nothing if something can't be imported could be useful, but this is pretty big semantic change and we need to be careful about return values to avoid any possible confusion.

💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3554829049)
> But in the medium turn with embedded asmap data present but not turned on by default, you dislike the fact that -asmap=1 would load a file called 1 if we don't take additional steps to prevent this.

Yes, this is what has bothered me most about the arg for a long time and why I had included my minimal fix for this in the original embedding PR already, just implemented a bit differently than you suggest but with the same effect. I think it's just not acceptable that you couldn't even use the
...

🤔 mzumsande reviewed a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3484869685)
ACK c5b301f6ac0d3f882cc55397e3fb0cf95058704b

Fun fact: The 6-block warning is very old, introduced by Satoshi in https://github.com/bitcoin/bitcoin/commit/94cfec07fd302c9ff9b6a80c47418d4fe56596ae

💬 mzumsande commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2543670655)
nit: isn't "at least" imprecise, and you need _more than_ 6 blocks?
The validation codes uses `>` instead of `>=` and the test fails if I create just 6 blocks on top of the tip.

🤔 hebasto reviewed a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851#pullrequestreview-3484940469)
Maybe update `xcb_proto` as well?

💬 hebasto commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#discussion_r2543728983)
Why not [1.17](https://gitlab.freedesktop.org/xorg/lib/libxcb/-/commits/libxcb-1.17.0?ref_type=tags)?

💬 achow101 commented on pull request "wallet: Add separate balance info for non-mempool wallet txs":
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3554858459)
Concept NACK-ish

There's a lot of reasons that an unconfirmed transaction might be in the wallet but not the mempool, and I think giving an accurate balance of those transactions will be hard to get right. We absolutely do not want to mislead users into thinking they have more coins than they actually do, so I am hesitant to add something like this.

From a cursory glance at the code, I think 2 transactions that conflict with each other but aren't in the mempool would both get counted towar
...

💬 fjahr commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#discussion_r2543746081)
Fixed

💬 hebasto commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2543752271)
It would be helpful if someone could report this upstream.

💬 fanquake commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#discussion_r2543759301)
That required adding new dependencies, which I didn't want to do here. It's the same reason libxkbcommon isn't updated further, as that would require a new build tool/system (meson), and new dependencies.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2543764811)
Ah, I see, thanks for clarifying! Done.

💬 achow101 commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3554911366)
ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30

✅ waketraindev closed a pull request: "net: Decouple `CConnman::GetAddresses` from `CNode`"
(https://github.com/bitcoin/bitcoin/pull/33900)

💬 waketraindev commented on pull request "net: Decouple `CConnman::GetAddresses` from `CNode`":
(https://github.com/bitcoin/bitcoin/pull/33900#issuecomment-3554921486)
Closed. Going to let Net Split WG handle this one :) thanks for the comments.

👍 brunoerg approved a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3485040821)
reACK c5b301f6ac0d3f882cc55397e3fb0cf95058704b