🤔 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
(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.
(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
(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?
(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.
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1613994552)
Nit: This include could be dropped again.
🥰1
🤔 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?
(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!
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1614066880)
Interesting!
⚠️ dewy800 opened an issue: "Dewyboy "
(https://github.com/bitcoin/bitcoin/issues/30173)
(https://github.com/bitcoin/bitcoin/issues/30173)
✅ dewy800 closed an issue: "Dewyboy "
(https://github.com/bitcoin/bitcoin/issues/30173)
(https://github.com/bitcoin/bitcoin/issues/30173)
:lock: fanquake locked an issue: "Dewyboy "
(https://github.com/bitcoin/bitcoin/issues/30173)
(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
...
(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++.
(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
...
(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.
(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.
(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.
(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
...
(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?
(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?