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

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

I don't see the point of having one line ChainstateManager wrapper methods for all the notifications. It seems like they just make the call sequence harder to follow, make code hard coder to grep, and create name conflicts with other functions that have to be overcome with `::` specifiers and the `FatalErrorf` renaming.

The one benefit of these wrapper methods is that they redu
...

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

I think this method should just be printing the messages to cout, letting error handling code below clean up after the failure. The chainstate example program shouldn't need to be logging things and calling StartShutdown.

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

Would encourage using modern variable names in new code, especially if adding an API that is supposed to be part of the kernel interface. Specifically would suggest renaming `nProgress` to `progress_percent` or something like that here.

Also since this is a kernel API and we don't know what the application UI might be, maybe calling it "notify progress" might be better than "s
...

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

I think this signature should be cleaned up and not be added to kernel like this. Would suggest:

- Changing `bool` return type to `void`. The return value is never used and the only value that is ever returned is false. Having the return value makes the interface confusing because it's unclear how the value even could be used.
- Renaming `strMessage` parameter to `debug_message
...

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

Behavior isn't changing here, but it seems strange that `fWarned` check is only letting the warning notification to be sent and the GUI progress bar to be updated once over the lifetime of the application. May be something to look into in a followup.

:lock: fanquake locked an issue: "v0.6.5 on iPad mini 6 Security Center button incorrent"
(https://github.com/bitcoin/bitcoin/issues/27654)

:lock: fanquake locked an issue: "Mine bitcoin using FPGA"
(https://github.com/bitcoin/bitcoin/issues/27650)

:lock: fanquake locked an issue: "React-native. const { STATUS_CODES } = require('http'); Unable to resolve module http"
(https://github.com/bitcoin/bitcoin/issues/27644)

:lock: fanquake locked an issue: "Keep it simple"
(https://github.com/bitcoin/bitcoin/issues/27618)

:lock: fanquake locked an issue: "BIP 39 words update"
(https://github.com/bitcoin/bitcoin/issues/27649)

:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/27567)

💬 Xekyo commented on pull request "Switch hardened derivation marker to h":
(https://github.com/bitcoin/bitcoin/pull/26076#discussion_r1194046865)
The “nor” here seems misplaced:

```suggestion
For legacy wallets the `hdkeypath` field in `getaddressinfo`
and the serialization format of wallet dumps remain unchanged. (#26076)
```