💬 willcl-ark commented on issue ""netinfo" doesn't show IPv6 "Local addresses"":
(https://github.com/bitcoin/bitcoin/issues/30165#issuecomment-2194540124)
Presumably in your setup your reverse SSH is connecting via ipv4, therefore bitcoind will detect it has an "active" (localhost) ipv4 connection and decide that its external ip address must be the one you gave it via the `- externalip=143.22.123.17` option.
OP reads to me that you want to have Node (A) bitcoind advertising both ipv4 and (an) ipv6 address(es) which are both reachable on Node (B), so that other nodes can connect to your node (A) via ipv4 or ipv6 on Node (B). Is that correct?
...
(https://github.com/bitcoin/bitcoin/issues/30165#issuecomment-2194540124)
Presumably in your setup your reverse SSH is connecting via ipv4, therefore bitcoind will detect it has an "active" (localhost) ipv4 connection and decide that its external ip address must be the one you gave it via the `- externalip=143.22.123.17` option.
OP reads to me that you want to have Node (A) bitcoind advertising both ipv4 and (an) ipv6 address(es) which are both reachable on Node (B), so that other nodes can connect to your node (A) via ipv4 or ipv6 on Node (B). Is that correct?
...
📝 ryanofsky opened a pull request: "logging: Use LogFatal instead LogError for fatal errors"
(https://github.com/bitcoin/bitcoin/pull/30347)
Use `LogFatal` instead `LogError` for fatal errors.
Keep using `LogError` for nonfatal errors.
Also rename `LogWarning` to `LogCritical` to be clear it is only intended to be used for "severe problems that the node admin should address," as described in the documentation.
This PR is a draft so other ideas and approaches can be discussed.
(https://github.com/bitcoin/bitcoin/pull/30347)
Use `LogFatal` instead `LogError` for fatal errors.
Keep using `LogError` for nonfatal errors.
Also rename `LogWarning` to `LogCritical` to be clear it is only intended to be used for "severe problems that the node admin should address," as described in the documentation.
This PR is a draft so other ideas and approaches can be discussed.
⚠️ ryanofsky opened an issue: "RFC: Misused LogError and LogWarning macros"
(https://github.com/bitcoin/bitcoin/issues/30348)
As a programmer, when I see `LogError` and `LogWarning` macros, I assume `LogError` should be used to log information about a failure that happened, and `LogWarning` should be used to log information about an unusual condition which happened that might indicate a real problem depending on other factors and intent.
But [developer notes](https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md) suggest very different meanings for these macros. They
...
(https://github.com/bitcoin/bitcoin/issues/30348)
As a programmer, when I see `LogError` and `LogWarning` macros, I assume `LogError` should be used to log information about a failure that happened, and `LogWarning` should be used to log information about an unusual condition which happened that might indicate a real problem depending on other factors and intent.
But [developer notes](https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md) suggest very different meanings for these macros. They
...
💬 brunoerg commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2194572754)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2194572754)
Concept ACK
👍 theStack approved a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2145284260)
Code-review ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
Nice approach using a `EncryptedP2PState` subclass for each test scenario, I agree with other reviewers that this looks much cleaner now and is easier to follow.
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2145284260)
Code-review ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
Nice approach using a `EncryptedP2PState` subclass for each test scenario, I agree with other reviewers that this looks much cleaner now and is easier to follow.
💬 theStack commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1657086402)
small nit, potential follow-up: could set 'regtest' as default value for the `net` parameter in the `EncryptedP2PState` class constructor (`__init__`), so it doesn't have to be specified repeatedly here.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1657086402)
small nit, potential follow-up: could set 'regtest' as default value for the `net` parameter in the `EncryptedP2PState` class constructor (`__init__`), so it doesn't have to be specified repeatedly here.
💬 maflcko commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194662748)
> 61 cases where it appeared to be used incorrectly
No opinion about naming, but of the cases that are used incorrectly, I wonder if their surrounding functions are better suited to use `Result` instead of a `void` or `bool` return value. (I presume many of them were a result of the `bool error()` helper transformation/removal, which was the first step toward moving some of those functions toward `Result`)
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194662748)
> 61 cases where it appeared to be used incorrectly
No opinion about naming, but of the cases that are used incorrectly, I wonder if their surrounding functions are better suited to use `Result` instead of a `void` or `bool` return value. (I presume many of them were a result of the `bool error()` helper transformation/removal, which was the first step toward moving some of those functions toward `Result`)
💬 willcl-ark commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490)
I'm largely in agreement with the sentiment here and think the introduction of the `LogFatal` is nice to distiguish fatal errors.
Does
```cpp
LogCritical("pruned datadir may not have more than %d blocks; only checking available blocks\n",
```
...actually require user intervention or other corrective action?
nit: `LogWarning` remains in L728 after this change:
```md
The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for
```
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490)
I'm largely in agreement with the sentiment here and think the introduction of the `LogFatal` is nice to distiguish fatal errors.
Does
```cpp
LogCritical("pruned datadir may not have more than %d blocks; only checking available blocks\n",
```
...actually require user intervention or other corrective action?
nit: `LogWarning` remains in L728 after this change:
```md
The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for
```
💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1657145216)
I suggested `constexpr auto EXPECTED = std::to_array({` above but maybe it doesn't work for you?
[Should support `constexpr` that way under C++20 as far as I can tell](https://en.cppreference.com/w/cpp/container/array/to_array). Are you using some fancy interface to GitHub that doesn't yet show suggestion-embeds?
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1657145216)
I suggested `constexpr auto EXPECTED = std::to_array({` above but maybe it doesn't work for you?
[Should support `constexpr` that way under C++20 as far as I can tell](https://en.cppreference.com/w/cpp/container/array/to_array). Are you using some fancy interface to GitHub that doesn't yet show suggestion-embeds?
👍 tdb3 approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2145363000)
re ACK 4f6b00c94b59c8206c77647c97f4ed188fa99401
Ran rpc_users locally (passed).
Re-ran the manual check in https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2105557076 (same results).
Also sanity checked on a Windows machine running Python 3.9.13 that `platform.system()` does return "Windows".
Left one minor nit.
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2145363000)
re ACK 4f6b00c94b59c8206c77647c97f4ed188fa99401
Ran rpc_users locally (passed).
Re-ran the manual check in https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2105557076 (same results).
Also sanity checked on a Windows machine running Python 3.9.13 that `platform.system()` does return "Windows".
Left one minor nit.
💬 tdb3 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1657131463)
nit: spelling
```diff
- InterpretPermSting
+ InterpretPermString
```
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1657131463)
nit: spelling
```diff
- InterpretPermSting
+ InterpretPermString
```
💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194707816)
Level::Error now meaning that the node needs to be shut down does seem to be a deviation, and perhaps adding Level::Fatal is a good idea if this distinction is needed. Not sure about Level::Critical.
IMO the severity-level logging veered in an unfortunate direction with the unneeded proliferation of Log{Category} macros having hardwired names and an inconsistent API with varying numbers of args and documentation duplicated/moved from `enum class Level` to the developer notes.
All of that w
...
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2194707816)
Level::Error now meaning that the node needs to be shut down does seem to be a deviation, and perhaps adding Level::Fatal is a good idea if this distinction is needed. Not sure about Level::Critical.
IMO the severity-level logging veered in an unfortunate direction with the unneeded proliferation of Log{Category} macros having hardwired names and an inconsistent API with varying numbers of args and documentation duplicated/moved from `enum class Level` to the developer notes.
All of that w
...
💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657166039)
Suggest moving all this developer-facing documentation back to the Level enum class where the levels are defined.
User-facing documentation would be in the logging RPC help.
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657166039)
Suggest moving all this developer-facing documentation back to the Level enum class where the levels are defined.
User-facing documentation would be in the logging RPC help.
💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657169459)
It would be good to shed this proliferation of macros with hardwired names in favor of one single, consistent `Log` one.
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657169459)
It would be good to shed this proliferation of macros with hardwired names in favor of one single, consistent `Log` one.
💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194736217)
Tentative Concept ACK on adding Fatal if the distinction is considered essential, unsure about Critical replacing Warning.
(https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194736217)
Tentative Concept ACK on adding Fatal if the distinction is considered essential, unsure about Critical replacing Warning.
👍 AngusP approved a pull request: "Mining interface followups, reduce cs_main locking, test rpc bug fix"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2145441782)
Code Review ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2145441782)
Code Review ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed
👍 maflcko approved a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2141300770)
Left some nits/questions. Happy to re-ACK if you address them.
ACK f0e5681e97450b696de26ff21e61f961370a9946 📲
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4
...
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2141300770)
Left some nits/questions. Happy to re-ACK if you address them.
ACK f0e5681e97450b696de26ff21e61f961370a9946 📲
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4
...
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1654844892)
nit in 0807afa5c431f34f70dcbdb58de65480216ca800: Unused include?
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1654844892)
nit in 0807afa5c431f34f70dcbdb58de65480216ca800: Unused include?
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657159709)
nit in 818fb4aa837c748cf241f20efc81e1b2df0594e1: Missing GetRand and GetRandDur
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657159709)
nit in 818fb4aa837c748cf241f20efc81e1b2df0594e1: Missing GetRand and GetRandDur
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1656925084)
nit in 6d7f91db5069ddbbaa48c7e3f842ec68f0f717bb:
I see you want to preserve the previous behavior, but I think it makes sense to log this even when `ZEROS` are passed.
This makes it clear that any env-provided seed is ignored for this sub-test.
Also, the diff would be smaller by one line :sweat_smile:
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1656925084)
nit in 6d7f91db5069ddbbaa48c7e3f842ec68f0f717bb:
I see you want to preserve the previous behavior, but I think it makes sense to log this even when `ZEROS` are passed.
This makes it clear that any env-provided seed is ignored for this sub-test.
Also, the diff would be smaller by one line :sweat_smile: