💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1614030354)
I don't think this is needed.

💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1614029612)
I don't think this is needed? Maybe these patches would be a good opportunity to cleanup the includes of this module.

💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1613994552)
Nit: This include could be dropped again.

🤔 jonatack reviewed a pull request: "cli: improve error message on multiwallet and add validation to cli-side commands"
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-2078072832)
Approach ACK, modulo https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614026697 and the other comment.

Also, "protocol.h" in the first commit message body seems wrong or out-of-date here: `bringing the error message in line with the definition established in protocol.h ("error when there are multiple wallets loaded")`. I'm not sure, but did you mean "src/wallet/rpc/util.cpp" instead?

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1614066880)
Interesting!

⚠️ dewy800 opened an issue: "Dewyboy "
(https://github.com/bitcoin/bitcoin/issues/30173)

✅ dewy800 closed an issue: "Dewyboy "
(https://github.com/bitcoin/bitcoin/issues/30173)

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

💬 lcharles123 commented on issue ""netinfo" doesn't show IPv6 "Local addresses"":
(https://github.com/bitcoin/bitcoin/issues/30165#issuecomment-2130694823)
> Edit: probably not the issue, but did you try using the -discover config option?

Yes, but the addresses are unreachable anyway in local network, because the node are IPv4 only (and behind NAT).

I guess I found the cause, because function `AddLocal` checks for connectivity on each -externalip entry using local interfaces, for IPv4 and others, its easy because it takes the normal way to the address. But there is no solution to reach v6 addresses on a machine v4 only without some config and
...

💬 maflcko commented on pull request "fuzz: Fix wallet_bdb_parser stdlib error matching":
(https://github.com/bitcoin/bitcoin/pull/30169#issuecomment-2131027363)
> Was this caught by review or did you run into it? Just curious if there are others.

It can be tested by reverting this commit and running the OSS-Fuzz config (uses libc++), or the `ci_native_fuzz_msan` CI config (also uses libc++), or any other way that uses this fuzz target with libc++.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2131092539)
> The first two commits of https://github.com/bitcoin/bitcoin/pull/26812 would make it possible to test and fuzz how this code interacts with a router.

So ive been thinking about this, do we have a mockable replacement for `Sock socket(int domain, int type, int protocol)`? i was thinking of passing in a `SocketFactory` (please don't kill me, this could be just a functor 😅 ). It's slightly nicer than passing in a pre-made `Sock` because it allows testing the "create a socket of the right fami
...

📝 maflcko opened a pull request: "test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure"
(https://github.com/bitcoin/bitcoin/pull/30174)
Otherwise, the test may fail on slow hardware when running in valgrind.

Also, use named args for the absolute timepoint, while touching this file.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2131136559)
Addressed @vasild comments (thanks!) and rebased.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1614481665)
I decided against this. You don't strictly need to be root (https://hackmd.io/@willcl-ark/r19LVO2_6 or https://github.com/bitcoin/bitcoin/pull/24358#issuecomment-1083149220). Checking for getuid == 0 would require you to run as root.

💬 0xB10C commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2131226810)
> Concept NACK, consumers should just check if features are available

The [current recommendation](https://github.com/bitcoin/bitcoin/blob/42d5a1ff25a8045b6f4c09fa1fb71736dbccc034/doc/JSON-RPC-interface.md?plain=1#L69-L70) is already to use the `version` field from the `getnetworkinfo` RPC call. This PR doesn't really add anything new that wasn't already exposed and recommend for RPC clients to use.

---

Throwing this in for even more confusion about this new RPC and the software vs RPC
...

💬 king-11 commented on issue ""Rolling forward" at startup can take a long time, and is not interruptible":
(https://github.com/bitcoin/bitcoin/issues/11600#issuecomment-2131241170)
I have been encountering this myself the `bitcoind` runs inside of a docker container and when its asked to stop and doesn't respond to `SIGTERM` the docker after a timeout uses `SIGKILL` resulting in exit code of `137` being unclear stop. Can we attach listener for all the signals somewhere?

💬 epiccurious commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1614633882)
nit: this sentence might be better structured starting with a verb:

```
Implement associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying RPC interface JSON structure, including (but not limited to) the following:
```

💬 epiccurious commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1614634416)
nit: this sentence might be better structured starting with a verb:

```
Prefer to choose RPC JSON data types that are flexible to expansion without change of data type.
```

💬 epiccurious commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2131241674)
ACK 61853a25bbbdb6d5dfb2f1570e41f541a4b7de78.

⚠️ sipa opened an issue: "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets"
(https://github.com/bitcoin/bitcoin/issues/30175)
### Please describe the feature you'd like to see added.

Descriptor wallets currently do not support the "legacy" import commands `importprivkey`, `importpubkey`, `importaddress`, `addmultisigaddress`, `importmulti`, and `importwallet`. This was done because the semantics of these RPC cannot be replicated in descriptor wallets, as they just fundamentally work differently (being explicit about what to watch rather than "whatever matches the bag of keys and scripts we have").

I want to suggest
...