💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1446435204)
reworded

💬 achow101 commented on pull request "fuzz: set `nMaxOutboundLimit` in connman target":
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1883547121)
New compiler warning, using gcc 13.2.1

```
test/fuzz/connman.cpp: In function ‘void connman_fuzz_target(FuzzBufferType)’:
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vSeedNodes’ [-Werror=missing-field-initializers]
43 | connman.Init({ .nMaxOutboundLimit = max_outbound_limit });
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vWhite
...

💬 brunoerg commented on pull request "fuzz: set `nMaxOutboundLimit` in connman target":
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1883552043)
> New compiler warning, using gcc 13.2.1

I'm checking it atm.

📝 Kaikookpk12 opened a pull request: "Kaikookpk12 patch 1"
(https://github.com/bitcoin/bitcoin/pull/29210)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "Kaikookpk12 patch 1"
(https://github.com/bitcoin/bitcoin/pull/29210)

📝 fanquake locked a pull request: "Kaikookpk12 patch 1"
(https://github.com/bitcoin/bitcoin/pull/29210)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446448724)
I think UniValue type interpretation would be fine.

📝 brunoerg opened a pull request: "fuzz: fix `connman` initialization"
(https://github.com/bitcoin/bitcoin/pull/29211)
Fixes https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1883547121

💬 brunoerg commented on pull request "fuzz: fix `connman` initialization":
(https://github.com/bitcoin/bitcoin/pull/29211#issuecomment-1883603293)
cc: @achow101

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1883611431)
https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1855815358 exists, still, as well.

💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1883614535)
[clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)

💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1446491162)
> Given that LogInfo is unconditional, what would be your use case for adding a log category?

Simply that "info = unconditional" might not always be the case (and is an unusual convention to have hardcoded), and a user could rightly want to see only WARNING+ logs as well as, say, `net` or validation-related INFOs.

💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445260009)
In commit "desc spkm: Add functions to retrieve specific private keys" (ebd02d73241c0922f486cdf14719281c079b45c7)

Maybe add a comment saying it returns nullopt if the key doesn't exist, or can't be decrypted because the wallet is locked, or because there's decryption error. Otherwise it's not clear what the function is assuming or when it returns nullopt.

💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445262239)
In commit "wallet, rpc: Add gethdkeys RPC" (cbe990b1a95e6331106c3ce6d1f2abd860f88ea8)

I think this example is not right anymore since the parameter needs to be named. There seems to be a HelpExampleCliNamed function for this.

👍 ryanofsky approved a pull request: "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1809920961)
Code review ACK c10150b6150083440af4f0aa1110c8aa99ba2dc8. Left a lot of suggestions, but none are important. Overall this looks great and is very cleanly implemented.

💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446462443)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)

Would probably be good to change this to an assert. Otherwise it could be called in legacy code and seem to succeed but never return data.

💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446457022)
In commit "wallet: Refactor function for single DescSPKM setup" (8d107d5f48c2fae0d09bf9be79f9cf47daad6241)

Would be good for this to return a reference instead of a pointer. It seems to never return null, always return non-null or throw an exception.

💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446468708)
In commit "wallet: Be able to retrieve single key from descriptors" (8bcb89d3730d30a3202fcee172fd709a9f588a4e)

Again I think this would be a little better as an assert so if this is called in legacy wallet code it doesn't appear to succeed but never return anything.

💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445380623)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)

Should delete this comment if it just applies to calling code (it doesn't seem to be enforced here).

💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446464920)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)

std::move would be appropriate here