💬 russeree commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1647627017)
What the file copy method solves in a way is the fact that when you copy and move that data you will **likely** get new sectors and as such if you have a bad secor you have a chance or not hitting it or at least hitting it with the same file.
> Good point. This may actually still fail. I wonder if it makes sense to change that to make `-reindex` skip on exceptions instead of shutting down.
I would like to think about this for a bit before commenting further. With that said my first thought
...
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1647627017)
What the file copy method solves in a way is the fact that when you copy and move that data you will **likely** get new sectors and as such if you have a bad secor you have a chance or not hitting it or at least hitting it with the same file.
> Good point. This may actually still fail. I wonder if it makes sense to change that to make `-reindex` skip on exceptions instead of shutting down.
I would like to think about this for a bit before commenting further. With that said my first thought
...
💬 hebasto commented on pull request "Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647646239)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647646239)
Concept ACK.
💬 MarcoFalke commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1647647245)
> I would like to think about this for a bit before commenting further. With that said my first thought is that having a good solution to this even if it is as simple as very clear log level notifications would be a massive benefit to node runners while debugging failed disks or disk access requests from the OS.
Right. It may be better to adjust the error message to say that this is either a critical bug in the software, or a hardware issue. `-reindex` in both cases won't solve the underlying
...
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1647647245)
> I would like to think about this for a bit before commenting further. With that said my first thought is that having a good solution to this even if it is as simple as very clear log level notifications would be a massive benefit to node runners while debugging failed disks or disk access requests from the OS.
Right. It may be better to adjust the error message to say that this is either a critical bug in the software, or a hardware issue. `-reindex` in both cases won't solve the underlying
...
💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1647650203)
updated the PR to include better docs inspired from https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1269601639. left "RPC is for testing only" as it is since all hidden RPCs have that line in the help. `getaddrmaninfo`'s help looks like this now:
```
Provides high-level information about the node's address manager by returning the number of addresses in the `new` and `tried` tables and their sum for all networks.
This RPC is for testing only.
Result:
{ (json
...
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1647650203)
updated the PR to include better docs inspired from https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1269601639. left "RPC is for testing only" as it is since all hidden RPCs have that line in the help. `getaddrmaninfo`'s help looks like this now:
```
Provides high-level information about the node's address manager by returning the number of addresses in the `new` and `tried` tables and their sum for all networks.
This RPC is for testing only.
Result:
{ (json
...
💬 hebasto commented on pull request "refactor: Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647773124)
> Why casting in the first place since [`memcpy` would reinterpret it as `unsigned char *`](https://en.cppreference.com/w/cpp/string/byte/memcpy) anyways?
Is this unused as well: https://github.com/bitcoin/bitcoin/blob/d23fda05842ba4539b225bbab01b94df0060f697/src/key.cpp#L328 ?
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647773124)
> Why casting in the first place since [`memcpy` would reinterpret it as `unsigned char *`](https://en.cppreference.com/w/cpp/string/byte/memcpy) anyways?
Is this unused as well: https://github.com/bitcoin/bitcoin/blob/d23fda05842ba4539b225bbab01b94df0060f697/src/key.cpp#L328 ?
💬 MarcoFalke commented on pull request "refactor: Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647787075)
> Is this unused as well:
No? `begin()` returns a `const` iterator on the mutable `keyChild`. The cast is needed to remove the wrongly added `const`. As said previously, this is out-of-scope for this pull request: https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647295139
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647787075)
> Is this unused as well:
No? `begin()` returns a `const` iterator on the mutable `keyChild`. The cast is needed to remove the wrongly added `const`. As said previously, this is out-of-scope for this pull request: https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647295139
🤔 stickies-v reviewed a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1543209382)
Approach ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240
I think the `rpc/util` approach is much better than before. Would prefer seeing the `ParseSighash` signature simplified (and renamed), but not necessarily a blocker.
> Should throwing RPC_INVALID_PARAMETER be mentioned in the release notes?
The only behaviour change I can see here is the return code changing from -1 (RPC_MISC_ERROR) to -8 (RPC_INVALID_PARAMETER), the latter being strictly more helpful and I don't think this is a back
...
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1543209382)
Approach ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240
I think the `rpc/util` approach is much better than before. Would prefer seeing the `ParseSighash` signature simplified (and renamed), but not necessarily a blocker.
> Should throwing RPC_INVALID_PARAMETER be mentioned in the release notes?
The only behaviour change I can see here is the return code changing from -1 (RPC_MISC_ERROR) to -8 (RPC_INVALID_PARAMETER), the latter being strictly more helpful and I don't think this is a back
...
💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272088375)
nit: A bit confusing that `ParseSighashString` parses a `UniValue`, and `ParseSighash` parses a `string`. Naming other way around would be more logical imo? Larger diff but perhaps worth it?
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272088375)
nit: A bit confusing that `ParseSighashString` parses a `UniValue`, and `ParseSighash` parses a `string`. Naming other way around would be more logical imo? Larger diff but perhaps worth it?
💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272097256)
nit: Also, perhaps a docstring to document throwing behaviour and reference `ParseSighash`? (would be okay with an unstructured one-liner too)
```
/**
* Parse a sighash string representation
*
* @throws RPC_INVALID_PARAMETER if `sighash` is null or does not represent a valid sighash type.
*
* @see util::Result<int> ParseSighash(const std::optional<std::string>& sighash) for a less
* opinionated alternative
*/
```
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272097256)
nit: Also, perhaps a docstring to document throwing behaviour and reference `ParseSighash`? (would be okay with an unstructured one-liner too)
```
/**
* Parse a sighash string representation
*
* @throws RPC_INVALID_PARAMETER if `sighash` is null or does not represent a valid sighash type.
*
* @see util::Result<int> ParseSighash(const std::optional<std::string>& sighash) for a less
* opinionated alternative
*/
```
💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272119307)
Making `sighash` optional seems quite verbose and unintuitive for just the one callsite that needs the default sighash. It also makes the function behave quite ambiguously (imo), returning the default if no sighash is provided, but throwing if the sighash is not recognized. Would leave that logic to the callsite?
<details>
<summary>git diff on a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240</summary>
```diff
diff --git a/src/core_io.h b/src/core_io.h
index 551d09e021..b61ce10777 100644
--- a/
...
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272119307)
Making `sighash` optional seems quite verbose and unintuitive for just the one callsite that needs the default sighash. It also makes the function behave quite ambiguously (imo), returning the default if no sighash is provided, but throwing if the sighash is not recognized. Would leave that logic to the callsite?
<details>
<summary>git diff on a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240</summary>
```diff
diff --git a/src/core_io.h b/src/core_io.h
index 551d09e021..b61ce10777 100644
--- a/
...
👍 hebasto approved a pull request: "refactor: Remove C-style const-violating cast, Use reinterpret_cast"
(https://github.com/bitcoin/bitcoin/pull/28127#pullrequestreview-1543383880)
ACK fa474bce4a9d47be04a03a3e36f25c5118eaf1aa, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/28127#pullrequestreview-1543383880)
ACK fa474bce4a9d47be04a03a3e36f25c5118eaf1aa, I have reviewed the code and it looks OK.
💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272207260)
The optional behavior was requested by @hebasto to isolate the sighash-specific logic from the univalue-specific logic. Could just add a docstring instead to the `ParseSighash` function?
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272207260)
The optional behavior was requested by @hebasto to isolate the sighash-specific logic from the univalue-specific logic. Could just add a docstring instead to the `ParseSighash` function?
👍 darosior approved a pull request: "refactor: Remove C-style const-violating cast, Use reinterpret_cast"
(https://github.com/bitcoin/bitcoin/pull/28127#pullrequestreview-1543402319)
utACK fa474bce4a9d47be04a03a3e36f25c5118eaf1aa. This makes sense and looks good to me, though i'm far from a language lawyer.
nit: the first two commits share the same objective and title and could thus be squashed.
(https://github.com/bitcoin/bitcoin/pull/28127#pullrequestreview-1543402319)
utACK fa474bce4a9d47be04a03a3e36f25c5118eaf1aa. This makes sense and looks good to me, though i'm far from a language lawyer.
nit: the first two commits share the same objective and title and could thus be squashed.
💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272218244)
I think that comment was from when we still had the univalue helpers?
Now, we have a string parsing function (in `core_read.cpp`) and an rpc helper function (in `rpc/util.cpp`). The string parsing, imo, should do just that, and the rpc helper can inject rpc-specific logic, being that 1) we like using UniValues in the RPC and that 2) for all rpcs, we prefer to interpret missing sighash as SIGHASH_DEFAULT. I don't think it's ideal to make the string parsing opinionated.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272218244)
I think that comment was from when we still had the univalue helpers?
Now, we have a string parsing function (in `core_read.cpp`) and an rpc helper function (in `rpc/util.cpp`). The string parsing, imo, should do just that, and the rpc helper can inject rpc-specific logic, being that 1) we like using UniValues in the RPC and that 2) for all rpcs, we prefer to interpret missing sighash as SIGHASH_DEFAULT. I don't think it's ideal to make the string parsing opinionated.
💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272238862)
Btw if the main concern is with `#include <script/interpreter.h>` in `rpc.util.cpp`, can avoid that by just calling `ParseSighash("DEFAULT")` too.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272238862)
Btw if the main concern is with `#include <script/interpreter.h>` in `rpc.util.cpp`, can avoid that by just calling `ParseSighash("DEFAULT")` too.
📝 MarcoFalke opened a pull request: " ci: Keep system env vars as-is (bugfix)"
(https://github.com/bitcoin/bitcoin/pull/28138)
This fixes a bug where the `$PATH` from the host is used inside the container. This will lead to bugs when the `$PATH` is different. For example on a host of Fedora 38, and a container of `debian:bullseye`.
This can be tested with the `FILE_ENV=./ci/test/00_setup_env_arm.sh` CI env. On master:
```
Error: crun: executable file `bash` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found
```
On this pull:
(everything passes)
(https://github.com/bitcoin/bitcoin/pull/28138)
This fixes a bug where the `$PATH` from the host is used inside the container. This will lead to bugs when the `$PATH` is different. For example on a host of Fedora 38, and a container of `debian:bullseye`.
This can be tested with the `FILE_ENV=./ci/test/00_setup_env_arm.sh` CI env. On master:
```
Error: crun: executable file `bash` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found
```
On this pull:
(everything passes)
📝 MarcoFalke converted_to_draft a pull request: "ci: Start with clean env"
(https://github.com/bitcoin/bitcoin/pull/27976)
Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1564529747
(https://github.com/bitcoin/bitcoin/pull/27976)
Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1564529747
💬 hebasto commented on pull request "refactor: Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647896184)
As a follow up, I'm pretty sure that all casts of `ArgsManager::ParseParameters`'s arguments in `test/argsman_tests.cpp` can be just dropped as well.
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647896184)
As a follow up, I'm pretty sure that all casts of `ArgsManager::ParseParameters`'s arguments in `test/argsman_tests.cpp` can be just dropped as well.
📝 furszy opened a pull request: "test: create wallet specific for test_locked_wallet case"
(https://github.com/bitcoin/bitcoin/pull/28139)
Coming from https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265478128.
Several test cases are relying on the node1 default wallet, which thanks to 'test_locked_wallet' is encrypted.
And can be only accessed within a specific timeframe (100ms), a duration internally set by the same test.
This situation introduces a potential race condition, where other tests must complete their operations within
the specified 100ms window to pass (otherwise the wallet gets re-locked and they fa
...
(https://github.com/bitcoin/bitcoin/pull/28139)
Coming from https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265478128.
Several test cases are relying on the node1 default wallet, which thanks to 'test_locked_wallet' is encrypted.
And can be only accessed within a specific timeframe (100ms), a duration internally set by the same test.
This situation introduces a potential race condition, where other tests must complete their operations within
the specified 100ms window to pass (otherwise the wallet gets re-locked and they fa
...
💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1647982643)
@ajtowns @mzumsande Thanks, so it appears there are actually two mostly-unrelated network buffer deadlock issues, and Martin's test is likely triggering both of them.
I agree with AJ that we should still fix the network side one, even if we can't (or don't want to address) the application buffer side one. Fixing the application buffer side one indeed seems a lot harder, and probably needs discussion beyond this PR.
It would be good to have a test for the network side one, without it also t
...
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1647982643)
@ajtowns @mzumsande Thanks, so it appears there are actually two mostly-unrelated network buffer deadlock issues, and Martin's test is likely triggering both of them.
I agree with AJ that we should still fix the network side one, even if we can't (or don't want to address) the application buffer side one. Fixing the application buffer side one indeed seems a lot harder, and probably needs discussion beyond this PR.
It would be good to have a test for the network side one, without it also t
...