💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2528229709)
> Not all users will use language bindings, and regardless, I don't think the distinction between "direct usage" and "wrapped usage" is meaningful here. Our implementation introduces UB when a non-zero length is used with a nullptr, so our implementation is responsible for preventing the UB.

Makes sense

> How? In the API, a string is defined as {ptr, ptr+len}. Being empty means a size of 0, and size depends on len. We could introduce the requirement for strings to be null-terminated and re
...

💬 laanwj commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3533735681)
It's debian (Debian GNU/Linux forky/sid). Intentially chose that because i assumed it would be least hassle 😄

👍 andrewtoth approved a pull request: "net_processing: reorder the code that handles the VERSION message"
(https://github.com/bitcoin/bitcoin/pull/33792#pullrequestreview-3465930909)
ACK 52149b0e2b7897cc2ea059a9b50c4cb3b9fcd722

Moving the `WTXIDRELAY` and `SENDADDRV2` `MakeAndPushMessage`s doesn't change the order of messages pushed. Nothing between below where they are moved from and above where they are moved to has an effect on any of the members of `pfrom` accessed in `MakeAndPushMessage`.

Moving `mapped_as` and the subsequent log above does not change anything with `mapped_as` either, since the `pfrom.addr` is not modified in any of the lines between where it is m
...

💬 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_r2528317136)
Ok i think i understand what you are saying conceptually - but perhaps I'm being a bit dense - but toggling the `reject_reason` comment that I have in https://github.com/bitcoin/bitcoin/pull/31823/commits/94a1bbb0d7f70513b68b28d4111cac56efce864a doesn't seem to affect whether the `p2p_invalid_tx.py` test suite passes or fails. I.e. the `reject_reason` seems irrelevant?

I'm a bit short on time at the moment and will take a look further in the next day or 2, but figured I would throw that out
...

💬 laanwj commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3533900033)
i patched all versions of `python-xyz.scm` on the system (not sure how to determine which exactly i need to) with this:
```patch
diff --git a/gnu/packages/python-xyz.scm b/gnu/packages/python-xyz.scm
index cba99cce62..7d91059365 100644
--- a/gnu/packages/python-xyz.scm
+++ b/gnu/packages/python-xyz.scm
@@ -31650,6 +31650,7 @@ (define-public python-py-cpuinfo
(base32
"0h5wi1bfcqqr1x3j1pa7dmkx7siprsyksbsy80fl2sdrrgpji0b0"))))
(build-system python-build-system)
+
...

💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2528455954)
However, there really isn't a way to truly guard against UB here since a len could be inaccurate (greater than string length). Nothing that can really be done in that case though I guess.

💬 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_r2528459159)
No worries, I think it's indeed a bit confusing. The testing of a reject reason is done by asserting that the corresponding string appears in the bitcoind log file. If no explicit `reject_reason` is specified in an invalid tx test case, the default value of `""` (empty string) is used, i.e. a submission error is still expected, but the exact reason for why it matters doesn't matter.

You would see a difference in behaviour if you specified `reject_reason = None`, meaning that the expected outc
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2528472456)
Also, just thinking about 32-bit systems and possibility of overflows here. Although unlikely unless spammed with many `sendrawtransaction` for a long time, would it be best to maybe make `m_num_to_open` a `atomic_uint64_t` to remove all doubt?

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

🤔 TheCharlatan reviewed a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3466200695)
Tested the binaries on x86 ubuntu 24.04

Guix build
```
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b81de17c51d03c328763270f14f3 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarc
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3534061801)
> The second PR removing the deprecated methods could also swap the coinbase TX with an empty one as suggested to avoid getBlock returning it.

It makes sense to split this idea over two PR's. I have a local branch already, but still need to split out the commits and get the commit order right.

💬 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