💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768473)
I've un-removed the comment in 9eff5601059, as I agree with @ryanofsky that not changing the existant behaviour is the "safer" default, and new behaviour can be introduced with the new option.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768473)
I've un-removed the comment in 9eff5601059, as I agree with @ryanofsky that not changing the existant behaviour is the "safer" default, and new behaviour can be introduced with the new option.
👍 rkrux approved a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2144759617)
tACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)
`make, test/functional` are successful.
Thanks for testing the filtering by node functionality, and replacing the comments with info/debug logs as suggested by @tdb3.
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2144759617)
tACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)
`make, test/functional` are successful.
Thanks for testing the filtering by node functionality, and replacing the comments with info/debug logs as suggested by @tdb3.
💬 alfonsoromanz commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2194199411)
Re-ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2194199411)
Re-ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
🤔 alfonsoromanz reviewed a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2144886875)
tACK 6809ffd8a6c589c515af48da2dd8367e4c6c2c26. I ran `test/functional/feature_coinstatsindex.py`, and the test passes.
+1 to informing the log as @tdb3 suggests.
You can now update your PR description to remove the note about the function reordering and the node restart.
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2144886875)
tACK 6809ffd8a6c589c515af48da2dd8367e4c6c2c26. I ran `test/functional/feature_coinstatsindex.py`, and the test passes.
+1 to informing the log as @tdb3 suggests.
You can now update your PR description to remove the note about the function reordering and the node restart.
💬 maflcko commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1656887109)
Yeah, seems fine to remove the recursion, given that real code probably doesn't evaluate the hash recursively.
Unrelated, I don't think `uint256` is generally guaranteed to be `uint64_t`-aligned. While probably not a problem here, the cast won't work in the general case:
```
bench/crypto_hash.cpp: runtime error: store to misaligned address 0x7ffe64f6caa1 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0x7ffe64f6caa1: note: pointer points here
00 00 00 00 00
...
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1656887109)
Yeah, seems fine to remove the recursion, given that real code probably doesn't evaluate the hash recursively.
Unrelated, I don't think `uint256` is generally guaranteed to be `uint64_t`-aligned. While probably not a problem here, the cast won't work in the general case:
```
bench/crypto_hash.cpp: runtime error: store to misaligned address 0x7ffe64f6caa1 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0x7ffe64f6caa1: note: pointer points here
00 00 00 00 00
...
💬 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