💬 eval-exec commented on issue "Win64 CI failure in feature_versionbits_warning.py":
(https://github.com/bitcoin/bitcoin/issues/28115#issuecomment-1646817279)
> `AssertionError: [node 0] Node returned unexpected exit code (3) vs (0) when stopping`
Hello, I think a clue to solve this issue is to find out where the `exit code (3)` is returned? But I didn't find where the `exit code (3)` is returned in the source code.
What do you think?
(https://github.com/bitcoin/bitcoin/issues/28115#issuecomment-1646817279)
> `AssertionError: [node 0] Node returned unexpected exit code (3) vs (0) when stopping`
Hello, I think a clue to solve this issue is to find out where the `exit code (3)` is returned? But I didn't find where the `exit code (3)` is returned in the source code.
What do you think?
💬 russeree commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271432003)
The above suggestion doesn't compile without syntax changes due to ```error: unterminated argument list invoking macro "STR_INTERNAL_BUG"``` This is because of a missing ```)``` to close off the arguments for STR_INTERNAL_BUG and then there is a loose comma after ```m_opts.oneline_description```. Once these are removed code does compile but the output is not as clean as the current commit due to an extra ```"``` on a new line after the name of the ```oneline_description```
The output of ```ST
...
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271432003)
The above suggestion doesn't compile without syntax changes due to ```error: unterminated argument list invoking macro "STR_INTERNAL_BUG"``` This is because of a missing ```)``` to close off the arguments for STR_INTERNAL_BUG and then there is a loose comma after ```m_opts.oneline_description```. Once these are removed code does compile but the output is not as clean as the current commit due to an extra ```"``` on a new line after the name of the ```oneline_description```
The output of ```ST
...
💬 russeree commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271432874)
The above suggestion doesn't compile without syntax changes due to ```error: unterminated argument list invoking macro "STR_INTERNAL_BUG"``` This is because of a missing ```)``` to close off the arguments for STR_INTERNAL_BUG and then there is a loose comma after ```m_opts.oneline_description```. Once these are removed code does compile but the output is not as clean as the current commit due to an extra ```"``` on a new line after the name of the ```oneline_description```
The output of ```ST
...
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271432874)
The above suggestion doesn't compile without syntax changes due to ```error: unterminated argument list invoking macro "STR_INTERNAL_BUG"``` This is because of a missing ```)``` to close off the arguments for STR_INTERNAL_BUG and then there is a loose comma after ```m_opts.oneline_description```. Once these are removed code does compile but the output is not as clean as the current commit due to an extra ```"``` on a new line after the name of the ```oneline_description```
The output of ```ST
...
💬 russeree commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646824223)
Pretty sure this is the issue from your supplied ```Debug CLI.log"
```2023-07-22T13:42:39Z Syncing coinstatsindex with block chain from height 0```
```2023-07-22T13:42:39Z ERROR: Commit: Failed to commit latest coinstatsindex state```
adding the startup flag -reindex-chainstate should fix this issue. As per your previous concern you will be removing the levelDB indexes which are quite fast to rebuild. Approx ~7GB at hash 000000000000000000009b48061fe1b12f94ab54e71b289c5db97e0e5fd41874
...
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646824223)
Pretty sure this is the issue from your supplied ```Debug CLI.log"
```2023-07-22T13:42:39Z Syncing coinstatsindex with block chain from height 0```
```2023-07-22T13:42:39Z ERROR: Commit: Failed to commit latest coinstatsindex state```
adding the startup flag -reindex-chainstate should fix this issue. As per your previous concern you will be removing the levelDB indexes which are quite fast to rebuild. Approx ~7GB at hash 000000000000000000009b48061fe1b12f94ab54e71b289c5db97e0e5fd41874
...
💬 new-light-106 commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1646834821)
jahad57ghasem@gmail.com
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1646834821)
jahad57ghasem@gmail.com
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1271448113)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1271448113)
Fixed
💬 hebasto commented on issue "libsecp256k1 not instrumented when building with sanitizers":
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1646846003)
FWIW, the libsecp256k1 is instrumented properly when using the CMake-based build system (see the full prototype [branch](https://github.com/hebasto/bitcoin/tree/pr25797.69) from https://github.com/bitcoin/bitcoin/pull/25797):
```
$ objdump --disassemble=secp256k1_xonly_pubkey_serialize build/src/test/fuzz/fuzz | grep __sanitizer_cov
d42e90: e8 8b 23 c0 ff call 945220 <__sanitizer_cov_trace_const_cmp8>
d42ff2: e8 39 21 c0 ff call 945130 <__sanitizer_cov_trace_pc_indi
...
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1646846003)
FWIW, the libsecp256k1 is instrumented properly when using the CMake-based build system (see the full prototype [branch](https://github.com/hebasto/bitcoin/tree/pr25797.69) from https://github.com/bitcoin/bitcoin/pull/25797):
```
$ objdump --disassemble=secp256k1_xonly_pubkey_serialize build/src/test/fuzz/fuzz | grep __sanitizer_cov
d42e90: e8 8b 23 c0 ff call 945220 <__sanitizer_cov_trace_const_cmp8>
d42ff2: e8 39 21 c0 ff call 945130 <__sanitizer_cov_trace_pc_indi
...
💬 hebasto commented on pull request "build: pass sanitize flags to instrument libsecp256k1 code":
(https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1646846285)
FWIW, CMake [works](https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1646846003) out of the box :)
(https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1646846285)
FWIW, CMake [works](https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1646846003) out of the box :)
📝 hebasto opened a pull request: "Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/28131)
Required for https://github.com/bitcoin/bitcoin/pull/27991 (see the [comment](https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1611472816)) and for the upcoming CMake-based build system.
(https://github.com/bitcoin/bitcoin/pull/28131)
Required for https://github.com/bitcoin/bitcoin/pull/27991 (see the [comment](https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1611472816)) and for the upcoming CMake-based build system.
💬 manfreddd commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646873055)
@russeree thank you for joining the support effort. I ran Bitcoin Core with the option that you and @MarcoFalke recommended. Here is the command and command line error message produced.
C:\Users\xxxx>bitcoind -reindex-chainstate -debug
Error: A fatal internal error occurred, see debug.log for details
This system won't let me load the entire 365 MB debug.log file. So, I am displaying below the tail end of the file including the fatal error message.
2023-07-23T14:28:27Z [validation] Bloc
...
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646873055)
@russeree thank you for joining the support effort. I ran Bitcoin Core with the option that you and @MarcoFalke recommended. Here is the command and command line error message produced.
C:\Users\xxxx>bitcoind -reindex-chainstate -debug
Error: A fatal internal error occurred, see debug.log for details
This system won't let me load the entire 365 MB debug.log file. So, I am displaying below the tail end of the file including the fatal error message.
2023-07-23T14:28:27Z [validation] Bloc
...
📝 petertodd opened a pull request: "Enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/28132)
In addition to Luxor (3.3% hash power), the following pools are have enabled full-rbf:
- [Antpool, 21%](https://mempool.space/tx/53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643)
- [Binance Pool, 8.2%](https://mempool.space/tx/64a26f750c7c58812bd475d054a9aa05248b6a8a2d53c0db38c0624197a4c68a)
- [KuCoinPool, 1.4%](https://mempool.space/tx/8df183b087d433eac4d8e44eca5c7162a2540ee078a28ca04316fd82e12ec435)
- [Poolin, 1.4%](https://mempool.space/tx/d14956420077974d8d28c2fcd005cd9
...
(https://github.com/bitcoin/bitcoin/pull/28132)
In addition to Luxor (3.3% hash power), the following pools are have enabled full-rbf:
- [Antpool, 21%](https://mempool.space/tx/53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643)
- [Binance Pool, 8.2%](https://mempool.space/tx/64a26f750c7c58812bd475d054a9aa05248b6a8a2d53c0db38c0624197a4c68a)
- [KuCoinPool, 1.4%](https://mempool.space/tx/8df183b087d433eac4d8e44eca5c7162a2540ee078a28ca04316fd82e12ec435)
- [Poolin, 1.4%](https://mempool.space/tx/d14956420077974d8d28c2fcd005cd9
...
💬 petertodd commented on pull request "Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1646874124)
FYI [mempool.space has enabled full-rbf by default](https://github.com/mempool/mempool/pull/3867), among many others.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1646874124)
FYI [mempool.space has enabled full-rbf by default](https://github.com/mempool/mempool/pull/3867), among many others.
💬 luke-jr commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1646878130)
>except storage
Blockchains don't do storage. There's a tool for that called ext4, and BitTorrent for distribution.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1646878130)
>except storage
Blockchains don't do storage. There's a tool for that called ext4, and BitTorrent for distribution.
💬 jonatack commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1271484187)
c75d9de0683a91151eb4a508cb64a8937ca92 `s/PAYMENT_H/PAYMENTS_H/` in `ifndef/define/endif` or rename the files to `silentpayment.{h,cpp}`
```bash
$ test/lint/lint-include-guards.py
src/wallet/silentpayments.h seems to be missing the expected include guard:
#ifndef BITCOIN_WALLET_SILENTPAYMENTS_H
#define BITCOIN_WALLET_SILENTPAYMENTS_H
...
#endif // BITCOIN_WALLET_SILENTPAYMENTS_H
```
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1271484187)
c75d9de0683a91151eb4a508cb64a8937ca92 `s/PAYMENT_H/PAYMENTS_H/` in `ifndef/define/endif` or rename the files to `silentpayment.{h,cpp}`
```bash
$ test/lint/lint-include-guards.py
src/wallet/silentpayments.h seems to be missing the expected include guard:
#ifndef BITCOIN_WALLET_SILENTPAYMENTS_H
#define BITCOIN_WALLET_SILENTPAYMENTS_H
...
#endif // BITCOIN_WALLET_SILENTPAYMENTS_H
```
💬 ChristopherA commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1646904832)
Concept ACK. Current limits on op_return force people to use hacks to store data hiding in op_codes. Using op_return is more `honest’. In my own case, current op_return limits keep me from posting a signed hash plus some metadata (<256 bytes) and thus I must use Taproot tricks instead.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1646904832)
Concept ACK. Current limits on op_return force people to use hacks to store data hiding in op_codes. Using op_return is more `honest’. In my own case, current op_return limits keep me from posting a signed hash plus some metadata (<256 bytes) and thus I must use Taproot tricks instead.
🤔 jonatack reviewed a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1542308295)
Approach ACK b89567f51ade926af8c918e4787046b7ccec8eb0
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1542308295)
Approach ACK b89567f51ade926af8c918e4787046b7ccec8eb0
💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271496876)
29e31d8
<details><summary><code>ParseSighash()</code> suggestions</summary><p>
```diff
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -47,7 +47,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string&
-util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
+[[nodiscard]] util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -245,7 +245,7 @@ bool ParseHashStr(const std::string& strHex,
...
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271496876)
29e31d8
<details><summary><code>ParseSighash()</code> suggestions</summary><p>
```diff
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -47,7 +47,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string&
-util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
+[[nodiscard]] util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -245,7 +245,7 @@ bool ParseHashStr(const std::string& strHex,
...
💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271533741)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 this ought to be added to `core_read.cpp` too; see full iwyu suggestions at https://cirrus-ci.com/task/4627440203464704?logs=ci#L1786
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271533741)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 this ought to be added to `core_read.cpp` too; see full iwyu suggestions at https://cirrus-ci.com/task/4627440203464704?logs=ci#L1786
💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271543977)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 When storing a `Result` type, I think it's clearer to name it `result` (seems to be becoming a convention). In particular, `util::ErrorString(parsed_sighash)` is a little confusing.
```cpp
const auto result{ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str()))};
if (!result) {
throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
}
...
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271543977)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 When storing a `Result` type, I think it's clearer to name it `result` (seems to be becoming a convention). In particular, `util::ErrorString(parsed_sighash)` is a little confusing.
```cpp
const auto result{ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str()))};
if (!result) {
throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
}
...
💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271540053)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 (feel free to ignore) I wouldn't find review harder if the missing brackets were added
```cpp
if (v.isStr()) {
strHex = v.getValStr();
}
if (!IsHex(strHex)) {
throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
}
```
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271540053)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 (feel free to ignore) I wouldn't find review harder if the missing brackets were added
```cpp
if (v.isStr()) {
strHex = v.getValStr();
}
if (!IsHex(strHex)) {
throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
}
```