💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2194112241)
Rearranging the order of the commits to make it easier to test the bugfix: the first test (53f714d8bfd2331651445bcadb773a10f4d30264) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix (65343ec49a6b73c4197dfc38e1c2f433b0a3838a).
I also updated the PR title and description to reflect that this PR is not a test-only PR anymore
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2194112241)
Rearranging the order of the commits to make it easier to test the bugfix: the first test (53f714d8bfd2331651445bcadb773a10f4d30264) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix (65343ec49a6b73c4197dfc38e1c2f433b0a3838a).
I also updated the PR title and description to reflect that this PR is not a test-only PR anymore
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768153)
Moved in 9eff5601059
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768153)
Moved in 9eff5601059
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768252)
Added in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768252)
Added in 49d5bfdd7e7
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768303)
Thanks, updated in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768303)
Thanks, updated in 49d5bfdd7e7
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768365)
Oh no! fixed in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768365)
Oh no! fixed in 49d5bfdd7e7
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768428)
I agree on reflection, gave them new names in 49d5bfdd7e7
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768428)
I agree on reflection, gave them new names in 49d5bfdd7e7
💬 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?