💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527763616)
No, but upon looking into it I think this is still better. The `size()` function uses the current file handle while `file_size()` would create a new one which could create race conditions etc.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527763616)
No, but upon looking into it I think this is still better. The `size()` function uses the current file handle while `file_size()` would create a new one which could create race conditions etc.
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527764328)
Done
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527764328)
Done
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527765721)
Added a more extensive test for this in a new commit.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527765721)
Added a more extensive test for this in a new commit.
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527766160)
Done
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527766160)
Done
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527777820)
That's a bug, nice find! The line where I had `seek(pos, SEEK_END)` in the `size()` implementation should have been `seek(0, SEEK_END)`, this lead to these wrong numbers. In these tests the position was at the end (7) and `seek(pos, SEEK_END)` moved the pos to EOF plus `pos`, which was 14. Fixed!
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527777820)
That's a bug, nice find! The line where I had `seek(pos, SEEK_END)` in the `size()` implementation should have been `seek(0, SEEK_END)`, this lead to these wrong numbers. In these tests the position was at the end (7) and `seek(pos, SEEK_END)` moved the pos to EOF plus `pos`, which was 14. Fixed!
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527778252)
Done
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527778252)
Done
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3533149878)
Addressed all feedback from @hodlinator , thanks for the review!
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3533149878)
Addressed all feedback from @hodlinator , thanks for the review!
💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3533199372)
ACK deac31dd779d28a8c01f1c3153650f5827cfe75d
Reviewed code and tested locally. I had grepped for other relevant mentions of the default location previously.
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3533199372)
ACK deac31dd779d28a8c01f1c3153650f5827cfe75d
Reviewed code and tested locally. I had grepped for other relevant mentions of the default location previously.
👍 stickies-v approved a pull request: "refactor: remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3465409234)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Fixes lifetime attributes, improves clarity of interface by excluding the possibility of null result.
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3465409234)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Fixes lifetime attributes, improves clarity of interface by excluding the possibility of null result.
💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3533276038)
Tested a Guix build from this branch on a Ubuntu 24.04 (aarch64 VM)
<img width="1130" height="747" alt="ldd" src="https://github.com/user-attachments/assets/dc0919be-7ee1-45c5-8bee-e2c58d0e9687" />
<img width="1028" height="723" alt="gui" src="https://github.com/user-attachments/assets/800aec26-f812-4bec-bac7-e527f7ac7a82" />
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3533276038)
Tested a Guix build from this branch on a Ubuntu 24.04 (aarch64 VM)
<img width="1130" height="747" alt="ldd" src="https://github.com/user-attachments/assets/dc0919be-7ee1-45c5-8bee-e2c58d0e9687" />
<img width="1028" height="723" alt="gui" src="https://github.com/user-attachments/assets/800aec26-f812-4bec-bac7-e527f7ac7a82" />
⚠️ laanwj opened an issue: "guix build fails on RISC-V due to python-py-cpuinfo test failure"
(https://github.com/bitcoin/bitcoin/issues/33873)
i'm trying to do a guix build (of the master branch) on RISC-V hardware. The device is a [Scaleway EM-RV1-C4M16S128-A](https://labs.scaleway.com/en/em-rv1/) server from Scaleway.
```
Architecture: riscv64
Byte Order: Little Endian
CPU(s): 4
On-line CPU(s) list: 0-3
```
```
processor : 0
hart : 0
isa : rv64imafdcsu
mmu : sv39
cpu-freq : 1.848Ghz
cpu-icache : 64KB
cpu-dcache : 64KB
cpu-l2cache : 1M
...
(https://github.com/bitcoin/bitcoin/issues/33873)
i'm trying to do a guix build (of the master branch) on RISC-V hardware. The device is a [Scaleway EM-RV1-C4M16S128-A](https://labs.scaleway.com/en/em-rv1/) server from Scaleway.
```
Architecture: riscv64
Byte Order: Little Endian
CPU(s): 4
On-line CPU(s) list: 0-3
```
```
processor : 0
hart : 0
isa : rv64imafdcsu
mmu : sv39
cpu-freq : 1.848Ghz
cpu-icache : 64KB
cpu-dcache : 64KB
cpu-l2cache : 1M
...
💬 hebasto commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3533560301)
Which OS?
I’ve found Debian to be more suitable for RISC-V.
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3533560301)
Which OS?
I’ve found Debian to be more suitable for RISC-V.
💬 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
...
(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 😄
(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
...
(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
...
(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)
+
...
(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.
(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
...
(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?
(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?