💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613925002)
"Objects of trivially-copyable types that are not potentially-overlapping subobjects are the only C++ objects that may be safely copied with [std::memcpy](https://en.cppreference.com/w/cpp/string/byte/memcpy) or serialized to/from binary files with [std::ofstream::write()](https://en.cppreference.com/w/cpp/io/basic_ostream/write) / [std::ifstream::read()](https://en.cppreference.com/w/cpp/io/basic_istream/read)."

I assume anything that can be memcpy'd can be memcmp'd.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613941027)
I certainly see why it's reasonable why trivial types would have a trivial comparison operator, but there is no guarantee for that. I think you can have a trivially-constructible type with an complex `operator==` (for such a type you'd expect that something that was memcpy'd you end up with a result that satisfies ==, but I don't think that's required, and it also doesn't work the other way around: objects could satisfy == without being bitwise identical).

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