💬 hebasto commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1647528259)
ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240.
Should throwing `RPC_INVALID_PARAMETER` be mentioned in the release notes?
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1647528259)
ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240.
Should throwing `RPC_INVALID_PARAMETER` be mentioned in the release notes?
💬 recursive-rat4 commented on pull request "Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1647569661)
I queried the explorer like this
```
~ $ curl -s https://mempool.space/api/v1/block/0000000000000000000153159d7b95debfb0dadcd1040aaf9dbeb0025a1ddeac/audit-summary | jq .fullrbfTxs
[
"53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643",
"5bc64344c56f847e2d992fab241567075473eec9423776afadc187299352bce1",
"64ec51dedd1404a775d590f530a01bbdc4239c8eb6d33ce4b9217ec2f0b8ddae"
]
```
Only 4 of 6 mentioned transactions seem to be full-rbf, or was it a wrong request?
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1647569661)
I queried the explorer like this
```
~ $ curl -s https://mempool.space/api/v1/block/0000000000000000000153159d7b95debfb0dadcd1040aaf9dbeb0025a1ddeac/audit-summary | jq .fullrbfTxs
[
"53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643",
"5bc64344c56f847e2d992fab241567075473eec9423776afadc187299352bce1",
"64ec51dedd1404a775d590f530a01bbdc4239c8eb6d33ce4b9217ec2f0b8ddae"
]
```
Only 4 of 6 mentioned transactions seem to be full-rbf, or was it a wrong request?
💬 russeree commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1272019533)
This compiles and looks great even for the existing errors! Modifying this does cause rpc_misc.py to fail test runner. This is quite nice.
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1272019533)
This compiles and looks great even for the existing errors! Modifying this does cause rpc_misc.py to fail test runner. This is quite nice.
💬 darosior commented on pull request "Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#discussion_r1272021098)
Why casting in the first place since [`memcpy` would reinterpret it as `unsigned char *`](https://en.cppreference.com/w/cpp/string/byte/memcpy) anyways?
(https://github.com/bitcoin/bitcoin/pull/28127#discussion_r1272021098)
Why casting in the first place since [`memcpy` would reinterpret it as `unsigned char *`](https://en.cppreference.com/w/cpp/string/byte/memcpy) anyways?
💬 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)