💬 janb84 commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3534152709)
Guix builds again :)

### My Guix Build Output

**Host architecture:** `aarch64`
**Commit:** `8ebf8d3`

```shell
a80099915046f4771235180303b1c92664d36c8cb1564e0e8afc410d3a5eec19 guix-build-8ebf8d35eb1c/output/aarch64-linux-gnu/SHA256SUMS.part
1c3b7cd6548ba04ce58c3d8c7645941fd6cdda313d21f171d0ce10067e11087b guix-build-8ebf8d35eb1c/output/aarch64-linux-gnu/bitcoin-8ebf8d35eb1c-aarch64-linux-gnu-debug.tar.gz
e3738b32f546e48dca9a1e1adb63349c206d11a6528c73f639701e3d376dabe3 guix-build-
...

📝 naiyoma converted_to_draft a pull request: "wallet: refactor ProcessDescriptorImport , validate descriptors before rescan"
(https://github.com/bitcoin/bitcoin/pull/33874)


Fixes: https://github.com/bitcoin/bitcoin/issues/33655

Currently, we validate and import descriptors in the same loop. If any descriptor fails validation, an error is added to the response array, but the complete response (including that error) is only returned to after a rescan completes—potentially wasting significant time when validation error could have been caught immediately.

This PR separates validation from processing: validate all descriptors first, and process in the second
...

💬 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_r2528842330)
I tried `reject_reason = None` locally and the `p2p_invalid_tx.py` tests seem to still pass 🤔.

If I understand this line of code correctly, it seems that passing in `None` or `""` as the `reject_reason` results in the same behavior? 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.

https://github.com/bitcoin/bitcoin/blob/e221b2524659d22cc5a2b0d0023254115a7a5622/test/functional/test_framework/p2p.py#L915

💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2528559215)
These lines are needlessly indented.

💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2528773169)
Please also add an inline doc for the `nullptr`.

💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2528855547)
I don't think the comments here add anything, can you remove them again?

💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2528846300)
The indentation should be fixed here.

👍 hodlinator approved a pull request: "test, refactor: Embedded ASmap selected preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3466738494)
tACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a

Happy to have helped uncover a bug (https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527777820). Thanks for incorporating my suggestions! New commit with `size_preserves_position`-test looks good, I especially like how it verifies that one can ask for size at the end bug not read without triggering an exception.

Started looking at #28792 and agree this is a reasonable subset of changes to peel off (although I would also consider peeling o
...

💬 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_r2528893504)
Taken in https://github.com/bitcoin/bitcoin/pull/31823/commits/5d748257c2fa17c03f732dc24b623d60d8dc8c76

💬 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.

💬 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
...

📝 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
...

💬 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
...

💬 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.

💬 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

💬 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.

💬 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.

💬 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
...

💬 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

👍 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