💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2528894022)
Ok, hopefully bringing this home. You are right that the `reject_reason = "tx-size-small"` makes sense. I've added it in https://github.com/bitcoin/bitcoin/pull/31823/commits/5d748257c2fa17c03f732dc24b623d60d8dc8c76 .
The other stuff I think is incorrect as `None` and `""`" seem to result in the same behavior.
Long winded way of saying you were right and I've incorporated your review.
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2528894022)
Ok, hopefully bringing this home. You are right that the `reject_reason = "tx-size-small"` makes sense. I've added it in https://github.com/bitcoin/bitcoin/pull/31823/commits/5d748257c2fa17c03f732dc24b623d60d8dc8c76 .
The other stuff I think is incorrect as `None` and `""`" seem to result in the same behavior.
Long winded way of saying you were right and I've incorporated your review.
💬 theStack commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2528903851)
Oops yes, you're right, sorry for creating even more confusion. It seems that all tx test cases in `invalid_txs.py` are expected to be rejected from mempool (which makes sense I guess, as they are... invalid), and omitting a `reject_reason` or setting it explicitly to `None` leads indeed to the same behaviour.
> I'm not super familiar with python so its a bit unclear to me if None and "" are evaluated as false in this if statement.
Yeah, an empty string is falsy:
```
>>> "true" if None e
...
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2528903851)
Oops yes, you're right, sorry for creating even more confusion. It seems that all tx test cases in `invalid_txs.py` are expected to be rejected from mempool (which makes sense I guess, as they are... invalid), and omitting a `reject_reason` or setting it explicitly to `None` leads indeed to the same behaviour.
> I'm not super familiar with python so its a bit unclear to me if None and "" are evaluated as false in this if statement.
Yeah, an empty string is falsy:
```
>>> "true" if None e
...
📝 hodlinator opened a pull request: "qa: Account for unset errno in ConnectionResetError"
(https://github.com/bitcoin/bitcoin/pull/33875)
The lack of errno can cause unclear and long log output.
Issue can be triggered by:
```diff
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -263,6 +263,7 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
/** HTTP request callback */
static void http_request_cb(struct evhttp_request* req, void* arg)
{
+ throw std::runtime_error{"Hello"};
evhttp_connection* conn{evhttp_request_get_connection(req)};
// Track active requests
{
```
and runnin
...
(https://github.com/bitcoin/bitcoin/pull/33875)
The lack of errno can cause unclear and long log output.
Issue can be triggered by:
```diff
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -263,6 +263,7 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
/** HTTP request callback */
static void http_request_cb(struct evhttp_request* req, void* arg)
{
+ throw std::runtime_error{"Hello"};
evhttp_connection* conn{evhttp_request_get_connection(req)};
// Track active requests
{
```
and runnin
...
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3534660813)
My guix build hashes:
```x86_64
fddfcf1d14219e7df35dd4c25546b99df2edb1fc29856c7ae1f7a9f27bfb24b1 guix-build-2594d5a189e5/output/aarch64-linux-gnu/bitcoin-2594d5a189e5-aarch64-linux-gnu-debug.tar.gz
488d6c14892a1753495091c8a49a00d1f0e3d2702d9d7b3536fa98e714e217ea guix-build-2594d5a189e5/output/aarch64-linux-gnu/bitcoin-2594d5a189e5-aarch64-linux-gnu.tar.gz
a02136d07e0aab9f6c0352fe280226db51bb34000f6db9163a64ca7ee05902b0 guix-build-2594d5a189e5/output/aarch64-linux-gnu/SHA256SUMS.part
c0
...
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3534660813)
My guix build hashes:
```x86_64
fddfcf1d14219e7df35dd4c25546b99df2edb1fc29856c7ae1f7a9f27bfb24b1 guix-build-2594d5a189e5/output/aarch64-linux-gnu/bitcoin-2594d5a189e5-aarch64-linux-gnu-debug.tar.gz
488d6c14892a1753495091c8a49a00d1f0e3d2702d9d7b3536fa98e714e217ea guix-build-2594d5a189e5/output/aarch64-linux-gnu/bitcoin-2594d5a189e5-aarch64-linux-gnu.tar.gz
a02136d07e0aab9f6c0352fe280226db51bb34000f6db9163a64ca7ee05902b0 guix-build-2594d5a189e5/output/aarch64-linux-gnu/SHA256SUMS.part
c0
...
💬 vostrnad commented on issue "Default ASmap file path is not used unless -asmap is set":
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3534768830)
@fjahr Yes, I think removing the default altogether is probably the best way forward, especially in light of a possible future release with embedded ASmap data, and it should remove any potential for confusion around how the option works.
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3534768830)
@fjahr Yes, I think removing the default altogether is probably the best way forward, especially in light of a possible future release with embedded ASmap data, and it should remove any potential for confusion around how the option works.
💬 theStack commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2529085149)
these two lines can be dropped; `expect_disconnect` was removed in #33050 (commit 876dbdfb4702410dfd4037614dc9298a0c09c63e) and `valid_in_block` is set to False by default anyways
```suggestion
```
other than that, lgtm
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2529085149)
these two lines can be dropped; `expect_disconnect` was removed in #33050 (commit 876dbdfb4702410dfd4037614dc9298a0c09c63e) and `valid_in_block` is set to False by default anyways
```suggestion
```
other than that, lgtm
💬 vostrnad commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529117675)
I also don't like the inconsistent `(default: none)`. I think it's pretty clear that if no default is mentioned, the feature is disabled. The help text for `-i2psam` should be fixed.
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529117675)
I also don't like the inconsistent `(default: none)`. I think it's pretty clear that if no default is mentioned, the feature is disabled. The help text for `-i2psam` should be fixed.
💬 vostrnad commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529118333)
nit: `strprintf` isn't needed anymore.
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529118333)
nit: `strprintf` isn't needed anymore.
💬 151henry151 commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3534886629)
It looks to me like GUI’s “Create unsigned” path still signs the transaction, so legacy inputs end up with non-empty `scriptSig` and the PSBT parser rejects them. Perhaps we could call `CreateTransaction` with signing disabled or explicitly clear each `scriptSig`/`scriptWitness` before building the PSBT, or maybe it would be better to reuse the RPC PSBT builder so the GUI and RPC share the same logic.
I haven't attempted to reproduce yet, just read through the issue and thinking about possible
...
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3534886629)
It looks to me like GUI’s “Create unsigned” path still signs the transaction, so legacy inputs end up with non-empty `scriptSig` and the PSBT parser rejects them. Perhaps we could call `CreateTransaction` with signing disabled or explicitly clear each `scriptSig`/`scriptWitness` before building the PSBT, or maybe it would be better to reuse the RPC PSBT builder so the GUI and RPC share the same logic.
I haven't attempted to reproduce yet, just read through the issue and thinking about possible
...
💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2529334053)
Done in a7c96f8
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2529334053)
Done in a7c96f8
👍 theStack approved a pull request: "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`"
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-3467369575)
ACK a7c96f874de13ace9814da92fd6160a126a97ebf
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-3467369575)
ACK a7c96f874de13ace9814da92fd6160a126a97ebf
📝 hebasto opened a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/33876)
The `py310-zmq` binary package is not available by default on NetBSD 10.1. It has been updated to `py313-zmq`, and the `python310` package is updated accordingly.
See: https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/index-all.html.
(https://github.com/bitcoin/bitcoin/pull/33876)
The `py310-zmq` binary package is not available by default on NetBSD 10.1. It has been updated to `py313-zmq`, and the `python310` package is updated accordingly.
See: https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/index-all.html.
📝 stringintech opened a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877)
Remove the int parameter from `btck_chainstate_manager_options_*_in_memory` functions and rename them from "update" to "set". These functions now only set the options to `true` for enabling in-memory mode. This, combined with explicit default initialization to `false`, removes ambiguity about what happens when these functions are not called and makes the typical use case (disk-based storage) require no action from the user.
(https://github.com/bitcoin/bitcoin/pull/33877)
Remove the int parameter from `btck_chainstate_manager_options_*_in_memory` functions and rename them from "update" to "set". These functions now only set the options to `true` for enabling in-memory mode. This, combined with explicit default initialization to `false`, removes ambiguity about what happens when these functions are not called and makes the typical use case (disk-based storage) require no action from the user.
📝 stringintech converted_to_draft a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877)
Remove the int parameter from `btck_chainstate_manager_options_*_in_memory` functions and rename them from "update" to "set". These functions now only set the options to `true` for enabling in-memory mode. This, combined with explicit default initialization to `false`, removes ambiguity about what happens when these functions are not called and makes the typical use case (disk-based storage) require no action from the user.
(https://github.com/bitcoin/bitcoin/pull/33877)
Remove the int parameter from `btck_chainstate_manager_options_*_in_memory` functions and rename them from "update" to "set". These functions now only set the options to `true` for enabling in-memory mode. This, combined with explicit default initialization to `false`, removes ambiguity about what happens when these functions are not called and makes the typical use case (disk-based storage) require no action from the user.
👍 hebasto approved a pull request: "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3468081115)
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1.
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3468081115)
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1.
👋 stringintech's pull request is ready for review: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877)
(https://github.com/bitcoin/bitcoin/pull/33877)
💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536509285)
https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3534660813
> My guix build hashes:
@151henry151
To make hashes easily comparable, they are sorted using `env LC_ALL=C sort`. The full command to retrieve the hashes is as follows:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536509285)
https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3534660813
> My guix build hashes:
@151henry151
To make hashes easily comparable, they are sorted using `env LC_ALL=C sort`. The full command to retrieve the hashes is as follows:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```
💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536510041)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536510041)
cc @purpleKarrot
🤔 hodlinator reviewed a pull request: "Embed default ASMap as binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895)
Reviewed e00deb2cfa60e846ea3de48bed35c7d3992db4bd
Seems like this PR has evolved a bit since before CMake. Pretty good shape now (using CMake byte header generation instead of Python script etc).
Left a bunch of nits but also some more in-depth suggestions & questions.
Skipped aspects addressed in other PR with similar commits (https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3441618331).
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895)
Reviewed e00deb2cfa60e846ea3de48bed35c7d3992db4bd
Seems like this PR has evolved a bit since before CMake. Pretty good shape now (using CMake byte header generation instead of Python script etc).
Left a bunch of nits but also some more in-depth suggestions & questions.
Skipped aspects addressed in other PR with similar commits (https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3441618331).
💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514801922)
nit: We tend to drop `const...&` for spans, similar to how we don't use `const int&`.
```suggestion
uint32_t Interpret(std::span<const std::byte> asmap, std::span<const std::byte> ip);
```
As the style in:
https://github.com/bitcoin/bitcoin/blob/e00deb2cfa60e846ea3de48bed35c7d3992db4bd/src/test/fuzz/asmap_direct.cpp#L15
(One can always add `const` for the function definition in the .cpp file, as it is the concern of the implementation).
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514801922)
nit: We tend to drop `const...&` for spans, similar to how we don't use `const int&`.
```suggestion
uint32_t Interpret(std::span<const std::byte> asmap, std::span<const std::byte> ip);
```
As the style in:
https://github.com/bitcoin/bitcoin/blob/e00deb2cfa60e846ea3de48bed35c7d3992db4bd/src/test/fuzz/asmap_direct.cpp#L15
(One can always add `const` for the function definition in the .cpp file, as it is the concern of the implementation).