💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1888844067)
CI failure is unrelated, #29234

⚠️ fanquake opened an issue: "rpc: actually deprecate `rpcuser` & `rpcpass`"
(https://github.com/bitcoin/bitcoin/issues/29240)
The logging to output that `Config options rpcuser and rpcpassword will soon be deprecated.` was added ~8 years ago in #7044. However these options still continue to get new usage today, i.e https://github.com/lightningnetwork/lnd/pull/8354#discussion_r1450166947, and it's easy to forget they are considered deprecated, given they aren't under the normal deprecation mechanism/there aren't more verbose warnings.

If we are going to deprecate these options, now seems like a good time to do so. We
...

🤔 glozow reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1817851701)
+1 Could you update the OP with motivation for this PR? I didn't see much from the linked PRs

💬 glozow commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1450185018)
concept ack to having a TestFramework-wide setting to whitelist everybody for immediate tx relay. e5593fc9bf48b27227786e7f4145b58c849367a3 could be its own PR, as I don't think there are any instances where we are specifically making outbound connections and need immediate tx relay.

💬 glozow commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1450191936)
43614707c4fe9ce91bb90b959574ee4dbf6f7b05

Why is the approach to add "in" and "out" instead of just having specified permissions apply to both? Are there instances where you want to specify different permissions for the same address depending on the connection direction?

✅ fanquake closed an issue: "`./configure` fails for clang-14 on Ubuntu 23.10"
(https://github.com/bitcoin/bitcoin/issues/29161)

💬 fanquake commented on issue "`./configure` fails for clang-14 on Ubuntu 23.10":
(https://github.com/bitcoin/bitcoin/issues/29161#issuecomment-1888920640)
Going to close this as wontfix. If someone is using an operating system that is modern enough to be shipping GCC 13.2 and Clang 16 as the deafult compilers, then I'd suggest using either of those as their compiler. Also given that this is a bug in Clang, but only when coupled with a certain version of libstdc++, it's not easy for us to "fix". An alternative is to try with libc++. For older systems, clang-14 remains supported (for now).

🤔 glozow reviewed a pull request: "doc: refer to "Node relay options" in policy/README"
(https://github.com/bitcoin/bitcoin/pull/29235#pullrequestreview-1818040779)
lgtm ACK 0d627c4ca8684653ddef3bb4041ad1e129ed3d4d

✅ fanquake closed an issue: "`security-check.py` produces false positive result for stack smashing protection"
(https://github.com/bitcoin/bitcoin/issues/29084)

💬 fanquake commented on issue "`security-check.py` produces false positive result for stack smashing protection":
(https://github.com/bitcoin/bitcoin/issues/29084#issuecomment-1888967871)
The claim here is that if you change `-fstack-protector-all` to `-fno-stack-protector` in our compile flags, then `check_ELF_Canary` in `security-check.py` should fail during a Guix build.

However that wont happen, because the check looks for the presence of `__stack_chk_fail`, and that will still be in the binary, because libs in depends are compiled with the on-by-default stack-protector flags. This can easily be checked after Guix building with the diff above:
```bash
CXXFLAGS =
...

💬 Eunovo commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1888973157)
ACK [7f0be89](https://github.com/bitcoin/bitcoin/pull/29156/commits/7f0be8969e721de69fc352f76db5787836803b76)

Adding an example for this to descriptors.md is really helpful but I think users who are unfamiliar with miniscript and descriptors might benefit from the addition of the fragments, `thresh` and `after` to the `Reference` section. Seeing these fragments used in the examples but not included the reference can introduce confusion.

🚀 glozow merged a pull request: "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/28885)

🚀 glozow merged a pull request: "doc: refer to "Node relay options" in policy/README"
(https://github.com/bitcoin/bitcoin/pull/29235)

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1889089160)
Updated ffc1e9fd8cf53cfcf4d7926ca2963958eacf73d7 -> 3c311734d2fc6a4ca410f254ba3c8e923d58be70 ([`pr/iparams.3`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.3) -> [`pr/iparams.4`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.3..pr/iparams.4)) to fix unused variable in test https://cirrus-ci.com/task/4527517751574528?logs=ci#L2260

re: https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1888552974
...

🤔 shaavan reviewed a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1818189422)
Thanks for the update, @furszy
The new error message and the settings.json warnings are very precise in their details and I believe will be better in guiding the user in case of error, and refraining them from modifying settings without being cautious.

I just have one suggestion I want to add.

💬 shaavan commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1450404094)
Regarding commit 634be7d206b31d34b36de7e62bd216dbbc360195:

The commit message states:

> "...file unless they are certain about the potential consequences."

While this addition is valuable, the subsequent update only advises users against modifying the file while the node is running.
Considering this, would it be beneficial to think about revising the commit message to more effectively caution users before they adjust the settings? This would enhance clarity and help users make informed
...

💬 ryanofsky commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450411706)
IMO, being able to filter log messages and only see messages from a particular component is a useful thing to support. Current bitcoin behavior of defining categories and not actually using them is weird, and wanting to bake this behavior into the logging API seems even weirder. The python logger is very flexible and of course it allows you to log per-component or globally. But if you did both these things in a single python project, it would be confusing and weird there too.

🤔 furszy reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818093579)
Code reviewed, looking good. Left few comments, mostly testing time improvements. Nothing blocking.

As the scenario being covered does not require verifying p2p mempool relay, you could speed up the new `wallet_rescan_unconfirmed.py` test by ~2.5x (tested locally, ~3 secs versus ~8 secs) by using a single node instead of creating three of them. See commit f8273512 (feel free to pull it).

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450354940)
in https://github.com/bitcoin/bitcoin/commit/c39e82926a3740642f8f1bb72c6b10b67a6dc724:

nit: `disable_private_keys` is false by default. And, because this is a descriptors-only test (per `self.add_wallet_options`), the `descriptors` flag is also redundant.

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450376850)
In c39e82926a3:

Can simplify this by only importing the used address descriptor.
```suggestion
descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0, "label": "w0 import"}]
```