💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193954353)
Yea, gonna do it!

💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548022004)
> I think the last reason to hang on to mapRelay is the reason I gave above

My understanding is that `REPLACED` would also need to be provided to peers: https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255. Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool?

However, that may be solved, but just replacing the `mapRelay` lookup by a `vExtra
...

💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1548028401)
Force-pushed

- Rebased
- Moved static globals into `initialize_setup` - https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193099637

Thanks @MarcoFalke for the review

💬 furszy commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548034031)
Still, this is not the case but have to add that a psbt creation on a wallet only with unconfirmed balance isn't possible (coins received from an external source are treated as "untrusted"). In the same way as it is not enabled for regular spending neither in the GUI.

💬 kroese commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548035439)
Thanks, when updating the keys I found that `keys.txt` no longer exists in this repo, so I solved it by doing:

```
KEY_URL=https://raw.githubusercontent.com/bitcoin-core/guix.sigs/main/builder-keys

curl -sSL ${KEY_URL}/CoinForensics.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/Emzy.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/Sjors.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/TheCharlatan.gpg | gpg --import - \
&& curl -sSL ${KEY_URL}/achow101.gpg | gpg --import - \
...

💬 MarcoFalke commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1548038156)
Should be ready for review

💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1193975226)
All good, I was just wondering if I may have been missing something.

💬 MarcoFalke commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548044235)
> So why is there not a single file that bundles all the current keys so that I dont have to download them all individually?

No one can know all the current keys, because they can change for every release. However, it is possible to find them for a given release by looking into the folder. For example, you can look into https://github.com/bitcoin-core/guix.sigs/tree/main/25.0rc2 to find all current ones for that tag.

💬 MarcoFalke commented on pull request "build: Drop support for g++-8":
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1548047843)
Looks like the only affected code is `constexpr` functions calling `assert(...)`.

Failure can be tested with:

```
git show HEAD~|git apply --reverse # First checkout this pull, then revert the centos 9 bump commit
podman kill ci_i686_centos && MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_i686_centos.sh" ./ci/test_run_all.sh # Run the CI task
```

```
...
qt/addresstablemodel.cpp: In function ‘constexpr AddressTableEntry::Type translateTransactionType(wallet::AddressPurpo
...

💬 GregTonoski commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548052614)
> So, probably the `importdescriptors` RPC command call was done with `timestamp=now` and no rescan was performed.

I confirm that there was the `importdescriptors` RPC command call done with `timestamp=now` and no rescan was performed.

💬 kroese commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548055212)
For a script it will be hard to parse the directory names for a Github folder reliably with only `curl` calls.

It was so much easier if there was a text-file containing a list of names, so that I could just download that and loop over that line by line. In any case I made an issue in the `guix.sigs` repo about this, because this is a bit off-topic here. Thanks.

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1548057302)
I was getting some sybil-stalls at <=10 limit, so for now uncapped it.

I also added in logic to ensure that at least one of the 3 slots is taken by an outbound peer. Added more extensive tests.

💬 MarcoFalke commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548061754)
Not sure what your goal is, but if you want to blindly download all keys, you can also use `git` to clone the whole folder https://github.com/bitcoin-core/guix.sigs/tree/main/builder-keys and then just import each file into gpg?

💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1548091322)
> I'd be happy to have an alternative way to query the actual, unfiltered numbers per network

I think that would be useful, too, perhaps by passing an argument to getnodeaddresses and -adidrinfo, and/or returning both pre and post filter in -addrinfo.

💬 kroese commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1548097518)
Yes, but I do this from a script and I wanted to avoid having to install `git` just to quickly verify a binary release.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193811410)
In PR version 2f9c2d245360b3fad6d469a76c2916d75b027b57:

This is a notifications interface, so I don't think these methods should be `= 0` pure virtual. If you defined them to do nothing by default `{}` that should make the notification interface easier to use because you don't have to implement handlers for notifications you do not care about. Also it can make the interface more future proof because adding new notification types will not break existing code.

🤔 ryanofsky reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1426519811)
Light code review ACK 2f9c2d245360b3fad6d469a76c2916d75b027b57. Overall looks good, but I mostly left high level feedback and did not look closely at all the code yet. I think the suggestions I left would make this cleaner, but it already looks pretty good in it's current form.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193855223)
In commit "refactor: Pass BlockNotify in options to ChainstateManager" (620faa21ce0210dddca243511b7674c3b8f4e901)

Would suggest `ValidationNotifications& notifications` or `ValdiataionNotifications* notifcations{nullptr}` here, and adding a `std::unique_ptr<node::ValidationNotificationsImpl>` member to `NodeContext`. I don't think this should be a shared pointer and also don't think it should be const.

On const: This is an options struct that is supposed to be filled with options set by ex
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193951680)
In commit "refactor: Pass DoWarning in options to ChainstateManager" (bcd417ad1847cd4c93bffbc9d8a32255e7f2b156)

I think "notify warning" would be more descriptive than "do warning" and more consistent with other methods.

Also commit title "Pass DoWarning in options to ChainstateManager" maybe should be updated to something like "Add NotifyWarning kernel notification"

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193904693)
In commit "refactor: Pass BlockNotify in options to ChainstateManager" (620faa21ce0210dddca243511b7674c3b8f4e901)

Note: After this commit, there is only one line of code sending the `uiInterface.NotifyBlockTip` global boost signal, so it should be easier to drop the signal, probably replacing it with a `std::list<NotifyHeaderTipFn>` or similar member. Other boost signals could be eliminated after this in a similar way.