💬 TheCharlatan commented on pull request "fuzz: Handle missing BDBRO errors":
(https://github.com/bitcoin/bitcoin/pull/30172#discussion_r1613938921)
Is this still used?

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613943540)
Ok, yeah, agreed.

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614026697)
I prefer the documentation in https://github.com/jonatack/bitcoin/commit/610397665795b702e380f879db43a000ed81aad3 because it makes clear that for the CLI a different method is needed (`-rpcwallet`).

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1614029973)
Just adding for posterity because I went down a rabbit hole for this:

It seems clang's `__is_trivially_equality_comparable` builtin would do what we want here:
> Returns true if comparing two objects of the provided type is known to be equivalent to comparing their object representations. Note that types containing padding bytes are never trivially equality comparable.

Not that it's worth using here.

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614031474)
nit, here and lines 1141 and 1149, can drop the trailing `.` as the other CLI error messages don't have one

```suggestion
throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s", command));
```

🤔 TheCharlatan reviewed a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2077784982)
Nice change, just left some cosmetic nits.
ACK 51cb837e0a392b96661e00f6e502fbe6458a4df5

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

💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1614015484)
^ cc @hebasto

💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1613955561)
Nitty nit: Trailing whitespace. There are some other small formatting issues elsewhere too, maybe run `clang-format-diff` once over the commits?

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