💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527590240)
I don't think we are changing behaviour here (though I might stand corrected still), so if a change is required to them, the error catchers would have been buggy from the beginning. I have long questioned though to how we catch the db write errors, and if it might lead to an infinite loop in some cases. The exception handling just doesn't seem very robust or thought-through to me.

💬 stickies-v commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3532938522)
Concept ACK, approach ~0 leaning towards nack. I think the approach that merging the backwards-incompatible interface early is not unreasonable but not my preferred one, because:
- I don't think we should at all care about breaking interfaces at the moment, the kernel API is experimental and upstream development flexibility and pace should absolutely be prioritized until kernel is in a more stable state
- having the interface behave in unexpected ways is just not desirable, even if this gap is
...

💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3533008173)
> This delay could mean logs won't be produced by context-free operations

Thanks, I'll look into this. I am a little surprised there would be any context-free operations except very basic ones that shouldn't log. It'd expect there to be some context object, even a minimal one, associated with all non-trivial operations to control things like logging, random number generation, caching, etc.

Of course I do understand your abstract concern that adding a connection parameter "suggests that set
...

💬 vasild commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3533027320)
I can reproduce this. A cursory debug shows that `txin.scriptWitness` IsNull but `txin.scriptSig` is not empty:

https://github.com/bitcoin/bitcoin/blob/e221b2524659d22cc5a2b0d0023254115a7a5622/src/psbt.h#L1253-L1254

A transaction created via the RPC does not exhibit this problem. Only when created from the GUI.

📝 ryanofsky converted_to_draft a pull request: "kernel: Improve logging API"
(https://github.com/bitcoin/bitcoin/pull/33847)
Simplify kernel logging API and try to connect it to the rest of the kernel API by letting log options be set on `Logger` objects directly and `Logger` objects to be associated with `Context` objects directly. Also drop `logging_disable()` function and avoid having library accumulate log messages in a 1MB buffer by default.

Changes are intended to make the API a little less surprising and more future proof. Rationale is described in some more detail the commit message.

This change is not b
...

💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3533073827)
> prefer to convert this to draft

Sure happy to do that while there is conceptual discussion.

> having the interface behave in unexpected ways is just not desirable

I think I've pointed out practical ways the current interface behaves in unexpected ways and is counterintuitive, and practical ways this PR is an improvement. The concerns you and stringintech raised do seem plausible, but also vague so if there are any concrete scenarios you can think of where this PR might create a proble
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2527749355)
Ahh right, and we don't have to recheck `current_value` each time which will have 1 less atomic operation if there is a lost race. If you push again maybe touch up to:
```C++
size_t current_value{m_num_to_open};
size_t new_value;
do {
new_value = current_value > n ? current_value - n : 0;
} while (!m_num_to_open.compare_exchange_weak(current_value, new_value));
```

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527752096)
Seems reasonable, done.

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

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(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.

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(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!

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(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!

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

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

💬 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" />

⚠️ 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
...

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